Sign in to follow this  
Essel

[SDL] Problems referencing SDL_Surface-h/w, among other things.

Recommended Posts

So, I'm programming this 2D sidescroller, and was working on adding a map format for it, along with collision today. Well I had a background image that needed to be the exact size of the screen, but when I reference the h and w ints in the SDL_Surface class, it crashes the game. :P Code: http://www.teamgaben.com/sdl_game.rar [Edited by - Essel on January 18, 2009 11:53:33 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Essel
Well I had a background image that needed to be the exact size of the screen, but when I reference the h and w ints in the SDL_Surface class, it crashes the game.

Have you checked if you're actually pointing at a surface? (assuming its a pointer). If you try to get the w/h from a surface thats not there you'll get a segmentation fault and your program will crash.

Share this post


Link to post
Share on other sites
Normally, most people don't want to go crawling through files and files of sourcecode looking for someone else's bugs. Hey, we barely want to go crawling through sourcecode looking for our own bugs. [wink]

You are much more likely to get help in the future if you copy and paste the relevant* parts of your code in between two [ source ] and [ /source ] tags, to place them in a nice scrollbox for us to easily read.

*It's just as bad if you copy and paste your entire code. Only post the parts that are messing up.

I downloaded the code (which I rarely do), and there are 16 source files for me to shift through, when you sound like already know where it's crashing at.

Anyway, here is what I've found:




When you SDL_FreeSurface() a surface, it does not point it to NULL after it frees it.

As a result, when you do things like this: (CMap::Load() function in cmap.cpp)
if ( m_pSky->h < SCREEN_HEIGHT || m_pSky->w < SCREEN_WIDTH )
{
printf( "Sky background height or width too small!!!" );

SDL_FreeSurface( m_pSky );
}
else
if ( m_pSky->h > SCREEN_HEIGHT || m_pSky->w > SCREEN_WIDTH )
{
printf( "Sky background height or width too large!!!" );

SDL_FreeSurface( m_pSky );
}




m_pSky ends up pointing to garbage. You can't check it for NULL, because it's not pointing to NULL, it's pointing to some seemingly random portion of memory.

You should set every SDL_Surface to NULL after freeing them, unless you are in a destructor, so you are 100% sure the pointer isn't being used anymore.
SDL_FreeSurface( m_pSky );
m_pSky = NULL;



I see that you did this several times: (CMap::Load() function in cmap.cpp)
if ( m_pSky = NULL )
return false;

This sets m_pSky to NULL, it doesn't check if it is NULL, so this always returns false.
You want two equal signs, like this:
if ( m_pSky == NULL )
return false;


If you make this mistake alot, you can type your if-statements backwards, like this:
if ( NULL == m_pSky )
return false;

And the compiler will catch alot of those mistakes when you are compiling. Example:
if ( NULL = m_pSky ) //Compiler: "I can't set NULL to anything! Error!"
return false;

Ofcourse, this doesn't work when comparing two variables, and personally, I don't like this way, but it's a option I thought I'd mention.



You are using C++, right? I mean, you have .cpp files, you have C++ style classes, you are using the singleton pattern... So why aren't you using C++ style strings or C++'s file streams? It makes things alot easier on yourself. [smile]




So, in short, your problem is most likely this: (I wasn't able to compile and confirm, I don't have Visual C++ properly configured yet)

//bool CMap::Load( char *Filename ) in cmap.cpp:

m_pSky = CEngine::Engine()->LoadImage( skyFile ); //Load the image, return a SDL_Surface pointer to the image.
//...

//Set the pointer to NULL, and since NULL is equal to 0, this becomes
//if( m_pSky ) which is if( NULL ) which is if( 0 ) which is false,
// which doesn't trigger the if() statement, so we don't return but
//instead we continue, but now with m_pSky as NULL.
if ( m_pSky = NULL )
return false;

//...

//Here, we try to read m_pSky's 'h' variable. But m_pSky is NULL, so this crashes the program.
if ( m_pSky->h < SCREEN_HEIGHT || m_pSky->w < SCREEN_WIDTH )
{
printf( "Sky background height or width too small!!!" );

SDL_FreeSurface( m_pSky );
}




I'm tired, it's late, if I didn't explain anything well, please go ahead and ask and I'll answer tomorrow.

I just want to tell you, though, that your code looks very neat and clean. Excellent job! It's not commented too well in comments, but clean code is its own comment, and I had no trouble reading your code.

My only recommendation (since I'm also still a young programmer) is to embrace C++'s standard library. If you use C++, you might as well use the default tools it gives you, or get tools to replace the defaults (Like boost, which I personally haven't used).

It doesn't make sense to use the poor tools for the job when better ones are sitting two feet away. But hey, if it works for you, then go with what's comfortable and what helps you complete your game. Just try not to let comfort hinder progress. [smile]

Share this post


Link to post
Share on other sites
Thanks for reading the code and helping me out, I woke up this morning realizing that I hadn't told you where exactly it was crashing at. Anyway I can't believe I didn't catch those if statements not having ==.

Share this post


Link to post
Share on other sites
Quote:
Original post by Essel
I can't believe I didn't catch those if statements not having ==.


If you didn't get a warning for that, you are probably not using the highest warning level for your compiler. For example, in VC++ the default level is 3, but the highest is 4.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this