Skip to content
This repository has been archived by the owner on Nov 29, 2022. It is now read-only.

Add cone shape #158

Merged
merged 3 commits into from
Nov 28, 2021
Merged

Add cone shape #158

merged 3 commits into from
Nov 28, 2021

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Nov 27, 2021

Note: I didn't add #[non_exhaustive] to CollisionShape because of the match in rapier/src/shape.rs line 236. It would otherwise require adding a _ => panic!("shape not supported"), branch to the match expression. I thought it would be more questionable than not adding #[non_exhaustive].

@jcornaz
Copy link
Owner

jcornaz commented Nov 27, 2021

Hi, thank you very much :-)

In the current state of CollisionShape, I'd consider that we have a bug. Because there is one variant behind a feature flag (Cone behind the 3d feature), it means a consumer code that compiles correctly could suddently no longer compile if the feature 3d is enabled (and that may even be done by another dependency of the consumer code). In other words we violate this rule: https://doc.rust-lang.org/cargo/reference/features.html#semver-compatibility. And I'd consider this behavior as a bug of heron.

So we have two choices to solve it: Either we add #[non_exhaustive] on the collision shape. Or we don't put the cone variant behind a #[cfg(feature = "dim3")]. I have a strong preference for the #[non_exhaustive]solution. Because it would also bring the benefit that future new shapes won't be a breaking change. And having a 2d cone implementation seams like unecessary confusion and complexity to me. So, I'd say we do add #[non_exhaustive].

Of course, that forces us to implement a _ => .... arm in the pattern matching of the renderer. There we could indeed panic (unreachable!()), which would be okay-ish. As long as it cannot actually happen, we at least don't have a bug. But of course, that would be a fragility, because it would be very easy to forget to update the renderer when we add a new collision shape, chich means it may be too easy to introduce bugs in the future. But panicking is not the only solution! We may decide to render nothing, and log a warning for the consumer: warn!("There are collision shapes that are not yet supported by the debug renderer"), so that if we "forget" to update the renderer when we add a new shape, it at least doesn't lead to a crash.

@nicopap
Copy link
Contributor Author

nicopap commented Nov 27, 2021

You make a good case for non_exhaustive I added it to the PR. Still looking into the failing tests

Copy link
Owner

@jcornaz jcornaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for your contribution :-)

@jcornaz jcornaz enabled auto-merge (squash) November 28, 2021 08:41
@jcornaz jcornaz merged commit ea5dd3b into jcornaz:main Nov 28, 2021
@jcornaz jcornaz mentioned this pull request Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants