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

Add warning for using concave shape on CharacterBody3D #86576

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

bikemurt
Copy link
Contributor

@bikemurt bikemurt commented Dec 28, 2023

As per #86574

Tested a simple scene to confirm that ConcaveShapes do not work for collisions on CharacterBody3D (same as RigidBody3D).

Added the following node config warning:
image

I didn't update the language documentation as I assume that's handled separately

Bugsquad edit:

@bikemurt bikemurt requested a review from a team as a code owner December 28, 2023 12:41
@AThousandShips AThousandShips added this to the 4.x milestone Dec 28, 2023
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This wording isn't correct, it should be more clear, CharacterBody3D does support this shape, it just isn't recommended:

When used for collision, ConcavePolygonShape3D is intended to work with static CollisionShape3D nodes like StaticBody3D and will likely not behave well for CharacterBody3Ds or RigidBody3Ds in a mode other than Static.

The warning should match the one for RigidBody3D instead

@bikemurt
Copy link
Contributor Author

ConcavePolygonShape3D doesn't support RigidBody3D in another mode than static.

RigidBody3D uses this wording, so would we change both of them?

Initially I found this description to be wordy, but I see what @AThousandShips is saying - you can use concave shapes, it just won't work with moving bodies.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 28, 2023

The warning should be descriptive, so it shouldn't be changed for RigidBody3D IMO, but both could be updated, on second thought I'm not 100% sure what is meant by the static mode here, or if it applies to both types, the documentation for the shape should probably be improved to clarify this 🙂

I would suggest to update both descriptions to be clearer that it's not recommended to use these

Edit: It might refer to the freeze mode of RigidBody3D, but that is very vague and unclear, unsure how to approach this here

@AThousandShips AThousandShips requested a review from a team December 28, 2023 12:59
@AThousandShips
Copy link
Member

AThousandShips commented Dec 28, 2023

The physics team would have to confirm but I think the documentation for the shape needs to be made more clear, something like:

When used for collision, ConcavePolygonShape3D is intended to work with static CollisionShape3D nodes like StaticBody3D and will likely not behave well for or RigidBody3Ds (except when frozen and freeze_mode set to FREEZE_MODE_STATIC) or CharacterBody3Ds.

Or something like that

And then change all the wordings here to align with that, saying something like "not recommended", take the scale warning below as a reference

I think wordiness here is not a problem, on the contrary the warning should be clear and detailed to avoid confusion and misunderstandings 🙂

@bikemurt
Copy link
Contributor Author

bikemurt commented Dec 28, 2023

I just did a little bit of testing by using a RigidBody3D (with convex collision shape) and pushing it into a RigidBody3D and CharacterBody3D, both with concave shapes.

When the RigidBody3D is set to freeze/static, the concave shape does actually work for collisions. It behaves exactly like a StaticBody3D in that case.

I think CharacterBody3D does not work at all with a concave collision shape (as far as I can tell). It doesn't interact with a StaticBody3D (my floor) or the RigidBody3D with a convex shape.

When used for collision, ConcavePolygonShape3D is intended to work with static CollisionShape3D nodes like StaticBody3D and will likely not behave well for or RigidBody3Ds (except when frozen and freeze_mode set to FREEZE_MODE_STATIC) or CharacterBody3Ds.

I like this. Could split out the description for RigidBody3D and CharacterBody3D:

RigidBody3D:

When used for collision, ConcavePolygonShape3D is intended to work with static CollisionShape3D nodes like StaticBody3D and will likely not behave well for or RigidBody3Ds (except when frozen and freeze_mode set to FREEZE_MODE_STATIC).

CharacterBody3D:

When used for collision, ConcavePolygonShape3D is intended to work with static CollisionShape3D nodes like StaticBody3D and will likely not behave well for CharacterBody3Ds.

Edit: looks like there is probably one more comment to clean up, but I haven't worked with WorldBoundaryShape3Ds so I'm not sure what the comment should actually be.

WorldBoundaryShape3D doesn't support RigidBody3D in another mode than static.

@bikemurt
Copy link
Contributor Author

Here's what I have for now. Also added a new line so it looks a little cleaner:

image

@milkiq
Copy link
Contributor

milkiq commented Dec 28, 2023

Here's what I have for now. Also added a new line so it looks a little cleaner:

image

It looks nice! Easier to understand than before

scene/3d/collision_shape_3d.cpp Outdated Show resolved Hide resolved
scene/3d/collision_shape_3d.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I suggest adding the same warning for VehicleBody3D and VehicleWheel3D, as these have the same constraints due to their dynamic nature.

@bikemurt
Copy link
Contributor Author

bikemurt commented Jan 1, 2024

I suggest adding the same warning for VehicleBody3D and VehicleWheel3D, as these have the same constraints due to their dynamic nature.

Makes sense. I took another look, and it appears that VehicleBody3D is (currently) the only node that inherits RigidBody3D. So I updated the code to use a string substitution in the RTR - not sure if we prefer writing out the strings verbosely.

Also, to me it seems that VehicleWheel3D shouldn't have a CollisionShape3D added to it since it doesn't inherit CollisionObject3D, so I didn't add a warning for that.

Edit: I should say that there's a separate warning for attempting to parent a CollisionShape3D to a VehicleWheel3D, so that's covered

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 4, 2024
Additionally updated descriptions for RigidBody3D and VehicleBody3D
@bikemurt bikemurt force-pushed the no-concave-warnings branch from d03e12d to 6838fe3 Compare January 4, 2024 14:34
@bikemurt
Copy link
Contributor Author

bikemurt commented Jan 4, 2024

Sorry - missed that in the documentation for amending commits. Will do it that way next time

@akien-mga akien-mga merged commit f4059d0 into godotengine:master Jan 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No warnings when using ConcavePolygonShape3D with CharacterBody3D
5 participants