c++ - Using shared_ptr for unique ownership (kind of) - is this good practice?

Question

this is quite hard to explain but I'll try my best. So, I have a RenderComponent, EventManager and RenderSystem. In my RenderComponents constructor, I raise a renderComponentCreated event which the RenderSystem subscribes to. Using an event args object, I pass a RenderNode as data which contains the information the renderSystem needs to draw the thing (drawable, position and type).

So far so good. Now, when the renderComponent is deleted, I want the RenderNode to be removed from the RenderSystem automatically while still leaving the option to remove it manually e.g. as a reaction to some event. This could be done using a RenderComponentRemoveNodeEvent which again the RenderSystem subscribes to.

Now the 'problem'. From my understanding (and from what I want) the renderNode should be something uniquely owned by the RenderComponent (hence unique_ptr). However, this would require me to either copy (and implement a comparison operator for the renderNode --> to be able to find it when I want to remove it) or pass a reference / raw pointer to the renderNode. However, (If I'm correct) there is no way to know whether a reference still refers to a valid object which would mean that automatic removal could not be implemented.

My solution was to make the RenderNode (that is uniquely owned by the RenderComponent) shared and pass weak pointers to the event. The RenderSystem also now maintains a list of weak pointers that it checks for whether they are still pointing to a valid object and automatically removes them if not. So essentially what I would have wanted is creating a weak pointer from a unique one. However, the way it is now, someone could just create a shared pointer from the weak pointer and keep the RenderNode alive longer than it should. Since the managed object itself (RenderNode) contains references to other objects that will not exist longer than the RenderComponent this may cause serious issues.

My question now: can this be considered good design or have I missed something?

PS: Sorry if this explanation reads a bit clunky (English is not my native) and thanks for your help!

Answer 1

There's certainly nothing wrong with using std::weak_ptr to grant access to an object that might be destroyed, that's what it was invented for. But it does necessitate that the object itself be held by a std::shared_ptr. Not only does this mask your intent to have the object lifetime controlled by its parent, it forces dynamic allocation of the object and precludes it from being a member variable of the parent.

An alternate approach is to keep track of the pointer through a handle, and have a handle manager that tracks whether the object is alive or dead. The safest way to implement this is to make the manager a base class of the object you're tracking, that way RAII ensures it is always up to date. Here's a sample implementation of that concept. Note: untested.

template<class Derived>
class HandleBased
{
public:
    typedef uint64_t handle_t;

    HandleBased() : m_Handle(NextHandle())
    {
        Map()[m_Handle] = this;
    }

    ~HandleBased()
    {
        auto it = Map().find(m_Handle);
        Map().erase(it);
    }

    handle_t ThisHandle()
    {
        return m_Handle;
    }

    static Derived* FindPtr(handle_t h)
    {
        auto it = Map().find(h);
        if (it == Map().end())
            return null_ptr;
        return static_cast<Derived*>(it->second);
    }

private:
    static handle_t NextHandle()
    {
        static handle_t next = 0;
        return next++;
    }

    static std::unordered_map<handle_t, HandleBased*>& Map()
    {
        static std::unordered_map<handle_t, HandleBased*> the_map;
        return the_map;
    }

    handle_t m_Handle;
};

And here's an example of how you'd use it:

class RenderNode : public HandleBased<RenderNode>
{
};

class RenderComponent
{
    std::unique_ptr<RenderNode> node1;
    RenderNode node2;

public:
    void Setup(RenderSystem& rs)
    {
        node1 = new RenderNode;
        rs.nodes.push_back(node1->ThisHandle());
        rs.nodes.push_back(node2.ThisHandle());
    }
};

class RenderSystem
{
public:
    std::list<RenderNode::Handle> nodes;

    void DoRender()
    {
        for (auto it = nodes.begin(); it != nodes.end(); )
        {
            RenderNode* p = RenderNode::FindPtr(*it);
            if (p == NULL)
                it = nodes.erase(it);
            else
            {
                p->DoSomething();
                ++it;
            }
        }
    }
};