ConcurrantModificationException

Newbie & Debugging Questions
#1

Here’s my code:


	public ArrayList<PlayerCharacter> goodGuyEntities = new ArrayList<PlayerCharacter>();
	public ArrayList<BadGuy> badGuyEntities = new ArrayList<BadGuy>();
	public ArrayList<Shot> goodGuyShotEntities = new ArrayList<Shot>();
	public ArrayList<Shot> badGuyShotEntities = new ArrayList<Shot>();
	public ArrayList<Shot> removeList = new ArrayList<Shot>();

		for (PlayerCharacter character : goodGuyEntities) {
			character.update();
		}

		for (BadGuy badGuy : badGuyEntities) {
			badGuy.update();
		}

		for (Shot shot : goodGuyShotEntities) { // this is the line that eclipse said threw the exception
			shot.update();
			for (BadGuy badGuy : badGuyEntities) {
				if (shot.bounds.intersects(badGuy.bounds)) {
					shot.hit(badGuy);
				}
			}
		}

		for (Shot shot : badGuyShotEntities) {
			shot.update();
			for (PlayerCharacter character : goodGuyEntities) {
				if (shot.bounds.intersects(character.bounds)) {
					shot.hit(character);
				}
			}
		}

		for (Entity remove : removeList) {
			removeEntity(remove);
		}

This threw a ConcurrantModificationException and as far as I understand the exception, it’s because I’m iterating through the ArrayLists and calling update() on each object in the ArrayList within the iteration. Is there any way to get around this besides using the traditional for loop:

for(int i = 0; i < goodGuyShotEntities.size(); i++)

this fixes it but I don’t like how it looks and is much harder to use and read.

#2

You are not allowed to add or remove any objects from an ArrayList while it is being iterated. That is why you are getting a ConcurrentModificationException.

You most likely remove the “Shot” object from the “goodGuyShotEntities” list inside the hit method, when it intersects with a “BadGuy”. Instead, you should use this ping-pong technique described here in lines 16-30: http://www.java-gaming.org/topics/arraylist-vs-linkedlist/27016/msg/240151/view.html#msg240151

So “Shot”-s should have an “isAlive” boolean field that you set to false inside the hit method. This way the loop will know not to add it to the other list.

#3

I personally just use the simple for-loop :persecutioncomplex:

I think it is a lot easier to just use that line of code (which shouldn’t confuse you at all), and not have to worry about multiple lists and the like.

On another note, I don’t agree with your use of a removal list, I would just use the boolean like ra4king recommended.

And also, look into having a single list if you can, it is much cleaner and often easier to handle as your game becomes more complicated (less loops).

#4

The simple for-loop is flawed in this situation because once you remove an item from the list, unless you properly deal with the index value, you will skip over the next item due to everything being shifted over.

#5

yeah, I guess, but you could just do a counter-- if it is critical. while not optimal, still easier than making another list.

#6

What would be the advantage of using boolean isAlive over a remove list? As it is now, with the simple for loop and the remove list, the objects aren’t removed in the hit() method, they are added to the remove list which is then handled in a different loop so the index doesn’t change within a loop.

The reason I’m using 4 different arrayLists for all of the entities is because it’s a part of brute force collision detection and I figured the less rectangles that needed to be compared, the easier it would be on the system. I just assumed this would make the game run faster, please correct me if I’m wrong.

#7

There are some situations where using a “CopyOnWriteArrayList” would solve the problem. A lot depends on the ratio of iteration vs deletes/inserts, and on how much trouble you might get into working from a snapshot of the ArrayList rather than the ArrayList itself.

More info here:
http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CopyOnWriteArrayList.html
It is also discussed in the concurrency section of the Java Tutorials.

Alternatively, I seem to recall princec explaining this method (but can’t find the original post):
(1) make a copy the ArrayList
(2) as you go through the originals, if there is something to delete, do it to the copy
(3) once the iteration is done, make the copy the new ArrayList.

Marking objects for live/dead in the collision pass makes a LOT of sense, combined with alternating the collision checks with the cleanups. Otherwise, if you start synchronizing the objects to prevent the concurrency error, the resulting blocking could really slow things down.

#8

Also, he doesn’t copy the ArrayList, he just creates two of them at the start.

#9

Thanks theagentd! I should have looked at ra4king’s link from the start.

Your code solution pretty much matches princec’s advice a couple posts later, yes?

I still like the alternative idea of making a copy of the screen that holds ID’s instead of graphics, and updating and consulting that instead of brute force checking or maintaining and consulting oct trees or bins. It certainly has its inefficient aspects, but has a sort of neat simplicity: you check the space you want to go to and see if it is occupied or not, and if it is, who is occupying it. Never mind the factorial cross-checking of object against object.

#10

I would use the standard for loop. You get very used to the format of it. I save iterators for hashMaps and other collections that are not easy to traverse with a simple loop.

#11

Ooh, a con-currant modification exception. Sounds delicious.

#12

Eli you’re back!

#13

You know, off and on. Busy busy busy. Trying to get a game out there while working full time and being married leaves little room for JGO.

#14

I came here to do this. :frowning: