Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Yellow Snake - w/Source (Updated 10/21)  (Read 13972 times)

0 Members and 2 Guests are viewing this topic.

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« 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.



Draw Mode:

Calmatory

  • Newbie
  • *
  • Posts: 16
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #1 on: October 17, 2011, 07:40:46 am »
At a very quick glance I stumbled upon this:

Code: [Select]

 sf::Sleep(1.0f/FPS);


You could replace this with something like

Code: [Select]

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).
The amount of effort we put into something arbitrary we do in our everyday lives is proportional to the amount we gain from it. It's fascinating how this applies to everything in our lives. Your task is to try to enjoy and make the most out of it.

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #2 on: October 17, 2011, 01:40:14 pm »
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

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Yellow Snake - w/Source (Updated 10/21)
« Reply #3 on: October 17, 2011, 02:11:40 pm »
Quote from: "Calmatory"
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.

Quote from: "Calmatory"
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Calmatory

  • Newbie
  • *
  • Posts: 16
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #4 on: October 17, 2011, 04:30:39 pm »
Quote from: "Nexus"
Quote from: "Calmatory"
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.
The amount of effort we put into something arbitrary we do in our everyday lives is proportional to the amount we gain from it. It's fascinating how this applies to everything in our lives. Your task is to try to enjoy and make the most out of it.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Yellow Snake - w/Source (Updated 10/21)
« Reply #5 on: October 17, 2011, 07:05:22 pm »
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 :)
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #6 on: October 18, 2011, 02:13:06 am »
Lots of great advice; thanks a lot guys for looking at this and commenting!

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #7 on: October 18, 2011, 05:09:37 am »
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!

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #8 on: October 18, 2011, 05:53:41 am »
Also it seems like adding the following code:

Code: [Select]

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.

bglaze

  • Newbie
  • *
  • Posts: 46
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #9 on: October 18, 2011, 06:09:56 am »
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...

sbroadfoot90

  • Jr. Member
  • **
  • Posts: 77
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #10 on: October 18, 2011, 11:26:54 am »
Quote from: "Nexus"
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

Code: [Select]
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
Quote
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

Code: [Select]
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.

Disch

  • Full Member
  • ***
  • Posts: 220
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #11 on: October 18, 2011, 04:50:00 pm »
Quote
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.

sbroadfoot90

  • Jr. Member
  • **
  • Posts: 77
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #12 on: October 18, 2011, 11:40:20 pm »
Yeah, entirely possible. Probably get someone who knows a bit more than me to clarify :)

meissner61

  • Newbie
  • *
  • Posts: 26
    • View Profile
    • Email
Yellow Snake - w/Source (Updated 10/21)
« Reply #13 on: October 21, 2011, 08:49:48 am »
you've been studying programming for 3 weeks and have already wrote a game? :(  Thats pretty impressive for you sir :)

Pyrius

  • Newbie
  • *
  • Posts: 39
    • View Profile
Yellow Snake - w/Source (Updated 10/21)
« Reply #14 on: October 21, 2011, 02:28:17 pm »
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.

 

anything