How to have a pointer to sf::Drawable in a base class?

[wh1t3crayon]

When running the code below I am getting the following runtime error:

Unhandled exception at 0x5E87CC6E (sfml-graphics-d-2.dll) in Project for SFML Web Problems.exe: 0xC0000005: Access violation reading location 0xCCCCCCD0.

// Project for SFML Web Problems.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "SFML\Graphics.hpp"
#include <deque>
#include <memory>

class WorldObject{
public:
        WorldObject(){ drawable = nullptr; }
        virtual ~WorldObject(){}

        sf::Drawable &GetDrawable(){
                return *drawable;
        }
protected:
        //accessing this pointer is invalid
        sf::Drawable *drawable;
};

class Entity : public WorldObject{
public:
        Entity(){
                sprite.setPosition(20, 20);
                sprite.setFillColor(sf::Color::White);
                sprite.setSize(sf::Vector2f(50, 50));

                drawable = &sprite;
        }
        virtual ~Entity(){}
private:
        sf::RectangleShape sprite;
};

class Holder{
public:
        Holder(){
                entities.push_back(Entity());
                objects.push_back(&entities[0]);       
        }
        ~Holder(){}

        std::deque<WorldObject*> &GetObjects(){
                return objects;
        }
        std::deque<Entity> &GetEntities(){
                return entities;
        }
private:
        std::deque<WorldObject*> objects;
        std::deque<Entity> entities;
};

int _tmain(int argc, _TCHAR* argv[])
{
        sf::RenderWindow window(sf::VideoMode(500, 500, 32), "Test");
        sf::Event ev;

        Holder holder;

        while(window.isOpen()){
                while(window.pollEvent(ev)){
                        if(ev.type == sf::Event::Closed){
                                window.close();
                        }
                }
                window.clear();
                for(unsigned int i= 0; i < holder.GetObjects().size(); i++){
                        //accessing sf::Drawable* throws error
                        window.draw(holder.GetObjects()[i]->GetDrawable());
                }
                window.display();
        }

        return 0;
}

 

My question is pretty straightforward. How do I make a pointer to sf::Drawable in a Base class so that in a Base container I can draw that drawable, be it a sf::Sprite, sf::RectangleShape, etc.?

[Hapax]

sf::Drawable is a virtual class so it makes no sense on its own.
I'm not sure why you're referring to sf::Drawable as a base class since you have no classes that inherit from it, which is what it should do.

[wh1t3crayon]

I'm sorry I must have mispoken. As you can see in the code, sf::Drawable isn't the base class itself. WorldObject is the base class, and WorldObject owns a pointer to sf::Drawable. Accessing this pointer from a container of WorldObjects throws the runtime error.

[Laurent]

std::deque::push_back may invalidate previously stored pointers to its elements, if it needs to reallocate itself somewhere else in memory to grow.

[Mr_Blame]

little correction:
when you init something from base class in child class's constructor you need to write something like this:

class Entity : public WorldObject{
Entity::Entity(){
    WorldEntity::drawable = &sprite;
}
};
 

[dabbertorres]

No, that is not required. Only when you have the name of a base class' member hidden, do you have to specify the scope.

[Mr_Blame]

I will recommend use references in this code instead

[Hapax]

It looks to me like this is happening:

• Pointer to sf::Drawable declared
• That pointer is set to nullptr
• That pointer (nullptr) is then dereferenced to return as a reference

This means that nullptr is being dereferenced.

[AlexAUT]

std::deque::push_back may invalidate previously stored pointers to its elements, if it needs to reallocate itself somewhere else in memory to grow.

Nope deque won't "copy/move" the elements, according to this: http://www.cplusplus.com/reference/deque/deque/push_back/ pointer and references to objects will remain valid.


To the OP just debug it, make a breakpoint before you access the pointer and look at the content of the pointer. Or you could debug the constructor of Entity to see if the pointer gets set correctly


AlexAUT

[AlexAUT]

Btw, the error occurs because when you push_back the element into the deque with an temporary object("Entity()"), it will get copied into the deque and you don't have a copy constructor so the pointer will get shallow copied and will point to the sprite of the temporary object (which will get destroyed after the push_back call).

So you have 2 options to fix this:
1) entities.emplace_back(); instead of entities.push_back(Entity()); and delete the copy constructor to avoid errors in the future
2) Define the copy constructor so that the pointer will point to the right sf::Sprite instance.

As a rule of thumb always define/delete the copy constructor if you have a pointer which owns memory (in this case it basically owns it because it points at a member of its own instance)


AlexAUT

[Nexus]

@wh1t3crayon: Your classes are badly encapsulated. If you return references to your private containers, you can as well make them public. You should find a design that need not access internal implementation details.

Making member variables protected is also often an indicator of bad information hiding; consider making as much as possible private, and provide protected methods if necessary. Why does the base class not have a constructor that takes the drawable by reference? Why the useless initialization with a nullptr?

Furthermore, use the standard main signature int main() or int main(int argc, char** argv). Link to sfml-main if you're building a Windows application.

sf::Drawable is a virtual class so it makes no sense on its own.

You probably mean abstract class. Only functions and inheritance can be virtual.

when you init something from base class in child class's constructor you need to write something like this:

Entity::Entity(){
    WorldEntity::drawable = &sprite;
}

No. And as mentioned, accessing base class variables is mostly bad style. The constructor initializer list should be used.

std::deque::push_back may invalidate previously stored pointers to its elements, if it needs to reallocate itself somewhere else in memory to grow.

No, std::deque remains valid while growing. Only iterators are invalidated, but not references and pointers. That's one great advantage over std::vector. [Source].

So not this is the problem, but what AlexAUT mentioned. However, instead of writing the Big Three, I would remove the pointer in the base class and add a pure virtual method  const sf::Drawable& WorldObject::getDrawable() const that can be implemented in the derived classes. Or maybe make the classes directly inherit sf::Drawable.

[Jesper Juhl]

++Nexus;

[Hapax]

You probably mean abstract class. Only functions and inheritance can be virtual.

Right. Of course. My mistake 

[AlexAUT]

The solution from nexus is much better, I'm just influenced by my universitity prof. which teached us to always have a copy constructor when dealing with pointers 


AlexAUT

[Nexus]

Actually, implementing The Big Three/Five again and again also indicates a flawed design. These methods are only needed for low-level classes that deal with custom ownership or resource semantics. If you end up providing them in every C++ class, you haven't understood the concept.

To elaborate a bit, C++ uses value semantics by default: you can copy and assign values, pass them to functions and return them from functions. Objects behave like values, whether they're class instances or integers. Now, whenever you deviate from that, you should have a reason to do so. A class is copyable/movable by default if all its members can be copied/moved one by one, and usually, this is enough.

Then there are cases where you don't want copy/move semantics of values. For example, your class might:

• have a resource owned only by one instance
• have a resource owned and shared by multiple instances
• store a pointer to a stateful base class which requires a deep copy
• store a pointer to an interface on which a function is invoked
• be registered at another class, and a pointer to itself needs to be rebound

You can now either rewrite The Big Five and increase your LOC and maintenance effort, or you can use smarter classes to take care of your copy/move semantics:

• std::unique_ptr for single ownership
• std::shared_ptr for shared ownership
• aurora::CopiedPtr for deep polymorphic copies
• std::function for functionals
• your custom observer-like class that automates such dependency rebindings

In many cases, rethinking the design and using low-level building blocks for abstraction in high-level classes helps avoid re-writing The Big Five. Whenever that's possible, do it. The Big Five are a pain to write and maintain, there's no clean way around code duplication, and it's incredibly easy to introduce bugs whenever the object state changes.

There are still cases where this is simply not possible by nature. For example when you're implementing a low-level building block itself. Or when member-wise copy/move doesn't give you the control you need for rollback semantics in exception safety scenarios.