Rendering multiple (simple)animation crashes with: ConcurrentModificationException

Question

I'm trying to render simple animations using java.awt(simple explosion on a mouse click at the cursor location).

First my Animation class:

package animation;

import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;

import javax.imageio.ImageIO;

public class Animation {

    private BufferedImage spriteSheet;
    private int currentStateNum;
    private int spriteSize;
    private int spritsInWidth; // number of sprites in one row of the spritesheet
    private long startTime;
    private long delay;        // delay between each sprite
    private long sicleTime;
    private long lifeTime;
    private boolean alive;

    public Animation(String spriteSheetPath, int spriteSize, 
                     int spritsCount, int spritsInWidth, long delay,
                     long lifeTime) {
        try {
            this.spriteSheet = ImageIO.read(new File(spriteSheetPath));
            this.currentStateNum = 0;
            this.spriteSize = spriteSize;
            this.delay = delay;
            this.spritsInWidth = spritsInWidth;
            this.sicleTime = delay * spritsCount;
            this.lifeTime = lifeTime;
            this.alive = true;
            this.startTime = System.currentTimeMillis();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public BufferedImage getCurrentState() {
        int x = (currentStateNum % spritsInWidth) * spriteSize;
        int y = (currentStateNum / spritsInWidth) * spriteSize;
        return spriteSheet.getSubimage(x, y, spriteSize, spriteSize);
    }

    public void update() {
        long currTime = System.currentTimeMillis();
        long passedTime = currTime - startTime;
        alive = passedTime < lifeTime;
        long currSicleTime = passedTime % sicleTime;
        currentStateNum = (int) (currSicleTime / delay);
    }

    public boolean isAlive() {
        return alive;
    }
}

Now my first attempt of actually rendering the animation looks something like:

import java.awt.Canvas;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import java.awt.image.BufferStrategy;

import javax.swing.JFrame;

import animation.Animation;

public class Main {

    public static void main(String[] args) {

        JFrame frame = new JFrame("Animation test");
        Canvas canvas = new Canvas();

        canvas.setPreferredSize(new Dimension(600, 400));
        canvas.setBackground(Color.white);
        canvas.addMouseListener(new MouseAdapter() {

            @Override
            public void mouseReleased(MouseEvent e) {

                Animation a = new Animation("res/explosion.png", 64, 16, 
                                            4, 40, 40 * 16);

                BufferStrategy bs;
                Graphics g;
                while (a.isAlive()) {
                    bs = canvas.getBufferStrategy();
                    g = bs.getDrawGraphics();
                    g.clearRect(0, 0, canvas.getWidth(), canvas.getHeight());
                    g.drawImage(a.getCurrentState(), e.getX(), e.getY(), null);
                    g.dispose();
                    bs.show();
                    a.update();
                }

                bs = canvas.getBufferStrategy();
                g = bs.getDrawGraphics();
                g.clearRect(0, 0, canvas.getWidth(), canvas.getHeight());
                g.dispose();
                bs.show();
                super.mouseReleased(e);
            }

        });

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().add(canvas);
        frame.pack();
        frame.setVisible(true);

        canvas.createBufferStrategy(3);
    }
}

While this approach works for a single animation on screen at a time, it is needless to say that this fails miserably for multiple clicks. I quickly realized that MouseListener puts the events in queue what is causing the animations to render one after another with a delay instead of being render all together, after a few minutes I realized that its actually good since its probably a bad idea to edit the same raster form multiple threads at the same time.

So my next approach was instead of rendering the animation inside the mouseReleased(MouseEvent e) method, is to create a Map of animations and just add them to the map and render using the map elsewhere.

The code:

import java.awt.Canvas;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Point;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;
import java.awt.image.BufferStrategy;
import java.util.HashMap;
import java.util.Map;

import javax.swing.JFrame;

import animation.Animation;

public class Main {

    static boolean running = true;

    public static void main(String[] args) {

        JFrame frame = new JFrame("Animation test");
        Canvas canvas = new Canvas();
        Map<Animation, Point> explosions = new HashMap<Animation, Point>();

        canvas.setPreferredSize(new Dimension(600, 400));
        canvas.setBackground(Color.white);
        canvas.addMouseListener(new MouseAdapter() {

            @Override
            public void mouseReleased(MouseEvent e) {
                explosions.put(new Animation("res/explosion.png", 64, 16, 
                               4, 25, 25 * 16),
                               new Point(e.getX() - 32, e.getY() - 32));

                super.mouseReleased(e);
            }

        });

        frame.setDefaultCloseOperation(JFrame.DO_NOTHING_ON_CLOSE);
        frame.addWindowListener(new WindowAdapter() {
            public void windowClosing(WindowEvent we) {
                running = false;
                explosions.clear();
                System.exit(0);
            }
        });
        frame.getContentPane().add(canvas);
        frame.pack();
        frame.setVisible(true);

        while (running) {

            BufferStrategy bs = canvas.getBufferStrategy();
            if (bs == null) {
                canvas.createBufferStrategy(3);
                continue;
            }
            Graphics g = bs.getDrawGraphics();

            g.clearRect(0, 0, canvas.getWidth(), canvas.getHeight());

            for (Animation a : explosions.keySet()) {
                Point p = explosions.get(a);
                if (a.isAlive()) {
                    g.drawImage(a.getCurrentState(), p.x, p.y, null);
                    a.update();
                } else {
                    explosions.remove(a);
                }
            }

            g.dispose();
            bs.show();

        }

    }
}

This (kinda)works, when it dose render all the animations correctly and without a delay it frequently crashes with java.util.ConcurrentModificationException

stack trace is:

Exception in thread "main" java.util.ConcurrentModificationException
    at java.util.HashMap$HashIterator.nextNode(Unknown Source)
    at java.util.HashMap$KeyIterator.next(Unknown Source)
    at Main.main(Main.java:65)

My first assumption was that the problem is that I remove keys from the map while iterating through it, so I removed the explosions.remove(a) but still got the same problem, in fact I STILL think that the problem is changing the map while iterating through it since an event can trigger while I'm rendering from the map which will add a new key to it.

What I need is an insight whether my assumption is correct and if not then what is the problem? Although (if my assumption is correct)I can probably solve this in some nasty way but I would prefer a tip about a nice elegant solution for this issue.

Answer 1

Ok so it seems that my assumption was indeed correct, and the problem was solved by simply:

synchronized (explosions) {
    Iterator<Map.Entry<Animation, Point>> it = explosions.entrySet().iterator();
    for (; it.hasNext();) {
        Map.Entry<Animation, Point> entry = it.next();
        if (entry.getKey().isAlive()) {
            g.drawImage(entry.getKey().getCurrentState(), entry.getValue().x, entry.getValue().y, null);
            entry.getKey().update();
        } else {
            it.remove();
        }
    }
}

Answer 2

Concurrent hash map should provide better performance, especially as only removal or insert operations will actually block the map