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

Restore RigidBody2/3D, SoftBody names in physics #64894

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

fabriceci
Copy link
Contributor

@fabriceci fabriceci commented Aug 25, 2022

Close: godotengine/godot-proposals#5183
Superseed #64548
Superseed #64546

I found some missing in the mentioned PR so I preferred to go back to zero to be sure as much as possible that everything is there, because this is the kind of PR that can break things.

It would be cool if this could be merged quickly because I've already had conflicts :D

@rburing If you can take a look.

@akien-mga
Copy link
Member

Header guards for SoftBody need fixing: https://github.com/godotengine/godot/runs/8022198750?check_suite_focus=true

@akien-mga
Copy link
Member

You'll have to remove the rename from the project converter: https://github.com/godotengine/godot/runs/8022198755?check_suite_focus=true

@fabriceci fabriceci force-pushed the remove-dynamic-bodies-name branch 2 times, most recently from f315e40 to 468cbcd Compare August 25, 2022 19:02
@Mickeon
Copy link
Contributor

Mickeon commented Aug 25, 2022

This may sound a bit picky, and that's because it is, but is this both of my previous PRs merged together? As in, does it also include comments and very specific mentions of these Nodes in them, such as "// rigid dynamic body"?

@fabriceci
Copy link
Contributor Author

@Mickeon I checked quickly and found some missing things (an enum, etc.), so since this is a huge change that can introduce nasty bugs, I found it easier and safer make myself from scratch, comparing it with the original PR that introduced these changes. I just felt more confident doing it that way!

@Mickeon
Copy link
Contributor

Mickeon commented Aug 25, 2022

That's ok! I just wanted to make sure our combined thoroughness made the PR more complete, including comments, too.

@Diddykonga
Copy link
Contributor

If I am being honest, this makes it even worse, now Rigid is an alias for Dynamic, which is not technically correct, although based on current pragmatic usage I guess it is. :(

@@ -1340,11 +1340,11 @@
<constant name="BODY_MODE_KINEMATIC" value="1" enum="BodyMode">
Constant for kinematic bodies. In this mode, a body can be only moved by user code and collides with other bodies along its path.
</constant>
<constant name="BODY_MODE_DYNAMIC" value="2" enum="BodyMode">
Constant for dynamic bodies. In this mode, a body can be pushed by other bodies and has forces applied.
<constant name="BODY_MODE_RIGID" value="2" enum="BodyMode">
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Would it be better to leave these as BODY_MODE_DYNAMIC*? Would they apply to soft bodies or other body types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need to understand how the bodies are organised internally to use the PhysicServer.

All bodies are RigidBodies with different modes. For example, a StaticBody is a RigidBody with infinite mass. As this can be difficult for a beginner to understand, there are several nodes.

The only exception is the SoftBody which inherits from MeshInstance.

In one of the two proposals the user has made a small tree that may help to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for clarifying!

@akien-mga akien-mga merged commit 1c97dde into godotengine:master Aug 26, 2022
@akien-mga
Copy link
Member

Thanks!

@fabriceci fabriceci deleted the remove-dynamic-bodies-name branch August 26, 2022 14:30
@JohanAR
Copy link
Contributor

JohanAR commented Aug 27, 2022

In the future, please try to include the old names in the commit message when renaming classes, e.g. "Renamed RigidDynamicBody3D to RigidBody3D". Makes it much faster to fix projects after rebasing Godot :)

@fabriceci
Copy link
Contributor Author

@JohanAR You should not do anything, the old names have been added to the compatibility classes: https://github.com/godotengine/godot/pull/64894/files#diff-e4c9d70a3ecc449ede1e49b7ea01ed9128d2931c21e284d7a50d3f63cd7a13e2R1046-R1047

@JohanAR
Copy link
Contributor

JohanAR commented Aug 27, 2022

Is that something you have to enable in the editor? Because it definitely refused to run my test project before I had renamed all the RigidDynamicBody3D in the code

@fabriceci
Copy link
Contributor Author

Oh yes sorry I was not precise, in the code if you use it as a type, yes you have to modify it, it is in the editor it is automatic (so only fully automatic for those who do not type).

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.

Rename RigidDynamicBody to either RigidBody or DynamicBody
6 participants