search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
86.29k stars 19.2k forks source link

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

Closed akien-mga closed 2 years ago

akien-mga commented 6 years ago

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:

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

A :tada: 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* https://github.com/godotengine/godot/issues/16863#issuecomment-403253127~ (See https://github.com/godotengine/godot/pull/47789#issuecomment-854149653) ~- [ ] Label: Consider renaming to TextLabel https://github.com/godotengine/godot/issues/16863#issuecomment-502517425~ (see https://github.com/godotengine/godot/pull/40124#issuecomment-751741421)

Methods

Properties

Signals

Enums

Constants

Theme items

Objects

Shading language

Project settings

File formats

leonkrause commented 6 years ago

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

AlexHolly commented 6 years ago

16116

AlexHolly commented 6 years ago

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

AlexHolly commented 6 years ago

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

AlexHolly commented 6 years ago

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.

Chais commented 6 years ago

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.

reduz commented 6 years ago

Animation.track_remove_key_at_position should be Animation.track_remove_key_at_time

neikeq commented 6 years ago

Methods

AlexHolly commented 6 years ago

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

Chaosus commented 6 years ago

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

AlexHolly commented 6 years ago

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

They are a bit long you are right.

mysticfall commented 6 years ago

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.)

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

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 commented 6 years ago

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 commented 6 years ago

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.

mysticfall commented 6 years ago

@akien-mga

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

I'm aware of the fact that it's a naming convention of its own. But it's still not something commonly used outside Godot, and also a bit confusing because sometimes the same word like BlendShape is used in methods which follow two different naming convention.

Personally, I'd like to see them all renamed GetPrefix* and SetPrefix* for consistency, but maybe other people might have different opnions about it.

vnen commented 6 years ago

The methods changed in #16757. The argument order makes more sense, but it breaks API compatibility between 3.0 and 3.1 (#19648).

Zylann commented 6 years ago

I'll raise #9128 again: translation in 3D vs position in 2D is an odd dissimilarity. It was raised ages before 3.0 but was closed after 3.0 went out due to... 3.0 being out.

romeojulietthotel commented 6 years ago

OS.execute should use posix_spawn.

groud commented 6 years ago

Another one might be renaming "margin" to "offset" for control nodes. Since margins are negative on the right side, this misleads people, especially when comparing with StyleBoxes

Zylann commented 6 years ago

@groud I feel offset is too generic though, margins used to be the correct word because they were not negatively oriented on right side when first introduced

groud commented 6 years ago

@groud I feel offset is too generic though, margins were a good term (and werent negative when first introduced)

Well, margin is misleading now that they are negative. Offset is generic, but makes more sense. I don't think it's a problem that they are generic since it is in the context of control nodes. Anyway I'm open to better suggestions. I just wanted to drop this idea here since such property name change has already been suggested. See here for example.

jmf commented 6 years ago

The size of boxes/cubes is named inconsistently. BoxShape for collisions uses extents. CubeMesh has a size property with x, y and z, which are each half the extents. CSGBox has a Width, Height and Depth property which are defined like the x, y and z size in CubeMesh.

In addition to the size property, sometimes "Cube" is used and sometimes "Box" is used. It would make sense to use "Box" for everything since x, y and z for CubeMesh can be set independently and it is thus also a box.

leonkrause commented 5 years ago

Since we have e.g. HTML5 and UWP as targets, which aren't exactly operating systems, I propose renaming OS to Platform (PlatformWindows, PlatformUnix,...). Makes sense with the OS/Display split too.

isaacremnant commented 5 years ago

From this #20228, Label.clip_text is not necessary anymore. I believe it's the same for Button.clip_text.

Piet-G commented 5 years ago

Camera2D currently has two different properties both named offset (Regular offset and the one split up in V and H) that are two totally different things, this is really confusing.

neikeq commented 5 years ago

Methods

- Dictionary: erase_checked should be removed (this method is not exposed to scripts). - Dictionary: erase should be changed to return bool to determine whether the pair with the specified key was removed or not (see erase_checked implementation).

20945

akien-mga commented 5 years ago

@neikeq This could be done now IMO, changing Dictionary.erase's return value from void to bool shouldn't break any project.

Zylann commented 5 years ago

@akien-mga but it would break GDNative API compatibility, isn't it?

neikeq commented 5 years ago

@akien-mga Wouldn't that break forward compatibility? Are we allowed to make changes that potentially make 3.1 scripts not work in older versions like 3.0?

akien-mga commented 5 years ago

@neikeq Yes, 3.1 scripts are already incompatible with 3.0 (class_name, tons of API changes with new optional parameters or brand new properties/methods, new classes, etc.). We only take care of backward compatibility, not forward.

neikeq commented 5 years ago

Oh, I see! If that's the case I will make those changes now.

LikeLakers2 commented 5 years ago

If I can have one added to the list, the LineEdit and TextEdit control nodes would really benefit from having consistent APIs with each other so they can be used (mostly) interchangeably. Right now it feels like a mess trying to work with them together, to the point where looking at one node gets me confused about the other.

aaronfranke commented 5 years ago

@eska014 In addition, the scons option is already platform. It makes sense to be consistent.

leonkrause commented 5 years ago

The project settings 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.

Chaosus commented 5 years ago

Camera's near and far properties have different names than its setters/getters ( eg set_znear/set_zfar). This should be changed ?

aaronfranke commented 5 years ago

znear and zfar are confusing. It doesn't have anything to do with the Z axis in the world space. It could be changed to clip_near and clip_far since they control clipping planes, or just near and far.

Chaosus commented 5 years ago

Yeah, there are two ways to solve this problem.

reduz commented 5 years ago

We should get rid (or seriously discuss) binary resource extensions.. (RES_BASE_EXTENSION)

aaronfranke commented 5 years ago

gdscript_function.cpp and gdscript_functions.cpp are very similarly named, I keep getting them mixed up. Worth renaming? @vnen

I changed https://github.com/godotengine/godot/pull/21425 to rename "decimals" to "step_decimals" but keep "decimals" as an alias. Assuming it's merged, we can remove "decimals" in the next compatibility breakage, if not, just renaming.

@mysticfall In my opinion it's better to not have the word "get" in method names when unnecessary. Classes like Vector3 only contain get in one place, "get_axis", which is necessary because there's already an enum called axis. Actually the best thing is properties:

Sometimes you want a property to be able to be both get and set, but control access. In C#, properties allow you to do this and control access just by reading and assigning as if it was a field, which is nice.

 var thing = CollisionObject.ShapeOwner;
 CollisionObject.ShapeOwner = thing;

However, in GDScript, properties are not a thing (?). This could be possible if properties were added to GDScript. We could also have a method name without the word get. You can't overload methods so we must have "set". Most methods return something so it's better to have an implicit get-ness than set-ness: Since GDScript has properties I suggest using them more often. Note that I couldn't find any documentation on this. I did find documentation on how to do it inside of GDScript with setget but what about adding via C++?

In a nutshell, I agree that it's not good to have "get" in inconsistent places, but the ideal solution in my opinion is not really a possible at the moment in GDScript properties, or we could remove "get" and keep "set".

vnen commented 5 years ago

@aaronfranke GDScript does have properties in a way, since engine classes can define a getter and setter (or only a getter) and expose to GDScript as properties. That said, I'm against removing get and set from the methods because 1) it makes the name clearer and 2) it makes a distinction between getter and setter. For instance Mesh.SurfaceFormat() sounds like a method that "formats the surface", and not one that "gets the format". Also, most (if not all) of those can be ignored and used as properties instead anyway.

Now, I don't care that much about gdscript_function.cpp and gdscript_functions.cpp. One has the GDScriptFunction class, the other contains the definition of the GDScript functions, it's always clear to me which is which (though I'm used to it). It's also not a breaking change, so it doesn't need to be here.

neikeq commented 5 years ago

Yes, GDScript has properties. C# properties are generated from the ClassDB, which is where GDScript gets them from.

TGRCdev commented 5 years ago

There's a few methods for RigidBody and it's related classes whose parameters should be swapped for consistency:

Class and method list

aaronfranke commented 5 years ago

@TGRCdev I would vastly prefer changing apply_impulse to (force, position) rather than changing add_force to (position, force). Position of the force is an optional parameter so it should go at the end. But all forces and impulses must have a force vector.

TGRCdev commented 5 years ago

@aaronfranke Fair point. In that case, the list of required swaps is:

mysticfall commented 5 years ago

@aaronfranke I agree that using Get- or Set- prefixes is sort of a 'Javaism' which is better be avoided in C#.

My main concern was the usage of 'domain prefix' as in the case like shape_owner_get_shape or node_get_input_count, so if we can reimplement them as proper C# properties without Get- or Set- prefixes, it'd be even better.

On a side note, I always thought it rather odd that properties in Godot's C# API have matching set of getters and setters, like for example, Node.Name and Node.GetName()/Node.SetName().

It feels rather redundant to me, but if there's any reason why we keep such a convention, I suppose we would get both NodeInputCount and GetNodeInputCount()/SetNodeInputCount() anyway, if we are to rename node_get_input_count as suggested.

akien-mga commented 5 years ago

Guys, please keep discussions on the C# API and usual conventions outside this issue, which is focused on the Godot API. The Godot API (C++, C and GDScript) will not be adapted for C#'s sake, unless it's also an improvement for other languages.

mysticfall commented 5 years ago

@akien-mga I don't think it's C# specific thing that discussing if node_get_input_count could be renamed to something like get_node_input_count.

akien-mga commented 5 years ago

No, I mean anything C# specific should not be in this issue. There can be another issue for C#-specific compatibility breakages if need be (though there already are several of those IINM).

KoBeWi commented 5 years ago

How about renaming Spatial to Node3D? I always found it weird.

aaronfranke commented 5 years ago

@KoBeWi The naming scheme that Godot currently uses is "Thing2D" for 2D stuff and just "Thing" for 3D stuff, so it's fairly consistent with the rest of the 2D code. Of course then the logical thing to call "Spatial" would be "Node" following the pattern of removing "2D", but that name is already taken of course.