I have written a Snake game in Java, and it works awesome for the most part. The problem is if I try to move the snake too fast, the keyboard inputs are resolved before the game has a chance to update itself. The main loop is running on a thread.
For example, suppose the snake is moving to the right horizontally across the screen. If I hit down-arrow and left arrow, the snake head should move down one square, then continue on to the left with the rest of the body following, which happens just fine as long as I don't hit down-arrow left-arrow really fast. I think I understand why this is happening but I am not sure what to do to fix it. The thread sleeps for 100ms between movements, so I think what's happening is if I'm inputting the direction changes too fast, the game wakes up and only sees that the direction is left (it missed out on the "down" command) and sees that part of the snake is there and ends the game. I hope I'm explaining this well enough, I'll try to attach my code if it lets me (although it's not the most elegant code you'll ever see ).
Any way around this, to force the game to resolve the keyboard commands in order?
Thanks a bunch!
Ok looks like I can't attach the source file. I don't think you'll really need it, but if you do I will copy and paste it. Thanks!
We may need to see the code, since all we can do it talk generally about how you should do things.
First: You should not be sleeping in the Event Dispatch Thread - No idea if you are, but if you are, then you should stop.
- One of the things you could do is each keyboard key press, don't assign a single variable, but rather create a List of all values gathered since the last time the snake moved. Then when the Snake moves, you:
--- 1) Make a copy of the List of moves.
--- 2) Create a new List to hold moves between now and the next move
--- 3) Iterate over the copy and perform the actions in order.
But this may not be what you really want. A user might be able to spoof input or direct the snake really really fast (cheating), perhaps letting the snake move faster than the rate you want. So the fact that you only read one movement for each action cycle may be a good thing. But that is up to you to decide.
Joined: Feb 19, 2012
Thank you for that, I really like that idea of creating a list of movements and iterating from the list, that seems like it would work perfectly. I will post my code (I'm sorry if it's ugly or inelegant, I kind of hacked my way through it). I have read about the EDT, I'm not 100% sure if my creating a separate thread is interfering with that at all.
Any constructive advice/criticism is totally appreciated and welcome! I know this code could probably be cleaned up a bit, especially now that I'm looking over it, but at the time I just wanted it to work. I thought this was going to be easier than it turned out to be
Working code is always a good start Once you get that you can then go back and re-factor to make it clean. So these comments are only meant as constructive suggestions, not trying to put the code down or anything.
First, since this is the Threads and Synchronization forum, I will begin with that. You are sleeping in your processing/background thread, not the GUI thread - this is a good thing. You don't want to change that. I also like that you are using one of the collections in the java.util.concurrent package for sharing the snake's segments from one thread to the other - I like even more that you are using a Queue implementation. My only issue with that is that I think you are using the wrong queue: a ConcurrentLinkedQueue will probably better match your use case. The BlockingArrayQueue is really used for a produce-consumer situation where you expect one thread to be destructively consuming the Queue while another thread builds it. That doesn't really match your use case. I don't see a scenario where the difference will have an effect on your code - you protected yourself against it pretty well, but I think the intent would be clearer.
The rest of what I will address is design related. I will take them in pieces:
- There is a lot of repeated code. When you see that you should do your best to refactor it into a method, or re-arrange it so that you only need to call it once. An example is the moveSnake() method. There are a number of conditions (one for each direction), but all of the code in them is identical except the initial increment of the X or Y position. So you could change it to something like this:
That's 25ish lines of code shorter than the method you originally had, does exactly the same thing in the same way, and includes more white space and comments. Every line of code you can remove means fewer bugs.
Note that I used two comments in the method to describe what each section was doing. The fact that I could easily break the task in half like that indicates that could (should?) be made into two separate methods. But that is more your call. Short methods that do one thing makes troubleshooting and adding/changing features easier in the long run.
This method is just one example of the problem, I saw it in several places, so be sure to review the code to see where else you could consolidate the code into methods and fewer lines (if you have 2 or 3 lines of code that you repeat 2 or 3 times, consider making a method out of them, even if you have to pass several parameters in to get it correct).
- Most importantly: You have basically one super class which does everything. It is your GUI, holds your data, runs the game logic, etc... It has some inner classes that sort of break the functionality up, but you don't go far enough. Try to break your code up into tiny little pieces, where each piece can take care of itself. For example, you have a class for the Snake's XY segments, but you do a lot of work on the segments in the main class.
My suggestion would be to see if you could pull all of the methods and functionality out of the main class - make a new class which:
-- holds a collection of the snake segments
-- runs the thread which moves the snake
-- has the methods for adding and removing segments
-- has the methods for initializing the length of the snake
-- has the methods for clearing the snake (at game's end)
-- has the methods for moving the snake
-- has the methods for setting speed and length of the snake
Essentially, does everything that the snake requires except drawing it. Then come up with a third class which knows how to draw the snake, and has a public void paintSnake(Graphics g, Snake s) method which your main class can delegate to.
If you do this successfully, think about doing it for all the other 'things' in your game: Food, Wall, GameBoard, Scoreboard. Each Thing has a class for all the data it needs and the methods to manage that data, and a second class which knows how to paint the Thing. Then your main class simply manages these Things (makes sure each Thing has access to the other Things it needs, that they get created when required, started, stopped, destroyed, etc...), and delegates painting to the ThingPainter classes.
Joined: Feb 19, 2012
Wow, man, thank you so much. Much more than I expected, thank you, really! I will work on cleaning up the code, for sure, your suggestions are awesome and will help me with future projects as well.
Do you still think, having seen my code, that an input queue would be the best way to go to solve the problem with the input happening too fast? It sounds like it might, so for the direction checks, I would add another input to the queue, and when the thread wakes up process them all in the right order?