[TRACKER] More API renames for Godot 4.0

akien-mga commented 2 years ago

This is a follow-up to the mega tracker #16863 which we've used for the past 3 years to keep a list of things we'd like to rename in the API for Godot 4.0.

Many of these renames have been done, some are still pending with open PRs, others might actually not be wanted, as we didn't do thorough discussion of all proposals on that issue.

Now that we're getting closer to the 4.0 release, it's time to give this a second look, and start anew from the current API.

Are there more classes, methods, properties, or signals, which have an awkward name and could be improved before we freeze the API to preserve compatibility?

Here's my workflow proposal for this tracker:

Add your suggestions as comments below. Include links to relevant proposals or issues/comments if they exist (but you don't necessarily need to open a proposal for it, unless it's a very substantial change to the API).
Upvote the proposals you support, downvotes the ones you disagree with. Don't take downvotes personally, we need to assess the general feeling of the community for a given proposal to see if it makes sense to rename, beyond core maintainers' own intuition.
If you're motivated, you can also salvage unresolved suggestions from #16863 so they are considered again here (beware, there are 500 comments...).
Suggestions (not approved yet, so don't open PRs for them) will be listed by repo maintainers in the second comment of this issue.
Avoid lengthy discussions of suggestions in this tracker, as it makes it hard to keep track of things. Prefer discussion on the Contributor Chat, and if needed, open dedicated proposals for non-consensual suggestions that require more debate.
After review, approved suggestions will be moved to the task list below in this first comment, so they can be implemented by contributors and merged.

Important: This tracker is only for API renames. I.e. changing a name to another name, not changing behavior, adding/removing logic, etc. Anything that changes the behavior of the engine should be discussed in a proposal on https://github.com/godotengine/godot-proposals.

For this tracker, let's also exclude suggestions for changes in the Project Settings and Editor Settings. Those should probably have their own trackers, as the settings do need a cleanup for 4.0.

Approved renames

None yet.

akien-mga commented 2 years ago

Suggested renames

Classes

Methods

Properties

Signals

Enums

Constants

Theme properties

arkology commented 2 years ago

Array (some changes also apply to PackedArrays) https://github.com/godotengine/godot/issues/16863#issuecomment-441376010:

Rename remove to remove_at (remove by index) to avoid ambiguity: https://github.com/godotengine/godot/pull/50139
Rename erase to remove_value (remove by value) to avoid ambiguity

A comment: With current names you should keep in mind the difference (and/or periodically read the docs to remember the difference).

aaronfranke commented 2 years ago

@arkology For reference, there is a PR for renaming remove to remove_at: #50139

display/window/size/test_width and test_height should be renamed to window_width and window_height. We should also consider renaming the width and height settings, maybe render_width and render_height, or viewport_width and viewport_height. https://github.com/godotengine/godot/issues/16863#issuecomment-412308210 This already has a PR open here: #47522

Calinou commented 2 years ago

Suggestion: rename the GlobalScope PROPERTY_USAGE_NOEDITOR to PROPERTY_USAGE_NO_EDITOR. This would be consistent with constants such as Object's PROPERTY_HINT_COLOR_NO_ALPHA.

We may want to go through all constants that contain _NO and see if any others need to modified this way.

Edit: Done in https://github.com/godotengine/godot/pull/54571.

clayjohn commented 2 years ago

Shader builtin matrices rename suggestions

Spatial Shaders

WORLD_MATRIX rename to MODEL_TO_WORLD
WORLD_NORMAL_MATRIX rename to MODEL_TO_WORLD_NORMAL
MODELVIEW_MATRIX rename to MODEL_TO_VIEW
MODELVIEW_NORMAL_MATRIX rename to MODEL_TO_VIEW_NORMAL
CAMERA_MATRIX rename to VIEW_TO_WORLD
INV_CAMERA_MATRIX rename to WORLD_TO_VIEW
PROJECTION_MATRIX rename to VIEW_TO_CLIP
INV_PROJECTION_MATRIX rename to CLIP_TO_VIEW

CanvasItem Shaders

WORLD_MATRIX rename to MODEL_TO_WORLD
CANVAS_MATRIX rename to WORLD_TO_CANVAS
SCREEN_MATRIX rename to CANVAS_TO_SCREEN

qarmin commented 2 years ago

Here is list of all classes, signals, functions etc. exposed in GDScript from Godot 3 and 4 - I used this for creating script converter, but maybe this will help to see what thing needs to be renamed

Godot 3 62f56af69 classes3.txt constants3.txt enums3.txt functions3.txt properties3.txt signals3.txt

All3.zip

Godot 4 b2ab5cb50 classes4.txt constants4.txt enums4.txt functions4.txt properties4.txt signals4.txt

All4.zip

MagdielM commented 2 years ago

I'm not sure what I would replace it with, but AnimatedSprite may mislead newcomers into thinking Sprite2D nodes can't be animated.

KoBeWi commented 2 years ago

search_in_file_extensions in Project Settings should be renamed to find_in_files_extensions, to be consistent with Script Editor naming.

spindlebink commented 2 years ago

It's a big one, but may I submit CanvasItem's update() -> request_redraw() or update_canvas() or something like that? Since CanvasItem is base for 2D nodes, we have node types way down the line which all share an update function which doesn't really communicate what it is that's being updated.

Calinou commented 2 years ago

Should we rename the ignore stretch aspect mode to stretch (or perhaps distort)? This would make its effect more obvious.

golddotasksquestions commented 2 years ago

@Calinou I think ignore is still the most accurate description of those options. (simply because stretch applies to all other aspect settings too, and distort is not always true. You can also get undistorted stretch when using ignore mode.)

jejay commented 2 years ago

I have posted this in the big issue and imho this should be fixed as its arguably beyond taste, you could consider it a bug:

The counterparts Camera::unproject_position() and Camera::project_position() do the exact opposite of what you would expect / what is commonly agreed upon. Camera::unproject_position() does a normal camera projection, i.e. it takes a 3D position and projects it onto the screen. Camera::project_position() does what people would call an unprojection, i.e. 2D->3D. (Technically everything is a projection, but we're not speaking correct mathematical terms here but graphics slang.)

If you're not convinced see https://www.google.com/search?q=camera+unproject or https://dondi.lmu.build/share/cg/unproject-explained.pdf to convince yourself that practically every other library uses the terms project and unproject in the opposite directions to what they are currently implemented in Godot.

A few more thoughts in the original comment.

spindlebink commented 2 years ago

VSeparator and HSeparator seem to be semantically flipped: I understand that VSeparator stretches visually up and down, but it separates things on the horizontal axis and serves as a child of a horizontal stack. I feel like the visual, in-editor dimensions of the VSeparator are less important for meaningful/consistent naming than the actual purpose it serves---that of being a horizontal separator. Same goes for HSeparator.

MagdielM commented 2 years ago

VSeparator and HSeparator seem to be semantically flipped: I understand that VSeparator stretches visually up and down, but it separates things on the horizontal axis and serves as a child of a horizontal stack. I feel like the visual, in-editor dimensions of the VSeparator are less important for meaningful/consistent naming than the actual purpose it serves---that of being a horizontal separator. Same goes for HSeparator.

Not gonna lie, the current naming makes more sense to me. It's a separator that's vertical. Very self-evident.

spindlebink commented 2 years ago

Ah, maybe it's just me, then.

Has thought been given to consolidating both into a single Separator class, possibly with a toggle for direction? I know the UI system's getting a pretty big upgrade coming up so it might be worth considering, but if there's good reason otherwise I don't want to open a whole proposal.

The counterparts Camera::unproject_position() and Camera::project_position() do the exact opposite of what you would expect / what is commonly agreed upon.

What about naming it in accordance with conversion methods elsewhere in the engine, as Camera::viewport_to_world/world_to_viewport or something similar? The current names made sense to me when I first learned them, as they gave me the sense of projecting through the screen into the world and vice-versa. The same's probably true for other people who aren't coming from a 3D graphics background. The inverses seem to be unanimously the correct versions, but maybe in the vein of Godot's clarity/obviousness-centered design it might be worth going for something less technical?

jejay commented 2 years ago

CAMERA_MATRIX rename to VIEW_TO_WORLD

INV_CAMERA_MATRIX rename to WORLD_TO_VIEW

I just realized that @clayjohn's sugggestions imply that the project/unproject weirdness is consistent with with the current namings/definitions of (INV_)CAMERA_MATRIX, which are apparently equally inverted compared to the convention. It doesn't take long to find people being confused about this, too. It also makes my rambling about the documentation being incorrect incorrect, as it clearly is all consistent throughout Godot, albeit the opposite of the convention everywhere else. I guess @clayjohn's proposal fixes this issue without confusing the community that has gotten used to this.

I would like to add that those references to these matrices in the documentation should also be changed at some point. I.e. where it currently says "the camera projection" in Camera::project_ray_normal()/Camera::project_ray_origin(), it should either be reworded to "the inverse camera projection" or there needs to be a wording found that avoids implying a canonical direction at all. But that is not release critical, I guess.

AaronRecord commented 2 years ago

Array (some changes also apply to PackedArrays) #16863 (comment):

Rename remove to remove_at (remove by index) to avoid ambiguity: Rename remove() to remove_at() when removing by index #50139

Rename erase to remove_value (remove by value) to avoid ambiguity

A comment: With current names you should keep in mind the difference (and/or periodically read the docs to remember the difference).

See also https://github.com/godotengine/godot-proposals/issues/2885#issuecomment-864282198, https://github.com/godotengine/godot/pull/49701#issuecomment-863752431, it seems some people aren't keen on remove_value()

spindlebink commented 2 years ago

Also: Transform2/3D.interpolate_with -> Transform2/3D.lerp (and elsewhere if I've missed a lerp) in accordance with the other interpolation method names.

seichter commented 2 years ago

I have posted this in the big issue and imho this should be fixed as its arguably beyond taste, you could consider it a bug:

The counterparts Camera::unproject_position() and Camera::project_position() do the exact opposite of what you would expect / what is commonly agreed upon. Camera::unproject_position() does a normal camera projection, i.e. it takes a 3D position and projects it onto the screen. Camera::project_position() does what people would call an unprojection, i.e. 2D->3D. (Technically everything is a projection, but we're not speaking correct mathematical terms here but graphics slang.) If you're not convinced see https://www.google.com/search?q=camera+unproject or https://dondi.lmu.build/share/cg/unproject-explained.pdf to convince yourself that practically every other library uses the terms project and unproject in the opposite directions to what they are currently implemented in Godot.

A few more thoughts in the original comment.

I was just about to file a bug report as I stumbled upon this. The naming is absolutely wrong, an unprojection is transfering from vector space Rn to Rn+1 .. hence 2D to 3D or 3D > 4D (homogeneous coordinates)

seichter commented 2 years ago

CAMERA_MATRIX rename to VIEW_TO_WORLD

INV_CAMERA_MATRIX rename to WORLD_TO_VIEW

I just realized that @clayjohn's sugggestions imply that the project/unproject weirdness is consistent with with the current namings/definitions of (INV_)CAMERA_MATRIX, which are apparently equally inverted compared to the convention. It doesn't take long to find people being confused about this, too. It also makes my rambling about the documentation being incorrect incorrect, as it clearly is all consistent throughout Godot, albeit the opposite of the convention everywhere else. I guess @clayjohn's proposal fixes this issue without confusing the community that has gotten used to this.

I would like to add that those references to these matrices in the documentation should also be changed at some point. I.e. where it currently says "the camera projection" in Camera::project_ray_normal()/Camera::project_ray_origin(), it should either be reworded to "the inverse camera projection" or there needs to be a wording found that avoids implying a canonical direction at all. But that is not release critical, I guess.

Thanks for pointing this out. Honestly, I got really frustrated with many of the camera related concepts going against the common convention in computer graphics. I kept running circles for exposing the projection matrix but the argument was it is too complicated and thus Godot is stuck with the heavy parameterization of the intrinsic camera parameters. I would have hoped that 4.0 is a chance to change this.

likeich commented 2 years ago

Theme font colors have different verb tenses. The font colors for the Button Node are font_color font_color_disabled font_color_focus font_color_hover font_color_pressed

So either font_color_focus -> font_color_focused and font_color_hover -> font_color_hovered or font_color_disabled -> font_color_disable and font_color_pressed -> font_color_press

The first makes more sense because it expresses an action that is currently happening that changes the font color.

Mickeon commented 2 years ago

In the previous tracker https://github.com/godotengine/godot/issues/16863#issuecomment-595585595 brought up combining Camera2D's limits into one property and, for being a suggestion that goes beyond renaming, here's a matching proposal https://github.com/godotengine/godot-proposals/issues/3604

golddotasksquestions commented 2 years ago

Rename "physical_key" In the Input and Input Map class to "QWERTY_key".

Example: instead of

if Input.is_physical_key_pressed(KEY_Q): will print "Q" on a QWERTY keyboard will print "A" on a AZERTY keyboard

rename it to this

if Input.is_QWERTY_key_pressed(KEY_Q): will print "Q" on a QWERTY keyboard will print "A" on a AZERTY keyboard

"Physical key" tells the user nothing what this is about. Both AZERTY and QWERTY keyboards can be "physical". The opposite of physical is virtual. But input method has nothing to do with the difference between physical and virtual keyboards. It only has to do with the keyboard layout, and with QWERTY in particular.

When the user reads "QWERTY" it is very likely they understand this is related to a specific keyboard layout. "physical" does not do that. This has nothing to do with the keyboard being physical or not.

Calinou commented 2 years ago

Rename "physical_key" In the Input and Input Map class to "QWERTY_key".

The issue is that there is no single "QWERTY" layout out there. You're probably referring to the US QWERTY layout implicitly. However, there are plenty of derivatives that still use QWERTY, but with different secondary key placements that will impact physical key location. Such examples would be Spanish QWERTY and French Canadian QWERTY. The Wikipedia page for QWERTY lists dozens of variants.

golddotasksquestions commented 2 years ago

@Calinou: I don't understand why this would be an issue. This is just about the name of the method. This thread is not about behaviour. "physical" has no meaning in this context.

"QWERTY" has meaning. Because when you are using this function, you are using the QWERTY layout (the US QWERTY, I would assume)

golddotasksquestions commented 2 years ago

@Mickeon can you help me understand where you see the benefit of calling it "physical"? All hardware keyboards are physical, regardless what layout they use. What keyboard layout you use has nothing to do with it being physical or not.

Mickeon commented 2 years ago

@golddotasksquestions It's not that I do not see the benefit of calling it "physical", it's that I do not find replacing every instance of "physical key" with "QWERTY key" or "qwerty key" beneficial. I wanted to simply express distaste. It just doesn't sound right.

Don't take downvotes personally, we need to assess the general feeling of the community for a given proposal to see if it makes sense to rename, beyond core maintainers' own intuition.

golddotasksquestions commented 2 years ago

@Mickeon Like all of these renames we are discussing here, the benefit is not for current users who already know the methods and names and what they do, but for new uses who come new to Godot and GDScript and need to learn everything. For current users renaming things will always be additional effort, but should make life and work for future use and newcomers much more intuitive and easier to learn.

The word "physical" has nothing to do with what this function does. There is no connection to anything physical, or it's opposite "virtual". There is connection to QWERTY however, since this function uses the QUERTY layout rather than the native one. Using this method is like going into your OS keyboard settings and changing your AZERTY layout to QWERTY (what physical keyboard you have is totally irrelevant in any of this).

Mickeon commented 2 years ago

Looking back at it, I feel like some Camera2D properties are in need to be renamed. Particularly, rotating could be rotate_view, since it's what it is. The Camera2D node itself is able to rotate even when it is disabled, it just doesn't display.

Mickeon commented 2 years ago

Furthermore, Camera2D drag_margin_h_enabledand drag_margin_v_enabled could be called drag_margin_horizontal and drag_margin_vertical. However, the reason behind not doing this is understandable. There would be no hint at them being booleans.

KoBeWi commented 2 years ago

We could remove Scale On Expand (Compat) stretch option in TextureRect. It behaves like Keep when expand is off and Scale when it's on. As name suggests, it was kept that way for compatibility, so now we can break.

EDIT: I also realized that expand option is not descriptive anymore. The doc says If true, the texture scales to fit its bounding rectangle., but what it actually does is it removes the minimal size. It needs a better name and updated description.

CsloudX commented 2 years ago

Signals: Tree: item_activated()->item_double_clicked() item_double_clicked()->item_icon_double_clicked()

bluenote10 commented 2 years ago

Regarding the in-consistency of Mesh.surface_set_material and MeshInstance.set_surface_material:

It looks like on master this is now Mesh.surface_set_material and MeshInstance.set_surface_override_material. Personally I find this pair still a bit awkward, due to the shuffling of "set".

I assume the main reason to keep the name for Mesh was this comment. Wouldn't that reasoning imply that it should be MeshInstance.surface_set_matrial_override as well? CC @madmiraal @clayjohn

madmiraal commented 2 years ago

MeshInstance.set_surface_override_material(Material) applies to the MeshInstance and overrides all surfaces not a particular surface. Mesh.surface_set_material(surf_idx, Material) applies to a specific surface and not to the Mesh. Note the additional surf_idx parameter. If it helps to understand the logic, try reading it as Mesh_surface.set_material().

Calinou commented 2 years ago

Suggested renames for CanvasItem:

draw_polygon() → draw_polygon_colors() (it draws a polygon with multiple colors).
draw_colored_polygon() → draw_polygon() (it draws a polygon with a single color).

This would be more consistent with draw_polyline*() and draw_multiline*().

bluenote10 commented 2 years ago

MeshInstance.set_surface_override_material(Material) applies to the MeshInstance and overrides all surfaces not a particular surface.

From what I can see on master, MeshInstance is per-surface as well, right?

https://github.com/godotengine/godot/blob/0e261523926e776bc0b1b85178a60fa476490a9c/scene/3d/mesh_instance_3d.cpp#L340

After having a second glance, the placement of "set" in surface_set_material and set_surface_override_material still feels somehow inconsistent to me.

nathanfranke commented 2 years ago

OS.get_data_dir -> OS.get_user_home OS.get_user_data_dir -> OS.get_project_data_path

Not sure if these should end with dir, directory, or path.

OS.get_name should return Web instead of HTML5

KoBeWi commented 2 years ago

local_to_scene in Resource could be renamed to local_to_instance, to be more precise.

Mickeon commented 2 years ago

It seems like Documentation favours bytes over raw, as in PoolByteArray. Strange, but small inconsistency.

Variant.TYPE_RAW_ARRAY -> Variant.TYPE_BYTE_ARRAY

Calinou commented 2 years ago

It seems like Documentation favours bytes over raw, as in PoolByteArray. Strange, but small inconsistency.

The inconsistency comes from PoolByteArray (now PackedByteArray) being called RawArray in Godot 2.x. We can surely change this in 4.0 now :slightly_smiling_face:

In fact, core constant bindings haven't been updated yet: https://github.com/godotengine/godot/blob/28174d531b7128f0281fc2b88da2f4962fd3513e/core/core_constants.cpp#L661-L669

Edit: Done in https://github.com/godotengine/godot/pull/56224.

Mickeon commented 2 years ago

I'm not very caught on the progress of Godot 4, and it's possible that a different implementation may have rendered this method useless, as I can't find it even exists in the latest build, but:

@GDScript.typeof() ->@GDScript.type_of()

nathanfranke commented 2 years ago

I'm not very caught on the progress of Godot 4, and it's possible that a different implementation may have rendered this method useless, as I can't find it even exists in the latest build, but:

@GDScript.typeof() ->@GDScript.type_of()

I think the reason it is named this way is similarity to the keyword sizeof in C++. Maybe this should be removed in favor of Object.get_class()?

KoBeWi commented 2 years ago

Maybe this should be removed in favor of Object.get_class()?

typeof works for any Variant and for classes it returns VARIANT_OBJECT (probably). They are different methods.

KoBeWi commented 2 years ago

ItemList has styleboxes named "cursor" and "cursor_unfocused". They are used in multi-select mode for the currently highlighted item. Maybe we could come up with a better name? Something like "highlight", idk. Also it should be "focus" not "unfocused", because it's inconsistent with other styles.

YuriSizov commented 2 years ago

I want to do a pass on all theme properties before the beta, because consistency in their naming is very very poor across the board.

Calinou commented 2 years ago

There's are two old issues about renaming particle properties: https://github.com/godotengine/godot/issues/24468 and godotengine/godot-proposals#4283

Should we go forward with any of those renames? (Note that Fract Delta will likely be removed in 4.0.)

AnidemDex commented 2 years ago

What about virtual methods of EditorPlugin? Virtual methods in Node starts with a _ (like _ready) but in EditorPlugin virtual methods doesn't has that prefix (like edit). Is that suggestion related to this issue?

KoBeWi commented 2 years ago

They were already renamed to begin with _.

kleonc commented 2 years ago

Node.print_stray_nodes() -> Node.print_orphan_nodes()

Seems like "orphan" is used everywhere else across the engine, and this inconsistency might be confusing.

h0lley commented 2 years ago

@expose instead of @export as a keyword to expose variables to the inspector.

I think there's a couple of downsides in terms of ambiguity to the export keyword. since there's also the export in terms of exporting the project, looking up documentation can be cumbersome. I've often found myself on the wrong docs page or having to sift through search results. it can also be confusing when asking questions about it. for instance, which kind of export is being referred to occasionally needs to be clarified in Discord help channels.

and lastly, I think expose just makes more sense in describing what the feature does.

I mentioned this already over here but realized that this thread is probably a much better place for it.

pycbouh already commented on it:

It's not all that the keyword does, though. It also makes that field serializable when you save resources using that script. That's not just scenes, that's also resources. And you can expose a property to the inspector without making it serializable too, but not with the keyword. So whether the export term is bad, expose is technically even less correct.

although I don't quite follow their reasoning. I think there's no need for a keyword to reflect the technicalities of how the engine implements something under the hood - rather, it should reflect what happens from the perspective of the user. also they do not address the issue in regards to ambiguity.

godotengine / godot