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

rapier 0.20 feature: RevoluteJoint::angle #530

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Jun 17, 2024

add helper method to get the angle of a revolute joint ; from rapier 0.20.

We noticed a bug on joints because of rapier 0.20, so this builds up on #547.

I wanted to take the ImpulseJointSet as parameter but that doesn't work with MultibodyJointSet so I settled to pass rigidbodies.

I tried to implement a Generic ImpulseJoint<JointDescription>, but that proved very impractical for functions such as sync removal. we could revisit that if one day bevy supports trait queries, but currently I feel it's not worth it.

Because we need both ImpulseJoint and RevoluteJoint from RapierContext, I couldn't settle on a better place than RapierContext for this angle_for_entity_impulse_revolute_joint function.

Failed improvements

I would have liked to make it more typed, but that's the best I could do.

I think it boils down to bevy_rapier::ImpulseJoint not exposing directly body1 and body2, which are necessary to compute the angle. I imagine it's current entity + bevy_rapier::ImpulseJoint::parent, but I went the route to get the data from rapier directly, it doesn't complicate the public facing API.

I feel a method angle(entity, entity) would only be more difficult to follow invariants.

@Vrixyz Vrixyz marked this pull request as ready for review July 4, 2024 13:36
@Vrixyz Vrixyz requested review from sebcrozet and removed request for sebcrozet July 4, 2024 13:36
/// according to its [`RevoluteJoint`] description.
///
/// Parameter `entity` should have a [`ImpulseJoint`] or [`MultibodyJoint`] component with a [`JointDescription::RevoluteJoint`] variant as `data`.
pub fn angle_for_entity_impulse_revolute_joint(&self, entity: Entity) -> Option<f32> {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this could be named impulse_revolute_joint_angle (even if it takes an Entity)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My objective was to insist on the requisite for given entity to be a ImpulseJoint, but I'm ok with shortening it.

Looking at this makes me realize the documentation comment is wrong, I couldn't make this work with multibodyjoint: as multibodyjoints have multiple bodies, the same API cannot really be applied (if I understand correctly)

src/plugin/context.rs Outdated Show resolved Hide resolved
src/plugin/context.rs Outdated Show resolved Hide resolved
src/plugin/context.rs Outdated Show resolved Hide resolved
src/dynamics/revolute_joint.rs Show resolved Hide resolved
Comment on lines 31 to 58
impl JointDescription {
/// The underlying generic joint.
pub fn generic_joint(&self) -> &GenericJoint {
match self {
JointDescription::FixedJoint(j) => &j.data,
JointDescription::GenericJoint(j) => j,
JointDescription::PrismaticJoint(j) => &j.data,
JointDescription::RevoluteJoint(j) => j.data(),
JointDescription::RopeJoint(j) => j.data(),
#[cfg(feature = "dim3")]
JointDescription::SphericalJoint(j) => j.data(),
JointDescription::SpringJoint(j) => j.data(),
}
}
/// The underlying generic joint.
pub fn generic_joint_mut(&mut self) -> &mut GenericJoint {
match self {
JointDescription::FixedJoint(ref mut j) => &mut j.data,
JointDescription::GenericJoint(ref mut j) => j,
JointDescription::PrismaticJoint(ref mut j) => &mut j.data,
JointDescription::RevoluteJoint(ref mut j) => &mut j.data,
JointDescription::RopeJoint(ref mut j) => &mut j.data,
#[cfg(feature = "dim3")]
JointDescription::SphericalJoint(ref mut j) => &mut j.data,
JointDescription::SpringJoint(ref mut j) => &mut j.data,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These could be AsRef/AsMut implementations instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I'll do that.

For the story behind this explicit naming: ideally I would have liked to wrap our joints (bevy_rapier::FixedJoint, etc...) around the correct joint from rapier (rapier::FixedJoint, etc.), so a user could use the relevant functions from rapier if some functions were not exposed by bevy_rapier (it can easily be forgotten...), but I had difficulties, probably the builder had to be updated significantly so I dropped the idea.

See #529 for similar reasoning logic.

src/dynamics/joint.rs Outdated Show resolved Hide resolved
Vrixyz and others added 2 commits July 5, 2024 11:01
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants