Skip to content
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

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

Open
Astralchroma opened this issue Aug 14, 2024 · 4 comments
Labels
2D The issue is specifically about the 2D version of Rapier. 3D The issue is specifically about the 3D version of Rapier. A-Geometry C-Bug Something isn't working D-Medium P-Medium

Comments

@Astralchroma
Copy link

Astralchroma commented Aug 14, 2024

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 added a commit to Astralchroma/Solarscape that referenced this issue Aug 14, 2024
@Vrixyz
Copy link
Contributor

Vrixyz commented Sep 4, 2024

Thanks for the report!

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

@Vrixyz Vrixyz added C-Bug Something isn't working 2D The issue is specifically about the 2D version of Rapier. 3D The issue is specifically about the 3D version of Rapier. A-Geometry P-Medium D-Medium labels Sep 4, 2024
@Soham1803
Copy link
Contributor

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
Copy link
Contributor

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

@Vrixyz
Copy link
Contributor

Vrixyz commented Sep 12, 2024

The work has begun upstream on parry :

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2D The issue is specifically about the 2D version of Rapier. 3D The issue is specifically about the 3D version of Rapier. A-Geometry C-Bug Something isn't working D-Medium P-Medium
Projects
None yet
Development

No branches or pull requests

3 participants