Implement interaction groups test mode and cofficient combine rule #741
Conversation
Ughuuu
commented
Sep 22, 2024
Ughuuu
commented
- Continues Add InteractionGroups::test_or, increase to 64 bits #170
- Fixes Feature: Add support for custom friction and bounce CombineRules #622
|
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) |
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
update fix docs cargo format fix update feedback update Update CHANGELOG.md ignore line for error docs fix update
Ughuuu
force-pushed
the
Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2
branch
from
bb2b387 to
0db7c72
Compare
|
Fixed the doc test from CI. |
| match effective_rule { | ||
| 0 => (coeff1 + coeff2) / 2.0, | ||
| 1 => coeff1.min(coeff2), | ||
| 1 => coeff1.min(coeff2).abs(), |
There was a problem hiding this comment.
Should it be max(0.0) ?
There was a problem hiding this comment.
(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not sure about the godot use case, this is just the exact code they have right now, so for parity I have that. Their use case is that they also have negative coefficients, and use this with absorbent field.
https://github.com/godotengine/godot/blob/0eadbdb5d0709e4e557e52377fa075d3e2f0ad1f/scene/resources/physics_material.h#L57
Then they do abs here, so the negative one becomes positive:
https://github.com/godotengine/godot/blob/0eadbdb5d0709e4e557e52377fa075d3e2f0ad1f/modules/godot_physics_3d/godot_body_pair_3d.cpp#L259
There was a problem hiding this comment.
Should i add a comment in code around that match explaining that the coefficient can be negative in some cases?
There was a problem hiding this comment.
Should i add a comment in code around that match explaining that the coefficient can be negative in some cases?
Yes ; you can add a link to this discussion so the reader knows where this comes from 👍
Godot documentation for rough and absorbent is also relevant: https://docs.godotengine.org/en/stable/classes/class_physicsmaterial.html#class-physicsmaterial
sebcrozet
left a comment
sebcrozet
left a comment
There was a problem hiding this comment.
Thank you for this PR. Just a couple of clarifications needed and it’s good to go.
(Sorry for the delay for reviewing this.)
| match effective_rule { | ||
| 0 => (coeff1 + coeff2) / 2.0, | ||
| 1 => coeff1.min(coeff2), | ||
| 1 => coeff1.min(coeff2).abs(), |
There was a problem hiding this comment.
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.
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
…-add-more-rules-take-2
ThierryBerger
force-pushed
the
Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2
branch
from
43c28fc to
636c01b
Compare
sebcrozet
left a comment
sebcrozet
left a comment
There was a problem hiding this comment.
Thank you for this PR. Just a nitpick and it’s good to go.
|
Thank you! |
3 participants
