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.

Sign up for GitHub

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

Jump to bottom

[TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage #16863

Closed
65 of 83 tasks
akien-mga opened this issue Feb 20, 2018 · 517 comments
Closed
65 of 83 tasks

[TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage #16863

akien-mga opened this issue Feb 20, 2018 · 517 comments
Labels
breaks compat enhancement topic:core tracker
Milestone
4.0

Comments

@akien-mga
Copy link
Member

akien-mga commented Feb 20, 2018 •
edited

This issue is meant to keep track of awkwardly named or deprecated methods, properties and signals that we would like to rename next time we have the opportunity.

This can't be done lightly as it breaks compatibility for users using their old names, so any such change has to be done:

  • either after following a deprecation procedure: marked as deprecated - using it shows a warning - for at least one full minor release cycle (e.g. 3.1.x), then removed in a future minor or major version bump,
  • or within a major compatibility breaking release (such as 3.0 was to 2.1; but such situations won't happen often - if ever again - so the deprecation workflow should be preferred).

To properly deprecate methods, properties and signals, we need #4397 implemented.

A 🎉 reaction added by @akien-mga or @Calinou means the suggestion in the comment was incorporated into this post.

Classes

- [ ] OS* could be renamed to Platform* #16863 (comment) (See #47789 (comment))
- [ ] Label: Consider renaming to TextLabel #16863 (comment) (see #40124 (comment))

Methods

Properties

Signals

Enums

Constants

  • Global Scope: PROPERTY_USAGE_STORE_IF_NONZERO and PROPERTY_USAGE_STORE_IF_NONONE should be fully dropped, also from GDNative Remove obsolete enums #37693

Theme items

Objects

Shading language

  • WORLD_MATRIX is actually a model-view matrix and should be renamed. @reduz suggests to replace it (and CAMERA_MATRIX, which is a view matrix) by actual view and model matrices, e.g. CANVAS_MATRIX and ITEM_MATRIX.

Project settings

File formats
@akien-mga akien-mga mentioned this issue Feb 20, 2018
Closed
@leonkrause
Copy link
Contributor

Too tedious for me, but Godspeed to whoever fixes instance as instantiate where used as a verb.

@akien-mga akien-mga mentioned this issue Feb 21, 2018
Merged
@AlexHolly
Copy link
Contributor

#16116

This was referenced Feb 21, 2018
Closed
Closed
Closed
@akien-mga akien-mga mentioned this issue Mar 13, 2018
Closed
@AlexHolly
Copy link
Contributor

AlexHolly commented Apr 10, 2018 •
edited

Sprite.set_region(val) -> Sprite.set_region_enabled(val)
Sprite.is_region() -> Sprite.is_region_enabled()

@AlexHolly
Copy link
Contributor

TileMap.set_y_sort_mode(val) -> TileMap.set_y_sort_enabled(val)
TileMap.is_y_sort_mode_enabled() -> TileMap.is_y_sort_enabled()

@AlexHolly
Copy link
Contributor

rect_min_size
Control.set_custom_minimum_size(value) -> Control.set_rect_min_size(value)
Control.get_custom_minimum_size() -> Control.get_rect_min_size

There are many more in Control class the get/set should all have the same name as the variable it is annoying to check the docks every time when you know the variable name.

This was referenced Apr 18, 2018
Closed
Closed
@Chais
Copy link

Chais commented Apr 18, 2018

The TileMap class has a bunch of getter and setter methods that don't agree with their respective properties. In fact I'd suggest renaming most getters and setters in that class, so they agree with their properties as well as the naming in other classes.

@akien-mga akien-mga mentioned this issue May 25, 2018
Closed
@reduz
Copy link
Member

reduz commented Jun 5, 2018

Animation.track_remove_key_at_position should be
Animation.track_remove_key_at_time

@neikeq
Copy link
Contributor

neikeq commented Jun 5, 2018 •
edited

Methods

@AlexHolly
Copy link
Contributor

LineEdit has a parameter new_text on "text_changed" but TextEdit does not.

https://github.com/godotengine/godot/blob/master/scene/gui/text_edit.cpp#L6104
https://github.com/godotengine/godot/blob/master/scene/gui/line_edit.cpp#L1466

@akien-mga akien-mga mentioned this issue Jun 7, 2018
Closed
@akien-mga akien-mga changed the title Methods, properties and signals to consider for renaming in next planned compatibility breakage [TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage Jun 7, 2018
@Chaosus
Copy link
Member

Chaosus commented Jun 8, 2018 •
edited

I would like if VBoxContainer/HBoxContainer/GridContainer is renamed to simple VBox/HBox/Grid...

@AlexHolly
Copy link
Contributor

And later it will be renamed again because it is too short like pos->position :D

They are a bit long you are right.

@akien-mga akien-mga mentioned this issue Jun 14, 2018
Closed
@mysticfall
Copy link
Contributor

mysticfall commented Jun 15, 2018 •
edited

I noticed that Godot currenly has two different naming conventions in its method names.

Sometimes, it follows a common convention that can be found in APIs of such languages like C# or Java, like [action][object]() form: i.e.)

  • Mesh.GetBlendShapeName()
  • AnimationPlayer.GetCurrentAnimation()
  • Button.GetButtonIcon()

However, in other places, it follows a different convention of [prefix][action][object](), like:

  • Mesh.SurfaceGetFormat()
  • AnimationTreePlayer.NodeGetInputCount()
  • CollisionObject.ShapeOwnerGetOwner()

They are just a few examples out of many.

If we can afford to do sweeping compatibility breaking changes sometime in future, I'd like to see they can be renamed to follow a single, well defined naming convention (hopefully the former, as it's more often used both inside and outside Godot).

@akien-mga
Copy link
Member Author

However, in other places, it follows a different convention of [prefix][action][object](), like:

  • Mesh.SurfaceGetFormat()
  • AnimationTreePlayer.NodeGetInputCount()
  • CollisionObject.ShapeOwnerGetOwner()

I didn't double check all usages, but from what I've seen this style of method naming, though a bit awkward, also follows a specific convention. For example the shape_owner_get methods:

doc/classes/CollisionObject.xml
101:            <method name="shape_owner_get_owner" qualifiers="const">
110:            <method name="shape_owner_get_shape" qualifiers="const">
121:            <method name="shape_owner_get_shape_count" qualifiers="const">
130:            <method name="shape_owner_get_shape_index" qualifiers="const">
141:            <method name="shape_owner_get_transform" qualifiers="const">

The "prefix" refers to the first argument, and the part after get is what you'll actually be getting in that prefix. For example, shape_owner_get_shape_index(owner_id, shape_id) is conceptually similar to a get_shape_owner(owner_id)->get_shape_index(shape_id), but there's no ShapeOwner exposed to the scripting API, hence this "shortcut".

Same story for the surface_get methods:

doc/classes/ArrayMesh.xml
88:             <method name="surface_get_array_index_len" qualifiers="const">
97:             <method name="surface_get_array_len" qualifiers="const">
106:            <method name="surface_get_arrays" qualifiers="const">
114:            <method name="surface_get_blend_shape_arrays" qualifiers="const">
122:            <method name="surface_get_format" qualifiers="const">
131:            <method name="surface_get_material" qualifiers="const">
140:            <method name="surface_get_name" qualifiers="const">
148:            <method name="surface_get_primitive_type" qualifiers="const">

doc/classes/VisualServer.xml
2363:           <method name="mesh_surface_get_aabb" qualifiers="const">
2374:           <method name="mesh_surface_get_array" qualifiers="const">
2385:           <method name="mesh_surface_get_array_index_len" qualifiers="const">
2396:           <method name="mesh_surface_get_array_len" qualifiers="const">
2407:           <method name="mesh_surface_get_arrays" qualifiers="const">
2418:           <method name="mesh_surface_get_blend_shape_arrays" qualifiers="const">
2429:           <method name="mesh_surface_get_format" qualifiers="const">
2440:           <method name="mesh_surface_get_index_array" qualifiers="const">
2451:           <method name="mesh_surface_get_material" qualifiers="const">
2462:           <method name="mesh_surface_get_primitive_type" qualifiers="const">
2473:           <method name="mesh_surface_get_skeleton_aabb" qualifiers="const">

Or the *node_get methods in ATP:

doc/classes/AnimationTreePlayer.xml
35:             <method name="animation_node_get_animation" qualifiers="const">
44:             <method name="animation_node_get_master_animation" qualifiers="const">
53:             <method name="animation_node_get_position" qualifiers="const">
109:            <method name="blend2_node_get_amount" qualifiers="const">
146:            <method name="blend3_node_get_amount" qualifiers="const">
172:            <method name="blend4_node_get_amount" qualifiers="const">
225:            <method name="mix_node_get_amount" qualifiers="const">
255:            <method name="node_get_input_count" qualifiers="const">
264:            <method name="node_get_input_source" qualifiers="const">
275:            <method name="node_get_position" qualifiers="const">
284:            <method name="node_get_type" qualifiers="const">
315:            <method name="oneshot_node_get_autorestart_delay" qualifiers="const">
324:            <method name="oneshot_node_get_autorestart_random_delay" qualifiers="const">
333:            <method name="oneshot_node_get_fadein_time" qualifiers="const">
342:            <method name="oneshot_node_get_fadeout_time" qualifiers="const">
478:            <method name="timescale_node_get_scale" qualifiers="const">
523:            <method name="transition_node_get_current" qualifiers="const">
532:            <method name="transition_node_get_input_count" qualifiers="const">
541:            <method name="transition_node_get_xfade_time" qualifiers="const">

@akien-mga
Copy link
Member Author

I've updated the OP with most of the suggestions given so far.

I would like if VBoxContainer/HBoxContainer/GridContainer is renamed to simple VBox/HBox/Grid

I'm not convinced, in Godot we try to give everything explicit names, and for example Grid doesn't tell me that it's a Container. For VBox and HBox one could argue that boxes are containers by definition, but since we have BoxContainer which is not the same as Container, I think the verbosity is still warranted.

LineEdit has a parameter new_text on "text_changed" but TextEdit does not.

I don't think it would be useful to add new_text to TextEdit. On LineEdit, it simply contains the whole text of the LineEdit, not the text that changed, so I'd even argue that it should not be there in LineEdit's text_changed. Yet, it's more common that you want to use the full text of a LineEdit on input, than to do things with the full text of a multiline TextEdit when a new character is pressed.

@Jummit
Copy link
Contributor

Jummit commented Aug 23, 2021

TouchScreenButton:

1. `normal` → `texture` (or `texture_normal`?)

2. `pressed` → `texture_pressed`

3. Add `disabled` and `texture_disabled` properties.

Shouldn't it be pressed_texture, disabled_texture etc, similar to albedo_texture of SpatialMaterial.

@dalexeev
Copy link
Member

Shouldn't it be pressed_texture, disabled_texture etc, similar to albedo_texture of SpatialMaterial.

TextureButton:

This is a debatable issue. In one case, the advantage is that the name corresponds more to the English language, and in the other case, the advantage is that similar properties have one prefix and are located next to each other.

It's necessary to bring all such cases to consistency.

@Jummit
Copy link
Contributor

Jummit commented Aug 23, 2021 •
edited

Personally, I find english-sounding variable names lead to much more readable code.

@willnationsdev
Copy link
Contributor

Likewise, I prefer it when reading the reference or using the autocompletion of a scripting API keeps all related things together (which you get when they all have a texture_ prefix).

@qarmin
Copy link
Contributor

qarmin commented Aug 28, 2021

When working on converter from Godot 3.x to Godot 4 I found, that renaming this functions(first string) to another one(second string), works for some classes, but broke others.
I think that this should be unified to help users to upgrade its projects

{ "_set_name", "get_tracker_name"}, // XRPositionalTracker - CameraFeed use this
{ "_unhandled_input", "_unhandled_key_input"}, // BaseButton, ViewportContainer broke Node, FileDialog,SubViewportContainer
{ "get_extents", "get_size" }, // BoxShape, RectangleShape broke ReflectionProbe
{ "get_h_offset", "get_drag_horizontal_offset"}, // Camera2D, broke PathFollow, Camera
{ "get_mode", "get_file_mode"}, // FileDialog broke Panel, Shader, CSGPolygon, Tilemap
{ "get_name", "get_tracker_name"}, // XRPositionalTracker broke OS, Node
{ "get_recognized_extensions", "_get_recognized_extensions" }, // ResourceFormatLoader, EditorImportPlugin broke ResourceSaver
{ "get_translation", "get_position" }, // Node3D broke GLTFNode which is used rarely
{ "get_type", "get_tracker_type"}, // XRPositionalTracker broke GLTFAccessor, GLTFLight
{ "get_v_offset", "get_drag_vertical_offset"}, // Camera2D, broke PathFollow, Camera
{ "get_zfar", "get_far" }, // Camera3D broke GLTFCamera
{ "get_znear", "get_near" }, // Camera3D broke GLTFCamera
{ "has_font", "has_theme_font" }, // Control broke Theme
{ "has_icon", "has_theme_icon" }, // Control broke Theme
{ "has_stylebox", "has_theme_stylebox" }, // Control broke Theme
{ "instance", "instantiate" }, // PackedScene, ClassDB - Broke
{ "is_listening", "is_bound"}, // PacketPeerUDP broke TCPServer, UDPServer
{ "listen", "bound"}, // PacketPeerUDP broke TCPServer, UDPServer
{ "load", "_load"}, // ResourceFormatLoader broke ConfigFile, Image, StreamTexture2D
{ "save", "_save"}, // ResourceFormatLoader broke ConfigFile, Image, StreamTexture2D
{ "set_autowrap", "set_autowrap_mode" }, // Label broke AcceptDialog
{ "set_color", "surface_set_color"}, // ImmediateMesh broke Light2D, Theme, SurfaceTool
{ "set_extents", "set_size"}, // BoxShape, RectangleShape broke ReflectionProbe
{ "set_flag", "set_particle_flag"}, // ParticlesMaterial broke Window, HingeJoint3D
{ "set_normal", "surface_set_normal"}, // ImmediateGeometry broke SurfaceTool, WorldMarginShape2D
{ "set_region", "set_region_enabled" }, // Sprite2D, Sprite broke AtlasTexture
{ "set_translation", "set_position" }, // Node3D - this broke GLTFNode which is used rarely
{ "set_zfar", "set_far" }, // Camera3D broke GLTFCamera
{ "set_znear", "set_near" }, // Camera3D broke GLTFCamera
{ "translation", "position" }, // Node3D - broke GLTFNode

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 28, 2021 •
edited

@qarmin Can you elaborate on what you mean by "broke"? What exactly happened to your project when you renamed those methods?

Did you rename every method without using any context and therefore accidentally renamed unrelated, similarly named methods as well?

PS. Also, I'm pretty sure that "_unhandled_input" and "_unhandled_key_input" are both valid existing methods.

@qarmin
Copy link
Contributor

qarmin commented Aug 28, 2021 •
edited

Broke means that renaming e.g. all occurrences of _unhandled_input to _unhandled_key_input fixes errors about missing functions in Godot4 with BaseButton class but renamed also previously valid function in Node become invalid.

I listed all functions which I found and I know, that some are valid and don't need to be changed

@aaronfranke
Copy link
Member

aaronfranke commented Aug 28, 2021 •
edited

@qarmin What do you mean by "I think that this should be unified"? We don't want the OS name to be get_tracker_name...

I think I do see what you mean - we should, for example, rename those methods in GLTFNode and GLTFCamera.

@YuriSizov
Copy link
Contributor

I assume @qarmin meant that it should be collected in one place for everyone to be aware 🙃 But yes, most of those should not be renamed in the other classes. Instead when doing a rename one should pay attention to the context and the class that those methods belong to.

@Rubonnek Rubonnek mentioned this issue Aug 29, 2021
Merged
@aaronfranke aaronfranke mentioned this issue Aug 31, 2021
Merged
@mhilbrunner mhilbrunner mentioned this issue Sep 7, 2021
Merged
@Calinou
Copy link
Member

Calinou commented Sep 24, 2021 •
edited

We should probably rename RID_Owner's getornull() to get_or_null() (also for RID_Alloc and RID_PtrOwner).

Edit: Done in #53227.

@Calinou Calinou mentioned this issue Sep 29, 2021
Merged
@mhilbrunner mhilbrunner mentioned this issue Oct 7, 2021
Merged
@akien-mga akien-mga mentioned this issue Oct 23, 2021
Closed
@mhilbrunner mhilbrunner mentioned this issue Oct 29, 2021
Merged
@briansemrau briansemrau mentioned this issue Nov 5, 2021
Merged
@dalexeev
Copy link
Member

dalexeev commented Nov 8, 2021

Is there a convention as to whether the name of the base class should be prefixed or postfixed in the names of derived classes? It looks like this is the most inconsistent part in the project.

Postfixes:

*Texture{,2D,3D}
*Mesh
*Material{,3D}
*Shape{2D,3D}
*Script
*Translation
*Shader
*Tweener
*PackePeer

Prefixes:

AnimationNode*
AudioEffect*
AudioStream*
InputEvent*
SkeletonModification{2D,3D}*
StyleBox*
TileSet*
VideoStream*
VisualShaderNode*

And there are even exceptions with special naming logic:

{Syntax,Code,EditorSyntax}Highliter
VisualScriptNode,VisualScript*
AudioEffect*Instance
AudioStream*Playback*
*Peer(*|Extension)

These are not complete lists, but examples only.

Should we do something about it? Or should we defer this to 5.0 since we already have enough renames in 4.0? Even so, perhaps we should adopt a naming convention for future classes now.

Postfixes look more natural and prefixes more logical.

@akien-mga
Copy link
Member Author

akien-mga commented Nov 10, 2021 •
edited

@dalexeev That's a good topic for a dedicated proposal on godot-proposals, it will be easier to follow than on an issue with 515 comments :)

I'll close this issue as superseded by #54161 as this one is too big to still be useful. Most proposed changes have been done, others might not be consensual yet or should be moved to #54161 or dedicated proposals if they are far reaching.

@Mickeon Mickeon mentioned this issue Nov 26, 2021
Closed
@dalexeev dalexeev mentioned this issue Dec 2, 2021
Closed
@madmiraal madmiraal mentioned this issue Dec 3, 2021
Closed
@madmiraal madmiraal mentioned this issue Jan 7, 2022
Closed
@YuriSizov YuriSizov mentioned this issue Sep 6, 2022
Merged
@sigrudds1 sigrudds1 mentioned this issue Mar 13, 2023
Closed
@Vennnot
Copy link

Vennnot commented Apr 25, 2023

Maybe it's worth opening a new issue that mirrors this one. There's a lot of history here.

@ParadoxV5 ParadoxV5 mentioned this issue Apr 29, 2023
Closed
edg1000 pushed a commit to edg1000/https-github.com-godotengine-godot that referenced this issue Apr 23, 2024
@Calinou @MarcusElg
Rename various TileMap methods/properties for clarity and consistency
31d463f 
The YSort renames were tracked in godotengine/godot#16863.

This closes godotengine/godot-proposals#814.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaks compat enhancement topic:core tracker
Projects
None yet
Milestone
4.0
Development

No branches or pull requests