-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mesh support #156
base: main
Are you sure you want to change the base?
Add mesh support #156
Conversation
@lorycontixd do you prefer a squash and merge or can you please clean the commit history? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lorycontixd! I wrote some initial comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @lorycontixd, it's a great start. First minor changes, I'll review the logic in more detail after these ones are addressed.
bf8bd78
to
195cb68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on trimesh usage. Almost good on my side, I'll do a final check tomorrow.
For the records, this is the function called to load a mesh in trimesh: |
@diegoferigo before merging this, do we want to set meshes support as default or should it be activated using an environment variable? |
Yes I need to properly assess the implications of merging this PR on existing appplications. Automatically adding the collidable points of all meshes is a huge change of behavior and it will make existing simulations unbearably slow. |
@diegoferigo @flferretti As of now, random point sampling from a mesh is supported. I think it would be a good idea if I tried to benchmark different values of sampling points and find a good tradeoff between performance and simulation results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost ready, last comments on the changes before started assessing the new default behavior of mesh collisions in JaxSim.
@@ -74,6 +75,7 @@ style = | |||
pre-commit | |||
testing = | |||
idyntree >= 12.2.1 | |||
networkx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this new dependency used for?
if mesh_collision is not None: | ||
collisions.append(mesh_collision) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever happen? I guess that in the case utils.create_mesh_collision
fails, an exception is raised.
if mesh.is_empty: | ||
logging.warning(f"Mesh {collision.geometry.mesh.uri} is empty, ignoring it") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can this happpen? The mesh
variable is populated by trimesh
. If the parsing fails, I'd expect that trimesh raises. If the library does not raise, I'd change the code as follows:
if mesh.is_empty: | |
logging.warning(f"Mesh {collision.geometry.mesh.uri} is empty, ignoring it") | |
return | |
if mesh.is_empty: | |
raise RuntimeError(f"Failed to process '{file}' with trimesh") |
Reminder to myself: bump the version of the ROD dependency before merging this PR. |
0dddfc1
to
c28ae05
Compare
Co-authored-by: Lorenzo Conti <[email protected]>
- Moved mesh parsing logic inside method for creating mesh collisions - Removed vs code settings - Removed empty mesh parsing test
c28ae05
to
1b2bcc8
Compare
for more information, see https://pre-commit.ci
… add_mesh_support
for more information, see https://pre-commit.ci
@lorycontixd if you see commits pushed by a bot, don't worry. We can squash those. This should stop as soon as #197 gets merged |
I was wondering 😂 Thanks for letting me know! |
- Restructured mesh_collision creation method - Removed unnecessary hash inheritance
… add_mesh_support
- Implemented AAP algorithm - Restructured collision parsing to accept the new algorithms - Wrote tests for AAP algorithm - Updated JaxSim dependencies
- New mesh wrapping algorithms (mesh decimation, object mapping, aap, select points over axis) - Implemented tests of above except first algorithm - Updated manifold3d dependency (used in object mapping) - Restructured meshes module
- Renamed some function paramaeters - Added a few tests TODO: migrate MeshMapping static class to inheritance structure
- Redefined methods using classes - Adapted rod parser to new structure - Reimplemented tests with new structure
Implement a set of new mesh wrapping algorithms for extra performance
- Removed a line that would always set the extracted points to the vertices - Added a few debug lines using logger
- Added string magics on mesh wrapping methods for easier logging - Added log to indicate number of extracted points - Added extra check on objectmapping method
Must find a way to inject the wrapping method from outside the api ( |
This pull requests aims to extend Jaxsim's parsing functionalities on robot descriptions to meshes.
It does so by using third-party library trimesh to parse the mesh file, and custom algorithm
create_mesh_collisions
to wrap a mesh with collidable points.📚 Documentation preview 📚: https://jaxsim--156.org.readthedocs.build//156/