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

Implement interaction groups test mode and cofficient combine rule #741

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,17 @@

### Added

- `InteractionTestMode`: Specifies which method should be used to test interactions. Supports `AND` and `OR`.
- `CoefficientCombineRule::Sum` - Adds the two coefficients and does a clamp to have at most 1.
- `RigidBodySet` and `ColliderSet` have a new constructor `with_capacity`.

### Modified

- `InteractionGroups` default value for `memberships` is now `GROUP_1` (#706)
- `ImpulseJointSet::get_mut` has a new parameter `wake_up: bool`, to wake up connected bodies.
- `InteractionGroups` struct now contains `InteractionTestMode`. Continues [rapier/pull/170](https://github.com/dimforge/rapier/pull/170) for [rapier/issues/622](https://github.com/dimforge/rapier/issues/622)
- `InteractionGroups` constructor now requires an `InteractionTestMode` parameter. If you want same behaviour as before, use `InteractionTestMode::AND` (eg. `InteractionGroups::new(Group::GROUP_1, Group::GROUP_1, InteractionTestMode::AND)`)
- `CoefficientCombineRule::Min` - now makes sure it uses a non zero value as result by using `coeff1.min(coeff2).abs()`

## v0.22.0 (20 July 2024)

Expand Down
6 changes: 4 additions & 2 deletions examples2d/collision_groups2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ pub fn init_world(testbed: &mut Testbed) {
/*
* Setup groups
*/
const GREEN_GROUP: InteractionGroups = InteractionGroups::new(Group::GROUP_1, Group::GROUP_1);
const BLUE_GROUP: InteractionGroups = InteractionGroups::new(Group::GROUP_2, Group::GROUP_2);
const GREEN_GROUP: InteractionGroups =
InteractionGroups::new(Group::GROUP_1, Group::GROUP_1, InteractionTestMode::AND);
const BLUE_GROUP: InteractionGroups =
InteractionGroups::new(Group::GROUP_2, Group::GROUP_2, InteractionTestMode::AND);

/*
* A green floor that will collide with the GREEN group only.
Expand Down
6 changes: 4 additions & 2 deletions examples3d/collision_groups3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ pub fn init_world(testbed: &mut Testbed) {
/*
* Setup groups
*/
const GREEN_GROUP: InteractionGroups = InteractionGroups::new(Group::GROUP_1, Group::GROUP_1);
const BLUE_GROUP: InteractionGroups = InteractionGroups::new(Group::GROUP_2, Group::GROUP_2);
const GREEN_GROUP: InteractionGroups =
InteractionGroups::new(Group::GROUP_1, Group::GROUP_1, InteractionTestMode::AND);
const BLUE_GROUP: InteractionGroups =
InteractionGroups::new(Group::GROUP_2, Group::GROUP_2, InteractionTestMode::AND);

/*
* A green floor that will collide with the GREEN group only.
Expand Down
12 changes: 10 additions & 2 deletions examples3d/vehicle_joints3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ pub fn init_world(testbed: &mut Testbed) {

let body_co = ColliderBuilder::cuboid(0.65, 0.3, 0.9)
.density(100.0)
.collision_groups(InteractionGroups::new(CAR_GROUP, !CAR_GROUP));
.collision_groups(InteractionGroups::new(
CAR_GROUP,
!CAR_GROUP,
InteractionTestMode::AND,
));
let body_rb = RigidBodyBuilder::dynamic()
.position(body_position.into())
.build();
Expand Down Expand Up @@ -85,7 +89,11 @@ pub fn init_world(testbed: &mut Testbed) {
// is mathematically simpler than a cylinder and cheaper to compute for collision-detection.
let wheel_co = ColliderBuilder::ball(wheel_radius)
.density(100.0)
.collision_groups(InteractionGroups::new(CAR_GROUP, !CAR_GROUP))
.collision_groups(InteractionGroups::new(
CAR_GROUP,
!CAR_GROUP,
InteractionTestMode::AND,
))
.friction(1.0);
let wheel_rb = RigidBodyBuilder::dynamic().position(wheel_center.into());
let wheel_handle = bodies.insert(wheel_rb);
Expand Down
6 changes: 5 additions & 1 deletion src/dynamics/coefficient_combine_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ pub enum CoefficientCombineRule {
Multiply,
/// The greatest coefficient is chosen.
Max,
/// The sum of the two coefficients.
Sum,
}

impl CoefficientCombineRule {
Expand All @@ -27,8 +29,10 @@ impl CoefficientCombineRule {

match effective_rule {
0 => (coeff1 + coeff2) / 2.0,
1 => coeff1.min(coeff2),
1 => coeff1.min(coeff2).abs(),
Copy link
Contributor

@Vrixyz Vrixyz Nov 15, 2024

Choose a reason for hiding this comment

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

Should it be max(0.0) ?

Copy link
Contributor

@Vrixyz Vrixyz Nov 15, 2024

Choose a reason for hiding this comment

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

(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?

Copy link
Member

Choose a reason for hiding this comment

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

Should it be max(0.0) ?

I think there is some use-case for this in godot but that would be worth clarifying by @Ughuuu.

(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?

I don’t think there is a particular reason for this.

2 => coeff1 * coeff2,
4 => (coeff1 + coeff2).clamp(0.0, 1.0),
// 3 is missing as Max is the default one in case of mismatch.
_ => coeff1.max(coeff2),
}
}
Expand Down
59 changes: 49 additions & 10 deletions src/geometry/interaction_groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,18 @@
/// - The interaction groups filter.
///
/// An interaction is allowed between two filters `a` and `b` when two conditions
/// are met simultaneously:
/// are met simultaneously for [`InteractionTestMode::AND`] or individually for [`InteractionTestMode::OR`]::
/// - The groups membership of `a` has at least one bit set to `1` in common with the groups filter of `b`.
/// - The groups membership of `b` has at least one bit set to `1` in common with the groups filter of `a`.
///
/// In other words, interactions are allowed between two filter iff. the following condition is met:
/// In other words, interactions are allowed between two filter iff. the following condition is met
/// for [`InteractionTestMode::AND`]:
/// ```ignore
/// (self.memberships & rhs.filter) != 0 && (rhs.memberships & self.filter) != 0
/// (self.memberships.bits() & rhs.filter.bits()) != 0 && (rhs.memberships.bits() & self.filter.bits()) != 0
/// ```
/// or for [`InteractionTestMode::OR`]:
/// ```ignore
/// (self.memberships.bits() & rhs.filter.bits()) != 0 || (rhs.memberships.bits() & self.filter.bits()) != 0
/// ```
#[cfg_attr(feature = "serde-serialize", derive(Serialize, Deserialize))]
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
Expand All @@ -23,25 +28,39 @@ pub struct InteractionGroups {
pub memberships: Group,
/// Groups filter.
pub filter: Group,
/// Interaction test mode
pub test_mode: InteractionTestMode,
}

#[cfg_attr(feature = "serde-serialize", derive(Serialize, Deserialize))]
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, Default)]
/// Specifies which method should be used to test interactions
pub enum InteractionTestMode {
/// Use [`InteractionGroups::test_and`].
#[default]
AND,
/// Use [`InteractionGroups::test_or`].
OR,
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

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

The case is not idiomatic:

Suggested change
#[default]
AND,
/// Use [`InteractionGroups::test_or`].
OR,
#[default]
And,
/// Use [`InteractionGroups::test_or`].
Or,

Copy link
Member

@sebcrozet sebcrozet Nov 15, 2024

Choose a reason for hiding this comment

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

@Vrixyz Can you check why the non-idiomatic case wasn’t caught by CI please? I feel like this should have been a warning (that is denied in CI).

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be caught with this clippy lint:

https://rust-lang.github.io/rust-clippy/master/index.html#upper_case_acronyms

with additional options in clippy.toml :

avoid-breaking-exported-api = false
upper-case-acronyms-aggressive = true

Unfortunately, This option also warns against SAPRegion and similar (+ TOIEntry, CCDSolver...) . We could suppress it for those specifics.

}

impl InteractionGroups {
/// Initializes with the given interaction groups and interaction mask.
pub const fn new(memberships: Group, filter: Group) -> Self {
pub const fn new(memberships: Group, filter: Group, test_mode: InteractionTestMode) -> Self {
Self {
memberships,
filter,
test_mode,
}
}
Ughuuu marked this conversation as resolved.
Show resolved Hide resolved

/// Allow interaction with everything.
pub const fn all() -> Self {
Self::new(Group::ALL, Group::ALL)
Self::new(Group::ALL, Group::ALL, InteractionTestMode::AND)
}

/// Prevent all interactions.
pub const fn none() -> Self {
Self::new(Group::NONE, Group::NONE)
Self::new(Group::NONE, Group::NONE, InteractionTestMode::AND)
}

/// Sets the group this filter is part of.
Expand All @@ -59,21 +78,41 @@ impl InteractionGroups {
/// Check if interactions should be allowed based on the interaction memberships and filter.
///
/// An interaction is allowed iff. the memberships of `self` contain at least one bit set to 1 in common
/// with the filter of `rhs`, and vice-versa.
/// with the filter of `rhs`, **and** vice-versa.
#[inline]
pub const fn test(self, rhs: Self) -> bool {
// NOTE: since const ops is not stable, we have to convert `Group` into u32
// to use & operator in const context.
pub const fn test_and(self, rhs: Self) -> bool {
(self.memberships.bits() & rhs.filter.bits()) != 0
&& (rhs.memberships.bits() & self.filter.bits()) != 0
}

/// Check if interactions should be allowed based on the interaction memberships and filter.
///
/// An interaction is allowed iff. the groups of `self` contain at least one bit set to 1 in common
/// with the mask of `rhs`, **or** vice-versa.
#[inline]
pub const fn test_or(self, rhs: Self) -> bool {
(self.memberships.bits() & rhs.filter.bits()) != 0
|| (rhs.memberships.bits() & self.filter.bits()) != 0
}

/// Check if interactions should be allowed based on the interaction memberships and filter.
///
/// See [`InteractionTestMode`] for more info.
#[inline]
pub const fn test(self, rhs: Self) -> bool {
match self.test_mode {
InteractionTestMode::AND => self.test_and(rhs),
InteractionTestMode::OR => self.test_or(rhs),
}
}
Comment on lines +102 to +107
Copy link
Member

Choose a reason for hiding this comment

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

This will create some hard to predict behavior if one of the interaction groups is set to AND while the other is set to OR. In that situation, whichever is self in this test will take precedence (and predicting which collider will be first here is not possible as it depends on some chains of events happening on the broad/narrow phase). I suggest setting an explicit precedence rule where InterecationTestMode::AND always has the priority:

Suggested change
pub const fn test(self, rhs: Self) -> bool {
match self.test_mode {
InteractionTestMode::AND => self.test_and(rhs),
InteractionTestMode::OR => self.test_or(rhs),
}
}
pub const fn test(self, rhs: Self) -> bool {
if self.test_mode == InteractionTestMode::And || rhs.test_mode == InteractionTestMode::And {
self.test_and(rhs)
} else {
self.test_or(rhs)
}

This precedence rule should be explained in the InteractionTestMode and InteractionGroups docs.

}

impl Default for InteractionGroups {
fn default() -> Self {
Self {
memberships: Group::GROUP_1,
filter: Group::ALL,
test_mode: InteractionTestMode::AND,
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/geometry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub use self::contact_pair::{
pub use self::interaction_graph::{
ColliderGraphIndex, InteractionGraph, RigidBodyGraphIndex, TemporaryInteractionIndex,
};
pub use self::interaction_groups::{Group, InteractionGroups};
pub use self::interaction_groups::{Group, InteractionGroups, InteractionTestMode};
pub use self::mesh_converter::{MeshConverter, MeshConverterError};
pub use self::narrow_phase::NarrowPhase;

Expand Down