Derived class and destructor problem

[PascalM123]

Dear forum,

I have a problem concerning derived classes and their destructors. I have a base class Shape and I have Spheres, Planes and ObjFiles (shapes from .obj-files) that derive from that class. I also have a Scene class which holds a Vector (like this: vector<Shape*> shapes;). In the display callback I call a draw-function from the scene class which draws all the shapes.

/* Draw all shapes */
void Scene::draw_shapes() {
  for(int i = 0; i < shapes.size(); i++) {
    if(shapes[i]->get_type() == SPHERE) {
      Sphere sphere = *((Sphere *) shapes[i]);
      sphere.draw();
    }
    else if(shapes[i]->get_type() == PLANE) {
      Plane plane = *((Plane *) shapes[i]);
      plane.draw();
    }
    else if(shapes[i]->get_type() == OBJFILE) {
      ObjFile obj = *((ObjFile *) shapes[i]);
      obj.draw();
    }
  }
}

The problem is that when I add an ObjFile to the scene, that objfile contains an array of vertices and has a destructor which deletes the vertices.
But after I draw the objfile the first time, it not only deletes the vertices from the newly created object, but also from the one in the vector with shapes.

When I change the draw part to:

ObjFile *obj = (ObjFile *) shapes[i];
obj->draw();

the following message occures:

pure virtual method called
terminate called without an active exception
Aborted

What is the best way to solve this problem?

[Disch]

The problem is that when I add an ObjFile to the scene, that objfile contains an array of vertices and has a destructor which deletes the vertices.
But after I draw the objfile the first time, it not only deletes the vertices from the newly created object, but also from the one in the vector with shapes.

You either need to write a proper copy ctor and assignment operator... or disable them so that copies cannot be made.

Disabling them is easy. Simply make the private in the class, and do not give them bodies:

class Shape
{
private:
  // disable copying...
  Shape(const Shape&);
  Shape& operator = (const Shape&);

// .. put the rest of the class here
};

BUT

This whole if(type == this_type) cast_to_type(); crap you're doing defeats the entire point of polymorphism.

The solution here is just to make Shape::Draw a (pure) virtual function:

class Shape
{
//...
  virtual void Draw() = 0;
};

Then you don't need to know the type of shape. Just call Draw() and it will call the appropriate shape's Draw function. Your draw_shapes function can be greatly reduced:

void Scene::draw_shapes()
{
  for(int i = 0; i < shapes.size(); i++)
    shapes[i]->Draw();  // this is all you have to do
}

[moorecm]

That looks like an unsafe downcast. Regardless, I'm seeing more concerning issues with your design than that.

Let's talk about inheritance and polymorphism in C++. Consider an abstract base class which defines nothing more than the interface to the family of related types:

struct Shape
{
    virtual void draw() const = 0;
    virtual ~Shape() {}
};

Now, consider a couple subclasses, which define the draw method:

struct Circle : public Shape
{
    virtual void draw() const { /* draw implementation */ }
};

struct Square : public Shape
{
    virtual void draw() const { /* draw implementation */ }
};

Now, the virtual method draw, called through the base class abstraction (interface), will perform the appropriate action to draw itself. The abstractions can be treated uniformly, without trying to determine what type it really is or performing any casting. For example:

// perhaps this exists in some kind of aggregate class
typedef vector<Shape *> Container;
Container c;
//...
for( Container::const_iterator i = c.begin(); i != c.end(); ++i ) {
    (*i)->draw(); // draw the shape
}

Now, just because I've been doing some interesting reading lately, I'll hit this example with some more detailed design theory. Note that when possible an abstract base class should have no members. It is certainly possible to combine a concrete base with an abstraction but doing so adds a few subtle (during implementation) limitations. Formally called the Dependency Inversion Principle¹ (DIP), recommends "pushing policy (interfaces) up and implementation down." The idea is to have the implementation depend on the abstraction rather than the typical contradiction.

¹ Item 36, in C++ Coding Standards: 101 Rules, Guidelines, and Best Practices by Herb Sutter and Andrei Alexandrescu

[PascalM123]

I do know that it should be possible to just do a for-loop and call the draw method of each shape, but when I do that, the following message occurs:

pure virtual method called
terminate called without an active exception
Aborted

I've designed the shapes like this:

/* This class defines the simplest object used in the physics engine */
class Shape {
  protected:
   /* items as position, velocity, inverse mass etc. */
    
  public: 
    /* Constructor */
    Shape();
    
    /* Destructor */
    virtual ~Shape();

  /* other members (like the Newton-Euler integration method) */
    
    /* Pure abstract function */
    virtual void draw() const = 0;
};

And a sphere looks like this:

class Sphere : public Shape {
  public:
    /* Constructors */
    Sphere();
    Sphere(double rad, Vector3 c);
    
    /* few other members functions, like setting radius */
    
    /* Concrete draw function */
    virtual void draw() const;
    
  protected:
    /* other members */
};

Is there something that I've missed? Thanks for your help so far!

Regards,
PascalM123

[bluecoder]

Hi,
Have you failed to add the derived class object in the container c . and also Why you have declared virtual to the draw function in the derived classs.

[PascalM123]

the virtual part is just to see if it made a difference. This is what I do in the main file:

Scene scene;
Sphere bullit(3); // 3 = radius

void init() {
  // setup etc.

  scene.add_shape(&bullit);
}

In scene.cpp I do this:

/* Add a shape to the scene */
void Scene::add_shape(Shape *shape) {
  // shapes = vector<Shape*> shapes, declared in scene.h
  shapes.push_back(shape);
}

But when I just do a for-loop in the draw-function, I get the error 'pure virtual method called' and when I use an iterator 'request for member 'draw' in ..., which is of non-class type 'Shape*'.

[moorecm]

[...] and when I use an iterator 'request for member 'draw' in ..., which is of non-class type 'Shape*'.

You have just pointed out an error in my post above. It should dereference the iterator and then use operator -> to call the method, because it is a container of pointers. I'll correct it now.

[Disch]

for( Container::const_iterator i = c.begin(); i < c.end(); ++i )

If the goal by using iterators here is to make this work with any arbitrary container, then you should not do i < c.end(), as that only really works with vectors.

i != c.end() would be the way to go.

[moorecm]

Fixed above. Thanks.

[PascalM123]

I dereferenced the iterator, so that part works, but now the output is the same as when I'm using a for-loop like this:

for(int i = 0; i < shapes.size();i ++) {
  shapes[i]->draw();
}

So the problem lies not in the for-loop itself. Could it be that I'm not adding it right to the vector (see post 6) ?

Regards,
PascalM123

[Disch]

Are you trying to add something that isn't derived from Shape?

Is ObjFile derived from Shape?

[moorecm]

Are you sure you are storing pointers in the container? Are you sure you are passing a pointer (not a reference) to the add_shape method? It sounds like the object is being sliced somewhere.

[PascalM123]

In the 6th post in this topic I've posted some of the code. It shows how I call the add_shape functions and how I add something to the vector.

All the classes I've made so far (which is Sphere, ObjFile and Plane) derive from Shape.

[moorecm]

And they are global objects that do not go out of scope after you store the pointers, right? You have tried a clean build, too?

I think you should paste the current code.

Are you still doing something like this?
Sphere sphere = *((Sphere *) shapes[i]);

[PascalM123]

I have some new information regarding this problem. In order to find out where this goes wrong, I've done the following:

/* Add a shape to the scene */
void Scene::add_shape(Shape *shape) {
  shape->draw();
  shapes.push_back(shape);
}

So when I am in the add_shape function and I call the draw function, I works.

This is the class Scene itself:

/* This is a basic scene that holds all the shapes and checks for collision */
class Scene {
  public:
    /* Constructor */
    Scene() {}
    
    /* Destructor */
    ~Scene();
    
    /* Add a shape to the scene */
    void add_shape(Shape* shape);
    
    /* Draw all the shapes */
    void draw_shapes();
    
    /* Check for collisions */
    void handle_collisions();
    
    /* Update position */
    void update_positions(double elapsed_time);
  
  protected:
    /* A list of all the shapes */
    vector<Shape*> shapes;
};

I do not see where there is something off.
Maybe it is better to make a linked list instead of a vector and find out if that helps.

Update:
I've created a linked-list and added a way to add shapes to the list. The result is that it draws the objects! I still have to adjust the other functions, but it appears that the problem was the vector. With the linked list I do not have to cast shapes to their specific objects and use polymorphism as it should.

Thanks for your help!

[Disch]

Where is the actual shape stored? I suspect the lifetime of the shape is ending (and it's being destroyed) before the scene is being rendered.

Are all of your shapes global? Are they allocated with new? Where exactly are they?

[PascalM123]

The shapes are global, but not allocated with new. With the single linked list, this is not a problem. I do not know if it is necessary for the vector.

Regards

[Disch]

You must have some other problem elsewhere. There's no reason I can see as to why you'd be having this problem.