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

Make 2D and 3D node names more explicit #37340

Merged
merged 3 commits into from
Mar 27, 2020
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Mar 26, 2020

To avoid confusion from users, most 3D nodes and 2D nodes (as well as resources) were renamed to match their type, as an example:

  • Area -> Area3D
  • MeshInstance -> MeshInstance3D
  • Sprite -> Sprite2D

Additionally, "Spatial" as a base for 3D nodes no longer exists, it was renamed to "Node3D".

Some nodes that are obviously 3D or 2D only remain as such, such as GIProbe, or YSort, but far most of them were renamed.

Compatibility option was added so old scenes can still load without problem.

Below is the list of 3D nodes and their new names:
image

Bugsquad edit: Fixes #30736

@aaronfranke
Copy link
Member

aaronfranke commented Mar 27, 2020

Shouldn't VehicleBody and VehicleWheel also have a 3D suffix? While Godot may only have 3D vehicle physics right now, it's completely feasible for there to be 2D vehicles.

@bojidar-bg
Copy link
Contributor

Likewise for ClippedCamera, shouldn't it be ClippedCamera3D? In the end, it is a clipped "Camera3D" now, so I don't see why the suffix should be dropped.

@akien-mga
Copy link
Member

Nodes left to consider (probably left alone on purpose, but we should confirm that we don't want to rename them):

  • In scene/2d and inheriting Node2D:

    • BackBufferCopy
    • CanvasModulate
    • ParallaxLayer
      • ParallaxBackground inherits Node but its logic depends on Node2D, Camera2D
    • TileMap
    • TouchScreenButton
  • In scene/3d and inheriting Node3D:

    • All ARVR nodes - but that was left for @BastiaanOlij probably, and they're meant to also be renamed to change ARVR to XR. Should they get 3D suffix too? (e.g. XRCamera3D)
    • BakedLightMap (dead code, kept for future reimplementation)
    • GIProbe
    • ReflectionProbe
    • VehicleBody, VehicleWheel
    • WorldEnvironment: inherits Node but its logic depends on Node3D

Nodes that could maybe be moved:

  • CanvasItem is in scene/2d, but only depends on scene/main stuff. Should maybe be moved in main? Especially since it's also used by scene/gui.
  • SkeletonIK3D is in scene/animation, but it depends on 3D nodes. Should it move to scene/3d?

@akien-mga
Copy link
Member

Likewise for ClippedCamera, shouldn't it be ClippedCamera3D? In the end, it is a clipped "Camera3D" now, so I don't see why the suffix should be dropped.

Already renamed locally.

@akien-mga akien-mga force-pushed the rename-3d-nodes branch 2 times, most recently from 758119a to b6dc150 Compare March 27, 2020 08:38
@akien-mga
Copy link
Member

Added more renames, namely:

  • ClippedCamera -> ClippedCamera3D
  • HeightMapShape -> HeightMapShape3D
  • Particles2D -> GPUParticles2D
  • World -> World3D

Also renamed editor plugins to match the new node names that they handle, and fixed a few missing file renamings to match their new main class name.

@aaronfranke
Copy link
Member

aaronfranke commented Mar 27, 2020

TouchScreenButton

Why isn't this Control derived? Disclaimer: I haven't looked at what this actually is, but I would expect buttons to all be GUI elements derived from Control.

All ARVR nodes

There is no possibile way to have non-3D ARVR nodes, so I don't think they need the suffix. EDIT: Well, if we have Camera3D then it does make sense to have XRCamera3D so may as well do the other ones too.

WorldEnvironment: inherits Node but its logic depends on Node3D

If that is the case, why does its icon contain both red and blue? Blue is for 2D.

CanvasItem is in scene/2d ... also used by scene/gui

Not a suggestion, just a thought: What about scene/canvas/2d and scene/canvas/gui (or scene/canvas/control) to make the folder structure match the node inheritance?

Also, there is the case of non-node things to rename (such as Transform -> Transform3D).

@@ -31,7 +31,7 @@
#ifndef SPATIAL_EDITOR_GIZMOS_H
#define SPATIAL_EDITOR_GIZMOS_H
Copy link
Member

Choose a reason for hiding this comment

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

Should also change the include guards along with the class and file name.

Copy link
Member

Choose a reason for hiding this comment

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

Too difficult to do consistently with all those renames, I'll write a script to do it (#37290).

@reduz
Copy link
Member Author

reduz commented Mar 27, 2020

@aaronfranke touchscreenbutton is not control derived simply because it will process its own input (touch). its completely unrelated to any of the control logic (mouse).

@akien-mga akien-mga force-pushed the rename-3d-nodes branch 2 times, most recently from 871cd84 to b429da6 Compare March 27, 2020 13:52
@akien-mga
Copy link
Member

akien-mga commented Mar 27, 2020

Nodes that could maybe be moved:

  • CanvasItem is in scene/2d, but only depends on scene/main stuff. Should maybe be moved in main? Especially since it's also used by scene/gui.
  • SkeletonIK3D is in scene/animation, but it depends on 3D nodes. Should it move to scene/3d?

Done.

In scene/3d and inheriting Node3D:

  • VehicleBody, VehicleWheel

Done.

@Calinou
Copy link
Member

Calinou commented Mar 27, 2020

@aaronfranke In Godot 3.2 at least, WorldEnvironment can be used to enable glow and HDR in 2D. I'm not sure about the master branch though.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Should be good to merge as far as I'm concerned if the CI builds pass.

@akien-mga
Copy link
Member

Pushed some extra changes for leftover String references to old node names, and fixing the editor plugin names for some of the renamed node types.

Rename editor plugins to match the new node names.
@reduz reduz merged commit 307b1b3 into godotengine:master Mar 27, 2020
@Demolishun
Copy link

Will there be a way to automatically update a project to the new names?

@Anutrix
Copy link
Contributor

Anutrix commented Mar 27, 2020

Just a note: Docs and header gaurds need to be updated.

@danilw

This comment has been minimized.

@aaronfranke
Copy link
Member

@danilw That's not related to this PR. If you want to discuss that, check out this discussion: godotengine/godot-proposals#629

@Calinou
Copy link
Member

Calinou commented Oct 11, 2020

@Demolishun This is already implemented:

Compatibility option was added so old scenes can still load without problem.

It works automatically, you don't need to do anything specific.

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.

Proposal for renaming nodes for Godot 4.0
9 participants