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

[TRACKER] More API renames for Godot 4.0 #54161

Closed
akien-mga opened this issue Oct 23, 2021 · 174 comments
Closed

[TRACKER] More API renames for Godot 4.0 #54161

akien-mga opened this issue Oct 23, 2021 · 174 comments

Comments

@akien-mga
Copy link
Member

akien-mga commented Oct 23, 2021

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 [TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage #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
Copy link
Member Author

Suggested renames

Classes

Methods

Properties

Signals

Enums

Constants

Theme properties

@arkology
Copy link
Contributor

arkology commented Oct 23, 2021

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

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

@aaronfranke
Copy link
Member

@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. #16863 (comment) This already has a PR open here: #47522

@Calinou
Copy link
Member

Calinou commented Oct 23, 2021

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 #54571.

@clayjohn
Copy link
Member

clayjohn commented Oct 23, 2021

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

qarmin commented Oct 24, 2021

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 62f56af
classes3.txt
constants3.txt
enums3.txt
functions3.txt
properties3.txt
signals3.txt

All3.zip

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

All4.zip

@MagdielM
Copy link

MagdielM commented Oct 24, 2021

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

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2021

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

@spindlebink
Copy link

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

Calinou commented Nov 10, 2021

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

@golddotasksquestions
Copy link

golddotasksquestions commented Nov 13, 2021

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

jejay commented Nov 14, 2021

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

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

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

spindlebink commented Nov 16, 2021

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

jejay commented Nov 18, 2021

  • 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
Copy link
Contributor

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

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

See also godotengine/godot-proposals#2885 (comment), #49701 (comment), it seems some people aren't keen on remove_value()

@spindlebink
Copy link

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

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

seichter commented Nov 25, 2021

  • 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
Copy link
Contributor

likeich commented Nov 25, 2021

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.

@xiongyaohua
Copy link
Contributor

xiongyaohua commented Aug 14, 2022

I made this proposal in the process of refactoring PathFollower2D/3D. I fell the change of Stage 1 is essential, Stage 2 is more a matter of personal tastes. What you guys think?

The gist is:

stage 1:
for both PathFollower3D and PathFollower2D

  • offset -> progress,
  • unit_offset -> progress_ratio

for PathFollower3D

  • add f_offset (f for forward) beside h_offset and v_offset for completeness

stage 2:
for PathFollower3D
combine v_offset ,h_offset, f_offset into a Vector3 offset

for PathFollower2D
combine v_offset, h_offset into a vector3 offset,

@Mickeon
Copy link
Contributor

Mickeon commented Aug 14, 2022

I prefer distance over offset, especially because three/four properties vaguely named offset in the same Class can be really confusing. There may be a better alternative, however, as I personally associate "distance" with actual distance in space, and its related Vector2/Vector3 methods.

"v" and "h" are used so frequently across the engine (and other programs as well), that it's easy to see a couple of them as shortened "vertical" and "horizontal". On the other hand, the meaning of the "f" in f_offset can potentially prove difficult to gather while reading code. As it probably makes sense to implement that new property, it's likely worth naming all three more explicitly.

But now that I think about it, can't v_offset, h_offset and f_offset just be a Vector2/Vector3?

@Mickeon
Copy link
Contributor

Mickeon commented Aug 14, 2022

How about...?
PathFollower2D and PathFollower3D:
offset -> progress
unit_offset -> progress_ratio or progress_percent

@fire-forge
Copy link
Contributor

fire-forge commented Aug 14, 2022

for PathFollower3D

  • v_offset -> up_offset
  • h_offset -> right_offset
  • f_offset -> forward_offset

for PathFollower2D

  • v_offset -> forward_offset ,
  • h_offset -> right_offset

It'd be a lot more straightforward to convert those to a single offset Vector2/Vector3 property instead.

@bend-n
Copy link
Contributor

bend-n commented Aug 14, 2022

Why are there h_* and v_* (and z_) options together, ever? Why not just Vector2/3.


It's not just PathFollower that has v_ and h_ variables.

Like Camera2D has "offset_v"
And literally every control... Although this is already being tracked.

And theres the width/height ones too...

They're also difficult to interact with in code...
follower.v_offset = vector.y
follower.h_offset = vector.x

Shouldn't we change them all?

It just doesn't make sense to have many options that could be one thing.
But we cant change one, and not all, as that would be confusing for people, and would just add to the already large list of engine inconsistencies. (Which this issue was created for fixing, no?)

Not to mention, some nodes have offset/size variables that are vectors... So its already confusing.

@fire-forge
Copy link
Contributor

Why are there h_* and v_* options together, ever? Why not just Vector2/3.

It's not just PathFollower that has v_ and h_ options.
Should we change them all? They're also difficult to interact with in code...

Most of the places where h_* and v_* are separate are in themes, which only support a limited number of types: int, Color, StyleBox, Font, and Texture2D. Those can't be changed to Vector2/Vector3 without significant changes to Theme and Control. There is some discussion around changing theme properties to variants in godotengine/godot-proposals#4486.

Any of the places that use separate h_* and v_* variables outside of themes should definitely be changed to use Vector2/Vector3 though. I have a similar proposal open here: godotengine/godot-proposals#4076.

@xiongyaohua
Copy link
Contributor

xiongyaohua commented Aug 15, 2022

How about...? PathFollower2D and PathFollower3D: offset -> progress unit_offset -> progress_ratio or progress_percent

I am ok with that. And I changed the proposal accordingly.

But now that I think about it, can't v_offset, h_offset and f_offset just be a Vector2/Vector3?
Any of the places that use separate h_* and v_* variables outside of themes should definitely be changed to use Vector2/Vector3 though

I totally agree. Hope 4076 could follow through.

@Scraphead
Copy link

Scraphead commented Aug 17, 2022

We had a small discussion on discord on function name plus_file.

It appends one string to another as a subpath with '/' as seperator.
It could be a file, but can also be another directory, or even used to build a url.

The name plus_file is not seen in other programming languages, so its hard to find/search for existing developers.

Examples:

C#:     Path.Combine("a", "b")
Python: os.path.join("a", "b")
JS:     path.join("a", "b")
Ruby:   File.join("a", "b")
Java:   Paths.get("a", "b")
Rust:   Path::new("a").join("b")

There was also the suggestion of removing plus_file and make it a part of a dedicated class.
Something like Directory.new().join("a", "b")

Is this something that should be renamed/removed before 4.0?

@Mickeon
Copy link
Contributor

Mickeon commented Aug 17, 2022

There was also the suggestion of removing plus_file and make it a part of a dedicated class.
Something like Directory.new().join("a", "b")

Godot 4 supports static methods in built-in classes, so it could actually just be Directory.combine(), theoretically.

@aaronfranke
Copy link
Member

@Scraphead There is already a Directory class which is used for performing directory operations on the file system, so I'm not sure it would be a suitable place for string operations. I'd suggest renaming plus_file to path_join or similar.

@Scraphead
Copy link

Yeah, rename for now.
path_join is also my preferred alternative, and is very readable coming from other programming languages.

Some other names from discord. If any caches your fancy.

"a".path_join("b")
"a".combine_path("b")
"a".concat_path("b")
"a".append_path("b")
"a".add_subpath("b")

I'll make a proposal for a dedicated path manipulation class later.

@bend-n
Copy link
Contributor

bend-n commented Aug 18, 2022

why not join_path?

@Scraphead
Copy link

why not join_path?

String already have a join, so we would have two sort of unrelated functions that begins the same (join and join_path)
That also overlap a bit for script autocomplete.

@Calinou
Copy link
Member

Calinou commented Aug 18, 2022

I find String.plus_file() more intuitive to think with compared to join_path() (or anything of the sort, such as Node.js's path.join()).

@Scraphead
Copy link

Scraphead commented Aug 18, 2022

I find String.plus_file() more intuitive to think with compared to join_path() (or anything of the sort, such as Node.js's path.join()).

If you are used to it, its sort of fine, but the method name did not stand out to me when combining directory paths.
It's used for more things then just plussing a file (name) to string.

I can agree on not using the name join_path or path_join, but I find the other suggested alternatives better then plus_file

I would be pleased with the name plus_path. Since thats more descriptive of what it does.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2022

This issue isn't for discussion. The plus_file() rename is controversial (given how many comments it spawned), so it needs a dedicated proposal.

@mhilbrunner
Copy link
Member

mhilbrunner commented Aug 19, 2022

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 23, 2022

As discussed in a maintainers meeting today, we'll be bikeshedding over the renames that have been PR'd this Thursday. Anything that didn't make the cut, c'est la vie! We need to draw the line somewhere, and the timeframe for the beta means we need to do it now. For all intents and purposes, this tracker has served its purpose and is now closed.

Thanks for all the suggestions! Wait for reviews on the relevant PRs, or join the meeting on Thursday if you have strong opinions in favor or against that you'd like to voice!

PS. You can see the time of the meeting in the shared calendar: https://calendar.google.com/calendar/u/0/r?cid=dXBwOGIwZXU0a3BlZjFjNTB2dTJmM2tjOGNAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

@2fd5
Copy link

2fd5 commented Jun 27, 2023

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

is this moved to separate issue, and tracked / tagged against any future release ?
As I am banging my head with issue described in #19800 the built-ins naming convention and lack of documentation examples of how to utilise them does not help.
I am just worried that this rename will be lost in this issue comment and forgoten, shall I create separate issue for it ?

@akien-mga
Copy link
Member Author

@2fd5 The agreed upon renames have been made in 4.0: #59268. No further renames are planned.

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

No branches or pull requests