MeshShape without Assimp

[mkoval]

From #451 and #452 I've spent a lot of time looking at the internals of Assimp. I am starting to realize that the entire library, as it's name implies, is designed around importing meshes and aiScene is very much not intended to represent a mutable structure. As such, I feel think that we should move away from using aiScene as the in-memory representation of meshes in DART for mutable geometry and, possibly, all collision geometry.

@psigen, @PyryM, and I had a long discussion about this, so I"ll do my best to summarize. I know this is a large change, so this issue is mostly to start a discussion.

Multiple Heaps

Much of the Assimp library is designed to guarantee that memory is always freed in the same shared library that allocated it. This is what the Assimp documentation has to say on the issue:

By design, aiScene's are exclusively maintained, allocated and deallocated by Assimp and no one else. The reasoning behind this is the golden rule that deallocations should always be done by the module that did the original allocation because heaps are not necessarily shared. GetOrphanedScene() enforces you to delete the returned scene by yourself, but this will only be fine if and only if you're using the same heap as Assimp. On Windows, it's typically fine provided everything is linked against the multithreaded-dll version of the runtime library. It will work as well for static linkage with Assimp.

I haven't hit this problem before, probably because I rarely use unmanaged memory, so I did a bit of digging on StackOverflow. In general, it's complicated. This may be a problem on Windows when using certain versions of the C runtime, mixing libraries with different debug/release settings, and different versions of the standard library. This blog post is a good summary.

Immutability

To get around this issue, all dynamically allocated memory in an aiScene is both allocated and freed by the Importer that loaded it. If you are using a built-in importer, like we do in DART, this means that it was allocated on Assimp's heap. It is not possible to safely delete or re-allocate from within DART. This is why Assimp always returns aiScene objects by const pointer.

Programmatic Construction

It is safe to to programmatically construct an aiScene in DART as long as we also delete it in DART. This leads to the confusion situation as #452, where some aiScene objects must be deleted by delete and others must be deleted by aiReleaseImport. Once #451 is fixed, I believe that most Assimp operations will nominally work on scenes constructed in this way. However, I am not sure if it is safe to free the scene that results from this: it might contain a mixture of memory allocated in Assimp and DART.

Maintainability

Assimp has, in my opinion, a very confusing API that consists of a mixture of C and C++ code. For example, here are the gymnastics that are necessary to iterate through the textures in a model. I found a disturbing number of magic numbers and potential buffer overflows (e.g. copying a dynamic-sized input into a fixed-size buffer without any bounds checking) while browsing the source code.

I don't think think it's a good idea to tie DART, which generally has very high quality source code, to this type of baggage. In my opinion, it would be nice to move away from Assimp entirely if a viable alternative presents itself.

Proposal

I propose that we move away from using aiScene as DART's internal mesh representation. Specifically, I think MeshNode should contain the bare minimum amount of information necessary to perform collision checking and a dynamical simulation. Additionally, DART should provide an URI to the original resource that can be loaded for visualization. The user is responsible for loading any additional information, e.g. textures and material properties, that is necessary only for visualization.

Here is my reasoning behind this:

1. There is a standard representation of meshes for collision checking: a list of vertices and a list of faces.
2. There is no standard set of additional vertex properties (e.g. vertex color, vertex normal, vertex tangents, etc).
3. There is is no standard representation of material properties (e.g. texture maps, bump maps, normal maps, displacement maps, cube maps, mipmaps, etc).
4. Different applications support and/or require different material properties.

This mimics the API that OpenRAVE uses for visual geometry. It works quite well there: we've successfully used mesh formats for visualization that OpenRAVE can't load as collision geometry!

Changes to DART

This actually requires relatively minimal changes to DART:

1. Define custom Vertex and Face classes.
2. Add getVertices and getFaces accessor functions to MeshShape
3. Modify the collision checkers in DART to use these functions instead of accessing the aiScene directly.
4. Provide a conversion from an aiScene to a `MeshNode
5. Deprecate the accessor functions that operate directly on aiScene for removal in DART 6.0.
6. Alternatively to (4) and (5), we could move those functions to an AssimpMeshShapesubclass and leave them in DART 6.0.

Thoughts? :smile:

[mxgrey]

I would be totally okay with this. I've thought about moving away from assimp for internal mesh structures, and the only thing that stopped me from proposing it was a lack of sufficient motivation. But I think you've laid out the motivation for it pretty well.

With the upcoming major API changes (like many of the features of Shape being migrated into the ShapeNode class) I think now would be a good time to introduce this kind of major change.

[mklingen]

I started implementing this here: https://github.com/mklingen/dart/tree/feature/mesh_refactor but haven't finished. For one thing the aiNode and aiScene are used all over the place in the renderer.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.