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

Conversation

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 23, 2024

Ok, added this mainly to stop using my fork and to try to use rapier default in godot-rapier. This way I hope to no longer need to maintain that and use this repo as it is. (if this gets merged)

src/geometry/interaction_groups.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/geometry/interaction_groups.rs Outdated Show resolved Hide resolved
src/geometry/interaction_groups.rs Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2 branch 3 times, most recently from b05dc45 to 1d0db7e Compare September 24, 2024 09:26
@Ughuuu Ughuuu requested a review from Vrixyz September 26, 2024 20:23
update

fix docs

cargo format fix

update feedback

update

Update CHANGELOG.md

ignore line for error docs fix

update
@Ughuuu Ughuuu force-pushed the Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2 branch from bb2b387 to 0db7c72 Compare September 27, 2024 08:09
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 27, 2024

Fixed the doc test from CI.

@@ -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.

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. Just a couple of clarifications needed and it’s good to go.
(Sorry for the delay for reviewing this.)

Comment on lines +102 to +107
pub const fn test(self, rhs: Self) -> bool {
match self.test_mode {
InteractionTestMode::AND => self.test_and(rhs),
InteractionTestMode::OR => self.test_or(rhs),
}
}
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.

Comment on lines +40 to +43
#[default]
AND,
/// Use [`InteractionGroups::test_or`].
OR,
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.

@@ -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
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.

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.

Feature: Add support for custom friction and bounce CombineRules
3 participants