SFML community forums
General => SFML projects => Topic started by: bglaze on October 17, 2011, 02:08:45 am
-
I've been programming in C++ for about 3 weeks. About a week into it, I stumbled upon SFML! I wanted my first project to be a game that my kids could play. I also wanted to make something I was familiar with, so I chose to make a snake game.
When my wife and I were dating in high-school, we use to compete on her cell-phone playing the snake game, so this is also something we can have fun with together.
This game has 2 modes: NORMAL and DRAW mode. Normal mode is classic snake game. Draw mode is what I made for my kids (they are 3 and 4 years old)--your snake never dies, and you grow at a very fast pace.
There are also 3 speeds. The game saves your last speed to a file, so it will load that same speed the next time you launch the game.
I am definitely open to criticism, seeing that I am new to C++ in general. I am out to better my skills and logic techniques, so keep the criticisms flowing! Thanks!
So far I only have a Windows version.
[Edit (10/21): I have implemented some changes suggested by the people on this post. Also, the game now tracks the person who holds the High-Score; and the game now starts in a paused state]
You can download it here: http://bglz.net/files/cpp/projects/Yellow_Snake.zip
And the source code and CB project here: http://bglz.net/files/cpp/projects/Yellow_Snake_src_10-21.zip
Here are some screenshots.
(http://bglz.net/files/cpp/projects/ys_images/ys1.jpg)
Draw Mode:
(http://bglz.net/files/cpp/projects/ys_images/ys2.jpg)
-
At a very quick glance I stumbled upon this:
sf::Sleep(1.0f/FPS);
You could replace this with something like
if(1.0/window.GetFrameRate() < 1.0/FPS) {
sf::Sleep(1.0/FPS-(1.0/window.GetFrameRate()));
}
It calculates the actual delay between frames for desired FPS. Say your target FPS is 60, the desired delay between frames is 1.0/60 = ~0.01667 seconds. Now, if your game runs at 80 FPS, the actual delay between each frame is 1.0/80 = 0.0125 seconds. Because 0.01667 - 0.0125 is 0.004 seconds, the program sleeps for approximately 4 milliseconds to increase the frame time to ~0.01667, and making FPS to a approximate of the targeted 60. However, note that this isn't an ideal approach either, because if FPS fluctuates a lot between frames, this can't provide consistency needed to cope with it, but for this case it should help things a bit.
Do you have compiler optimizations turned on? On what kind of a machine do you run the code?
There's lots of room for improvement in the code, however, of what I can tell it's hardly anything complex which a compiler shouldn't be able to optimize away anyway(e.g. there are multiple if()'s inside a loop, which all could be avoided by doing comparisons and branches before entering the loop).
You're also initializing a sf::Event local variable inside the main loop of game::Run() for some reason. You could make it a member variable of the class and avoid always generating a new variable. This isn't likely to hurt your performance though, but it's horribly bad code when you think about it. Local variables get "freed" at the end of the scope, so you keep initializing and freeing the variable for nothing.
After you handle the events, you test if the snake eats food with Terrarium[0], this is fine and good. However, after that, you loop through the snake and do the following per every node of the snake:
CollideBody() to check if the head has it part of the body
EatFood() to check if any of the nodes have eaten food
Update() to move the nodes of the body
Draw
The bolded one makes no sense. The head has already traversed the exact positions of the nodes already and they are certainly free of any food, so there's no way for the rest of the body to stumble upon food if you change your food::NewFood function to make sure the new food does not spawn on locations of the snake's body. I also suspect that the EatFood() function is the root of all evil here as it calls the function which does pixel perfect collision, which is definitely slow.
Also the CollideBody() function could be changed to loop through the snake body inside the function rather than calling the function per every node of the snake's body. This wouldn't only be good design but also would avoid the function call+return overhead(which isn't much, but still something you should avoid in this case).
Also initializing the Terrarium vector with a big enough value to hold the snake from the beginning without having to resize every now and then can save you from a few hiccups which happen because when the vector gets full, it has to allocate new memory(usually double size of the old) and copy the contents to that new memory and free the old memory. This usually happens when the vector reaches size of 2, 4, 8, 16, 32, ... etc. It only gets "visible" in forms of performance loss when the vector contains lots of data to copy around.
As always though, these are mere guesses and hints for you to take a closer look at, I am not able to run the code and as such it's nearly impossible to get an actual understanding of what truly holds you back in terms of performance. The only real way to measure that would be to run the code and use a performance profiler to get an idea what functions are being called the most and what functions take longest to finish, and as such focus on those slow/frequent functions in terms of optimizing. Usually in these cases the popular 80-20 rule applies; 80 % of time is spent by 20 % of code. Locating and focusing on that 20 % is the first and most important thing to do when considering code performance and optimizing. As always, measure the performance before and after you make any changes.
..and remember, after all the most important thing to do is to measure twice and cut once(..and curse three or four times).
-
I am on my way to work, so I haven't had time to implement those changes, and no I wasn't away of compiler optimizations until I read your post, but I will look them up and make sure they are turned on!
All your advice is spectacular! I plan on addressing each of those issues and learning from them how to code better for the future. Thank you so much for taking the time to toss some advice my way!
Anything further that anyone else sees is also very welcome. My goal is to be a good coder, not just to have a good snake game. So I am absorbing whatever advice I can get.
Thanks!
BG
-
You're also initializing a sf::Event local variable inside the main loop of game::Run() for some reason. You could make it a member variable of the class and avoid always generating a new variable. This isn't likely to hurt your performance though, but it's horribly bad code when you think about it. Local variables get "freed" at the end of the scope, so you keep initializing and freeing the variable for nothing.
In fact, the opposite is horrible, namely having variables with a much wider scope than actually needed. The event is not part of the object's state, so don't make it a member.
Local variables are mostly the better choice, because you directly see their scope and avoid unwanted side effects and accesses to the variable. sf::Event even is a POD, so no initialization is performed at definition. Always declare variables as local as possible, except if you have measured a relevant performance gain.
Also initializing the Terrarium vector with a big enough value to hold the snake from the beginning without having to resize every now and then can save you from a few hiccups
Just as a supplement: std::vector::reserve() exists for exactly that purpose.
-
You're also initializing a sf::Event local variable inside the main loop of game::Run() for some reason. You could make it a member variable of the class and avoid always generating a new variable. This isn't likely to hurt your performance though, but it's horribly bad code when you think about it. Local variables get "freed" at the end of the scope, so you keep initializing and freeing the variable for nothing.
In fact, the opposite is horrible, namely having variables with a much wider scope than actually needed. The event is not part of the object's state, so don't make it a member.
Local variables are mostly the better choice, because you directly see their scope and avoid unwanted side effects and accesses to the variable. sf::Event even is a POD, so no initialization is performed at definition. Always declare variables as local as possible, except if you have measured a relevant performance gain.
This is actually true, making it a member of a class is even worse than having it as a local. Defining it outside the scope of the loop(before the loop) is the best approach indeed.
-
I would even say the best approach is to keep it as local as possible, i.e. inside the loop. Then it is clear where the variable is used, code becomes more readable, and it can not happen that it is accidentally accessed outside the loop.
The construction and destruction of a sf::Event requires no time, as stated above it is a POD type. Okay, it might need a stackpointer movement, but on the other side it is more likely to be kept in a register. In the end, processors optimize a lot, while micro-optimizations on user side are rarely helpful, especially not if they imply other disadvantages (premature optimization). Of course, things look different if we talk about heavy objects with thousands of bytes and expensive constructors.
As already mentioned, you should always declare variables as local and as late as possible, except if there is a good reason not to do so :)
-
Lots of great advice; thanks a lot guys for looking at this and commenting!
-
Calmatory,
I don't quite understand which of the if-statements in my loop could be moved elsewhere. I have examined them all, and I don't see to be able to find a solution that could take place out of the loop. Any advice on which particular ifs I should take a closer look at?
Thank you!
-
Also it seems like adding the following code:
if(1.0f/Window.GetFrameTime() < 1.0f/FPS){
sf::Sleep(1.0f/FPS-(1.0f/Window.GetFrameTime()));
}
Is really what is making a difference in my newly found speed increase! However, it's weird, this 'if' block doesn't compile correctly on all of my computers even though I am using the same versions of Code::Blocks, Mingw, and SFML. On my work computer, the 'if' statement is ignored, and my game runs at 1200+FPS (which makes my snake plant himself permanently into a wall). At home it seems to function very well, but my FPS stays constant at 59-60 FPS regardless what my FPS variable is set to. If I set FPS to 30, I still get 60 FPS, if I set it to 1000, I still get 60 FPS... This seems very odd to me.
-
Actually, nevermind... that Sleep function wasn't taking affect at all. I had Vertical sync enabled on code at home, and it was keeping my frame rate at 60 fps pretty much constantly. That's it didn't matter what FPS was set to...
-
I would even say the best approach is to keep it as local as possible, i.e. inside the loop. Then it is clear where the variable is used, code becomes more readable, and it can not happen that it is accidentally accessed outside the loop.
The construction and destruction of a sf::Event requires no time, as stated above it is a POD type.
...
As already mentioned, you should always declare variables as local and as late as possible, except if there is a good reason not to do so :)
Nexus is right. Consider it like this
void SomeFunction () {
some code here
SomeType temporaryObject;
for(int i = 1; i <= n; i++ ) {
temporaryObject = some value;
more code
}
some more code here
}
In this snippet, temporaryObject is constructed outside the loop and reused. This results in
1 construction
1 desctruction
n assignments
Furthermore, temporaryObject exists in the bit of code "some more code here". This is what Nexus meant by Then it is clear where the variable is used, code becomes more readable, and it can not happen that it is accidentally accessed outside the loop.
Compare to this
void SomeFunction () {
some code here
for(int i = 1; i <= n; i++ ) {
SomeType temporaryObject(some value);
more code
}
some more code here
}
In this snippet, temporaryObject exists only in the scope each iterate of the loop. This results in
n constructions
n destructions.
So it's really a toss up between which takes longer, construction/destruction, or assignments. Seeing as sf::Event is a POD type, it'll be faster to use the second option, and again it makes your code a lot cleaner, and easier to read.
-
So it's really a toss up between which takes longer, construction/destruction, or assignments. Seeing as sf::Event is a POD type, it'll be faster to use the second option,
I fail to see how it could be faster. I'm pretty sure they'd both compile to practically the exact same thing.
-
Yeah, entirely possible. Probably get someone who knows a bit more than me to clarify :)
-
you've been studying programming for 3 weeks and have already wrote a game? :( Thats pretty impressive for you sir :)
-
If you play Draw mode too much, like I did, the red square starts jumping around everywhere until it finds a place where it isn't touching a snake node. You could fix this by making the red square transparent until it finds a spot not touching a snake node.
Another thing is that the game starts right away when you run it. IMO the game should be paused when opened, then when you press enter the game starts. A way to pause the game while playing would be great too.
Other than those two things, the game is great! I decided to fill the screen in Draw mode. Gahh, that took a while.
EDIT: The pause screen could just be the game in the background, not moving, with "Yellow Snake!" in front, like when you start the game.
-
meissner61: Thank you for the compliment! But I did have some experience in Python before I started learning C++, so I was familiar with classes, functions, containers, variables, etc... C++ is definitely a different animal, but I am finding it to be a blast. And I have actually found some things in C++ come easier to me than they did or still have in Python. I think I am sold on this language for a while, and I'm sold on SFML for sure.
Pyrius: Excellent suggestions, and both of them should be very easy to implement! Thank you so much for playing it and commenting, and I will implement both of those ideas today!
[Edit: Pyrius, I implemented both of your suggestions, which were dead on. Others have agreed wholeheartedly that the game needed to start up in a Paused state. Also, solving the food issue (where you can see it spawning and jumping around if you have a really long snake) was as easy as adjusting the draw order so that the food draws before the body. This way it will always draw underneath the body if it spawns in the same piece of the grid. Again, thanks for those suggestions.]
-
I have really appreciated all the input on my game! I updated the original post with the most current version and the most current source-code.
Any and all criticism and suggestions are welcome!! I have no pride concerning my programming abilities; all I am seeking to do is improve. So criticize away!! =)
Thanks to everyone who has helped me thus far!
Oh, and my snake now animates (or blinks rather), so maybe i should rename it Yellow Disco Snake.
-
I played it again, and the blinking seems kind of strange. Maybe a way to toggle the blinking?
I also noticed that when you restart the game or change the game mode, the window closes and reopens. You could create an enum to hold the different game modes and have the main loop act differently depending on which game mode is currently stored in your game mode object (with switch/case). Instead of restarting the window with the new game mode, you'd just constantly check if the user presses F12 and change the game mode accordingly. This would probably make it easier to implement new game modes if you'd ever want to.
EDIT: The blinking is too frequent; the nodes are orange most of the time.
-
Pyrius:
Thank you! The game no longer blinks by default. I made it so you can toggle it on by hitting F9, however, this isn't so much a feature that I'll make known any longer as it is a way for me to experiment with tiled_images and animations. I also slowed down the blinking a tad, just in case someone did want to turn it on.
Also, thanks for the advice on how the game restarts, etc... That will take a bit more to implement, so I will look into that today and see what I can come up with.
I appreciate your advice!
-
Pyrius:
I implemented both of the changes you suggested. Restarting and Changing modes no longer restarts the entire program. It just calls a Reset() member function which resets all the settings based on the current mode (or switches modes and resets accordingly).
Thanks for the suggestion!
-
:) It's great now! It seems much more professional. I can't think of any other things that can be improved right now, but I haven't looked at the code yet.
-
Wow thanks. Those suggestions were definitely needed.
-
I've played your game yesterday, nice work! Especially because you're using c++ for just about a month, and you've already came up with a game, and also because the source is not a sphagetti code what I've expected. Congratulations! :)
-
Early,
Thank you for that comment! I am sure there is a lot about my code that could be done in more of "the C++ way," but I'm sure I'll continually improve it over time. However, to hear that it was at least coherent for someone else to read is very encouraging! Thanks you so much!
-
I've played your game yesterday, nice work! Especially because you're using c++ for just about a month, and you've already came up with a game, and also because the source is not a sphagetti code what I've expected. Congratulations! :)
What have you against the Italian coders? :'D
I don't think that outside Italy there are only good programmers.
-
I see I haven't commented on Yellow Snake itself yet, sorry ;)
Very nice game for your first project! It works very well. And it's funny you connect some personal anecdotes of your life with it :)
I remember one of my first games (or even the first?) was a snake clone, too... At the time I used SDL. My code was horrible, I used a single .cpp file, goto statements and a 500 line main() :D
-
<offtopic>
What have you against the Italian coders? :'D
I don't think that outside Italy there are only good programmers.
You know what, actually I love sphagetti! :D
I remember one of my first games (or even the first?) was a snake clone, too... At the time I used SDL. My code was horrible, I used a single .cpp file, goto statements and a 500 line main()
My heart skipped a beat when I've read 'goto'! :)
</offtopic>
-
I remember one of my first games (or even the first?) was a snake clone, too... At the time I used SDL. My code was horrible, I used a single .cpp file, goto statements and a 500 line main() :D
Thanks for the encouragement, Nexus! My approach started out far more condensed and sloppy (not to claim that my current version isn't sloppy). However, with the help if this thread, I've been able to learn some better techniques, so I must give the credit to those who have helped me here.
I am going to be working today to port this game over to SFML 2.0, because I've begun using it since writing this, and I like it a lot!