Error in my restart method..

Newbie & Debugging Questions
#1

Hi,
this is the restart method:


	public void Restart() {
		object.clear();
		LoadImageLevel(loader.loadImage("/level.png"));
		cam.setX(-32);
	}

and this is the LoadImageLevel method:


	public void LoadImageLevel(BufferedImage image) {
		int w = image.getWidth();
		int h = image.getHeight();

		for (int xx = 0; xx < h; xx++) {
			for (int yy = 0; yy < w; yy++) {
				int pixel = image.getRGB(xx, yy);
				int red = (pixel >> 16) & 0xff;
				int green = (pixel >> 8) & 0xff;
				int blue = (pixel) & 0xff;

				if (red == 0 && green == 0 && blue == 100) {
					addObject(new Background(0, 0, ObjectId.Background)); // background
					addObject(new Block(xx * 32, yy * 32, 0, ObjectId.Block));

				}
				if (red == 100 && green == 60 && blue == 0) { // / block
					addObject(new Block(xx * 32, yy * 32, 0, ObjectId.Block));

				}
				if (red == 100 && green == 50 && blue == 0) { // /block 1
					addObject(new Block(xx * 32, yy * 32, 1, ObjectId.Block));
				}
				if (red == 100 && green == 40 && blue == 0) { // /block 2
					addObject(new Block(xx * 32, yy * 32, 2, ObjectId.Block));
				}
				if (red == 100 && green == 30 && blue == 0) { // /block 3
					addObject(new Block(xx * 32, yy * 32, 3, ObjectId.Block));
				}
				if (red == 255 && green == 255 && blue == 255) { // /block 4
					addObject(new Block(xx * 32, yy * 32, 4, ObjectId.Block));
				}
				if (red == 0 && green == 255 && blue == 33) { // /block up
					addObject(new Block(xx * 32, yy * 32, 5, ObjectId.Block));
				}
				if (red == 0 && green == 245 && blue == 33) { // /block up1
					addObject(new Block(xx * 32, yy * 32, 6, ObjectId.Block));
				}
				if (red == 128 && green == 128 && blue == 128) { // / box
					addObject(new Box(xx * 32, yy * 32, this, ObjectId.Box));

				}
				if (red == 150 && green == 150 && blue == 150) { // /
																	// changedable
																	// block
					addObject(new ChangeableBlock(xx * 32, yy * 32, this,
							ObjectId.ChangeableBlock));

				}
				if (red == 255 && green == 0 && blue == 0) { // /red button
					addObject(new Button(xx * 32, yy * 32, 1, this,
							ObjectId.Button));
				}
				if (red == 0 && green == 0 && blue == 245) { // /blue button
					addObject(new Button(xx * 32, yy * 32, 2, this,
							ObjectId.Button));
				}
				if (red == 255 && green == 30 && blue == 30) { // ///ex
					addObject(new Ex(xx * 32, yy * 32, this, ObjectId.Ex));

				}
				if (red == 0 && green == 0 && blue == 255) { // / player
					addObject(new Player(xx * 32, yy * 32, this,
							ObjectId.Player));

				}

			}
		}
	}

Now,the wired thing is that 50% of the times it works and the other 50% it crashes with this error:


Exception in thread "Thread-2" java.lang.NullPointerException
	at com.shuster.killtheex.objects.Box.tick(Box.java:37)
	at com.shuster.killtheex.window.Handler.tick(Handler.java:36)
	at com.shuster.killtheex.window.Game.tick(Game.java:84)
	at com.shuster.killtheex.window.Game.run(Game.java:66)
	at java.lang.Thread.run(Unknown Source)

And every time it happens with a different object…
Thanks alot!!

#2

Can I see the addObject() method?

#3

Could we see the method “box.tick”?

#4

This is almost certainly a data race, esp. with the additional evidence in that “Thread-2” is reporting the discrepancy. Post any code related to threading.

It looks like restart clears the object list while Thread-2 is reading the list, causing this error. That’s my guess. Test it by synchronizing access to the list:

public void Restart() {
      synchronized (object) {
            object.clear();
      }
      LoadImageLevel(loader.loadImage("/level.png"));
      cam.setX(-32);
   }

And do the same wherever you read from the list, probably Handler if I know this tutorial.

#5

I replaced it with my restart method but that still doesnt work…
I read from that list in the LoadImageLevel which I posted in the topic…I didnt really understand where to put it.


	public void addObject(GameObject object) {
		this.object.add(object);

	}



	public void tick(LinkedList<GameObject> object) {
		y += velY;
		if (falling) {
			velY += gravity;
		}
		if (velY > MAX_SPEED) {
			velY = MAX_SPEED;
		}

But that doesnt really matter because it crahses each time with a different object…

#6

Your Handler.tick() looks something like this, no? (I’ve seen that tutorial before)


public void tick() {
    
    for (GameObject gobj : object) {
        gobj.tick();
    }
    // ... other stuff maybe
}

Change it to


public void tick() {
    
    synchronized (object) { // prevents clear() from being called while looping over the list
        for (GameObject gobj : object) { // if you are using linked list, for the love of god don't use the c-style loop
            gobj.tick();
        }
    }
    // ... other stuff maybe
}

synchronized has to be used on both ends or it doesn’t do anything.

Also, ditch the LinkedLists, they’re usually misused and if you’re using the code straight from that tutorial, see this: http://www.java-gaming.org/topics/handling-blocks-entities-separately/33642/msg/316299/view.html#msg316299

I’m fairly confident that your loading code has nothing to do with the error, although it wouldn’t hurt to synchronize it as well.

#7

I have added the synchronized to the tick method and I think it reduced the number of times that it crahes but it still happens…
and I’m not sure how to change the LinkedList with the array list…
Is it okay if I send you the source code so you can take a look at it?
Thanks for your help!!

#8

Is the error still thrown from Handler.tick() --> Box.tick() or is the trace different?

#9

I got this error this time :


Exception in thread "Thread-2" java.lang.NullPointerException
	at java.util.LinkedList.node(Unknown Source)
	at java.util.LinkedList.get(Unknown Source)
	at com.shuster.killtheex.window.Handler.render(Handler.java:52)
	at com.shuster.killtheex.window.Game.render(Game.java:106)
	at com.shuster.killtheex.window.Game.run(Game.java:69)
	at java.lang.Thread.run(Unknown Source)

You can see here all of my code if it helps

#10

Let me guess (haven’t looked yet) that you iterate over the list in Handler.render() as well? That needs to be synchronized too.

#11

Yes, but I’m using that list almost in all my code >.>
When I saw the tutorial I thought that there must be a better way to do it but I didnt know how

#12

Yeah, there’s a fair bit “wrong” with that code… first thing is if you find yourself repeating stuff that much, something could probably be done better:

public void render(Graphics g) { 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Background) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Block) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Ex) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Box) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Player) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.ChangeableBlock) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Button) { 
                tempObject.render(g); 
            } 
        } 
        for (int i = 0; i < object.size(); i++) { 
            tempObject = object.get(i); 
            if (tempObject.getId() == ObjectId.Bodypart) { 
                tempObject.render(g); 
            } 
        } 
    }

Can be reduced to:

public void render(Graphics g) { 
    for (GameObject tempObject : object) {
        if (tempObject.getId() == ObjectId.Background) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Block) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Ex) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Box) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Player) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.ChangeableBlock) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Button) { 
            tempObject.render(g); 
        } else if (tempObject.getId() == ObjectId.Bodypart) { 
            tempObject.render(g); 
        }
    } 
}

Which is still pretty bad, instead you should add a boolean renderable; to GameObject, and just check that:

public void render(Graphics g) { 
    for (GameObject tempObject : object) {
        if (tempObject.renderable) { 
            tempObject.render(g); 
        }
    } 
}

And if you are modifying the list in a different thread (probably the GUI thread), then you have problems, as you have experienced. You can either continue trying to make it all thread safe with locks and such, or simply shunt tasks off to the main thread:
(a little messy, but bear with me)

PriorityBlockingQueue<Runnable> taskQueue = new PriorityBlockingQueue<>(10);

public void tick() {
    /*
     *  Run any scheduled tasks that came in from the other thread
     */
    while(!taskQueue.isEmpty()) {
        taskQueue.take().run();
    }

    // now do the normal stuff
    for (GameObject tempObj : object) { 
        tempObject.tick(object); 
    } 
}

public void Restart() {
    /*
     *  We need something done, but not in this thread, we'll add it to the 'to-do list'
     */
    taskQueue.add(new Runnable() {
        public void run() {
            object.clear();
            LoadImageLevel(loader.loadImage("/level.png")); 
            cam.setX(-32); 
            counter = 0; 
        }
    });
}

Also, as a general rule, LinkedLists suck. (at least until you have the experience to judge whether or not they are correct to use in a given situation):

[icode]-public LinkedList object = new LinkedList();
+public List object = new ArrayList(); // should be able to just drop this in without any other changes[/icode]

Whew! That’s a long post! There’s more, but then there’s always more…
I would take that tutorial as something to learn what not to do from if I were you.

#13

Wow, thank you very much for all of your help!!I cant thank you enough, I really appreciate it!!

About the render , I used so many loops because I wanted it to render in layers, for example at first render the background, than all the blocks, and etc… In this way the first thing that is checked is the first to be rendered.

I’m sorry, but the PriorityBlockingQueue was too complicated for me to understand, I tried to put it instead of my handler.restart and hander.tick, but when I tryed to restart nothing happend…

And when I used the Array list :


public List<GameObject> object = new ArrayList<GameObject>();

instead of the LinkedList I got this error:


The type List is not generic; it cannot be parameterized with arguments <GameObject>

And again thanks for your help, your awesome!

#14

Change the List<> to java.util.List<>, you must be importing java.awt.*, which has it’s own List class, leading to confusion.

[quote]I used so many loops because I wanted it to render in layers
[/quote]
Ah, fair enough, although what about this:

Add int z; to GameObject, represents the z-order of an object, use that instead of ID
Also add this:


public static final Comparator<GameObject> COMPARATOR = new Comparator<GameObject>()  {
    @Override
    public int compare(GameObject o1, GameObject o2) {
        return Integer.compare(o1.z, o2.z);
    }
}

Then to render, you sort the objects by z-order and render them in that order:


public void render(Graphics g) { 

    Collections.sort(object, GameObject.COMPARATOR);

    for (GameObject tempObject : object) {
        if (tempObject.renderable) { 
            tempObject.render(g); 
        }
    } 
}

That way you have an effectively infinite number of layers available, all without having to create and check new ID types.
You could even have set of named z-values:


public class ID {

    public static final int BACKGROUND = 0;
    public static final int BLOCK = 1;
    public static final int BOX = 2;
    ...
}

So everything remains clear when creating GameObjects:

[icode]Box box = new Box(… … ID.BOX); // looks about the same as before![/icode]

#15

WOOT now it works!!
Now that I changed the LinkedList to an array list the performace has been improved and it doesnt crash when I restart
Thanks :slight_smile: