VertexArrayObject and the ownership of the buffers Topic is solved

[bishopnator]

Back in the days, when I used Ogre 1.x I remember that the shared pointers were used in Ogre very intensively which lighten the burden of thinking about the ownership of the pointers. It was very comfortable to work with such interface and to ensure the lifetime of an Ogre's object, it was just necessary to store the share pointer to your own interface (if needed).

Now in ogre-next I have to say, it is horrible (sorry for criticism) - the worst thing which API can do, is forcing developer to constantly look in its implementation about the deletion of the objects.

If a VAO is destroyed, the VaoManager doesn't delete its vertex/index buffers. It is up to the developer to track the ownership of the pointers - an example of bad implementation is in Ogre::SubMesh::destroyVaos - tracking where the pointers are placed and delete the pointers only once (if they are shared between multiple VAOs within the same SubMesh). If I create multiple VAOs for different LODs using same vertex buffer shared between multiple sub-meshes, I have a problem - but interface doesn't restrict anything here.

Here I think using shared pointers make perfect sense and solves a lot of problems and allows to remove such implementation like in Ogre::SubMesh. Why the shared pointers are not used there? I see in ogre-next at several places some kind of motivation to avoid usage of shared pointers, but I don't see any good reason for that. Deletion of the objects doesn't happen too often, so the framerate shouldn't be influenced here.

The modern c++ tend to avoid usage of raw pointers (replaced with e.g. std::unique_ptr, std::shared_ptr, etc.) so I am really surprised from the beginning of studying ogre-next that there are so many usages of raw-pointers. They are problematic as the ownership is not clear.

At my work, our software uses another rendering API, which deals only with raw pointers and tried to track the ownership internally - the problem is, that if we keep such pointer outside the API (at our side), there we had to put really huge effort to ensure the life time of the objects which engine can delete by itself as it doesn't know about the references outside it - shared pointers can solve all such problems.

For me a good c++ modern API limits the wrong usages by using input types properly (e.g. using references instead of pointers as input where the input cannot be nullptr), clearly implicitly states the ownership of the objects (input and also output) - using unique and shared pointers and using move semantic to move ownership between the calls. Ogre-next seems to avoid those principles a lot at many places.

[dark_sylinc]

OK I see various things going on I'll try to address them all:

1. About "chaotic ownership" Part I

If a VAO is destroyed, the VaoManager doesn't delete its vertex/index buffers. It is up to the developer to track the ownership of the pointers - an example of bad implementation is in Ogre::SubMesh::destroyVaos - tracking where the pointers are placed and delete the pointers only once (if they are shared between multiple VAOs within the same SubMesh). If I create multiple VAOs for different LODs using same vertex buffer shared between multiple sub-meshes, I have a problem - but interface doesn't restrict anything here.

You are correct in the sense that "anything is theoretically possible". That's a good point.

The reason VAOs don't own VertexBuffers is that, as you point out, they can be shared for LOD purposes. But that doesn't mean we will share VBs between unrelated meshes.

2. About Shared Ptrs

The modern c++ tend to avoid usage of raw pointers (replaced with e.g. std::unique_ptr, std::shared_ptr, etc.) so I am really surprised from the beginning of studying ogre-next that there are so many usages of raw-pointers. They are problematic as the ownership is not clear.

I tend to shy away from shared ptrs because they increase build times, fatten memory footprint (by adding a ref count), add spurious atomic operations on certain cases, and are just a lazy approach as to who owns what. It can also introduce subtle memory leaks because a shared_ptr is never released, existing forever long after it's not needed anymore. Raw ptrs have the same problem but at least they will show up in Valgrind / ASAN.

Another issue is that VaoManagers are ultimately owned by the VaoManager, which can cause problems on shutdown. The following is still an issue on the shared ptrs we have:

1. FooBufferManager returns a FooBufferPtr

2. User shutdowns OgreNext, shutting down FooBufferManager

3. User releases his own internal structures, resetting FooBufferPtr

4. FooBufferPtr tries to use FooBufferManager for destruction, which is now a dangling or nullptr. Unfortunately, the callstack is hideous (it points to the UserClass' destructor that owns FooBufferPtr) which makes it hard for most users to understand what they did wrong.

Meshes for example are in a MeshPtr.
But you'll notice that at RenderQueue level, it has no understanding of Meshes. It deals with VAOs, MovableObject, Renderables, and HlmsDatablocks.

RenderQueue can potentially iterate through 100k objects every frame. Smart ptrs are a poor choice at this level.

3. About "chaotic ownership" Part II

OgreNext's approach in general to Managers is that, unless explicitly stated, whoever calls createFooBuffer() is responsible for calling destroyFooBuffer().

In this case SubMesh creates the VB, IB and VAOs, and it is also the one who destroys it. OgreNext isn't doing this right now (and I don't see a reason why, I'm just saying it as an example), but it might be possible in the future that if SubMehes wanted to share a vertex buffer, then Mesh (who owns all SubMeshes) would be responsible for creating and destroying the VBs & IBs.

So while yes, you're right that technically nothing prevents code from having VB shared across multiple meshes and you had no quick way of checking that out, it would be a really bad practice.

The same goes for any Item out there:

1. All Item/SubItem are created based on a Mesh/SubMesh.

2. While the Item is alive, the Mesh must stay alive.

3. The Mesh will keep the VAO alive.

4. Destroying the Mesh early will corrupt its Items in many ways (including the SubItem's VAO).

The rule of thumb is "he who called createVertexBuffer must call destroyVertexBuffer". If that's not the case, it must be clearly communicated throughout API documentation.

4. About clarity
I acknowledge that if you feel you're having trouble tracking "who owns what" or "when it is safe to release", then there's something wrong.
Either documentation, practices, or extra idioms that will help you find this out quickly.

You may grep through our source code the term " owns " appears often.

Would it be easier perhaps if we annotate code more explicitly? Like this:

Code: Select all

#define ogre_owner

class _OgreExport SubMesh
{
     // ...
     // We own & delete the buffers inside these VAOs.
     VertexArrayObjectArray ogre_owner mVao[NumVertexPass];
     // ...
};

It's not an enforcement like weak and strong are in shared ptr, but it would annotate intent and "ogre_owner" is easily searchable.

Unfortunately, there is no equivalent of Rust's borrow checker where the owner has R/W access to the pointer, but all other users have Read-only access to it. We follow this pattern quite frequently.

[bishopnator]

Unfortunately in my case, the annotations wouldn't easy my goal. I need to take loaded Ogre::Mesh, add new channel to it and create adjacency (convert index buffer to have 6 indices per triangle instead of 3, for the missing adjacency, I have to add additional vertices to the vertex buffer).

This means that for a single Ogre::SubMesh, I have to collect all unique vertex buffers with all possible index buffers (due to LOD and considering what you wrote, there can be a single VB with different IBs) - every combination of VB+IB can lead to insertion of new "fake" vertices to satisfy the geometry shader. At the end, I have to remap everything back to the creation of VAOs. This just sounds too compex (not saying that it is not doable), but it shouldn't such complicated.

[dark_sylinc]

It may be worth pointing out as an implementation detail: SubMesh is not keeping track of the VB/IB externally; so you could grab the SubMesh' VAO, you completely destroy it (i.e. including the VB/IB in it) and then create a new VAO with buffers created by you. Finally assign the VAO to the SubMesh. And the SubMesh will destroy the VAO & its buffers not knowing it was altered.

This will work as long as there are no live Items using the Mesh.

[bishopnator]

yes, I am going exactly in this direction.