Java - Two threads out of synch

Question

I am programming a simple Top Down Shooter right now. The player is able to hold the down mouse to continously shoot. Since there is no native way to check if the mouse button is held down I implemented this solution: https://stackoverflow.com/a/6828990/3716866

This works perfectly fine. But the problem is that I put each bullet object and each GameObject (enemies, player, barrels, etc.) in a Linked List (two lists one for the objects, one for the bullets). When you shoot a bullet it get's added to the LinkedList. Each runthrough of the gameloop the whole List get's checked through for collision. After firing about 2 shots the game crashes with a Null Pointer Exception. I think the problem is, since I use a second thread to implement the mouse button hold down, that a bullet object is getting created while in the other thread it checks through the list. Since I am checking through the list, while the size is getting altered it is checking through the list, it crashes. But that is just my theory. Anybody got an idea on how to keep them at synch?

If you need more code etc. just tell me and I will upload it. I just wasn't sure what I should post. So before I spam everything I wanted to wait for your feedback.

The Exception:

Exception in thread "Thread-2" java.lang.NullPointerException
    at shooter.main.Handler.checkForCollision(Handler.java:50)
    at shooter.main.Handler.tick(Handler.java:41)
    at shooter.main.Game.tick(Game.java:189)
    at shooter.main.Game.run(Game.java:290)
    at java.lang.Thread.run(Unknown Source)

Handler Class that runs all the tick and update methods for every Object, including collision:

http://pastebin.com/mb206jug

My MouseListener Class where the other Thread is implemented:

http://pastebin.com/LfGWa5se

My Main Game class, although I don't think it will be very important for this problem:

http://pastebin.com/kW3UmZuD

My abstract GameObject Class:

http://pastebin.com/HbsdCUch

Example of a GameObject:

http://pastebin.com/vXCWZwtf

My abstract Ammo Class:

http://pastebin.com/dBnyXwgM

Example of a Bullet:

http://pastebin.com/XapmnsBv

How a shoot method looks like:

public void shoot() {
    //play sound effect
    // fire bullet
    game.getHandler().addBullet(new Bullet9mm(game.getHandler().getPlayer().getX(), game.getHandler().getPlayer().getY(), game.getBulletImageManager(), game));
    // remove 1 bullet from magazine                    
    game.getHandler().getPlayer().getCurWeapon().setMagAmmo(-1);
}

I hope I provided every info necessary. If you still need something else from my code just tell me.

Answer 1

LinkedList remove shifts any subsequent elements to the left (subtracts one from their indices). Returns the element that was removed from the list. So, if you are doing remove it might not have the bullet or player that you are access. You can just highlight your class and method signatures, myself or someone can help you in designing it better.

Answer 2

Even without multiple thread access, your implementation fails because the outer for-loop does not take into account that you might remove objects while iterating. So the counter s is always incremented, even when an object is removed due to collisions. For that reason, every successor of an object that gets removed, is skipped in your implementation.

(Explanation: If object at position s is removed, all successive objects move one position to front, hence object at position s + 1 moves to position s. But as your for loop increases s afterwards, the new object at that position is skipped).

You should always favour iterators over for loops with counters. They offer safe removal of objects and are more flexible in case you want to change the implementations of your collections. (In your case, an iterator offers far better performance than a counter as index based operations on a LinkedList require sequential traversal of the list).

This is an example implementation that has some more improvements and should work in your case:

private void checkForCollision() {
    // use iterator instead of index based for loop
    while (Iterator<GameObject> iter = objects.iterator(); iter.hasNext();) {
        GameObject object = iter.next();
        // in case the object is the player, we can skip all collision tests
        if (object == getPlayer()) continue;

        // use implicit iterator instead of index based for loop
        for (Bullet bullet : bullets) {
            if (object.getCol().getBounds2D().intersects(bullet.getCol().getBounds2D())) {
                iter.remove(); // remove object via iterator
                break; // no need to check the rest => leave inner loop
            }
        }
    }
}

Note: This implementation is not thread safe, so in case your objects (either the collections objects or bullets or any object inside them) are accessed by multiple threads, either synchronization or thread-safe collections are required!