What do you think of this design? Too much dynamic memory?

[Duncan]

Hi guys. I've started to adopt this way of designing my Graphical applications. I have a Screen class (which is a singleton) which has an std::vector of Element pointers. I have an Element class which has four virtual methods. So it's like this:

class Element
{
  public:
    virtual void events()=0;
    virtual void logic()=0;
    virtual bool dead()=0;
    virtual void draw()=0;
};

class Screen
{
  private:
    std::vector<Element*> elements;
  public:
    void events();
    void logic();
    void check_dead();
    void draw();
    //The above methods just iterate through the vector calling each element's respective methods. The check_dead method does some sort of tricky stuff to check if an element is dead and removes it if necessary.

    void add(const Element * e); //This adds the argument to the vector.
};

So then I have stuff inherit from Element. For instance:

class HealthBar : public Element
{
//...
};

class Enemy : public Element
{
//...
};

//etc... 

Then what I do is I make an instance of Screen and add the elements (like the player's health bar and enemies that appear on screen as well as background images and stuff) using the add() method. Then in the main game loop I simply call the screen's events(), logic(), check_dead(), and draw() methods.

The only hangup that makes me feel like this set up is not really very good is the difficulty with preserving Elements that I pass into the vector. By that a I mean, say that whenever the user clicks I want an Explosion to occur at the place that they clicked. The explosion is to last 32 frames. So I have:

class Explosion : public Element
{
  private:
    //...
    int framecounter; //<- gets incremented in every logic() call
  public:
    //...
    bool dead()  {return framecounter > 32;}
    //...
};

and then in my main loop:

if (detect_click())
{
  Explosion e(mouse.x, mouse.y);
  screen.add(&e);
}

This SEEMS to work okay even when I run the program but the issue happens when I try to click more than once and hope for more than one explosion to occur. It's simple enough to deduce that the issue arrises because the pointer in the Screen's vector points to the object that is created in the if (detect_click()) block. Thus, when this block is executed subsequent to the previous execution, the old Explosion, e, gets destructed (because it gets overwritten, essentially) even if its framecounter is not greater than 16.

so my solution for this is to use dynamic allocation. That way the object will persist through multiple clicks.

if (detect_click())
{
  screen.add(new Explosion(mouse.x, mouse.y));
}

the difficulty with this is that eventually the Explosion object needs to get deleted so that its data can be freed... Obviously this should happen when the explosion's dead() method returns true. So it's not too complicated, really, to do some fancy stuff in the check_dead() method that, not only removes the pointer from the vector, but also frees the data that it points to.

The only issue with this is that, by implementing that functionality in the Screen class, we're requiring that every pointer in the vector points to a dynamically allocated object. This isn't the worst thing in the word... but objects like background images or menu buttons or something don't need to be dynamically allocated to function properly.

So, herein lies the snag. I feel like I need to make every pointer in the Screen::elements vector point to a dynamically allocated object, but on the other hand, I don't need every object to be dynamically allocated for objects such as Background images or Menu buttons.

Sooo what do you guys think? I mean I CAN just go ahead and Screen.add(new BGImage("bg.png")) and everything will work fine but I feel a bit odd about using so much dynamic memory allocation when it feels unnecessary... I mean let's not forget that the vector has to dynamically allocated for every pointer as well... so that's quite a lot of memory that our program is asking for and I don't much like that idea...

Soo, yeah... what do you guys think of the design? Is this a good idea, having a Screen object and all our objects that we intend to draw on the screen inheriting from an Element base class? Or... is this just a bad idea that will result in many more dynamic memory allocations than we can afford to make?

Thanks!

[Duncan]

Actually when re-reading this post I had an idea.. have two methods in the screen class.. a

void Screen::dynamic_add(const Element * e);

and

void Screen::static_add(const Element * e);

And then I could have some other vectors that keep track of which indexes are dynamically allocated and which indexes are statically allocated and will only call the delete operator if the index is in the dynamic index vector. Does that seem like a better design? I sort of feel like I'm doing a bit more hacking than necessary here... again, what do you guys think?

[jsmith]

I don't think your second idea is any better, the reason being that you are now relying on the user of
your Screen class to call the right function. For a user, this is equally difficult as telling the user that
they must only pass you pointers to dynamically allocated instances.

You are actually running into a C++ language issue, namely that polymorphism only really works through
references or pointers. Moreover, STL containers are NOT polymorphic, hence you are forced to store
pointers to base class elements rather than actual derived instances.

You almost asked the question - "How can I tell if an object is dynamically allocated on the heap or
allocated on the stack?". This question gets asked quite a lot, and the answer really is that you can't.

The bottom line is that you are stuck dynamically allocating these elements on the heap. In terms of
memory utilization, it really isn't that bad. Memory has to be allocated for the objects anyway, its just
a matter of on the heap vs. on the stack. The heap is in general much larger than the stack anyway.

Perhaps the only option I can suggest is to change your Screen::add function to take a boost::shared_ptr
instead of a raw pointer. This should automatically imply to the user that Screen::add takes a dynamically
allocated instance.

[helios]

I agree with jsmith. It's better to only use the stack when the object or whatever it is doesn't need to outlive the function. And if you're going to pass a pointer to an object that manages memory, then even more reason to use dynamic allocation. You wouldn't want to accidentally delete a stack address, now would you?
Compared to the heap, the stack is pretty limited, so the fewer things you put on it, the better.

Try to keep your comments no longer than 80 characters per line. This makes them easier to read in most editors and also doesn't stretch the forum. The same applies for the actual code.

[Duncan]

Thanks for the advice guys! I was thinking I'd probably have to go that direction (striclty heap memory) but I thought maybe there was a better solution? Well thanks anyway your advice was enlightening. And Helios, I will keep that in mind in future posts; it is a good point.