opened
mkoval
username_0
DART currently uses `aiScene` to represent meshes in `MeshShape` and `SoftBodyNode`. These are passed around by raw pointer and are, along with their contents, manually deleted with `delete`. This is unsafe because scenes returned by an Assimp importer must be deleted with `aiReleaseImport`. Conversely, it is unsafe to call `aiReleaseImport` on a scene that was allocated using `new aiScene` by the user.

Unfortunately, it is not possible to tell how a given `aiScene` was allocated. This means that the [`MeshShape` destructor](https://github.com/dartsim/dart/blob/master/dart/dynamics/MeshShape.cpp#L146) could produce a memory leak or a SEGFAULT. I suggest that we modify the DART API to manage all `aiScene`s with `shared_ptr`s. If the scene was created by an importer, then the `shared_ptr` will be constructed with a custom deleter that calls `aiReleaseScene` instead of `delete`.

This requires several breaking API changes. I suggest that we:

1. Modify `MeshShape` to store a `shared_ptr<const aiScene>` internally
2. Introduce a new `MeshShape` constructor, `setMesh` function, and  `loadMesh` function that operate on `shared_ptr`s
3. Deprecate the existing `MeshShape` and `setMesh` functions and modify them to create a `shared_ptr` with the default deleter.
4. Deprecate the existing `loadMesh` function. I'm not sure what we should name the replacement, since we can't overload based on return value.
5. Update all references in DART to use the `shared_ptr` versions of the functions.

@username_1 Is this a good summary of what we discussed offline?
aiScene requires a custom deleter
issue
created
mxgrey
username_1
This all sounds good to me.

I would propose ``MeshShape::importMesh`` as the replacement function name for ``loadMesh``.
comment
created
mkoval
username_0
`importMesh` works for me.

I am also working on some changes, related to `package://` URI resolution, that will require an API change (the addition of an optional argument) to `loadMesh`. Perhaps we can consolidate both changes into one merge and avoid breaking the API twice.

I hope to have a pull request up for those changes soon so we can discuss.
comment