shared_ptr, unique_ptr, ownership, Am I overdoing it in this particular case?

Question

I work on graphics applications and have been using shared & unique pointers essentially because it handles memory deallocation for me (aka as a convenience) which is probably bad (if that's the reason why I use them).

I read the answer to a question on Stackoverflow recently that mentioned that according to B. Stroustrup, unique/shared ptrs should generally not be used and that arguments should be passed by value instead.

I have a case in graphics for which I think using shared_ptr makes sense but I'd like to know from experts (I am not C++ experts) if I am over doing/thinking it and if so what they would be doing instead (to conform to C++ recommendations and for efficiency)

I will take a general problem that occurs in Rendering/Ray-Tracing. In this particular problem we have a pool of objects (we will use triangles for this explanation) and a structure which for the simplicity of the explanation we will refer to as regular 3D grid. Let's say that at some point we need to insert the triangles into the grid: what that means is that we need to check of the bounding volume of each inserted triangle overlaps any of the cells from the grid, and it does, then each overlapped cell needs to keep a pointer/reference to that triangle (for later use). A triangle may overlap more than 1 cell, so it can be referenced multiples times by several cells (you see where I am going with the shared_ptr here).

Note that outside of the grid structure, we don't need the pool of triangles (so technically the object that owns the pool of triangle here, is the grid or more precisely the grid's cells).

class Grid
{
    struct Cell
    {
        std::vector<std::shared_ptr<const Triangle>> triList; 
    };
    void insert(triangle*& tri_)
    {
        std::shared_ptr<const Triangle> tri = tri_;
        for (each cell overlapped by tri) {
            // compute cell index
            uint32_t i = ...
            cells[i].triList.push_back(tri);
        }
    }
    Cell cells[RES * RES * RES];
};

void createPoolOfTrianglesAndInsertIntoGrid()
{
    Grid grid;
    uint32_t maxTris = 32;
    Triangle* tris = new Triangles[maxTris];
    // process the triangles
    ...
    // now insert into grid
    for (uint32_t i = 0; i < maxTris; ++i) {
        // use placement new
        Triangle* tri = new (&tris[i]) Triangle;
        grid.insert(tri);
    }
    // no need to delete tris here ... it should be done by 
    // the grid when we go out of this function's scope
}

It sounds complicated but my motivation behind this design is to follow Stroustrup's recommendation. In this case the function createPoolOfTrianglesAndInsertIntoGrid doesn't owned the triangles, it's the cells of the grid that do. So I allocate the memory in the function createPoolOfTrianglesAndInsertIntoGrid because this is where I need to create the triangles, and I then use the placement new method to get a pointer to each triangle in that pool which I can then pass to the grid insert method (I defer memory management of that object to that method). In there, it converts the triangle into a shared_ptr and cells can now share a "reference" to it (using shared_ptr).

I wanted to know if to your opinion this was going in the right direction, or if this appears completely wrong, both in terms of implementation and in terms of possible loss of efficiency (I allocate a pool of memory, then use the new placement to create a temp triangle, which I then pass on to the grid insert method, then convert to shared_ptr, ...)

I am trying to both learn, and improve my code and hope you can provide valuable professional feedback.

EDIT: basically I am trying to find the right approach to that problem + I will try to provide later some modifications based on your comments

Answer 1

It looks to me like your Grid owns the triangles. I am assuming the triangles are relatively light (3-5 dimensions?).

I would think something like this might suit. I am using a container in the Grid to take ownership of the triangles by value. The container will delete the triangles when the Grid goes out of scope.

Then each Cell simply uses raw pointers to keep track of which triangles it references. The Cells don't own the triangles they simply hold pointers to the triangles that are owned by the Grid.

class Grid
{
    struct Cell
    {
        std::vector<Triangle*> triList; // non owning
    };

    void insert(Triangle tri) // pass by value
    {
        tris.push_back(tri); // Grid owns this by value

        for(each cell overlapped by tri) {
            // compute cell index
            uint32_t i = ...

            cells[i].triList.push_back(&tris.back());
        }
    }

    // Use a deque because it won't re-allocate when adding
    // new elements to the end
    std::deque<Triangle> tris; // elements owned by value

   Cell cells[RES * RES * RES]; // point to owned elements
};

void createPoolOfTrianglesAndInsertIntoGrid()
{
    Grid grid; // owns the triangles (by value)

    uint32_t maxTris = 32;

    std::vector<Triangle> tris(maxTris);
    // process the triangles
    // ...
    // now insert into grid
    for(auto tri: tris)
        grid.insert(tri);
    }

    // no need to delete tris here ... it should be done by
    // the grid when we go out of this function's scope
}

NOTE: I use a std::deque to store the triangles by value in the Grid. That's because it won't ever reallocate its internal memory when adding new triangles. If you used a std::vector here your raw pointers would end up dangling when the std::vector resized itself.

Alternatively:

Given that it looks like you build all your triangles in your function and then pass all of them into Grid, why do that one at a time? You could pass the whole container in in one go. If you do this using move semantics you won't even have to copy anything:

class Grid
{
    struct Cell
    {
        std::vector<Triangle*> triList; // non owning
    };

    // Accept the entire container in-tack
    // (no reallocations allowed after this point)
    void insert(std::vector<Triangle> tris) // pass by value (able to move in)
    {
        //
        for(auto& tri: tris)
        {
            for(each cell overlapped by tri) {
                // compute cell index
                uint32_t i = ...

                cells[i].triList.push_back(&tri);
            }
        }
    }

    // Using a vector so it MUST NOT be resized after
    // Cells have been pointed to its elements!!!
    std::vector<Triangle> tris; // elements owned by value
    Cell cells[RES * RES * RES]; // point to owned elements
};

void createPoolOfTrianglesAndInsertIntoGrid()
{
    Grid grid; // owns the triangles (by value)

    uint32_t maxTris = 32;

    // Build the triangles into
    std::vector<Triangle> tris(maxTris);
    // process the triangles
    // ...
    // now insert into grid
    grid.insert(std::move(tris)); // move the whole darn container (very efficient)

    // no need to delete tris here ... it should be done by
    // the grid when we go out of this function's scope
}

NOTE: Now I used a std::vector because you are not adding triangles one by one after they arrive in Grid. But you MUST make sure that inside Grid the owning `std::vector is not resized.