2d Game Making blog...

[Beernutts]

Hey All,

I've started making a 2d game using SFML, and i plan on documenting it to hopefully help others who want to make games:
http://2dgamemaking.blogspot.com/

Anyway, just wanted to share it here.

Thanks!

[Nexus]

Good idea!

But your codes are full of memory leaks. If you use new, you must also use delete. However, the better option is to avoid manual memory management at all, i.e. use automatic objects or smart pointers instead of raw pointers (why not std::map<std::string, sf::Image> instead of std::map<std::string, sf::Image*>?)

And I personally would use fewer global variables, they bring a lot of problems even if seeming comfortable in the first term. And last, you should declare variables like sf::Event as local as possible, and use const instead of #define for constants

[Beernutts]

Hi Nexus,

I appreciate the feedback, but I don't agree with everything you said.  I am certainly looking for things I could do better with SFML or other SDK API's, but I think you're nit-picking with some of the details you offered.

Good idea!

But your codes are full of memory leaks. If you use new, you must also use delete. However, the better option is to avoid manual memory management at all, i.e. use automatic objects or smart pointers instead of raw pointers (why not std::map<std::string, sf::Image> instead of std::map<std::string, sf::Image*>?)

I decided to see if you were right that it was "full of memory leaks".  Leak meaning, memory will continually be allocated and not freed, thus the leak will eventually cause out of memory.

Here is a list of all the "new" in the code:

GameSound.cpp 62 38:                SoundMap

[SoundStr] = new sf::SoundBuffer();
GameSound.cpp 130 18:    pSoundInst = new sf::Sound();
SmashPC.cpp 36 13:    gpMap = new SmashPcMap("", gu32ScreenX, gu32ScreenY, &App);
SmashPC.cpp 39 19:    gpOurPlayer = new SmashPcPlayer(&App, gpMap->GetSpace(), cpv(200, 200));
SmashPcPlayer.cpp 40 16:    mpSprite = new sf::Sprite();
Utiliies.cpp 116 31:        gImageMap[FileName] = new sf::Image();[/list]

The sound buffer is being allocated  once per loading of a sound file.  So, that's not a leak.
The Sound Instance is loaded once per use, and deleted when it's finished playing (or stopped).  not a leak.

The Map and Player are allocated once, and that's it.  Not a Leak.

Sprites are allocated once per SmashPcPlayer instance, which there's one, and deleted when the instance is deconstructed. Not a leak.

And images are allocated once per image file.  Not a leak.

Now, it's true I don't delete all the objects when the game is closed, and that's bad, I know.  I'll be sure to add that (it's a peeve of mine too).  But, it's not a leak, since the OS will clear all the memory allocated in the program.

And I personally would use fewer global variables, they bring a lot of problems even if seeming comfortable in the first term. And last, you should declare variables like sf::Event as local as possible, and use const instead of #define for constants

I may not need them to be global and might make them local.

You're welcome to use const's  for numeric constants, I'll continue to use defines.  To each their own.

And, again, I think you are kind of nit-picking with the Event declaration.

With all that said, I'll still encourage criticism of the code.  No-one is ever good enough they shouldn't listen to what others have to offer.

Thanks!

[Nexus]

I know some of the suggestions are details, I also wrote "I personally would use"

But missing deallocations are a problem. It's not only about memory, but all kind of resources (network connections, file handles, etc.), which the OS is not guaranteed to close correctly. If it comes to  custom operations like writing log files, this is even more important. If you don't call the delete operator, destructors are not invoked, and important cleanup code may be skipped. Tools that analyze your program will always report leaks and hence become useless.

And since the RAII idiom makes it extremely easy to manage resources correctly, I see no reason not to use it. As a nice side effect, your code gets cleaner and more readable. And another advantage is that beginners  reading your tutorial won't believe this is the correct way of handling memory in general

Concerning the locality of sf::Event: In my opinion, it's not just a matter of taste. We recently had this discussion, where some arguments have been mentioned. But of course it's not a severe issue.

[Beernutts]

Hi again Nexus!

But missing deallocations are a problem. It's not only about memory, but all kind of resources (network connections, file handles, etc.), which the OS is not guaranteed to close correctly.

Yes, it is, and I plan on fixing it ("it" being not deallocating on program end), but I just didn't agree that is was "full of memory leaks."  You made it sound like the game was going to crash if it ran too long.

I don't have much experience with smart pointers as I come from a low-level embedded systems background where we handle our own memory, which I am comfortable with.  I'll take a look at it though.

Thanks again!

[Beernutts]

Updated blog to add bullets and point player towards the mouse:
http://2dgamemaking.blogspot.com

Later.

[Silvah]

Concerning the locality of sf::Event: In my opinion, it's not just a matter of taste. We recently had this discussion, where some arguments have been mentioned. But of course it's not a severe issue.

I'd go even further and, apart from making the sf::Event variable as local as possible, I'd extract the event processing into a function of its own. I'd rather have several small functions that do one thing each, than one big one that runs the world. Nice win for readability and maintainability. Also, maybe not in this case though, extracting out functions can reduce the repetition: instead of copying and pasting code snippets that differ in one thing but are otherwise the same, you call a function with different argument.

It may help the performance as well: the compiler has more freedom in optimizing these functions in a way that is most friendly for the target CPU, and it could do better than otherwise in alias analysis due to the guarantees provided by the language, such as scoping rules.

In the unlikely case the profiler shows that it's the bottleneck, you can force inlining of these functions (see the documentation of your compiler for how to do that, there's no standard way), so the resultant code looks like there were one big function, while the source is still clear and easy to follow (except for these FORCEINLINE macros ).


Now I'm waiting for someone to yell "premature optimization" because I've written that it may help the performance :lol: