rapier icon indicating copy to clipboard operation
rapier copied to clipboard

`ColliderBuilder::trimesh` doesn't return an error, and does not document potential panics

Open Astralchroma opened this issue 1 year ago • 4 comments

ColliderBuilder::trimesh doesn't return an error, and does not document potential panics.

panicked at .cargo/registry/src/index.crates.io-6f17d22bba15001f/parry3d-0.17.0/src/shape/trimesh.rs:268:9:
A triangle mesh must contain at least one triangle.

The potential for this panic should be at least documented, though given the error message, it seems like this should instead be returning a Result

Astralchroma avatar Aug 14 '24 14:08 Astralchroma

Thanks for the report!

Ideally, we'd want to return an error indeed.

ThierryBerger avatar Sep 04 '24 13:09 ThierryBerger

I think we'll have to do some changes in shared_shape.rs and trimesh.rs. We may add a custom TrimeshError in trimesh.rs and return a Result<> utilizing this custom error from trimesh() function in the implementation of SharedShape.

Thus we can handle this result in collider.rs of this repo to return error.

Hope I'm not over complicating it.

Soham1803 avatar Sep 08 '24 15:09 Soham1803

So, shall I start working on this issue or the work already is in progress. Lmk if I could help somewhere.

Soham1803 avatar Sep 12 '24 10:09 Soham1803

The work has begun upstream on parry :

  • https://github.com/dimforge/parry/pull/262

I’ll have to update rapier’s handling of those new return types when we the above is merged and we update the parry dependency, but if you want to anticipate and help with that you’re most welcome 🙏🏼 ; that means targeting that branch and do a bit of maintenance at different time points (when it’s merged, when/if other API changes…). To be clear the maintaining/finishing line can be done by rapier’s maintainers if need be

ThierryBerger avatar Sep 12 '24 12:09 ThierryBerger