Sign in to follow this  

Game design dilemma

This topic is 3593 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi, I'm making a game and I'm not sure how to store things that are on the screen. At the moment I'm doing this:
std::vector <Fruit*> fruits;
Fruit aFruit(200, 200, APPLE); //Fruit is an object, the parameters are integers
fruits.push_back( aFruit );
    
    if( fruits.size() > 0 )
    {
        for( std::vector <Fruit*>::const_iterator iter = fruits.begin(); iter < fruits.end(); ++iter )
        {
             applySurface( (*iter)->ReturnX(), (*iter)->ReturnY(), (*iter)->ReturnImage(), screen );
        }
    }


So, I'm adding their addresses in a vector of pointers use the above code to draw them. Is it a better idea to store them on the heap?
std::vector <Fruit*> fruits;
fruits.push_back( new Fruit( 100, 100, APPLE ) );
// blah blah blah

//edit:Here's the function that's supposed to clean up the memory on the heap
void cleanUp( std::vector <Fruit*>* const fruits)
{
     if( (*fruits).size() > 0 )
    {
        for( std::vector <Fruit*>::const_iterator iter = (*fruits).begin(); iter < (*fruits).end(); ++iter )
        {
             delete *iter;
        }
    }
}   

//it doesn't seem to work though.. (when I check if fruits[0] == NULL, it's not

Thanks in advance. [Edited by - sheep19 on February 11, 2008 8:56:30 AM]

Share this post


Link to post
Share on other sites
In the first example you should be pushing the address of the fruit:

fruits.push_back( &aFruit );

In the second example you ought to pass by reference instead of a constant pointer:

void cleanUp(const std::vector <Fruit*>& fruits)

The decision between heap and stack allocations in C++ is usually largely down to scope and lifetimes. In the first example if aFruit goes out of scope then any pointers to it will be dangling. In the second example individual fruit instances are allowed to exceed the lifetime of the scope that they were constructed in, which is frequently an advantageous thing.

In this example I don't see why the vector needs to be storing pointers at all, you could simply have:
std::vector<Fruit> fruits;
fruits.push_back( Fruit(200, 200, APPLE) );

Share this post


Link to post
Share on other sites
Quote:
Original post by dmatter
In this example I don't see why the vector needs to be storing pointers at all, you could simply have:
std::vector<Fruit> fruits;
fruits.push_back( Fruit(200, 200, APPLE) );


You are right, I didn't think of that. Thanks a lot!

-----------

Actually there's an error. If I do this:
std::vector<Fruit> fruits;
fruits.push_back( Fruit(200, 200, APPLE) );


then the program flashes quickly and exits.


std::vector<Fruit> fruits;
fruits.push_back( Fruit(200, 200, APPLE) );

if( fruits.size() > 0 )
{
for( std::vector <Fruit>::const_iterator iter = fruits.begin(); iter < fruits.end(); ++iter )
{
applySurface( iter->ReturnX(), iter->ReturnY(), iter->ReturnImage(), screen );
}
}




----

If I do this:

std::vector<Fruit*> fruits;
fruits.push_back( new Fruit(200, 200, APPLE) );

if( fruits.size() > 0 )
{
for( std::vector <Fruit*>::const_iterator iter = (*fruits).begin(); iter < (*fruits).end(); ++iter )
{
applySurface( (*iter)->ReturnX(), (*iter)->ReturnY(), (*iter)->ReturnImage(), screen );
}
}
/*...*/
void cleanUp( std::vector <Fruit*>& fruits)
{
if( (fruits).size() > 0 )
{
for( std::vector <Fruit*>::const_iterator iter = fruits.begin(); iter < fruits.end(); ++iter )
{
delete *iter;
}
}
}




Are the surfaces actually deleted?

[Edited by - sheep19 on February 11, 2008 12:10:35 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
Actually there's an error. If I do this:
std::vector<Fruit> fruits;
fruits.push_back( Fruit(200, 200, APPLE) );


then the program flashes quickly and exits.

*** Source Snippet Removed ***

The code looks fine-ish (iter < fruits.end() will only work for vectors, you'd typically have iter != fruits.end() instead).
The problem must be elsewhere. If it worked before then it's likely to do with your Fruit class which must be properly and safely copy-constructable! If for example Fruit(200, 200, APPLE) allocates memory and deallocates it the destructor and Fruit doesn't have a suitable copy constructor then you can't safely insert it into an STL container directly.

Quote:
If I do this:
*** Source Snippet Removed ***

Are the surfaces actually deleted?

Assuming a Fruit is a surface then yes they will be deleted.
In that snippet you're not correctly accessing the vector in the top-most for-loop - you're deferencing it like as if it's a pointer when it isn't.

Share this post


Link to post
Share on other sites
Quote:
The code looks fine-ish (iter < fruits.end() will only work for vectors, you'd typically have iter != fruits.end() instead).
The problem must be elsewhere. If it worked before then it's likely to do with your Fruit class which must be properly and safely copy-constructable! If for example Fruit(200, 200, APPLE) allocates memory and deallocates it the destructor and Fruit doesn't have a suitable copy constructor then you can't safely insert it into an STL container directly.


I haven't declared a copy constructor. I thought I had to do it when I use the heap inside the class. (Which I'm not doing.)
--

Quote:

Assuming a Fruit is a surface then yes they will be deleted.
In that snippet you're not correctly accessing the vector in the top-most for-loop - you're deferencing it like as if it's a pointer when it isn't.


Why Am I not accessing it correctly? How should it be?

That's the Fruit class:

class Fruit
{
public:

Fruit( int x = 0, int y = 0, const int type = 0 );
~Fruit();
SDL_Surface* ReturnImage() const;
int ReturnX() const;
int ReturnY() const;

private:
SDL_Surface* m_Image;
int m_X, m_Y, m_Type;
};

#include "SDL/SDL.h"
#include "declarations.h"

Fruit::Fruit( int x, int y, const int type ): m_X(x), m_Y(y), m_Type(type)
{
if( type == APPLE )
{
m_Image = loadImage( "apple.bmp" );
}
}

Fruit::~Fruit()
{
SDL_FreeSurface(m_Image);
}

SDL_Surface* Fruit::ReturnImage() const
{
return m_Image;
}

int Fruit::ReturnX() const
{
return m_X;
}

int Fruit::ReturnY() const
{
return m_Y;
}



Thanks a lot for your help :)

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
I haven't declared a copy constructor. I thought I had to do it when I use the heap inside the class. (Which I'm not doing.)

But you are:

m_Image = loadImage( "apple.bmp" );

You also delete the image in the class' destructor. This means that the following line:

fruits.push_back( Fruit(200, 200, APPLE) );

will allocate an image, copy a Fruit instance along with a pointer to that image into the vector and then destroy the memory referenced by that pointer leaving a dangling m_Image pointer - hence the crash.
Simply put, your Fruit class isn't copy-safe.

I don't know SDL but a quick google reveals that SDL_Surface has a reference counter which is used for managing situations like this. I'm not sure if you're meant to fiddle with the reference count yourself or whether there's an SDL smart-pointer as part of the library.

The thing to note here is that you're not making proper use of the RAII idiom, which is almost practically required for putting things into a vector. Now the fact is that this Fruit class can make use of RAII properly (using, for example an SDL smart-pointer). Alternatively you can side-step the issue by using a vector of pointers, as you have done already.

Quote:
Why Am I not accessing it correctly? How should it be?

The code you posted was like this:
std::vector<Fruit*> fruits;
/*
...
*/
for( std::vector <Fruit*>::const_iterator iter = (*fruits).begin(); iter < (*fruits).end(); ++iter )


It ought to be like this:
std::vector<Fruit*> fruits;
/*
...
*/
for( std::vector <Fruit*>::const_iterator iter = fruits.begin(); iter != fruits.end(); ++iter )


Nothing major, I think you knew this already, I think it was just a casual error.

Share this post


Link to post
Share on other sites
I understand. I'm going to use the vector of pointers to avoid this problem, because my class will need more tweaking otherwise.
I can't believe I learned about constructors, copy constructors and destructors three days ago. :-/

Thanks a lot for your help, I REALLY appreciate it. :)

Share this post


Link to post
Share on other sites

This topic is 3593 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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