-
-
Notifications
You must be signed in to change notification settings - Fork 23.3k
Add support for per-shape physics materials #100748
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
base: master
Are you sure you want to change the base?
Conversation
6079b03
to
198281e
Compare
3afbf17
to
0713122
Compare
Fixed the xml method order and added support for concave shapes. |
0713122
to
0589886
Compare
Added support for changing materials at runtime. I had to add a new custom shape type |
08080d6
to
e72672f
Compare
@AThousandShips I guess this is ready for review. If you see something wrong or confusing let me know |
e72672f
to
65418b3
Compare
@jrouwe thanks! I saw |
49b2190
to
07de536
Compare
07de536
to
870c7db
Compare
Renamed |
870c7db
to
8967fc0
Compare
Rebased on latest |
7a3ac0c
to
0456e35
Compare
@mihe thanks for the review just merged all the changes. I see you removed some casts to |
0456e35
to
c50f336
Compare
@mihe this good to go now or you still got questions? just wanna make sure everything clean before it gets merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a fairly critical issue with the Godot Physics implementation, and some very minor cosmetic remarks in the Jolt implementation.
Outside of these review comments, I must admit that I'm becoming more and more uncomfortable with the API of these two PhysicsServer*D
methods:
void body_set_shape_friction_override(RID p_body, int p_shape_idx, bool p_enable, real_t p_friction = 0.0);
void body_set_shape_bounce_override(RID p_body, int p_shape_idx, bool p_enable, real_t p_bounce = 0.0);
I realize it's a bit late to be having this conversation, and that the current design stems from feedback that have been given here already, so I won't push too hard on this, but the whole thing with having p_enable
there, and then defaulting the values to 0.0
, just so you can pass false
without needing to give it a value. It makes for an odd-looking API in my opinion.
Unfortunately I struggle to think of an alternative that wouldn't ideally require a bool
to be added for each material state as well, which seems like a hefty price to pay for what is only (subjectively) a better API.
Anyway, I just figured I'd voice my concern at least, in case anyone had any other ideas, or know of other Godot APIs that do something similar already.
6fb0b72
to
269885f
Compare
@mihe switched this to use shape owner methods can you take a look if i did it right |
@Calinou looks like mihe doesnt have time to review. is anyone else free right now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again, it works as expected with GodotPhysics3D and Jolt Physics. Code and documentation look good to me.
I suggest having mihe take a final look at the API still, just in case. We can't change what's exposed once it lands in a stable release, so we need to make sure it's rock-solid.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another pass on this and found some bugs with the recent changes, which you'll find in the review comments. I also threw in some more nitpicks while I was at it.
I've also spent some more time thinking about the PhysicsServer*D
API, and I think I've arrived at a better API for this:
func body_set_shape_friction_override(body: RID, shape_idx: int, friction: float) -> void
func body_get_shape_friction(body: RID, shape_idx: int) -> float
func body_is_shape_friction_overridden(body: RID, shape_idx: int) -> bool
func body_clear_shape_friction_override(body: RID, shape_idx: int) -> void
This gets rid of NAN
from the API completely, by having body_get_shape_friction
fall back on the body's friction when there is no override set, while still not burdening implementers with a separate enabled state.
To save us some time, I've gone ahead and made these changes already, and just like before you'll find the changes here (patch here), which also includes fixes for the new review comments.
Like before, if you have no objections to these changes, I can go ahead and push directly to your fork/branch as well. We're only a few days away from the 4.5 feature freeze, and I don't see anything else in this PR that should prevent this from being merged by then.
EDIT: Changes rebased on top of latest push (de20ef0).
_FORCE_INLINE_ bool is_area() const { return area; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed anymore.
_FORCE_INLINE_ bool is_area() const { return area; } |
_FORCE_INLINE_ bool is_area() const { return area; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as with 2D.
_FORCE_INLINE_ bool is_area() const { return area; } |
if (shape.is_null()) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check doesn't make sense anymore.
if (shape.is_null()) { | |
return; | |
} |
if (shape.is_null()) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as with 2D.
if (shape.is_null()) { | |
return; | |
} |
return debug_color; | ||
} | ||
|
||
void CollisionShape2D::set_physics_material(const Ref<PhysicsMaterial> &p_material) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not disconnecting from the changed
signal when the physics material is cleared. See CollisionShape2D::set_shape
for reference.
GDVIRTUAL_BIND(_body_set_shape_bounce_override, "body", "shape_idx", "enable", "bounce"); | ||
GDVIRTUAL_BIND(_body_set_shape_friction_override, "body", "shape_idx", "enable", "friction"); | ||
|
||
GDVIRTUAL_BIND(_body_get_shape_bounce_override, "body", "shape_idx"); | ||
GDVIRTUAL_BIND(_body_get_shape_friction_override, "body", "shape_idx"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nitpicky, but in most other places friction is listed before bounce.
GDVIRTUAL_BIND(_body_set_shape_bounce_override, "body", "shape_idx", "enable", "bounce"); | |
GDVIRTUAL_BIND(_body_set_shape_friction_override, "body", "shape_idx", "enable", "friction"); | |
GDVIRTUAL_BIND(_body_get_shape_bounce_override, "body", "shape_idx"); | |
GDVIRTUAL_BIND(_body_get_shape_friction_override, "body", "shape_idx"); | |
GDVIRTUAL_BIND(_body_set_shape_friction_override, "body", "shape_idx", "enable", "friction"); | |
GDVIRTUAL_BIND(_body_set_shape_bounce_override, "body", "shape_idx", "enable", "bounce"); | |
GDVIRTUAL_BIND(_body_get_shape_friction_override, "body", "shape_idx"); | |
GDVIRTUAL_BIND(_body_get_shape_bounce_override, "body", "shape_idx"); |
ClassDB::bind_method(D_METHOD("body_set_shape_bounce_override", "body", "shape_idx", "enable", "bounce"), &PhysicsServer2D::body_set_shape_bounce_override, DEFVAL(0.0)); | ||
ClassDB::bind_method(D_METHOD("body_set_shape_friction_override", "body", "shape_idx", "enable", "friction"), &PhysicsServer2D::body_set_shape_friction_override, DEFVAL(0.0)); | ||
|
||
ClassDB::bind_method(D_METHOD("body_get_shape_bounce_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_bounce_override); | ||
ClassDB::bind_method(D_METHOD("body_get_shape_friction_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_friction_override); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nitpick about the ordering here.
ClassDB::bind_method(D_METHOD("body_set_shape_bounce_override", "body", "shape_idx", "enable", "bounce"), &PhysicsServer2D::body_set_shape_bounce_override, DEFVAL(0.0)); | |
ClassDB::bind_method(D_METHOD("body_set_shape_friction_override", "body", "shape_idx", "enable", "friction"), &PhysicsServer2D::body_set_shape_friction_override, DEFVAL(0.0)); | |
ClassDB::bind_method(D_METHOD("body_get_shape_bounce_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_bounce_override); | |
ClassDB::bind_method(D_METHOD("body_get_shape_friction_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_friction_override); | |
ClassDB::bind_method(D_METHOD("body_set_shape_friction_override", "body", "shape_idx", "enable", "friction"), &PhysicsServer2D::body_set_shape_friction_override, DEFVAL(0.0)); | |
ClassDB::bind_method(D_METHOD("body_set_shape_bounce_override", "body", "shape_idx", "enable", "bounce"), &PhysicsServer2D::body_set_shape_bounce_override, DEFVAL(0.0)); | |
ClassDB::bind_method(D_METHOD("body_get_shape_friction_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_friction_override); | |
ClassDB::bind_method(D_METHOD("body_get_shape_bounce_override", "body", "shape_idx"), &PhysicsServer2D::body_get_shape_bounce_override); |
if (sd.material == p_material) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, since it'll keep the same reference even when we change values within the material.
if (sd.material == p_material) { | |
return; | |
} |
if (sd.material == p_material) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as with 2D.
if (sd.material == p_material) { | |
return; | |
} |
#include "godot_area_2d.h" | ||
#include "godot_body_direct_state_2d.h" | ||
#include "godot_constraint_2d.h" | ||
#include "godot_physics_server_2d.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
As another last-minute decision, I've omitted the If anyone needs this in the future it can trivially be added then instead. So the func body_set_shape_friction_override(body: RID, shape_idx: int, friction: float) -> void
func body_clear_shape_friction_override(body: RID, shape_idx: int) -> void
func body_get_shape_friction(body: RID, shape_idx: int) -> float The branch/patch linked above should have updated accordingly, but here (patch here) it is again if you need it. EDIT: Changes have rebased on top of latest push (de20ef0). |
269885f
to
de20ef0
Compare
Does this have any chance of working with Heightmap Shapes? (I belive these construct a grid of Quad shapes under the hood, that are tested for in a specific way due to the nature of heightmaps.) In Terrain3D I would like to be able to send a physics material "ID" for each Quad. Since We have access to an index map that could determine the dominant texture for any given quad, and that texture has other properties associated with it already, linking each texture ID (upto 32) to a physics material, would essentially Let us have paintable physics materials. |
@Xtarsia Not as-is, no. The same goes for individual triangles in Either way, we'd need some way of actually assigning material values to groups of faces, both from a UX perspective and from an API perspective, which is likely quite a bit of work, and better suited for a future PR. The texture approach might make some sense for |
This lets users override bounce and friction for individual collision shapes instead of using the same properties for the whole body.
f59f57b
to
2dd8a5b
Compare
Closes godotengine/godot-proposals#7401.
This PR adds:
shape_set_friction
,shape_get_friction
,shape_set_bounce
,shape_get_bounce
toPhysicsServer3D
physics_material
toShape3D
This is implemented for both Godot and Jolt Physics (using a custom Jolt physics material like one of the official samples).
Example project that uses two different materials on one body: per-shape-materials.zip