Skip to content

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

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
fabriceci:remove-dynamic-bodies-name
Aug 26, 2022
Merged

Restore RigidBody2/3D, SoftBody names in physics#64894
akien-mga merged 1 commit into
godotengine:masterfrom
fabriceci:remove-dynamic-bodies-name

Conversation

@fabriceci

@fabriceci fabriceci commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@akien-mga

Copy link
Copy Markdown
Member

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

Comment thread scene/register_scene_types.cpp Outdated
@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

Mickeon commented Aug 25, 2022

Copy link
Copy Markdown
Member

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 fabriceci force-pushed the remove-dynamic-bodies-name branch from 468cbcd to 96c4041 Compare August 25, 2022 19:54
@fabriceci

Copy link
Copy Markdown
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

Mickeon commented Aug 25, 2022

Copy link
Copy Markdown
Member

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

@Diddykonga

Copy link
Copy Markdown
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. :(

Comment thread doc/classes/PhysicsServer3D.xml Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

@fabriceci fabriceci force-pushed the remove-dynamic-bodies-name branch from 96c4041 to f8cc88f Compare August 26, 2022 10:26
@akien-mga akien-mga merged commit 1c97dde into godotengine:master Aug 26, 2022
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

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

JohanAR commented Aug 27, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

JohanAR commented Aug 27, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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