Update MeshBuilder class

[diegoferigo]

This PR enhances the mesh support introduced in #33. In particular:

• The path to the mesh can now be an URI. Storing the URI instead of the absolute path is important if the builder is used to create a model programmatically that can be used on different machines. Now the URI is resolved only when the trimesh object is created, but it's stored as unresolved so that it can be serialized in a SDF/URDF model as URI.
• Allows to pass the mass and inertia tensor to the builder. This information stored in mesh files is not always what's used for the inertial properties, and not always valid.

cc @lorycontixd can you please try if the changes break your pipeline? For sure you have to update mesh_path with mesh_uri. Sorry for this change, I overlooked this use-case in your original contribution.

[diegoferigo]

cc @flferretti

[lorycontixd]

@diegoferigo With these changes, nothing is broken from my side!

[diegoferigo]

With these changes, nothing is broken from my side!

Excellent! I assume nothing broken except the change of name of mesh_uri.

[diegoferigo]

Waiting a quick check from @traversaro before merging.

[lorycontixd]

With these changes, nothing is broken from my side!

Excellent! I assume nothing broken except the change of name of mesh_uri.

Yes, exactly!

[traversaro]

I am a bit confused by the mass property in MeshBuilder . In all other <Something>Builder, the mass is taken from the PrimitiveBuilder, and then the inertia is computed as proportional to the mass (so the density is taken implicitly from the mass). Instead, in the MeshBuilder there is an additional mass attribute of MeshBuilder, and if the mass is set and the inertia is not set, the mass is ignored while computing the inertia, getting instead the density from trimesh (that is anyhow not set, so I guess it will default to 1.0, see https://github.com/mikedh/trimesh/blob/4.4.1/trimesh/triangles.py#L233-L234), is that intentional?

[traversaro]

It is also not clear to me where we are accounting for the center of mass when computing the inertia properties. For all other primitive shapes, the center of mass is at the origin of the frame in which the primitive shape is expressed, but this is not true for meshes.

[traversaro]

Perhaps it is better to just raise an error, instead of specifying a wrong value? So the user can specify the correct inertia if they want, instead of using a wrong value if the warning are overlooked (as it commonly happens)?

[diegoferigo]

This was done in this way for speeding up the generation of programmatic models having also meshes when correct inertial data is not needed. Setting the density of the mesh with trimesh is not a big effort, and since we have the support of automatically gathering the inertial properties from the mesh, I'm not against converting this warning to an exception.

[diegoferigo]

I am a bit confused by the mass property in MeshBuilder . In all other <Something>Builder, the mass is taken from the PrimitiveBuilder, and then the inertia is computed as proportional to the mass (so the density is taken implicitly from the mass). Instead, in the MeshBuilder there is an additional mass attribute of MeshBuilder, and if the mass is set and the inertia is not set, the mass is ignored while computing the inertia, getting instead the density from trimesh (that is anyhow not set, so I guess it will default to 1.0, see https://github.com/mikedh/trimesh/blob/4.4.1/trimesh/triangles.py#L233-L234), is that intentional?

All *Builder class, as you commented, need the user to specify the desired mass, and the defaul inertia (always expressed in the CoM since the user can define a custom one manually) is computed automatically from the shape.

Meshes are different, because it's not possible to compute the inertia automatically unless, as you linked, the density is set and the mesh is watertight. The MeshBuilder allows the following cases:

• The user creates the MeshBuilder specifying both the mass and the inertia tensor expressed at the CoM.
• The user creates the MeshBuilder from a mesh they developed and control (or at least, access and update programmatically), therefore they can store the proper information to let trimesh compute automatically the inertial properties.

It is also not clear to me where we are accounting for the center of mass when computing the inertia properties. For all other primitive shapes, the center of mass is at the origin of the frame in which the primitive shape is expressed, but this is not true for meshes.

I think this is related to what I asked in a previous review #33 (comment). Right now, all the builders expect the inertia expressed in a frame located at the CoM. If this is not the case, the user can specify a different pose of such frame when they call: link = builder.build_link().add_inertial(pose=Pose(...)).add_visual().add_collision().build().

[traversaro]

Thanks, the explanation of the different use case is more clear. To be even more clear, I will name the following use cases you mentioned.

• UC1: The user specify directly mass and inertia com in the MeshBuilder constructor.
• UC2: The user specify directly only the mesh.

However, I am bit confused as in the tests only the mass is specified, following the use case common in the rest of the primitive builder, i.e.

• UC3: The shape and only the mass are specified, and the software computes CoM and inertia accordingly.

Just to complete the analysis, there is a UC4 in which the user overrides the inertia specified at the shape level, i.e.

• UC4: The full inertia parametes (mass, center of mass, inertia) are specified explicitly via link = builder.build_link().add_inertial(pose=Pose(...)).add_visual().add_collision().build()

In the following I provide comments for all this use cases:

UC1

• UC1: The user specify directly mass and inertia com in the MeshBuilder constructor.

I think this is related to what I asked in a previous review #33 (comment).

Actually this was referred to the pose of the mesh, that is a transform between the link frame and the frame in which the mesh (i.e. the point composing the mesh) is expressed. For a generic mesh, there is no constraint for the center of mass of the mesh to lay at the origin of the frame in which the point of the mesh is expressed.

In general, a mesh can have a CoM in a point different from the origin of the frame in which the mesh is expressed (diffently from the primitive shapes currently supported), so I think it make sense to permit the user to also specify the CoM in this API.

UC2

• UC2: The user specify directly only the mesh.

The user creates the MeshBuilder from a mesh they developed and control (or at least, access and update programmatically), therefore they can store the proper information to let trimesh compute automatically the inertial properties.

I see two main problems in this use case:

• The mesh are specified via stl, dae, obj or similar mesh. Most of these formats do not support specifying the density, and even for the one that support it (like Collada, I guess) trimesh is not actually loading the density, see https://github.com/search?q=repo%3Amikedh%2Ftrimesh%20density&type=code . So even if you control the mesh, how can you specify the density of the mesh in trimesh if the trimesh object is just an internal local object of the __post_init__ method?
• Even if the density was specified, this workflow is not propagating the CoM of the mesh to the link (see comments

UC3

• UC3: The shape and only the mass are specified, and the software computes CoM and inertia accordingly.

Apparently this workflow is the one used in the tests.

Meshes are different, because it's not possible to compute the inertia automatically unless, as you linked, the density is set and the mesh is watertight. The MeshBuilder allows the following cases:

Just fyi the only condition if for the mesh to be watertight. As long as as the mesh is watertight, you can compute the density corresponding to a given mass as mass/volume, and you can compute the inertia at the CoM as InertiaWithRodMass = ((mass/volume)/trimeshdensity)*InertiaWithTrimeshdensity . So a possible strategy is to permit to compute the inertia from the mass, and just raise an error if the mesh is not watertight. However, in this case you also need to properly propagate the CoM from the mesh, that is not currently accounted for (see previous comments).

UC4

• UC4: The full inertia parametes (mass, center of mass, inertia) are specified explicitly via link = builder.build_link().add_inertial(pose=Pose(...)).add_visual().add_collision().build()

From what I understand, this UC seems clear.

[traversaro]

Ah, probably I would at least explicitly disallow the possibility of not setting the mass but setting explicitly the inertia.

[diegoferigo]

Getting back on this PR. Thanks for the feedback @traversaro!

For the moment, I'm focusing on other things with higher priority. Let's see when I find some time to process your suggestions. Leaving this PR as stale for a more while.

[CarlottaSartore]

@diegoferigo @traversaro what is the status of this PR ? thanks

[diegoferigo]

This PR streamlines building links with mesh-like collision and visual elements. There are open discussion points on how to handle the mass and inertia tensor, and specifically how to rely on trimesh for some sort of automatic computation from the mesh volume. If done well, the resulting logic (together with the mesh scale) could be taken as inspiration for extending hw optimization to meshed links.

It's something that need proper thought and testing. Personally, I won't have enough time to dedicate on this in the short term.

[CarlottaSartore]

@diegoferigo I will take it then thanks.

Is there an MVP PR you would like to merge before I take over ?

[diegoferigo]

Not really, excluding what detailed by @traversaro in #38 (comment), that was mainly related to the computation of mass|inertia tensor|CoM displacement, this PR was already working (if I recall) in the simplified use case that was first proposed by @lorycontixd. All Silvo's comments were additional considerations to make the final usage more complete and sound.

[CarlottaSartore]

Sorry, can’t we merge the simplified case used by @lorycontixd and defer improvements una. Subsequent PR ? @traversaro @diegoferigo

[traversaro]

Sure, no problem for me in merging as it is.

[diegoferigo]

Merging with failing isort since it will get fixed in #46.