0

[For clarification, 'monsters' is a list of NPCs that when shot by an object 'bullet' are removed from the list. ]

Error at start:

W/System.err: java.util.ConcurrentModificationException
W/System.err:     at java.util.ArrayList$Itr.next(ArrayList.java:831)
W/System.err:     at com.companyname.product.GameplayScene.update(GameplayScene.java:113)
//...

I believe this means that it is trying to perform an action on something in the list that isn't there. So my question is could I please get some help identifying the point where an object is being called that isn't there, as I can't seem to find it. I know which class it is in as the first line references it for each error.

        for (Bullet bullet : player.getList())
            if (npcManager.bulletCollide(bullet)) { //checks for a collision between all npcs and all bullets
                 player.getList().remove(bullet);//npc is removed in method above

        }

       if (!gameOver) {
        npcManager.update(); //updates movement of npcs
        if (npcManager.playerCollideNPC(player)) {
            //end game
        }
    }

    for (RectNPC NPC : npcManager.getList())
        if (obstacleManager.NPCCollide(NPC)) { //checks collision between npcs and other objects
            //
        }

        int i = 0;
        //checks collisions between NPCs themselves
        while (i < (Constants.NUMBER_ENEMIES - 1)) {
            if (npcManager.getList().size() <= 1)
                return;
            if (Rect.intersects(npcManager.getList().get(i).getRectangle(), npcManager.getList().get(i + 1).getRectangle()))
                npcManager.getList().get(i).setRectangle(200);
            i += 1;
        }
    }

(Above) I imagine the issue will be with calling something in the update (shown above) where the object is no longer available, is there an issue with any of how I am removing the NPCs?

 public boolean bulletCollide(Bullet bullet) {
    for(RectNPC npc : monsters) {
        if(npc.bulletCollideNPC(bullet)) {
            monsters.remove(npc);
            monsters.add(0, new RectNPC(new Rect(100, 100, 200, 200), Color.rgb(255, 0, 0), 25));
            return true;
        }
    }
    return false;
}

(Above) Code where I am removing the NPC, I've included incase it is helpful - however it does do what it's meant to so I don't believe this is the problem.

My main question is, is there anything here that would be causing this error (the game stopping for a few frames and giving the error at the top) - or is there something/somewhere else in specific I should be looking at? Also I know some of my syntax isn't great, I apologise it's my first Java project.

Current Iterators:

for(Iterator<RectNPC> iterator = monsters.iterator(); iterator.hasNext();) {
        if(iterator.next().bulletCollideNPC(bullet)) {
            iterator.remove();
            monsters.add(0, new RectNPC(new Rect(100, 100, 200, 200), Color.rgb(255, 0, 0), 25));
            return true;
        }
    }

and 

for (Iterator<Bullet> iterator = player.getList().iterator(); iterator.hasNext(); ) {
            if (npcManager.bulletCollide(iterator.next())) {
                iterator.remove();
                //
            }
        }
George
  • 45
  • 6
  • A ConcurrentModificationException means the Collection is being iterated and modified, not that an element is missing. Use an iterator rather than the for loop with the remove. See http://stackoverflow.com/questions/223918/iterating-through-a-collection-avoiding-concurrentmodificationexception-when-re – KevinO Mar 28 '17 at 22:23

3 Answers3

2

In java, you cannot do for( T variable : collection ) { collection.remove( variable ); } In other words, you cannot be removing items from a collection which is being iterated with a for loop. It will result in a ConcurrentModificationException. Google for "java fail-fast iterator" for more information about why and how.

You have two options:

  • Collect in a temporary list all the monsters that are to be removed, then once you are done iterating over the main list, iterate once more through the temporary list to remove them from the main list. The point is that you are not going to be removing from the same list that you are iterating.

  • Instead of a "foreach" (for) loop, use an actual iterator, and when you want to remove an item, remove it by invoking the iterator's remove() method, not the containing collection's remove() method. The iterator's remove() method does the necessary tricks to prevent a ConcurrentModificationException from being thrown.

Mike Nakis
  • 56,297
  • 11
  • 110
  • 142
  • I've made two iterators for both the monsters and bullets but I'm still having the same freezes, could you please check if I've written them correctly? (I'll put them as an edit in the post as it looks messy in comment) – George Mar 28 '17 at 23:22
  • they look okay. So, the freezing problem is elsewhere. – Mike Nakis Mar 28 '17 at 23:26
  • Could there be a need for an iterator in place of the first for loop in the original code? starting with - `for (Bullet shot : player.getList()) { shot.tick(true); //...` Because I did try to write one for it but it gave me a total black screen when I ran the code – George Mar 28 '17 at 23:28
  • There is no such code above. Anyhow, if the loop contains a deletion, it must be done with an iterator; if not, it can stay without an iterator. – Mike Nakis Mar 28 '17 at 23:32
  • Okay I'll go through and change them, if I am still having a problem with freezing after changing to iterations, is there anything else I should look at that could be the cause? – George Mar 28 '17 at 23:35
  • Use a profiler tool to profile your code and find where the problematic code is. – Mike Nakis Mar 28 '17 at 23:45
1

Your method is doing 3 different things. It's returning a boolean, removing an element from a list, and then adding an element. I would restructure the code so that you have 3 methods that do only 1 thing.

You can use the power of Java 8's internal iterator, as a starting point

      monsters.removeIf(e -> e.bulletCollideNPC(bullet));
Nick Ziebert
  • 1,258
  • 1
  • 9
  • 17
1

Use Iterator for a safe way to remove your Elements.

Note that Iterator.remove is the only safe way to modify a collection during iteration; the behavior is unspecified if the underlying collection is modified in any other way while the iteration is in progress. From Java SE documentation

static void filter(Collection<?> c) {
    for (Iterator<?> it = c.iterator(); it.hasNext(); )
        if (!cond(it.next()))
            it.remove();
}

Example

Gatusko
  • 2,503
  • 1
  • 17
  • 25