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

Created on 20 Feb 2018  Â·  366Comments  Â·  Source: godotengine/godot

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 :tada: reaction added by @akien-mga or @Calinou means the suggestion in the comment was incorporated into this post.

Classes

Methods

Properties

Signals

  • [ ] CanvasItem: hide should be renamed to hidden
  • [ ] Tabs: tab_close and tab_hover should be spelled tab_closed and tab_hovered
  • [ ] Tree: item_activated (label double-click) and item_double_clicked (icon double-click), names don't properly convey what they do #16839
  • [ ] XRController: button_release should be spelled button_released (like button_pressed)

Enums

Constants

  • [ ] Global Scope: PROPERTY_USAGE_STORE_IF_NONZERO and PROPERTY_USAGE_STORE_IF_NONONE should be fully dropped, also from GDNative

Theme items

  • [ ] ItemList, LineEdit, RichTextLabel, TextEdit and Tree: font_color_selected should be renamed to font_selected_color to match other _color properties. #30018

Objects

  • [x] CapsuleShape should be vertical by default #36488

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

breaks compat enhancement core tracker
Source
picture akien-mga
👍49 😕2 🚀1 👎1

Most helpful comment

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

picture leonkrause on 20 Feb 2018
👍19 🎉2

All 366 comments

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

leonkrause picture leonkrause on 20 Feb 2018
👍19 🎉2

16116

AlexHolly picture AlexHolly on 21 Feb 2018
🎉1

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

AlexHolly picture AlexHolly on 10 Apr 2018
🎉1

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

AlexHolly picture AlexHolly on 10 Apr 2018
👍2 🎉1

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.

AlexHolly picture AlexHolly on 15 Apr 2018
🎉1

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.

Chais picture Chais on 19 Apr 2018
🎉1 👍1

Animation.track_remove_key_at_position should be
Animation.track_remove_key_at_time

reduz picture reduz on 5 Jun 2018
👍3 🎉1

Methods

  • OS: execute should be split in two different methods, one blocking and the other non-blocking.
    e.g. names: execute/exec_process (blocking) and spawn_process (non-blocking) #19302
neikeq picture neikeq on 5 Jun 2018
🎉2

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

AlexHolly picture AlexHolly on 5 Jun 2018

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

Chaosus picture Chaosus on 8 Jun 2018
👍8 👎4

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

They are a bit long you are right.

AlexHolly picture AlexHolly on 8 Jun 2018

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

mysticfall picture mysticfall on 15 Jun 2018
❤1

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 picture akien-mga on 15 Jun 2018

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.

akien-mga picture akien-mga on 15 Jun 2018
👍5

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

mysticfall picture mysticfall on 15 Jun 2018

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

vnen picture vnen on 19 Jun 2018
🎉1

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.

Zylann picture Zylann on 20 Jun 2018
👍7 🎉1

OS.execute should use posix_spawn.

romeojulietthotel picture romeojulietthotel on 2 Jul 2018

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

groud picture groud on 2 Jul 2018
🎉2

@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

Zylann picture Zylann on 2 Jul 2018

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

groud picture groud on 2 Jul 2018

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.

jmf picture jmf on 2 Jul 2018
🎉1

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.

leonkrause picture leonkrause on 8 Jul 2018
👍7 🎉1

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

isaacremnant picture isaacremnant on 17 Jul 2018
🎉1

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.

Piet-G picture Piet-G on 23 Jul 2018
👍2 🎉1

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

neikeq picture neikeq on 24 Jul 2018

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

akien-mga picture akien-mga on 24 Jul 2018

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

Zylann picture Zylann on 24 Jul 2018

@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?

neikeq picture neikeq on 24 Jul 2018

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

akien-mga picture akien-mga on 24 Jul 2018

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

neikeq picture neikeq on 25 Jul 2018

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.

LikeLakers2 picture LikeLakers2 on 31 Jul 2018
👍2 🎉1

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

aaronfranke picture aaronfranke on 1 Aug 2018

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.

leonkrause picture leonkrause on 12 Aug 2018
👍6 🎉2

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

Chaosus picture Chaosus on 14 Aug 2018
👍2 🎉1

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.

aaronfranke picture aaronfranke on 14 Aug 2018

Yeah, there are two ways to solve this problem.

Chaosus picture Chaosus on 14 Aug 2018

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

reduz picture reduz on 16 Aug 2018
🎉1

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

aaronfranke picture aaronfranke on 28 Aug 2018

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

vnen picture vnen on 28 Aug 2018
👍1

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

neikeq picture neikeq on 28 Aug 2018

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

  • RigidBody.add_force(force, position) to add_force(position, force)
  • PhysicsDirectBodyState.add_force(force, position) to add_force(position, force)
  • PhysicsServer.body_add_force(force, position) to add_force(position, force)

Class and method list

TGRCdev picture TGRCdev on 29 Aug 2018
🎉1

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

aaronfranke picture aaronfranke on 29 Aug 2018
👍2

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

  • RigidBody.apply_impulse(position, impulse) to apply_impulse(impulse, position)
  • RigidBody2D.add_force(position, force) to add_force(force, position)
  • RigidBody2D.apply_impulse(position, impulse) to apply_impulse(impulse, position)
  • PhysicsDirectBodyState.apply_impulse(position, impulse) to apply_impulse(impulse, position)
  • Physics2DDirectBodyState.add_force(position, force) to add_force(force, position)
  • Physics2DDirectBodyState.apply_impulse(position, impulse) to apply_impulse(impulse, position)
  • PhysicsServer.body_apply_impulse(position, impulse) to body_apply_impulse(impulse, position)
  • Physics2DServer.body_add_force(position, force) to body_add_force(force, position)
  • Physics2DServer.body_apply_impulse(position, impulse) to body_apply_impulse(impulse, position)
TGRCdev picture TGRCdev on 29 Aug 2018
👍2

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

mysticfall picture mysticfall on 29 Aug 2018
👍1

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.

akien-mga picture akien-mga on 29 Aug 2018

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

mysticfall picture mysticfall on 29 Aug 2018

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

akien-mga picture akien-mga on 29 Aug 2018

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

KoBeWi picture KoBeWi on 30 Aug 2018
👍5

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

aaronfranke picture aaronfranke on 30 Aug 2018

The naming scheme that Godot currently uses is "Thing2D" for 2D stuff and just "Thing" for 3D stuff

So maybe change everything 3D to "Thing3D"? That crossed my mind too, but sounds like overkill.
Anyways, I just suggested. Not like it's very important. Also, Spatial2D is even worse than Spatial.

KoBeWi picture KoBeWi on 30 Aug 2018
👍2

So, String.right() returns n right characters from given position. Wouldn't it be more intuitive if it returned just n characters counting from the right?

"abcdef".right(2)
Instead of "cdef" would return "ef". IMO it'd be better.

KoBeWi picture KoBeWi on 8 Sep 2018
👍2 🎉1

I expected the same.

Python has the same method most user like to compare GDScript with Python. It would probably confuse even more changing it.

AlexHolly picture AlexHolly on 8 Sep 2018

@KoBeWi I agree. I don't see the difference between the current implementation and substring.

neikeq picture neikeq on 8 Sep 2018
👍2

The Godot substr forces you to indicate the size of the string if you want everyting on the right.

Zylann picture Zylann on 8 Sep 2018

@Zylann Most implementations allow to omit the second parameter. I would consider this an issue on Godot's side. Besides, I rather have substr make the second parameter optional than a new method with a different name.

neikeq picture neikeq on 8 Sep 2018

@Zylann @neikeq This is an unfortunate result of having non-overloadable methods, there's no way to have both length specification and no length specification.

aaronfranke picture aaronfranke on 9 Sep 2018

@aaronfranke Um, but default arguments do exist. 0 or -1 could count as no length specified.

KoBeWi picture KoBeWi on 9 Sep 2018
👍2

Need to remove get_selected_id() from OptionButton currently it always returns -1. After https://github.com/godotengine/godot/pull/21837 was merged get_selected_id() will return the same as get_selected().

AlexHolly picture AlexHolly on 10 Sep 2018
🎉1

There are a lot of tween methods which return true every time, they should probably be made void.

bojidar-bg picture bojidar-bg on 11 Sep 2018
🎉1 👍1

WindowDialog.get_close_button()
ConfirmationDialog.get_cancel() -> ConfirmationDialog.get_cancel_button()
AcceptDialog.get_ok() -> AcceptDialog.get_ok_button()

AlexHolly picture AlexHolly on 13 Sep 2018
👍3 🎉2

The Tree node has a get_selected() function with seems to return the focused TreeItem. It might be worth to rename it to something like get_active(), since focus and selection are different things.

groud picture groud on 30 Sep 2018
🎉1 👍1

load_from_globals() in InputMap should be load_from_project_settings().

merumelu picture merumelu on 3 Oct 2018
🎉1

I'll add a :tada: reaction to all suggestions which have been integrated into the OP, to have a better overview.

akien-mga picture akien-mga on 3 Oct 2018
🎉2

global_rotate should be renamed rotate_global and rotate_object_local should be renamed rotate_local for consistency and so that all rotate methods start with "rotate".

aaronfranke picture aaronfranke on 8 Oct 2018

global_rotate should be renamed rotate_global and rotate_object_local should be renamed rotate_local for consistency and so that all rotate methods start with "rotate".

Well, on the other hand I like that global-related function (like global_position, global_scale and global_transform) are next to each other in the auto-completion suggestions. Both solutions make sense IMHO.

groud picture groud on 8 Oct 2018
👍1

Maybe tree_exiting could be renamed to tree_exited as it seems it cause some confusion by now. See #22840.

groud picture groud on 8 Oct 2018

@groud There already is a tree_exited signal right?

Piet-G picture Piet-G on 8 Oct 2018

@groud There already is a tree_exited signal right?

Damn you're right. I guess the request in #22840 is valid then

groud picture groud on 8 Oct 2018

MenuItems.MENU_MAX is never used in LineEdit and TextEdit, should we remove it?

Same for Tabs.ALIGN_MAX https://github.com/godotengine/godot/blob/master/scene/gui/tabs.cpp#L750

AlexHolly picture AlexHolly on 15 Oct 2018

The Position3D and Position2D nodes are a bit ambiguous. Without reading the description, one might assume that they are like Spatial and Node2D but without rotation or scale. They should probably be renamed to PositionHint and PositionHint2D or perhaps some other word like Gizmo since it's a gizmo only in the editor or AxisMarker because it looks like a little axis marker.

If they were renamed to Gizmo then perhaps they could be given more general uses.

aaronfranke picture aaronfranke on 17 Oct 2018
👍2

Note that the same node in the Control tree is ReferenceRect, so, ReferenceAxis/2D might work as well.

bojidar-bg picture bojidar-bg on 17 Oct 2018

Loving this issue right now. May hate it later when the breakage actually hits ;)

Some proposals for the humble Array class:

  • duplicate → either copy or clone. I don't know of any language that calls this concept "duplicating". copy is what it's called in Python and in C++ so it would be the natural choice for Godot. clone is from Java and JavaScript and is maybe a bit more precise.
  • invert → reverse. The documentation even describes it as "reverse" so there is really no excuse.
  • remove vs. erase is confusing. Suggestion: remove_at and remove_value.

The last two also apply to all Pool*Array classes.

ttencate picture ttencate on 24 Nov 2018
👍8

Note: Duplicate → copy/clone applies to Dictionaries also.

bojidar-bg picture bojidar-bg on 24 Nov 2018

Rect2:

  • clip → intersection

AABB has intersection method but not clip, clipping generally means that we cut something out, which is not implemented in either class btw. Documentation describes it as:

Returns the intersection of this Rect2 and b.

Might as well rename:

  • intersects → overlaps
    in order not to be confused with the actual intersection operation:
Returns true if the Rect2 overlaps with another.
Xrayez picture Xrayez on 23 Dec 2018
👍2

grabber_area -> slider_progress
slider -> slider_background

image

Zylann picture Zylann on 1 Jan 2019

Some proposals for the humble Array class:
duplicate → either copy or clone.
...
Note: Duplicate → copy/clone applies to Dictionaries also.

And Nodes and Resources (basically any script-exposed data structure object, afaik).

willnationsdev picture willnationsdev on 15 Jan 2019

I prefer the word "Clone", I think it's more clear about what it does.

aaronfranke picture aaronfranke on 15 Jan 2019

Morning! @akien-mga couldn't we rename instance to new instead of instantiate? Having the same interface on PackedScene as Objects do for example removes some boilerplate for plugin creation especially, but probably more generally. @willnationsdev what do you think? I know you encountered this before.

Sorry if there is already a talk about this somewhere, I couldn't find it by skimming through.

grabber_area -> slider_progress
slider -> slider_background

image

or just:
grabber_area -> progress
slider -> background?

razcore-art picture razcore-art on 22 Jan 2019
👍1

I don't know if this has been discussed but AnimationPlayer has root_node with set_root & get_root, they should probably be set/get_root_node

razcore-art picture razcore-art on 23 Jan 2019

Rename CanvasItem.visible to CanvasItem.is_visible (and all other places where it's used)? to be in line with all (or most, maybe I missed some) bool properties?

razcore-art picture razcore-art on 24 Jan 2019
😕1 👍1

Rename Tween.tween_completed to Tween.tween_finished? Just like Animation.animation_finished? Generally prefer _finished over _completed? It feels like started/finished have a tighter relation than started/completed - biased from competition sports: start/finish - maybe :D

razcore-art picture razcore-art on 24 Jan 2019
👍3

Please consider rename the class JavaScript to HTML5 or something else.
This class has single method and it is not extends from Script and it is not used in other platforms.

The JavaScript can be used as the class name of JavaScript language bindings in the future.

Geequlim picture Geequlim on 26 Jan 2019

As pointed out by @clayjohn, WORLD_MATRIX in CanvasItem shaders is actually a modelview matrix. @reduz agrees that in 4.0 we should replace it by actual model and view matrices, e.g. CANVAS_MATRIX and ITEM_MATRIX.

akien-mga picture akien-mga on 26 Jan 2019
👍1

Consider renaming Array.invert to Array.reverse. Invert is more like upside down or the "reciprocal of" type of thing. See https://docs.godotengine.org/en/latest/classes/class_color.html#class-color-method-inverted

razcore-art picture razcore-art on 26 Jan 2019
👍3

Change CanvasItem.visibility_changed() signal to CanvasItem.visibility_changed(visibility: bool), ie. send the visibility status with it. Since this will be enough then remove CanvasItem.hide() signal

razcore-art picture razcore-art on 27 Jan 2019
👍2 👎1

Remove Resource::notify_change_to_owners, Resource::{un}register_owner.
They are used only by GridMap and CollisionShape, the rest of the code uses the "changed" signal.

bojidar-bg picture bojidar-bg on 25 Feb 2019
👍2 ❤1

Rect2: has_no_area should be renamed has_area that would prevent double negation logic checking the opposite in conditionals. Same applies to AABB: has_no_surface.

lupoDharkael picture lupoDharkael on 21 Mar 2019
👍2

Building on what @lupoDharkael said, Godot has several places where double negatives are used. Errors like "Condition !Math::is_nan(x) is false" are confusing.

aaronfranke picture aaronfranke on 21 Mar 2019
👍1

parse_input_event( InputEvent event )
Feeds an InputEvent to the game. Can be used to artificially trigger input events from code.

parse is misleading, parse would be receiving and processing but the description indicates sending or triggering an input

chrisb123 picture chrisb123 on 4 Apr 2019
👍3

As per #24153 - CanvasLayer uses layer to describe what layer to draw its nodes on. But nearly every other 2D node uses the terminology "Z Index" (z_index) to describe (what appears at first to be) the same thing. As @swarnimarun suggested https://github.com/godotengine/godot/issues/24153#issuecomment-444950584 improving the names for layers.

Sslaxx picture Sslaxx on 17 Apr 2019

Would OS.has_feature() / Platform.has_feature() be more sensible in something like ProjectSettings, since they don't exclusively convey anything about the OS?

avencherus picture avencherus on 25 Apr 2019
👍2

set_cell_item can be renamed to set_cell to unify GridMap with TileMap ?

Chaosus picture Chaosus on 4 May 2019
👍4

set_cell_item can be renamed to set_cell to unify GridMap with TileMap ?

Come to think of it, maybe there should be a set_cellv for GridMaps as well?

Jummit picture Jummit on 4 May 2019
🎉1

ARVRInterface:

  • ar_is_anchor_detection_enabled -> anchor_detection or similar
  • interface_is_initialized -> is_initialized or is_interface_initialized

AnimationPlayer:

  • play_backwards can be removed because play has an optional bool for that.

BaseButton / CollisionShape / CollisionPolygon / CollisionShape2D / CollisionPolygon2D:

  • Change the disabled bool to an enabled bool.

Bone2D:

  • get_index_in_skeleton -> get_skeleton_index
aaronfranke picture aaronfranke on 8 May 2019
  • play_backwards can be removed because play has an optional bool for that.

That would be bad for readability, at least as long as #6866 is not implemented. It's not a problem in C# as it supports named parameters.

neikeq picture neikeq on 8 May 2019

ID in id_pressed( int ID ) and id_focused( int ID ) should be lower case.

Jummit picture Jummit on 8 May 2019

^
That change actually wouldn't break any compatibility. It could get a separate issue probably.

KoBeWi picture KoBeWi on 8 May 2019

^
That change actually wouldn't break any compatibility. It could get a separate issue probably.

That's right!

Jummit picture Jummit on 8 May 2019

28746 - Node.replace_by might be confusing as a name. Not sure what exactly could be a better name though.

bojidar-bg picture bojidar-bg on 10 May 2019

@bojidar-bg maybe replace_self or swap_by? but I think that the only way to avoid any kind of confusion is by documenting it properly.

eon-s picture eon-s on 10 May 2019
👍2

If I have a Node2D with a script attached that contains class_name MyNode2D, the get_class() method returns Node2D instead of MyNode2D. This might be intentional but it's confusing and misleading.

EDIT: https://github.com/godotengine/godot/issues/26980

aaronfranke picture aaronfranke on 10 May 2019

@aaronfranke get_native_class() perhaps? Then you could get the script name from get_script().global_name if it has one.

Zylann picture Zylann on 11 May 2019
👍1

make_convex_from_brothers()
I guess "brothers" should be changed to "siblings", as that's the word used everywhere for sibling nodes.

KoBeWi picture KoBeWi on 19 May 2019
👍7 😄3

ARVRPositionalTracker: get_type() -> get_tracker_type()

  • get_tracker_type() is more explicit - get_type() could be confused with get_class()

  • GetType() is already used for something else in C# as noted here.

It returns a TrackerType, and there is also get_hand() which returns TrackerHand, so that could also be changed to get_tracker_hand() for consistency if desired.

ARVRPositionalTracker enum TrackerHand: TRACKER_LEFT_HAND -> TRACKER_HAND_LEFT (and right hand). Alternatively, TRACKER_HAND_UNKNOWN -> TRACKER_UNKNOWN_HAND, as long as it's consistent.

aaronfranke picture aaronfranke on 21 May 2019

Node.NOTIFICATION_TRANSLATION_CHANGED should probably become NOTIFICATION_LOCALE_CHANGED, as "translation" is used in Spatial nodes to mean "position" and not "language".

bojidar-bg picture bojidar-bg on 27 May 2019
👍4

set_as_toplevel() could be renamed to set_as_top_level(), but its behavior should be looked at.

aaronfranke picture aaronfranke on 3 Jun 2019
👍3

TouchScreenButton should be looked at for 4.0, as this change could be breaking: https://github.com/godotengine/godot/issues/28814

aaronfranke picture aaronfranke on 6 Jun 2019

CanvasItem method:

RID get_canvas_item() const
Returns the canvas item RID used by VisualServer for this item.

Should be renamed to get_rid() then.

get_canvas_item() is confusing because I'm already at CanvasItem node. It also ensures consistency as other classes have similar get_rid() method already: CollisionObject, Resource.

Xrayez picture Xrayez on 13 Jun 2019
👍3

Should Label be changed to TextLabel? Several times now I've wanted to put down a text object and forgot what it was called, so I search for "text" and only RichTextLabel shows up. TextLabel would be more consistent with RichTextLabel as it's still text but without the rich.

For reference, Unity has Text and TextMesh, and I personally refer to it as text rather than label.

aaronfranke picture aaronfranke on 17 Jun 2019
👍9 👎3 ❤2

I've also been wondering about Tree and GraphNode being renamed TreeView and GraphEditNode.
For Tree, the reason is it's way too broad of a name for a global GUI node IMO, all other GUI frameworks I know of use TreeView.
For GraphNode, it's because I worked on a few prototypes recently involving actual graphs structures, and I could neither use Node nor GraphNode which was quite annoying.

Zylann picture Zylann on 17 Jun 2019
👍4

@Zylann since the Graph nodes are visual/controls for graphs (not trees), GraphContainer may be better. Not sure about about GraphNode.

eon-s picture eon-s on 17 Jun 2019

Should we have lerpf/lerpv/lerpc instead of Color/Vector2/3.linear_interpolate? At least rename Color/Vector2/3.linear_interpolate to Color/Vector2/3.lerp

razcore-art picture razcore-art on 19 Jun 2019
👍2

As mentioned in #29598 http_escape / http_unescape to uri_encode / uri_decode

Geequlim picture Geequlim on 19 Jun 2019

Should we have lerpf & lerpv instead of Vector2/3.linear_interpolate? At least rename Vector2/3.linear_interpolate to Vector2/3.lerp

Shortening to vector.lerp() sounds good. At least as of https://github.com/godotengine/godot/pull/16106 usage of lerp() in script has a switch to support vectors and colors.

avencherus picture avencherus on 19 Jun 2019
👀1 👍1

Vector2.angle_to_point() should be renamed. It's name is not consistent with current description. Not sure what a good name would be, and whether this function is worth keeping.

Original issue: #16307

menip picture menip on 19 Jun 2019
👍2

@razcore-art Already a PR open about this https://github.com/godotengine/godot/pull/20371, and it keeps backwards compatibility (I think).

EDIT: Doesn't anymore.

aaronfranke picture aaronfranke on 19 Jun 2019
👍1

Area should really be Volume, since it's 3D. And Area2D should then be Area. An area is 2D by nature.

GridMap should maybe be CubeMap. Not sure on this one, just that "grid" sounds like a 2D thing to me.

CheckButton control should be SwitchButton or something. It's confusing because there is also Checkbox. Or maybe it should be removed altogether. It serves the same function as Checkbox anyway, as far as I can tell.

Rodeo-McCabe picture Rodeo-McCabe on 20 Jun 2019
😕3 ❤2 👎2 👍1

I agree re: CheckButton, and the others are just cosmetic.

Zireael07 picture Zireael07 on 20 Jun 2019

Or maybe it should be removed altogether. It serves the same function as Checkbox anyway, as far as I can tell.

UX-wise, CheckBox and CheckButton are different things, despite having the same functionality.
https://uxmovement.com/buttons/when-to-use-a-switch-or-checkbox/

KoBeWi picture KoBeWi on 20 Jun 2019
👍3

@Rodeo-McCabe Area is called like that for consistency with the 2D counterpart, also one of the definitions of area is a particular extent of space or surface.
Cubemaps are 3D images, syntax correctness must be on context or it just add confusion.

eon-s picture eon-s on 20 Jun 2019

grabber_area -> slider_progress
slider -> slider_background
image

or just:
grabber_area -> progress
slider -> background?

Maybe:
grabber_area -> progress_area or progressed_area
slider -> progress_background?

Anutrix picture Anutrix on 20 Jun 2019

Cubemaps are 3D images, syntax correctness must be on context or it just add confusion.

@eon-s Fair enough, I didn't know that.

Area is called like that for consistency with the 2D counterpart

The problem is the 2D counterpart is also inconsistent. In math an area is length * height, there just isn't any third dimension. So it doesn't make sense to say Area2D, because it should be 2D by virtue of being an area. Another, more mathematical definition of "area" is:

the surface included within a set of lines, specifically: the number of unit squares equal in measure to the surface

Similarly, a 3D space in math is called a "volume". I was confused at first when I found Area was actually a 3D node, instead areas were strangely called "Area2D". Being that this is a game engine, the math definitions are the ones we should take.

Rodeo-McCabe picture Rodeo-McCabe on 21 Jun 2019

While I understand the reasoning, I don't know if there would be any particular benefit to changing the name, especially to new people. I do agree that "Volume" might be a more appropriate term for this, but I feel like having "Volume" for 3D and "Area" for 2D may be a little confusing. After all, a lot of nodes who have both 2D and 3D variants will be named "

I think that the naming scheme being there in the first place (as much as there are some nodes who only mostly stick to the scheme) means that we shouldn't have an exception to the rule, even where another term for one variant would make more sense, as otherwise it's likely to confuse users.

If I may, however... perhaps rename Area2D to Area, and Area to Area3D?

Edit: Seems text surrounded by <> doesn't appear unless you escape the <>

LikeLakers2 picture LikeLakers2 on 21 Jun 2019
😕2

@Rodeo-McCabe I get the impression the node naming intention is to express things in human language and not mathematical. So the "area" isn't meant to be a geometric one, but a place to be, like inside of a level or stage. IE - Area 1.

The node itself doesn't directly have any geometry or is itself a geometry, so others like myself would wonder where is its area/volume when creating it, and then why would I have to attach areas and volumes (Shapes) to an area and volume?

If it were to undergo renaming to avoid confusing it with geometric area, synonyms would probably be the way to go.

They might be called:

  • Region2D/3D
  • Space2D/3D
  • Zone2D/3D
  • Field2D/3D
  • etc.
avencherus picture avencherus on 21 Jun 2019
👍2

Btw, apart that this tracker is only for method, properties and signals (not Nodes), @reduz mentioned recently that there are intentions to minimize compatibility breakage on 4.0, so probably this huge list may need a review from core devs before we continue adding things ad eternum...

eon-s picture eon-s on 21 Jun 2019

It sounds like the goal is to leave things much as they are right now, so stuff like this might be kicked back until the next major version after 4.0?

Sslaxx picture Sslaxx on 21 Jun 2019

Please leave this issue for core contributors, it's not a place to make suggestions of big renamings all over the place, the aim is to fix actual inconsistencies that make the API cumbersome.

The changes outlined in the OP are not a big compatibility breakage and most will be done for 4.0, what @reduz referred to is compatibility breakage of the "your project can't be opened anymore" kind as between 2.1 and 3.0 (a lot more changed than just things getting renamed back then, such as the audio and particle systems being completely rewritten, the import system changing, etc.).

There will be some compatibility breakage in 4.0, otherwise it wouldn't be called 4.0. It will be reasonable and porting will be easy, likely with a converter to ensure that renamed properties, methods and signals are properly converted if used in scenes and scripts. It's not the place to discuss it in any case :)

akien-mga picture akien-mga on 21 Jun 2019
👍4

Control's _set_anchor() should also drop the beginning "_".

YeldhamDev picture YeldhamDev on 27 Jun 2019

PopupMenu's has index_pressed(index) and id_pressed(id) which work fine, but also id_focused which is actually emitted with the index instead of the id of the item.

bojidar-bg picture bojidar-bg on 2 Jul 2019
👍1

No issue for this yet but after suggesting it to Akien today worth putting on the list.

Refactor the ARVRServer so it is called the XRServer. When we designed it the term XR to indicate both VR and AR wasn't in wide use yet. And no, I'm not going for MRServer ;)

BastiaanOlij picture BastiaanOlij on 11 Jul 2019
😄3

Should RayCast have a "Disabled" property instead of "Enabled"? CollisionShape has a "Disabled" which is false by default (which means they're enabled by default). In contrast, RayCast's "Enabled" property is false by default, making them disabled by default.

Or, to use the positive form (which I think is less confusing overall), should CollisionShape have an "Enabled" property (true by default) instead of "Disabled"?

Calinou picture Calinou on 12 Jul 2019
👍1

@Calinou I dunno if that would require a separate issue, but if we need to be consistent, I'd really prefer such booleans to be Enabled always. Setting a boolean to true to disable something confused me often.

Zylann picture Zylann on 12 Jul 2019
👍6

@Calinou I agree with @Zylann double negatives are confusing and should be avoided whenever possible. We should instead change "Disabled" bools to "Enabled" ones. It's fine if the default value of something is "true". I had this discussion in #17212 and #21822.

aaronfranke picture aaronfranke on 12 Jul 2019
👍6

The speed property of the mouse and touch input events should be renamed to velocity since it's a Vector2.

MightyPrinny picture MightyPrinny on 22 Jul 2019

InputMap: get_action_list( String action ) the function name doesn't tell much about what it does.
Since it returns the EventInputs associated to a given action, it could be get_action_events(String action)

Also could help avoiding possible confusion as InputMap has another function called get_actions( ) and both names could mean the same thing out of context.

FeatherAntennae picture FeatherAntennae on 22 Jul 2019
👍2

Tangentially related to #30736, we should rename the two Godot physics engines to something like "GodotPhysics2D" and "GodotPhysics3D", since saying that something's broken with "GodotPhysics" can mean two very different things.

ghost picture ghost on 23 Jul 2019
👍7 ❤1

What about renaming Object._init() to Object._new()?

get — _get
get_property_list — _get_property_list
notification — _notification
set — _set

new — _init

dalexeev picture dalexeev on 24 Jul 2019

I guess _init actually followed Python's __init__, while new is a method of the class, not the instance.

Zylann picture Zylann on 24 Jul 2019

Is something like String: empty() -> is_empty() a good fit for this thread? I always think it's a function to clear a string at first.

vortexofdoom picture vortexofdoom on 24 Jul 2019

@vortexofdoom if talking about method name inconsistencies and/or how they should be named, then yes.

I could add that String and NodePath have empty and is_empty methods respectively which do the same thing. The rest of the core built-in types seem to prefer empty name so maybe is_empty could be renamed in NodePath to make this consistent across all built-in types, but this could potentially involve some serious compatibility breakage I think.

I always think it's a function to clear a string at first.

Most methods use clear name in Godot for that.

Xrayez picture Xrayez on 25 Jul 2019

@Xrayez,

Most methods use clear name in Godot for that.

I know, just referring to the fact that empty is a verb as well as an adjective, and adding is_ makes clear which one is meant.

I'd be in favor of using is_empty across all built-ins for that bool.

vortexofdoom picture vortexofdoom on 25 Jul 2019
👍3

In Godot 3.0 and 3.1, we had Set methods in C#. However, these actually didn't add any actual functionality compared to just using a constructor and assignment, yet allowed code to silently fail, so they've been obsoleted. They were mostly added by me to try and be consistent, since a method for Quat already existed, which came from core having set methods. For more info see #30744

GDScript does have set_euler and set_axis_angle, but there are also constructors for doing the same thing (q.set_axis_angle(myvec3, myval) can be replaced with q = Quat(myvec3, myval). Core also has these methods for Basis, but they're not exposed to GDScript.

The question is then, what should be done about this? Should the Quat set methods be deprecated in favor of constructors and to be consistent with C#, or is it worthwhile to keep them to be explicit about the operation being performed? If the latter, should the Basis methods be exposed too?

aaronfranke picture aaronfranke on 29 Jul 2019

@aaronfranke I always found it strange those constructors had dedicated methods when basically nothing else does, so I'm for taking the former route if possible.

ghost picture ghost on 29 Jul 2019

@aaronfranke This issue is about renaming things in the API, not engine functionality or bugs.

akien-mga picture akien-mga on 18 Aug 2019
👀1 👍1

Geometry's point_is_inside_triangle() to is_point_in_triangle(), to match the other methods that also return bools in the same class.

YeldhamDev picture YeldhamDev on 25 Aug 2019
👍2

TreeItem.get_children() should be renamed get_first_child(). The doc should also explain that it's NOT returning the child items. Children are iterated using get_next().

Zylann picture Zylann on 30 Aug 2019
👍7

When working on #31976, I noticed the shadow atlas value must be a power of 2 (both for directional shadows and omni/spot shadows). However, the methods accept any integer value; if it's not a power of 2, the method will round it up to the nearest power of 2. We should probably make it accept an enum of values, so it can be presented in an user-friendly manner in the Project Settings.

Likewise, the anisotropic filtering level should be an enum (2×, 4×, 8×, 16×), since only power-of-two values make sense here.

Calinou picture Calinou on 4 Sep 2019
👍1

VisualServer's instance_create2() should be changed to actually describe what it does differently than instance_create().

YeldhamDev picture YeldhamDev on 5 Sep 2019
👍5

For object-relative translations, Spatial has translate_object_local which takes a Vector3, and Node2D has move_local_x and move_local_y, which take floats. Both Spatial and Node2D should have methods that take Vector3 and Vector2 respectively, and either translate_local or local_translate would be simpler names than translate_object_local.

aaronfranke picture aaronfranke on 11 Sep 2019

@leonkrause Instead of render_width and render_height it would make more sense to call it viewport_width and viewport_height, or perhaps canvas_width and canvas_height, since it's the reference resolution used for the canvas viewport, and doesn't actually affect rendering.

aaronfranke picture aaronfranke on 3 Oct 2019
👍2

@akien-mga please consider adding the Tree navigation methods to your list. They are very confusingly named and not well documented.

When I first encountered them I thought get_child() and get_next(), get_prev() operated an iterator much like they way Directory works. I had to do my own testing to find out that all they do is produce the same pointer each time they are called.

get_children() should be renamed to get_first_child().

get_next() and get_prev() should be renamed to get_next_sibling() and get_prev_sibling()

mwerezak picture mwerezak on 7 Oct 2019
👍6

What about renaming Engine.iterations_per_second to Engine.physics_fps for consistency with the Project Settings?

Calinou picture Calinou on 17 Nov 2019
👍5

is_processing:

Returns true if processing is enabled.

can_process:

Returns true if the node can process while the scene tree is paused

It might be better to rename can_process to something like can_process_paused to avoid confusion with is_processing method.

Xrayez picture Xrayez on 21 Nov 2019
👍1 
----
String print( Variant value, String indent="", bool sort_keys=false )

I think this method should be renamed.

dalexeev picture dalexeev on 22 Nov 2019

@dalexeev What should it be renamed to, and why?

Calinou picture Calinou on 22 Nov 2019

@Calinou This method does not actually print anything to the screen; it returns a string. The first thought is that JSON.print works just like Node.print_tree or OS.print_all_resources or like all other print* methods.

191123-1

What should it be renamed to? I'm not sure. JavaScript uses JSON.stringify for this. PHP has a json_encode function. Directly in GDScript there is a similar function - to_json.

UPD. JSON.serialize is also possible, but by the number of characters it is the same as JSON.stringify. :smile:

dalexeev picture dalexeev on 23 Nov 2019
👍1

I would suggest against the word "serialize" since that's usually used for binary serialization, and such an output would need to have further steps to get in that form.

Since this class only has two methods for going between JSON and Variant types, I would suggest against methods containing the word json since it's pointless.

Now, for things that would be a good name, the existing parse method doesn't really have a clear opposite word (see here). Perhaps one of these: encode, create, compose, generate? If encode is used it would make sense to rename the other method to decode.

aaronfranke picture aaronfranke on 23 Nov 2019

I've seen format and write being used as an inverse of parse. stringify has the benefit of being better-known among developers due to being used in JavaScript, however.

bojidar-bg picture bojidar-bg on 23 Nov 2019

I've seen format and write being used as an inverse of parse. stringify has the benefit of being better-known among developers due to being used in JavaScript, however.

str2var is VariantParser and var2str is VariantWriter internally (see #33241 for instance, I've even used the same term to describe it).

So I'm for write, consistency wins. 😛

Xrayez picture Xrayez on 23 Nov 2019

@Xrayez Change print to write? From Python to Pascal? :laughing:

dalexeev picture dalexeev on 23 Nov 2019
😄1

As suggested in https://github.com/godotengine/godot-proposals/issues/252#issuecomment-557901552, it might make sense to rename everything related to clear_color to background_color (including the project settings).

Calinou picture Calinou on 24 Nov 2019
👍3

I would expect 4.0 to bring a big wave of new people into Godot due to better performance, GI probes improvements, general hype and so on.

If those changes are made with 4.0, these people will get a hard landing as almost no existing tutorial (besides the official "first game" tutorial) will work for them anymore.

If those changes are made after 4.0, lets say in 4.1, it will be an extremely bumpy ride for those people. They just learned the basics a new engine, which they now have to relearn again. Beginnings are hard, having to go through this twice too shortly after another can be frustrating enough to give up altogether.

Would it therefore not make sense to have a "3.3" or "3.9" version before the 4.0 release that has all those compatibility breaking API changes, however gives the tutorial creators enough time to create some tutorials before expected massive influx of new users?

golddotasksquestions picture golddotasksquestions on 24 Nov 2019
😕1

Not sure if this is the right place or not but since this could be a possible partial solution to renaming issues I will pitch it here.
Why not make all the renaming changes that are required and then add a new language to translations called GodotOldNames/GodotPre4.0 which has all old names for all the changes.
This way in case someone doesn't find the old names present in old tutorials, they can just change language to GodotPre4.0 . This would also make switching to new names easier.
This doesn't solve the whole problem but might work along with #4397.

Anutrix picture Anutrix on 24 Nov 2019
👎2

@Anutrix This would add a lot of complexity (what about non-English languages?). Moreover, not all strings displayed in the editor are localizable in the first place.

Calinou picture Calinou on 24 Nov 2019

Physics "Layer", Render "Layer"

I remember going exactly through the same confusion as this poor person during my first few weeks of learning Godot.

Physics "Layer", Render "Layer" might make sense to someone coming from Object Orientated Design, but to anyone else it's very confusing. The term "layers" is used when describing multiple coatings of paint, or coats of cloth in a fabric, coats of skin on an onion (or ogre), coats of sediment on the planet surface, ...
Layers (plural, opposed to a single layer), seem to imply being stacked on top of each other in all those cases.

For many people "layering" especially when working with 2D graphics or animation implies working with a foreground, background, and layers in between or on top. Many people think of layers as celloloid films on top of a background, when in fact Godot like many other game engine uses various ways of "sorting" to accomplish this task.

The fact that we call the abstract commonality that Physics objects or Render objects might share "Layers", while at the same time these abstract commonalities have nothing to do with the stacking appearance of visual layers (that's "sorting"), is what makes this so confusing to some.

When I understood the real use and meaning of Physics "Layer", Render "Layer", I immediately wondered why these are not Physics Group, Render Group, and to be honest, I still don't know. They certainly don't have any ontop-stackable relation to each other, meaning their order or relation to each other is completely irrelevant. Naming them layer, imho archives nothing but confusing people, "group", "affected by group" for masks, or maybe some other term I can't think of right now would be more accurate.

golddotasksquestions picture golddotasksquestions on 27 Nov 2019
👍1

AnimationPlayer method
.stop(false) should be called .pause(true)
because that's what it does.

golddotasksquestions picture golddotasksquestions on 6 Dec 2019
👍1

Project > Project Settings > Display > Window > Stretch > Mode:

Stretch Mode "2D" should be called Stretch Mode "display", or "general purpose"
because it's confusing to call it 2D when it works just as well for 3D usecases, but not really well for all 2D usecases (like pixelart projects, while almost halve the Godot 2D projects appear to be pixelart.)
"display" would also be more descriptive for what it actually does: Rendering all graphics be it 2D graphics, 3D objects, 2D meshes, Line2D and Polygons at the display resolution.
More on that here.

golddotasksquestions picture golddotasksquestions on 6 Dec 2019

loop, repeat, oneshot, of animationplayer, timer, and similar nodes, could be clarified.

looping, repeating, and not being a oneshot, are all the same things.

TheDuriel picture TheDuriel on 11 Dec 2019
❤2 👍2

I think that
is_a_parent_of()
should be renamed to
is_parent_of()

KoBeWi picture KoBeWi on 11 Dec 2019
🚀1

@KoBeWi see discussion at #19801.

Xrayez picture Xrayez on 11 Dec 2019

I think that
is_a_parent_of()
should be renamed to
is_parent_of()

I don't think so, is_parent_of() suggests it only takes into account the first parent, while the function checks for all parents recursively.

groud picture groud on 11 Dec 2019

@groud In that case, it should be called is_ancestor_of(). If there's also a corresponding function for the other way around, then it should be called is_descendant_of().

aaronfranke picture aaronfranke on 11 Dec 2019
👍5

@groud In that case, it should be called is_ancestor_of(). If there's also a corresponding function for the other way around, then it should be called is_descendant_of().

Yes, but thinking about it there is not much of a need to have both functions, as the difference would only be a matter of switching the "calling" object and the function argument. Maybe we could simply replace the function by something like is_descendant_of() and we're done .

groud picture groud on 11 Dec 2019
👍3

We may want to rename push_error() and push_warning() to print_error() and print_warning() respectively. This would make them more discoverable thanks to autocompletion :slightly_smiling_face:

Calinou picture Calinou on 12 Dec 2019

@Calinou print_error() might be confused with printerr()

vnen picture vnen on 12 Dec 2019

Please, consider changing the name of TreeItem.get_children() method, as the name implies a collection of children to be the return value, when in practice it is the first child that is returned and then you have to iterate over the rest of them using get_next() (as described in #19796).

pycbouh picture pycbouh on 19 Dec 2019
👍6

Quoting @Zylann from #31404:

[Rename rpc() / rset() to] remote_call() / remote_set()?
Those methods have really short names as well, not sure if an alias is enough. You gotta really do multiplayer a lot with tons of these calls per script to justify 3-character identifiers (which I'm sure majority of games don't).

Edit (2020-01-03): On second thought, call_remote() and set_remote() might make more sense as we already have call_deferred() and set_deferred().

Edit: Don't mind the closing/reopening below, I clicked too fast.

Calinou picture Calinou on 24 Dec 2019
👍1

Following methods

OS.find_scancode_from_string
OS.get_scancode_string
OS.is_scancode_unicode

should be moved from the OS class to the Input class for consistency with the methods:

Input.get_joy_axis_index_from_string
Input.get_joy_axis_string
Input.get_joy_button_index_from_string
Input.get_joy_button_string
dalexeev picture dalexeev on 28 Dec 2019
👍3

TabContainer should have its tab_close and tab_hover signals edited as well.

YeldhamDev picture YeldhamDev on 6 Jan 2020
  • LightOccluder2D light_mask -> occluder_light_mask
  • CPUParticles Flags enum -> ParticleFlags
  • ARVRPositionalTracker get_type -> get_tracker_type
  • ARVRPositionalTracker get_name -> get_tracker_name
  • PathFollow2D rotate -> something else
  • ArrayMesh ArrayType enum -> delete this
  • Mesh BlendShapeMode enum -> give to ArrayMesh

Explanations in https://github.com/godotengine/godot/issues/15763#issuecomment-568908661

aaronfranke picture aaronfranke on 6 Jan 2020

RichTextLabel's meta_hover_started and meta_hover_ended signals should be renamed to match most other similar naming conventions: meta_hover_entered and meta_hover_exited. Not only does this make it more closely imitate the rest of the Godot API, but the current naming scheme, due to alphabetic sorting, causes their order to be reversed. This is disorienting a bit when you are reading the docs since, clearly, the chronological flow of operations is to first enter and then exit. It just all around improves readability and consistency to change it.

willnationsdev picture willnationsdev on 11 Jan 2020

I noticed that there is no Texture.size property. I needed to call the Texture.get_size() getter instead.

I asked in Discord on whether this was intended or not. One suggestion was to post here asking if this needs converted to a property, the other suggestion was that since there's no 'setter' for this, using get_size() is the currently expected behavior.

jmbjr picture jmbjr on 20 Jan 2020

@jmbjr Do we have any read-only properties exposed as properties (instead of just getter methods)? I remember seeing a built-in error handler when trying to set a read-only property. I'm just not sure if it's exposed to GDScript in the first place.

Calinou picture Calinou on 20 Jan 2020

Yeah, I think that if you were going to start supporting read-only exposed properties, then you'd need a USAGE flag for it and to also add GDScript parser/compiler code that supports it.

willnationsdev picture willnationsdev on 20 Jan 2020

SoftBody has an areaAngular_stiffness which should be area_angular_stiffness (same for all related methods).

akien-mga picture akien-mga on 23 Jan 2020
👍2

Input.joy_connection_changed (the method) seems like it should not be exposed to the scripting API, it's called internally by each platform's joystick handling code.

akien-mga picture akien-mga on 26 Jan 2020

@akien-mga When I first saw that method, very early after discovering Godot, I had similar thoughts, then I remembered how Kojima has build some legendary gameplay in MetalGearSolid around a method that must have been very similar to this and I thought: "Godot even gives me the means to beak the 4th wall just like Kojima with a single line of code, how awesome is that!"

golddotasksquestions picture golddotasksquestions on 26 Jan 2020

Label and RichTextLabel have inconsistent theme properties:

Label:
int line_spacing [default: 3]
Color font_color [default: Color( 1, 1, 1, 1 )]

RTL:
int line_separation [default: 1]
Color default_color [default: Color( 1, 1, 1, 1 )]

Also, due to different default values, the same font has different heights in Label and RichTextLabel.

200130-1

dalexeev picture dalexeev on 30 Jan 2020

TextEdit's request_completion signal could be renamed to completion_requested to use past tense. The current version sounds somewhat imperative, contrary to the callback-y nature of signals.

katoneko picture katoneko on 30 Jan 2020

another inconsistency with physics body signals

Area:

area_shape_entered( int area_id, Area area, int area_shape, int self_shape )
area_shape_exited( int area_id, Area area, int area_shape, int self_shape )
body_shape_entered( int body_id, Node body, int body_shape, int area_shape )
body_shape_exited( int body_id, Node body, int body_shape, int area_shape )

i suppose area_shape in these last two should be self_shape ? or ...

Rigid Body:

body_shape_entered( int body_id, Node body, int body_shape, int local_shape )
body_shape_exited( int body_id, Node body, int body_shape, int local_shape )

here it is called local_shape ...

FrederickDesimpel picture FrederickDesimpel on 7 Feb 2020
👍1

@FrederickDesimpel As far as I know, arguments can be renamed without breaking compatibility. Feel free to open a pull request for this :slightly_smiling_face:

Calinou picture Calinou on 7 Feb 2020

In Variant:

Real -> Float
Nil -> Null?

reduz picture reduz on 17 Feb 2020

I like the "real" name personally, since the term "float" is often used for 32-bit floats specifically, but Variant's real is a double, and most of the engine uses real_t which can be either.

P.S. We already did that rename the other way around in 2017.

aaronfranke picture aaronfranke on 17 Feb 2020
👍1

Did we consider renaming these:
shader_type canvas => shader_type 2d
shader_type spatial => shader_type 3d
CanvasItemMaterial => Material2D
SpatialMaterial => Material3D

Zylann picture Zylann on 22 Feb 2020
👍5

@Zylann SpatialMaterial was already renamed to StandardMaterial3D in master.

aaronfranke picture aaronfranke on 22 Feb 2020

Should the limit_ values on Camera2D be changed to a Rect2i? Right now they're just four ints.

EDIT: Drag margin could also be changed, to a Rect2.

aaronfranke picture aaronfranke on 6 Mar 2020

String::begins_with to String::starts_with.

Like in serious programming languages (Java, Python etc).

hilfazer picture hilfazer on 7 Mar 2020
😕3 😄3 👍1

InputEvent.is_action_pressed to InputEvent.is_action_just_pressed
InputEvent.is_action_released to InputEvent.is_action_just_released

https://github.com/godotengine/godot-proposals/issues/316#issuecomment-589426014

Although the word "just" is missing, event.is_action_...() corresponds to Input.is_action_just...().

Jummit picture Jummit on 8 Mar 2020
👍1

We should probably swap Node.add_child_below_node()'s first two arguments so the first argument is the same as Node.add_child(). See #19642.

Calinou picture Calinou on 14 Mar 2020
👍2

_semantical atelophobia speaking…_

Should Node2D.draw_circle be Node2D.draw_disk since it draws a disk and not a circle?
Node2D.draw_circle could still exist as a shortcut for draw_arc from 0 to TAU.

Goutte picture Goutte on 24 Mar 2020

@Goutte In English, "circle" can refer to either a hollow circle or a filled circle. I think the current name is more discoverable, so I wouldn't change it.

aaronfranke picture aaronfranke on 24 Mar 2020
👍2

In English, "circle" can refer to either a hollow circle or a filled circle.

I can't find anything supporting this. Do you have any source for this claim or is it a gut feeling?

About discoverability, we can still have draw_circle, it would simply draw a circle, and not a disk.

Goutte picture Goutte on 24 Mar 2020

I can't find anything supporting this. Do you have any source for this claim or is it a gut feeling?

https://www.merriam-webster.com/dictionary/circle

1 b : a closed plane (see plane entry 6 sense 2b) curve every point of which is equidistant (see equidistant sense 1) from a fixed point within the curve
1 c : the plane surface bounded by such a curve

(1 b) is a mathematical circle (perimeter), (1 c) is a mathematical disk (surface).

In terms of API, it's IMO more user-friendly to have draw_rect(bool filled) and draw_circle(bool filled) than draw_rect(), draw_filled_rect(), draw_circle() and draw_disk() (or what would be the mathematical name for a filled rectangle?).

Edit: Actually draw_circle() doesn't have a filled argument yet. I think we should add one, so that it can be used to draw both circles (perimeter) and disks (filled circle).

akien-mga picture akien-mga on 24 Mar 2020
👍5

Nice, thanks. Most of my students are french and all of them were confused by this, and so was I, and they blamed Godot and I could not let them, of course.

what would be the mathematical name for a filled rectangle?

That's a pretty good question ; all I can find is "rectangle area" or "rectangle interior", and none is adequate. The wiki even states that most people abuse the term _polygon_ to mean the _interior of a polygon_, without providing another word for it.

I guess a filled argument could help alleviate the ambiguity, but it's going to be a pain to decide where to put it in the list of arguments.

Goutte picture Goutte on 24 Mar 2020

@Goutte but it's going to be a pain to decide where to put it in the list of arguments.

Since it's an optional argument (it has a default value), and also for consistency with draw_rect, it should go at the end.

aaronfranke picture aaronfranke on 24 Mar 2020
🎉3

There are cases in which Control nodes are refereed to as Modal nodes. https://github.com/godotengine/godot/search?q=modal&unscoped_q=modal Mostly in relation to the Viewport.get_modal_stack_top() function

TheDuriel picture TheDuriel on 31 Mar 2020
👍1

EditorInterface's get_selected_path and get_current_path methods are confusing in name and in function. They also lack documentation.

These methods are just a thin layer on top of FileSystemDock's methods of the same name:

String FileSystemDock::get_selected_path() const {
    if (path.ends_with("/"))
        return path;
    else
        return path.get_base_dir();
}

String FileSystemDock::get_current_path() const {
    return path;
}

They should both adapt either "selected" or "current" (or something else, but for both of them) with one being get_*_path and the other get_*_dir.

pycbouh picture pycbouh on 8 Apr 2020

@Goutte Isn't Solid Rectangle an alternative to filled rectangle?

Anutrix picture Anutrix on 8 Apr 2020

Will the OP be updated with suggestions posted after the June of 2019? I understand that this is a lot of work, but this type of tracker is also perfect for contributors to tackle together. And I assume it's already the time when we can get to work on renaming more stuff?

Updating the OP would also mean that the suggestion posted is accepted as worthwhile by the team.

pycbouh picture pycbouh on 8 Apr 2020
👍2

@Anutrix "filled" is a better word to use than "solid", because "solid" can mean "not liquid or gas".

@pycbouh It would also be a good idea for linking PRs for each issue if there is one. The OP already does this, but not for the comments below it.

aaronfranke picture aaronfranke on 8 Apr 2020
👍3

I don't know if it was raised, but I didnt realise how often I stop to peek into the doc for this one:

  • Array.remove => remove_at (like C#) to remove by index
  • Array.erase => remove_value, or just remove (like C#) to remove by value

(or variants of that with erase)

Because right now, erase and remove mean the same to me, except one is by index, the other by value, I keep forgetting which is which.

Was raised already, my bad. Didn't notice, Github is folding the thread, my Ctrl+F search missed it xD
Not in the OP yet

Zylann picture Zylann on 8 Apr 2020
👍4 ❤1

@Zylann Here you go: https://github.com/godotengine/godot/issues/16863#issuecomment-441376010

pycbouh picture pycbouh on 8 Apr 2020

Seconding Zylann, I keep forgetting that remove is by index...

Zireael07 picture Zireael07 on 9 Apr 2020

The ButtonList enum likely should be renamed to MouseButtonList since they are mouse buttons.

Should the JoystickList enum be split up? It contains both buttons and axes, it might make more sense as JoypadButtonList and JoypadAxisList or similar.

aaronfranke picture aaronfranke on 13 Apr 2020

Just a question, why not MouseButton?
if button == MouseButton.LEFT:
reads nicer than
if button == MouseButtonList.LEFT:

Jummit picture Jummit on 13 Apr 2020
👍2

Good idea. In that case, there's also KeyList -> Key, MidiMessageList -> MidiMessage, and the joystick ones need to have List removed.

aaronfranke picture aaronfranke on 13 Apr 2020

I noticed some methods/properties use normal_map, whereas others use normalmap. We should settle on either of these, but probably not both. I'd prefer normal_map, but I'm OK with normalmap too.

Calinou picture Calinou on 17 Apr 2020
👍3 🎉1

GDScript

range_lerp() = map()
seed = set_seed()

TheDuriel picture TheDuriel on 18 Apr 2020
😕2

map() is probably reserved for the new functional programming features.

jabcross picture jabcross on 20 Apr 2020
👍1

Vector2.tangent() is described in the docs as: Returns a perpendicular vector., that's the definition of orthogonal or orthonormal if the returned vector is normalized. Since Vector2.tangent() returns a non-normalized vector I propose we should rename it to Vector2.orthogonal() or even Vector2.perpendicular() if people have something against math nomenclature or perhaps even Vector2.normal(), but people might get confused between normalized() and normal()

razcore-art picture razcore-art on 23 Apr 2020
👀1 🎉1 👍1

Anecdotal on my end, but my first time looking for this, my first searches in the docs were for _perpendicular_.

avencherus picture avencherus on 23 Apr 2020

You could also keep .tangent() as just an alias for .perpendicular(), no?

willnationsdev picture willnationsdev on 23 Apr 2020
👍1

I don't like the name orthogonal because it's not a common English word compared to the others. I didn't see a problem with tangent before, but I think perpendicular is a better name anyway.

aaronfranke picture aaronfranke on 23 Apr 2020

Yeah, orthogonal is rare. I vote for .perpendicular() but KEEPING the tangent() alias for compat as well as, it's shorter.

Zireael07 picture Zireael07 on 24 Apr 2020

@Zireael07 I would prefer if we only had one way to get the tangent. I don't think it's a very common operation after all.

Calinou picture Calinou on 24 Apr 2020
👍1

You'd be surprised how often I use it in my procgen scripts :)

Zireael07 picture Zireael07 on 24 Apr 2020

I didn't expect this to spawn a debate, but I'm personally against keeping Vector2.tangent() since it's an incorrect usage of the usual math term. The product of Vector2.tangent() is just wrong by definition. The name of this thread is next planned compatibility breakage so I don't see any reason why we should bend over backwards to keep compatibility. I'm personally fine with Vector2.perpendicular().

razcore-art picture razcore-art on 24 Apr 2020
👍7

Suggestion: make find_node()'s owner parameter false by default.

follower picture follower on 12 May 2020
👍1

Suggestion: rename Tree's methods/signals to be less confusing with regard to e.g. terms/statuses of "selected", "focused", and "nothing".

follower picture follower on 12 May 2020

I think Tree::ensure_cursor_is_visible is misleading and should be renamed to something like ensure_focused_item_visible or ensure_selected_item_visible.

Even the class reference says:

Makes the currently focused cell visible.

This will scroll the tree if necessary. In SELECT_ROW mode, this will not do horizontal scrolling, as all the cells in the selected row is focused logically.

Note: Despite the name of this method, the focus cursor itself is only visible in SELECT_MULTI mode.
HaSa1002 picture HaSa1002 on 16 May 2020
👍2

Script::get_instance_base_type() specifically returns the native type underneath. Now that we have named script classes, it makes more sense for this to be renamed to get_native_type() since the "base" of the script could potentially be the name of a script. It's misleading.

willnationsdev picture willnationsdev on 17 May 2020
👍1

@willnationsdev There's also get_class() https://github.com/godotengine/godot/issues/16863#issuecomment-491421079

aaronfranke picture aaronfranke on 17 May 2020

@aaronfranke Well, with get_class() it's universally used to refer to "what is the name of the thing I am currently looking at?". So, in the case of a Script, I would expect to get back "GDScript", "CSharpScript", or something of that nature. But yes, with non-Script classes, it should be a method that returns the name of the actual script class if it is named. There's even a dedicated issue for it. https://github.com/godotengine/godot/issues/21789#issuecomment-618588900

Edit: Oh, I guess, if we have get_class(), get_base_class(), and get_native_class(), you really need get_instance_base_type() to become get_instance_native_class() to keep with the naming convention and clarify that it's the instance rather than the script's info.

willnationsdev picture willnationsdev on 17 May 2020

I think autotile_coord (properties and methods) of TileMap should be renamed for tile_coord or tile_coordinate because both AUTO_TILE and ATLAS_TILE use this and the name may be confusing for new users. Docs will also need an update. I can make this change if there is no problem.

guilhermefelipecgs picture guilhermefelipecgs on 17 May 2020
👍2

Consider removing new_text argument from LineEdit.text_changed since it has the same behaviour as LineEdit.text.

Also consider adding old_text either in addition to or as a replacement of new_text.

Related to https://github.com/godotengine/godot/issues/16863#issuecomment-394745886

nathanfranke picture nathanfranke on 18 May 2020
👍3

Rename "collision layers", "VisualInstance layers", "render layer", ect to
"collision group", "VisualInstance group", "render group", "light group"

Confusion about why those "Layers" are not stacking comes up like minute clockwork on the community channels.

golddotasksquestions picture golddotasksquestions on 18 May 2020

Note that they are called layers in Maya, Lumberyard and Unity.

Jummit picture Jummit on 18 May 2020
👍1

Just because someone else does something doesn't mean it still doesn't cause
confusion and is a good idea.

On Mon, May 18, 2020, 1:09 PM Jummit notifications@github.com wrote:

Note that they are called layers in Maya, Lumberyard and Unity.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/16863#issuecomment-630349336,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABBEIS43HMTPCIFY4GYYK3LRSF2UTANCNFSM4ERRCEZA
.

Two-Tone picture Two-Tone on 18 May 2020
👍1

I'm surprised that no one mentioned it yet buuut
Camera2D.zoom
the camera is zoomed out when the zoom is greater, which is not how "zoom" works and it's counter-intuitive. Not sure what rename it to, maybe view_scale or something similar.

KoBeWi picture KoBeWi on 19 May 2020
👍5

@KoBeWi Instead of renaming zoom, shouldn't we invert its scale so it zooms in when you increase the value?

This will break compatibility just like renaming the property would. If we go the "invert scale" route, it can't be fixed automatically, unfortunately. Still, it might result in less confusion in the long term.

Calinou picture Calinou on 19 May 2020
👍1

@Calinou @KoBeWi Is there a use-case for using this zoom property directly though, or would everyone need to invert it each time to apply to scale?

pycbouh picture pycbouh on 19 May 2020

Viewport.get_final_transform() => Viewport.get_final_transform_2d() ?

Zylann picture Zylann on 20 May 2020
👍1

Viewport.get_final_transform() => Viewport.get_final_transform_2d() ?

Maybe, but where do we draw the line with that and something like Node2D.get_position_2d()?

Maybe we could name the class Viewport2D but that wouldn't be required unless we make a Viewport3D.

nathanfranke picture nathanfranke on 20 May 2020

@nathanfranke I proposed this method because in the name alone there is no way to tell if it's 2D or 3D, considering there are 2 types of Transform. (While other members have canvas_ or mouse_ already)

Zylann picture Zylann on 20 May 2020

@Zylann This method is actually really weird. There are properties called canvas_transform and global_canvas_transform, and the description of canvas_transform is this:

The canvas transform of the viewport, useful for changing the on-screen positions of all child CanvasItems. This is relative to the global canvas transform of the viewport.

So, you would expect get_final_transform to get both of these transforms combined. However, if you look at the code, that's not actually what it does.

The code for this is return stretch_transform * global_canvas_transform;. I've been trying to figure out what stretch_transform is for and I have no idea, it's not exposed, and it's certainly not set when changing canvas_transform. There is also a method called _stretch_transform which does not use stretch_transform. It's called by set_size, which takes the value generated by _stretch_transform and assigns it to stretch_transform. There's also canvas_transform_override which I haven't even mentioned and isn't exposed, but is used internally.

In the end I think this whole class should be looked over. Viewport has 4 different Transform2D members, 4 Rect2(i) members, 9 Vector2(i) members, and 2 Transform (3D) members. That's already 264 bytes (with single-precision floats) of just data structures storing the transformation information, if you add up all the pointers and such it's probably closer to 1 KB. Maybe Viewport does need all this, but it seems overcomplicated, and at the very least there should be comments (there are far too few in this file).

aaronfranke picture aaronfranke on 21 May 2020
👍3

Should Node2D/Node3D to_global and to_local be removed? I gave feedback on #38902 which proposed adding methods that only work with the basis, but there are some issues with these.

In the demo projects, to_local is not used anywhere, and $ grep -RIn "to_global" returns only 5 results, all of which are in the GUI in 3D demo, and the performance of the demo could be improved by changing this so that it caches the global transform and uses xform instead of using to_global.

In a nutshell, the concern with these methods is both that they are uncommonly used, and also that they encourage writing code that performs bad since it recalculates the global transform several times.

aaronfranke picture aaronfranke on 21 May 2020
😕1

27509

AnimationPlayer's animation_started and animation_finished are a bit counter-intuitive. The first one is called for all animations in the queue while the latter is only called on reaching the very end of the queue. It isn't clearly mentioned in the documentation.

If we want to detect when any particular animation ends, we have to use both animation_changed and animation_finished which isn't convenient.

I really like @txigreman's suggestion from that issue: We can use animation_finished whenever any single animation in the queue ends and add a new signal animation_all_finished(similar to Tween's tween_all_completed) which fires only when we reach the queue's end.

AnEnigmaticBug picture AnEnigmaticBug on 21 May 2020
👍3

What about animation_queue_finished and/or animation_queue_started?

aaronfranke picture aaronfranke on 21 May 2020

I can't find it here so I would propose,
find and findn pair.

image
image from 3.2 last stable build.

Maybe we can rename one to mention their nature of dealing with case sensitivity.

Say, find_ignorecase instead of findn?

swarnimarun picture swarnimarun on 22 May 2020
👍1

@swarnimarun We have many other case-insensitive methods ending with n. If we rename one of them, we should rename all of them.

Calinou picture Calinou on 22 May 2020
👍1

These methods (find/findn etc.) basically do the same. We could just add an argument whether they should be case sensitive or not.

KoBeWi picture KoBeWi on 22 May 2020
👍5

@Calinou I would very much prefer that, using GDScript after dropping it for a few months kinda has given me a new perspective to the API I would say,

The more obvious the better. Remembering the API for everything is hard on its own, a bit more verbosity in the naming of functions seems like a sane change.

swarnimarun picture swarnimarun on 22 May 2020

@KoBeWi being able to tell with the function name what it does rather than having to remember to add another field or while reading code(after forgetting/not-knowing that part of the API) is quite appreciated.

So I would still prefer a different function, even if the other function just calls the first one with different arguments or something.

swarnimarun picture swarnimarun on 22 May 2020

EditorInterface.get_editor_viewport() => get_editor_main_container()

This function does not return a Viewport, only a Control which happens to be the central one holding the 2D, 3D, Script editors and the Asset Library. For the record, the returned node is even a VBoxContainer*, but is abstracted away (yet it's important to know, because it affects your choices of settings).
The doc is also wrong, as its description links to the Viewport class. https://docs.godotengine.org/en/stable/classes/class_editorinterface.html#class-editorinterface-method-get-editor-viewport

Zylann picture Zylann on 26 May 2020
👍2

@Zylann Should be get_editor_main_screen() since it's not a Container, but it's the main screen.

aaronfranke picture aaronfranke on 26 May 2020
👍2

@aaronfranke which is also a Container node, actually^^ but yeah... main screen can work too

Zylann picture Zylann on 26 May 2020
👍1

200528-1

I want to ask: does anyone track proposals in this topic?

For example, above (https://github.com/godotengine/godot/issues/16863#issuecomment-557657413) I proposed renaming the JSON.print method. Several options were suggested, but none of them appeared in the first post.

I'm just worried that useful ideas get lost in so many posts. :smile:

dalexeev picture dalexeev on 28 May 2020

@dalexeev I recently did a first pass at updating the first post, but I didn't quite go through all comments yet.

Calinou picture Calinou on 28 May 2020
👍3 ❤1

I have one that potentially fixes some bugs in RichTextLabel. Currently we have bbcode_text and text, but both use internally the same data structure. Only the called methods are different, while set_bbcode falls back to set_textwhen use_bbcode is not enabled. So I went ahead and removed them in #39148. I added some other points there.

HaSa1002 picture HaSa1002 on 29 May 2020
👍4

GetSceneInstanceLoadPlaceholder() in Node look very wrong, first it doesn't return a placeholder as the name may suggest, but a bool and secondly this leaks details of inherited implementations into the base class with no real requirement (testing for the type of the node is possible in other ways)

Flavelius picture Flavelius on 31 May 2020

RayShape → SeparationRayShape, as initially suggested in godotengine/godot-proposals#711.

Xrayez picture Xrayez on 1 Jun 2020

How about removing sort_custom and making sort take an optional Callable?

KoBeWi picture KoBeWi on 5 Jun 2020
👍6

Should we remove OS.get_ticks_msec() and OS.delay_msec() in favor of OS.get_ticks_usec() and OS.delay_usec() respectively? This would avoid having two ways to achieve the same thing. If you need a value in milliseconds, you can multiply it or divide it by 1000.

Also, both GDScript and C++ allow adding separators in integer literals, which makes large values more readable.

# Since Godot 3.2.
OS.delay_usec(123_456_789)

// Since C++14.
OS::get_singleton()->delay_usec(123'456'789);
Calinou picture Calinou on 9 Jun 2020

Also, both GDScript and C++ allow adding separators in integer literals, which makes large values more readable.

If the removal happens, get_ticks_usec() description should mention the separators.

KoBeWi picture KoBeWi on 9 Jun 2020
👍1

@Calinou On the one hand, you are right, but on the other - in most cases, such great accuracy is not required.

dalexeev picture dalexeev on 9 Jun 2020

'Alpha scissor' in spatial material should become the standard 'alpha clip'

Flavelius picture Flavelius on 16 Jun 2020

@Flavelius I haven't seen the term "alpha clip" used often. I've seen "alpha test" used much more often than both "alpha clip" and "alpha scissor" for sure, though.

Calinou picture Calinou on 16 Jun 2020
👍2

That's a lot of stuff to change! Is someone working on implementing these suggestions?

MCrafterzz picture MCrafterzz on 16 Jun 2020

@MCrafterzz People open pull requests to rename things on a regular basis. This is done incrementally rather than all at once.

Calinou picture Calinou on 16 Jun 2020
👍1

Texture (Texture2D) and Image

  • get_data() in Texture (Texture2D) should be named get_image() and get_data() in Image should be named get_bytes() to be self-describing.
Coldragon picture Coldragon on 30 Jun 2020
👍1

IMO: get_image yes, get_bytes no.

texture.get_image().get_data()

nathanfranke picture nathanfranke on 30 Jun 2020
👍3

Mesh / MeshInstance

  • In Mesh you get/set Materials with those methods :
    surface_get_material/surface_set_material
  • In MeshInstance you get/set with those methods :
    get_surface_material/set_surface_material

It should be the same naming I guess ?

Coldragon picture Coldragon on 30 Jun 2020
👍2

@Coldragon It should be get_surface_material/set_surface_material and a property of surface_material.

aaronfranke picture aaronfranke on 1 Jul 2020

@Coldragon It should be get_surface_material/set_surface_material and a property of surface_material.

It's not "normal" set/get, they have an index for the target Surface (it's a Vector inside Mesh class)

Coldragon picture Coldragon on 1 Jul 2020
👍1

Array we should rename empty() to is_empty() to better illustrate it's boolean

Feniks-Gaming picture Feniks-Gaming on 1 Jul 2020
👍12

String we should drop find_last() in favor of rfind(). Not exactly a rename, but it's still worth discussing which one to keep.

hinlopen picture hinlopen on 3 Jul 2020
😕1

For single numbers, we have stepify. For Vector2/Vector3, we have snapped.

They do the same thing, the vectors call stepify for each component. One name should be chosen, but which one?

Poll: :heart: = both should be stepify, :rocket: = both should be snapped, :-1: = don't rename.

aaronfranke picture aaronfranke on 8 Jul 2020
🚀9 ❤2

AABB has intersection, while Rect2 has clip. They do the same thing. One name should be chosen, but which one?

Poll: :heart: = both should be intersection, :rocket: = both should be clip, :-1: = don't rename.

aaronfranke picture aaronfranke on 8 Jul 2020
❤12 🚀3

@aaronfranke yes, I previously suggested the intersection name in https://github.com/godotengine/godot/issues/16863#issuecomment-449628319. We then have intersects which could be renamed to overlaps in Rect2, but not sure about AABB regarding intersects → overlaps rename.

Xrayez picture Xrayez on 8 Jul 2020

I think find_node and/or get_node should be renamed because the names don't indicate they differences between the 2 at all.
Since find_node only looks at children, maybe find_child_node?
I'm not sure what a good alternate name for get_node would be. My first thought was get_tree_node since it can get a node from anywhere in the tree, but it can also be used outside of the scene tree with relative paths.

MuffinManKen picture MuffinManKen on 24 Jul 2020
👎4

I find get_node good, but find_node could be renamed find_child

Coldragon picture Coldragon on 24 Jul 2020
👀1

@MuffinManKen I think that get_node is perfectly understandable since, as you stated, it can fetch any node, anywhere, so long as it is connected with the same tree as the given node, regardless of whether they are part of the SceneTree or not.

@Coldragon I'm not sure I like renaming find_node to find_child either, as it then gives me the impression that it works only with direct children for some reason. The alternative would be to have it be called find_descendant or something, but that is far too wordy/complex. Cutting it down to just find() also doesn't make sense since it is then unclear what we are searching for. As such, I think that unless another alternative is sought, find_node should stay as it is too. It should just have clear documented differences in behavior for the API docs.

willnationsdev picture willnationsdev on 24 Jul 2020
👍3

In Godot 3.1, I added a standalone feature tag which evaluates to true when the project is running from an export template binary (debug or release). The opposite is editor, which evaluates to true when the project is running from an editor binary.

However, with time, I think it would be better to rename standalone to runner as it's shorter and slightly more self-explanatory. What do you think?

Calinou picture Calinou on 24 Jul 2020
👍1

What about exported or release?

aaronfranke picture aaronfranke on 24 Jul 2020
👍1

@aaronfranke exported implies the project has been exported, which isn't necessarily the case. You can use an export template binary to run a project directly from its source files, as long as assets were imported beforehand.

Also, release is already used for release-mode binaries (in contrast to debug, which is used for debug-mode binaries).

Calinou picture Calinou on 24 Jul 2020

runner doesn't seem very clear to me. standalone is ok. Removing it would be ok too, because you can just use !OS.get_feature("editor").

KoBeWi picture KoBeWi on 24 Jul 2020
👍7

Removing it would be ok too, because you can just use !OS.get_feature("editor").

This can't be used for overriding project settings though, since there's no .not selector there. It's probably feasible by swapping around the default setting and its override, but it feels a bit more convoluted.

Calinou picture Calinou on 24 Jul 2020

Why not app or game as a contrast to editor then? Or would that just be more confusing? Perhaps have a feature flag for no-editor to be more explicit?

willnationsdev picture willnationsdev on 25 Jul 2020

@willnationsdev game implies Godot can only be used for games. Many people are successfully using Godot for non-game applications, so I'd prefer to use a more inclusive term :slightly_smiling_face:

Also, it's not really self-explanatory: people might think it still applies to projects running from the editor (you're running the "game" from the editor after all). The same goes for app.

Calinou picture Calinou on 25 Jul 2020

What about "independent" ?

On Sat., Jul. 25, 2020, 5:49 a.m. Hugo Locurcio, notifications@github.com
wrote:

@willnationsdev https://github.com/willnationsdev game implies Godot
can only be used for games, which has proven not to be the case 🙂

Also, it's not really self-explanatory: people might think it still
applies to projects running from the editor. The same goes for app.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/16863#issuecomment-663835822,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AFMN5DM5U3KLXVUVIC2OGHTR5KTDXANCNFSM4ERRCEZA
.

MuffinManKen picture MuffinManKen on 25 Jul 2020

@MuffinManKen independent doesn't sound very self-explanatory to me either.

Calinou picture Calinou on 25 Jul 2020
👍1

Editor vs standalone is probably standard naming (at least one I see in many different engines) so no reason to reinvent the wheel imho.

Zireael07 picture Zireael07 on 25 Jul 2020
👍5

Get_node is not limited to getting descendents, so that would be a very
misleading name. The 2 methods need very different names because what they
do is very different.

On Sat., Jul. 25, 2020, 3:14 p.m. golddotasksquestions, <
[email protected]> wrote:

I remember the confusion I had for a long time in the beginning with
find_node and get_node. I would really like find_descendant and
get_descendant, as these would be true and informative @willnationsdev
https://github.com/willnationsdev @Coldragon
https://github.com/Coldragon
The "wordyness" might not be everyones cu of tea, but imho is not a really
issue as there is autocomplete and shorthand "$".

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/16863#issuecomment-663890652,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AFMN5DJNBNB6ZOUIV62VX2DR5MVJBANCNFSM4ERRCEZA
.

MuffinManKen picture MuffinManKen on 25 Jul 2020
👍1

I think in both Transform and Transform2D methods inverse and xform_inv should be renamed / removed because what these methods actually do is not what people expect them to do. More details: https://github.com/godotengine/godot/issues/39433#issuecomment-664024521.

kleonc picture kleonc on 26 Jul 2020

RayCast.cast_to should be renamed to RayCast.segment or something. I just had somone say it took him 40 min to realize it's a property and not a function. Probably cause it's a verb too.

razcore-art picture razcore-art on 19 Aug 2020

What about RayCast.target?

nathanfranke picture nathanfranke on 19 Aug 2020
👍4

@razcore-art I recently answered a question related to ray casting, and I used segment word to describe it, so I think makes sense. But this could also be rewritten as direction and length, so it will actually become something closer to a ray rather than a segment, speaking geometrically (or just provide alternative properties which could co-exist). The problem is that there's no way to set a normalized direction vector easily in the inspector. I wrote a proposal to make one, at least for 2D: godotengine/godot-proposals#397, but perhaps that's too far fetched.

EDIT: Upon further thinking, segment wouldn't make sense much in terms of RayCast node, but makes sense in terms of Physics2DDirectSpaceState.intersect_ray().

What about RayCast.target?

@nathanfranke I'd assume target to be a Node or a NodePath. So at least this could be RayCast.target_position. Maybe RayCast.cast_position or RayCast.cast_offset. Don't forget that ray casts can rotate, which can shift the actual cast position in global coordinates.

Xrayez picture Xrayez on 19 Aug 2020
🎉1 👍1

PhysX API confirms my idea of unit direction + length for configuring raycasts. 😛

That said, I'm leaning towards to cast_position... Because rewriting how ray casting works requires more core changes.

Xrayez picture Xrayez on 19 Aug 2020

@Xrayez I would avoid using the word "cast" at the start of a property, since I naturally read that as the verb "cast" and not the noun "cast", and therefore cast_position likely wouldn't have solved the original problem of not knowing this is a property and not a method (it would be easy to assume cast_position is a method that casts to a position). I like target_position.

aaronfranke picture aaronfranke on 19 Aug 2020
👍1

From https://github.com/godotengine/godot/issues/19325#issuecomment-394155128:

Can rename KEY_BACK to KEY_MEDIA_BACK and KEY_FORWARD to KEY_MEDIA_FORWARD. There may be other media keys that can use a MEDIA prefix.

bojidar-bg picture bojidar-bg on 28 Aug 2020

We should consider changing String begins_with() to starts_with().

It is that way in Java, C#, Python, JavaScript, etc.

Bugsquad edit: https://github.com/godotengine/godot/issues/16863#issuecomment-596069130

nathanfranke picture nathanfranke on 31 Aug 2020
👍2 😕1

bool, float, int are the only types/classes whose names start with a lowercase letter. I think they should be renamed (in GDScript) to Bool, Float, Int. This will automatically fix the problem with incorrect type syntax highlighting.

And bool, float, int should be used only for built-in Python-like functions from @GDScript page (they also include len and str).

Note that the situation is similar with str/String: str(x) and String(x) give the same result.

ADD. It should have looked like this:

# `Int` is type.
func get_key() -> Int:
    # `int` is Python-like function.
    return int(config.get_value("sect", "key"))
dalexeev picture dalexeev on 31 Aug 2020
👎9

@dalexeev They are lowercase because they are primitive types. You will see this in most other languages.
Classes like Node are reference types and do not copy on assignment.

var a := Node2D()
var b = a
a.position = Vector2(2, 2)
print(b.position) # (2, 2)

If anything, we should consider renaming String to string since String is not mutable and behaves more similarly to int than say Node.

Redacting this for now, since that would also be the case with Vector2, Vector3, etc.

nathanfranke picture nathanfranke on 31 Aug 2020

@nathanfranke In different languages it is different. For example, in Kotlin, Haxe, Ada names of all types are capitalized.

Moreover, primitiveness is a conditional concept. For me String, Color, Vector2, etc. are primitive types, especially since they are passed by value and not by reference.

The only obstacle is a violation of backward compatibility.

since String is not mutable

Surprisingly, strings in the GDScript are mutable:

var s = "abc"
s[0] = "xyz"
print(s) # xyzbc
dalexeev picture dalexeev on 1 Sep 2020

Vector2 isn't a primitive because it is composed by 2 float. However, Vector2 and float are built-in variant types.

Zylann picture Zylann on 1 Sep 2020

@Zylann Is it such a fundamental difference that it should be reflected in the type name?

(For me, 'primitive' is 'simple', not 'one-piece'.)

dalexeev picture dalexeev on 1 Sep 2020

I do not want to interpret this in my favor, but:

names of primitive types are capitalized. :smile:

dalexeev picture dalexeev on 1 Sep 2020

Here are the terms as I understand them:

| Type name | Primitive type | Value type | Mutable type | Built-in type |
| ------------- | ------------- | ------------- | ------------- | ------------- |
| int | Yes | Yes | N/A | Yes |
| Vector2 | No | Yes | N/A | Yes |
| Node | No | No | Yes | Yes |
| String | No | No | No | Yes |

Regardless, these names aren't going to be changed. They are fine as-is, and familiar to programmers of various languages. We should end the discussion here to avoid filling this tracker with pointless discussion.

aaronfranke picture aaronfranke on 1 Sep 2020
😕1 👍1

The Mutable type column is wrong: only Object-derived, Dictionary, and Array are mutable. (Vector2 might look mutable since you can do vec2.x = 0, but it actually translates to something like vec2 = vec2.with_x(0))

bojidar-bg picture bojidar-bg on 1 Sep 2020
👀2

Rename ShortCut to Shortcut

nathanfranke picture nathanfranke on 2 Sep 2020
👍8 🎉1

The following methods should be changed
Inconsistencies between

Input.is_action_just_pressed(action: String) -> bool
Input.is_action_just_released(action: String) -> bool
Input.is_action_pressed(action: String) -> bool

to

Input.is_action_pressed(action: String, allow_echo: bool = false) -> bool
Input.is_action_released(action: String) -> bool

for consistency with
and

InputEvent.is_action_pressed(action: String, allow_echo: bool = false) -> bool
InputEvent.is_action_released(action: String) -> bool

need to be corrected.

P. S. I proceeded from the principle "The fewer similar methods, the better".

dalexeev picture dalexeev on 6 Sep 2020
👀1

@dalexeev Boolean parameters are often less readable than having different methods, since true and false have no context.

Calinou picture Calinou on 6 Sep 2020
👍3

Yeah, but consistency would be good, too. Get rid of boolean parameters in InputEvent then?

Zireael07 picture Zireael07 on 6 Sep 2020
👍3

@Calinou In most cases, you do not need to check for echo events.

I don't see this as a big problem. When you type the code, the argument hint appears. When reading the code, you can use Shift + Click. The arguments to frequently used functions are quick to remember (for example, String.split).

Moreover, 208 optional boolean arguments were found in the doc/classes directory (this is only the core, and also 16 modules have a nested directory doc_classes).

AcceptDialog.xml

17:         <argument index="1" name="right" type="bool" default="false">

AnimatedSprite2D.xml

26:         <argument index="1" name="backwards" type="bool" default="false">

AnimationNode.xml

53:         <argument index="5" name="optimize" type="bool" default="true">
74:         <argument index="6" name="optimize" type="bool" default="true">

AnimationPlayer.xml

146:            <argument index="3" name="from_end" type="bool" default="false">
201:            <argument index="1" name="update" type="bool" default="false">
223:            <argument index="0" name="reset" type="bool" default="true">

Animation.xml

349:            <argument index="2" name="exact" type="bool" default="false">

Array.xml

130:            <argument index="1" name="before" type="bool" default="true">
146:            <argument index="3" name="before" type="bool" default="true">
172:            <argument index="0" name="deep" type="bool" default="false">
366:            <argument index="3" name="deep" type="bool" default="false">

AStar2D.xml

79:         <argument index="2" name="bidirectional" type="bool" default="true">
114:            <argument index="1" name="include_disabled" type="bool" default="false">
276:            <argument index="1" name="disabled" type="bool" default="true">

AStar.xml

74:         <argument index="2" name="bidirectional" type="bool" default="true">
94:         <argument index="2" name="bidirectional" type="bool" default="true">
113:            <argument index="2" name="bidirectional" type="bool" default="true">
131:            <argument index="1" name="include_disabled" type="bool" default="false">
293:            <argument index="1" name="disabled" type="bool" default="true">

Camera3D.xml

15:         <argument index="0" name="enable_next" type="bool" default="true">

CanvasItem.xml

274:            <argument index="2" name="filled" type="bool" default="true">
376:            <argument index="4" name="transpose" type="bool" default="false">
403:            <argument index="4" name="transpose" type="bool" default="false">
411:            <argument index="8" name="clip_uv" type="bool" default="true">

ClassDB.xml

55:         <argument index="1" name="no_inheritance" type="bool" default="false">
66:         <argument index="1" name="no_inheritance" type="bool" default="false">
88:         <argument index="1" name="no_inheritance" type="bool" default="false">
110:            <argument index="1" name="no_inheritance" type="bool" default="false">
134:            <argument index="2" name="no_inheritance" type="bool" default="false">

CodeHighlighter.xml

19:         <argument index="3" name="p_line_only" type="bool" default="false">

Color.xml

251:            <argument index="0" name="with_alpha" type="bool" default="true">

Control.xml

586:            <argument index="2" name="keep_margin" type="bool" default="false">
588:            <argument index="3" name="push_opposite_anchor" type="bool" default="true">
605:            <argument index="3" name="push_opposite_anchor" type="bool" default="false">
629:            <argument index="1" name="keep_margins" type="bool" default="false">
720:            <argument index="1" name="keep_margins" type="bool" default="false">
758:            <argument index="1" name="keep_margins" type="bool" default="false">
779:            <argument index="1" name="keep_margins" type="bool" default="false">

CryptoKey.xml

26:         <argument index="1" name="public_only" type="bool" default="false">
38:         <argument index="1" name="public_only" type="bool" default="false">
49:         <argument index="1" name="public_only" type="bool" default="false">
59:         <argument index="0" name="public_only" type="bool" default="false">

Curve2D.xml

121:            <argument index="1" name="cubic" type="bool" default="false">

Curve3D.xml

145:            <argument index="1" name="cubic" type="bool" default="false">
158:            <argument index="1" name="apply_tilt" type="bool" default="false">

Dictionary.xml

89:         <argument index="0" name="deep" type="bool" default="false">

Directory.xml

127:            <argument index="0" name="skip_navigational" type="bool" default="false">
129:            <argument index="1" name="skip_hidden" type="bool" default="false">

DisplayServer.xml

647:            <argument index="2" name="multiline" type="bool" default="false">

EditorInterface.xml

212:            <argument index="1" name="with_preview" type="bool" default="true">

EditorNode3DGizmoPlugin.xml

40:         <argument index="3" name="cancel" type="bool" default="false">
60:         <argument index="1" name="billboard" type="bool" default="false">
73:         <argument index="2" name="on_top" type="bool" default="false">
88:         <argument index="2" name="billboard" type="bool" default="false">
90:         <argument index="3" name="on_top" type="bool" default="false">
92:         <argument index="4" name="use_vertex_color" type="bool" default="false">

EditorNode3DGizmo.xml

37:         <argument index="2" name="billboard" type="bool" default="false">
39:         <argument index="3" name="secondary" type="bool" default="false">
53:         <argument index="2" name="billboard" type="bool" default="false">
66:         <argument index="1" name="billboard" type="bool" default="false">
103:            <argument index="2" name="cancel" type="bool" default="false">

EditorProperty.xml

30:         <argument index="3" name="changing" type="bool" default="false">

Expression.xml

36:         <argument index="2" name="show_error" type="bool" default="true">

File.xml

211:            <argument index="0" name="allow_objects" type="bool" default="false">
439:            <argument index="1" name="full_objects" type="bool" default="false">

Font.xml

47:         <argument index="5" name="outline" type="bool" default="false">

GIProbe.xml

19:         <argument index="1" name="create_visual_debug" type="bool" default="false">

HTTPClient.xml

32:         <argument index="2" name="use_ssl" type="bool" default="false">
34:         <argument index="3" name="verify_host" type="bool" default="true">

HTTPRequest.xml

110:            <argument index="2" name="ssl_validate_domain" type="bool" default="true">

ImageTexture.xml

43:         <argument index="1" name="immediate" type="bool" default="false">

Image.xml

226:            <argument index="0" name="renormalize" type="bool" default="false">
415:            <argument index="0" name="square" type="bool" default="false">
433:            <argument index="1" name="grayscale" type="bool" default="false">

ImmediateGeometry3D.xml

24:         <argument index="3" name="add_uv" type="bool" default="true">

InputEvent.xml

54:         <argument index="1" name="allow_echo" type="bool" default="false">

Input.xml

40:         <argument index="1" name="update_existing" type="bool" default="false">

InstancePlaceholder.xml

16:         <argument index="0" name="replace" type="bool" default="false">
33:         <argument index="0" name="with_order" type="bool" default="false">

ItemList.xml

19:         <argument index="1" name="selectable" type="bool" default="true">
32:         <argument index="2" name="selectable" type="bool" default="true">
58:         <argument index="1" name="exact" type="bool" default="false">
235:            <argument index="1" name="single" type="bool" default="true">

JavaScript.xml

18:         <argument index="1" name="use_global_execution_context" type="bool" default="false">

JSONRPC.xml

59:         <argument index="1" name="recurse" type="bool" default="false">

JSON.xml

28:         <argument index="2" name="sort_keys" type="bool" default="false">

KinematicBody2D.xml

78:         <argument index="1" name="infinite_inertia" type="bool" default="true">
80:         <argument index="2" name="exclude_raycast_shapes" type="bool" default="true">
82:         <argument index="3" name="test_only" type="bool" default="false">
96:         <argument index="2" name="stop_on_slope" type="bool" default="false">
102:            <argument index="5" name="infinite_inertia" type="bool" default="true">
125:            <argument index="3" name="stop_on_slope" type="bool" default="false">
131:            <argument index="6" name="infinite_inertia" type="bool" default="true">
145:            <argument index="2" name="infinite_inertia" type="bool" default="true">

KinematicBody3D.xml

80:         <argument index="1" name="infinite_inertia" type="bool" default="true">
82:         <argument index="2" name="exclude_raycast_shapes" type="bool" default="true">
84:         <argument index="3" name="test_only" type="bool" default="false">
98:         <argument index="2" name="stop_on_slope" type="bool" default="false">
104:            <argument index="5" name="infinite_inertia" type="bool" default="true">
127:            <argument index="3" name="stop_on_slope" type="bool" default="false">
133:            <argument index="6" name="infinite_inertia" type="bool" default="true">
158:            <argument index="2" name="infinite_inertia" type="bool" default="true">

Marshalls.xml

35:         <argument index="1" name="allow_objects" type="bool" default="false">
65:         <argument index="1" name="full_objects" type="bool" default="false">

Navigation2D.xml

43:         <argument index="2" name="optimize" type="bool" default="true">

Navigation3D.xml

46:         <argument index="2" name="use_collision" type="bool" default="false">
65:         <argument index="2" name="optimize" type="bool" default="true">

NavigationServer3D.xml

209:            <argument index="3" name="use_collision" type="bool" default="false">

Node2D.xml

63:         <argument index="1" name="scaled" type="bool" default="false">
74:         <argument index="1" name="scaled" type="bool" default="false">

Node.xml

126:            <argument index="1" name="legible_unique_name" type="bool" default="false">
146:            <argument index="1" name="legible_unique_name" type="bool" default="false">
159:            <argument index="1" name="persistent" type="bool" default="false">
189:            <argument index="1" name="recursive" type="bool" default="true">
191:            <argument index="2" name="owned" type="bool" default="true">
540:            <argument index="2" name="parent_first" type="bool" default="false">
599:            <argument index="1" name="keep_data" type="bool" default="false">
737:            <argument index="1" name="recursive" type="bool" default="true">

Object.xml

378:            <argument index="1" name="reversed" type="bool" default="false">

OS.xml

73:         <argument index="2" name="blocking" type="bool" default="true">
77:         <argument index="4" name="read_stderr" type="bool" default="false">
141:            <argument index="0" name="utc" type="bool" default="false">
150:            <argument index="0" name="utc" type="bool" default="false">
303:            <argument index="0" name="utc" type="bool" default="false">
451:            <argument index="0" name="short" type="bool" default="false">

PacketPeerDTLS.xml

17:         <argument index="1" name="validate_certs" type="bool" default="true">

PacketPeer.xml

36:         <argument index="0" name="allow_objects" type="bool" default="false">
57:         <argument index="1" name="full_objects" type="bool" default="false">

PCKPacker.xml

33:         <argument index="0" name="verbose" type="bool" default="false">

PhysicsDirectSpaceState2D.xml

62:         <argument index="4" name="collide_with_bodies" type="bool" default="true">
64:         <argument index="5" name="collide_with_areas" type="bool" default="false">
89:         <argument index="5" name="collide_with_bodies" type="bool" default="true">
91:         <argument index="6" name="collide_with_areas" type="bool" default="false">
107:            <argument index="4" name="collide_with_bodies" type="bool" default="true">
109:            <argument index="5" name="collide_with_areas" type="bool" default="false">

PhysicsDirectSpaceState3D.xml

63:         <argument index="4" name="collide_with_bodies" type="bool" default="true">
65:         <argument index="5" name="collide_with_areas" type="bool" default="false">

PhysicsServer2D.xml

21:         <argument index="3" name="disabled" type="bool" default="false">
351:            <argument index="3" name="disabled" type="bool" default="false">

PhysicsServer3D.xml

21:         <argument index="3" name="disabled" type="bool" default="false">
351:            <argument index="3" name="disabled" type="bool" default="false">
426:            <argument index="1" name="init_sleeping" type="bool" default="false">

PopupMenu.xml

36:         <argument index="2" name="global" type="bool" default="false">
70:         <argument index="3" name="global" type="bool" default="false">
118:            <argument index="3" name="global" type="bool" default="false">
133:            <argument index="3" name="global" type="bool" default="false">
195:            <argument index="2" name="global" type="bool" default="false">
219:            <argument index="2" name="global" type="bool" default="false">
526:            <argument index="2" name="global" type="bool" default="false">

ProjectSettings.xml

93:         <argument index="1" name="replace_files" type="bool" default="true">

Rect2.xml

146:            <argument index="1" name="include_borders" type="bool" default="false">

RenderingDevice.xml

29:         <argument index="4" name="sync_with_draw" type="bool" default="true">
413:            <argument index="3" name="arg3" type="bool" default="false">
493:            <argument index="1" name="allow_cache" type="bool" default="true">
565:            <argument index="6" name="sync_with_draw" type="bool" default="false">
591:            <argument index="9" name="sync_with_draw" type="bool" default="false">
677:            <argument index="2" name="sync_with_draw" type="bool" default="false">
691:            <argument index="3" name="sync_with_draw" type="bool" default="false">

RenderingServer.xml

847:            <argument index="0" name="swap_buffers" type="bool" default="true">
1812:           <argument index="3" name="color_format" type="bool" default="false">
1814:           <argument index="4" name="custom_data_format" type="bool" default="false">
2455:           <argument index="3" name="use_filter" type="bool" default="true">
2557:           <argument index="2" name="is_2d_skeleton" type="bool" default="false">

ResourceLoader.xml

61:         <argument index="2" name="no_cache" type="bool" default="false">
100:            <argument index="2" name="use_sub_threads" type="bool" default="false">

Resource.xml

24:         <argument index="0" name="subresources" type="bool" default="false">

RigidBody2D.xml

106:            <argument index="1" name="infinite_inertia" type="bool" default="true">

SceneState.xml

142:            <argument index="1" name="for_parent" type="bool" default="false">

SceneTree.xml

65:         <argument index="1" name="pause_mode_process" type="bool" default="true">

ScriptCreateDialog.xml

25:         <argument index="2" name="built_in_enabled" type="bool" default="true">
27:         <argument index="3" name="load_enabled" type="bool" default="true">

Script.xml

107:            <argument index="0" name="keep_state" type="bool" default="false">

Skeleton3D.xml

238:            <argument index="3" name="persistent" type="bool" default="false">

SkeletonIK3D.xml

25:         <argument index="0" name="one_time" type="bool" default="false">

StreamPeerSSL.xml

33:         <argument index="1" name="validate_certs" type="bool" default="false">

StreamPeer.xml

128:            <argument index="0" name="allow_objects" type="bool" default="false">
274:            <argument index="1" name="full_objects" type="bool" default="false">

String.xml

587:            <argument index="0" name="with_prefix" type="bool" default="false">
855:            <argument index="1" name="allow_empty" type="bool" default="true">
924:            <argument index="1" name="allow_empty" type="bool" default="true">
947:            <argument index="1" name="allow_empty" type="bool" default="true">
957:            <argument index="0" name="left" type="bool" default="true">
959:            <argument index="1" name="right" type="bool" default="true">

SurfaceTool.xml

216:            <argument index="0" name="flip" type="bool" default="false">

TextEdit.xml

61:         <argument index="1" name="adjust_viewport" type="bool" default="true">
73:         <argument index="1" name="adjust_viewport" type="bool" default="true">
75:         <argument index="2" name="can_be_hidden" type="bool" default="true">

Texture2D.xml

23:         <argument index="3" name="transpose" type="bool" default="false">
50:         <argument index="4" name="transpose" type="bool" default="false">
77:         <argument index="4" name="transpose" type="bool" default="false">
89:         <argument index="10" name="clip_uv" type="bool" default="true">

TileMap.xml

137:            <argument index="1" name="ignore_half_ofs" type="bool" default="false">
153:            <argument index="3" name="flip_x" type="bool" default="false">
155:            <argument index="4" name="flip_y" type="bool" default="false">
157:            <argument index="5" name="transpose" type="bool" default="false">
183:            <argument index="2" name="flip_x" type="bool" default="false">
185:            <argument index="3" name="flip_y" type="bool" default="false">
187:            <argument index="4" name="transpose" type="bool" default="false">

TileSet.xml

323:            <argument index="3" name="one_way" type="bool" default="false">

TreeItem.xml

22:         <argument index="3" name="disabled" type="bool" default="false">
205:            <argument index="0" name="wrap" type="bool" default="false">
229:            <argument index="0" name="wrap" type="bool" default="false">
439:            <argument index="2" name="just_outline" type="bool" default="false">
567:            <argument index="4" name="expr" type="bool" default="false">

UndoRedo.xml

103:            <argument index="0" name="increase_version" type="bool" default="true">

Viewport.xml

117:            <argument index="1" name="in_local_coords" type="bool" default="false">
165:            <argument index="1" name="in_local_coords" type="bool" default="false">

ADD. If/when it becomes possible to specify the name of the argument, then this will definitely cease to be a problem:

if e.is_action_pressed("ui_left", allow_echo = true):
    velocity += Vector2.LEFT

var arr = s.split(",", allow_empty = false)
dalexeev picture dalexeev on 6 Sep 2020

@dalexeev can you consider putting that in a spoiler to make scrolling easier for us?

nathanfranke picture nathanfranke on 7 Sep 2020

@dalexeev There are many cases where you need to check if a key is pressed and not only if it was just pressed. For example, a script for moving a character needs to do this with WASD or the arrow keys. Pretty much every game is going to need to process input, so I don't think it's wasteful to just have two methods for these things.

When reading the code, you can use Shift + Click.

Not if you are viewing diffs on GitHub. If the code requires an IDE to be readable, it is not good code.

aaronfranke picture aaronfranke on 7 Sep 2020
👍2

@aaronfranke For the other 207 cases, do you also suggest creating separate functions? If not, then this argument is not conclusive. In addition, I wrote above that if you can specify the parameter name, then it becomes readable without an IDE.

There are many cases where you need to check if a key is pressed and not only if it was just pressed.

Often, but not more often than without echo.

The presence of three methods (is_action_just_pressed, is_action_just_released, is_action_pressed) is confusing because you expect there to be an is_action_released method.

ADD. Which option is easier, at least visually?

dalexeev picture dalexeev on 7 Sep 2020
👀1

is_action_released can be achieved with not is_action_pressed. This not true for the just methods.

aaronfranke picture aaronfranke on 7 Sep 2020

As mentioned above, raw bools should be avoided. A flag like INPUT_ALLOW_ECHO/INPUT_NO_ECHO would be much better than a bool.

MuffinManKen picture MuffinManKen on 7 Sep 2020
😕2

@dalexeev That would only introduce confusion. is_action_pressed() and echo are 2 different things. You can check yourself that echo events are received after slight delay after first press, while is_action_pressed() has no delay.

KoBeWi picture KoBeWi on 7 Sep 2020

@KoBeWi You may be right and

is_action_pressed(action: String, allow_echo: bool = false) -> bool

should be changed to

is_action_pressed(action: String, allow_echo: bool = true) -> bool

since this is not consistent with

func _input(e):
    if !e.is_action("ui_left"):
        return
    $Label.text += "pressed: %s, echo: %s\n" % [e.is_pressed(), e.is_echo()]

pressed: True, echo: False
pressed: True, echo: True
pressed: True, echo: True
pressed: True, echo: True
pressed: False, echo: False
dalexeev picture dalexeev on 7 Sep 2020

@dalexeev What you propose is not correct. When we talk about echo events, we talk about repeated keyboards events while pressing a key, using the term here makes little sense, since the action system does not relies of event directly, it's state being updated in a per-frame fashion. Also, actions can also work with other devices like controllers or mouse buttons, where "echo" event do not exist.

groud picture groud on 7 Sep 2020
👍2

TreeItem's get_children() only returns the first child and not all children like the name (or the description in the docs) suggest.

[Edit]
Nvm the docs. The method description is updated in master, sorry.

Leleat picture Leleat on 11 Sep 2020
👍3

I recommend Resource's local_to_scene should be local_to_instance or unique_per_instance. "Local to Scene" denotes the behavior as local to the scene, when it's actually per instance of a scene.

markdibarry picture markdibarry on 21 Sep 2020
👍4 👀3 🚀1

Rename Image.load() → Image.load_from_file().

  1. Helps to alleviate possible confusion with load("image.png") files via code, see documentation changes at #42412.
  2. Image.load() won't be highlighted as a GDScript built-in name anymore: #28866.
  3. Consistent with Image.load_*_from_buffer() per image format. Buffer-related methods can be potentially unified into the same interface to prevent API bloat, but that's another topic for discussion.
Xrayez picture Xrayez on 30 Sep 2020
👍2

@Xrayez Might also be worth it to remove load in GDScript: https://github.com/godotengine/godot-proposals/issues/263

aaronfranke picture aaronfranke on 30 Sep 2020

The variable registering_order and related method set_registering_order in ProjectSettings is unused.

EricEzaM picture EricEzaM on 6 Oct 2020

RandomNumberGenerator should be renamed to Random and global random functions such as randf and rand_range should be deprecated or removed.

I see possible issues where one calls an untrusted function while the random seed is specified

seed(123)
randf()
call_method() # This could change the seed for its own random reasons!
randf()
nathanfranke picture nathanfranke on 9 Oct 2020

global random functions such as randf and rand_range should be deprecated or removed.

Discussed as part of godotengine/godot-proposals#1590.

I'd prefer these basic random methods to be at global scope for accessibility and prototyping purposes, at least some of them. But seed() and rand_seed() can be surely removed.

Xrayez picture Xrayez on 9 Oct 2020
👍2

It looks like FuncRef has become redundant since Callable was added.

dalexeev picture dalexeev on 11 Oct 2020

I discovered methods property_can_revert and property_get_revert in the Inspector and reported them in #43078. However, I think they should be renamed with leading underscores to follow the conventions of _get, _set, and _get_property_list.

willnationsdev picture willnationsdev on 25 Oct 2020
👍1

EditorImportPlugin and EditorExportPlugin look like they are related, however one is about importing resources and the other is about exporting a project. I suggest we rename EditorImportPlugin to EditorResourceImportPlugin or something along those lines.

Edit: And EditorPlugin.add_import_plugin must be renamed accordingly as well.

pycbouh picture pycbouh on 25 Oct 2020
👍4

Why not ResourceImportPlugin? Many (most?) editor classes don't contain the word "Editor" already, like SceneTreeDock or all of the animation stuff.

aaronfranke picture aaronfranke on 25 Oct 2020
👍3

The Tracking_status enum in ARVRInterface should be renamed to TrackingStatus, to be consistent with the styles of other enum names.

LikeLakers2 picture LikeLakers2 on 25 Oct 2020
👍3 👀1

Looking at ARVRInterface above, the quality of names is quite poor in general. Here are my suggested renames as well:

  • ar_is_anchor_detection_enabled → anchor_detection_enabled (property)
  • interface_is_initialized → initialized (property, could be further rewritten to enabled, as it has a setter)
  • interface_is_primary → primary (property)
  • get_render_targetsize() → get_render_target_size() (method)
  • uninitialize() → finalize() (method)

Otherwise the documentation is wrong. 🙂

Note that I haven't used this class at all, but those names look weird just by looking at the class.

Xrayez picture Xrayez on 25 Oct 2020

Should we rename Control.MOUSE_FILTER_PASS to Control.MOUSE_FILTER_PASSTHROUGH? This would make it more obvious that the event will be passed through the Control node to nodes located below it. I'm not sure if it's actually worth renaming it, though.

Calinou picture Calinou on 26 Oct 2020
👍6 👎1

@Calinou I don't imagine it would hurt, so I'd be in support. Like you mentioned, that change would make it a bit more obvious at a glance what that mouse filter mode is doing.

LikeLakers2 picture LikeLakers2 on 26 Oct 2020

@Calinou I find the current behaviour unusual. This scene setup yields "Click Child2, Click Scene"
image

(Note, all are set to Pass)

:smile: Proposition A: Perhaps something like Control.MOUSE_FILTER_PASSPARENTS for the current behaviour, since input events only seem to be sent to the parents.

:rocket: Proposition B: Change the constants to these:

  • STOP: Current Behaviour - Take event and stop all propagation
  • PASS_PARENT: Same behaviour as PASS currently
  • PASS_ALL: IGNORE, but takes events
  • IGNORE: Current Behaviour - Do not take event, but still propagate

:eyes: Proposition C: Whether or not the node takes gui inputs isn't really relevant, since one can just not connect any signals (or use a boolean flag). We can change the option to Control.propagation_mode and have these constants:

  • NONE
  • PARENT
  • ALL

That would look a lot cleaner in my opinion.

nathanfranke picture nathanfranke on 27 Oct 2020
👀2 🚀1

That is beyond renaming and should be discussed as a proposal.

pycbouh picture pycbouh on 27 Oct 2020
👍2

Why all these renames are so long? You're changing something simple and short to something really long.

You're changing clip to intersection twice as long to type.
You're also changing Control.MOUSE_FILTER_PASS to Control.MOUSE_FILTER_PASSTHROUGH
etc

I would prefer simpler and shorter less verbose changes. It's a function name not also a description of the function.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020
👎1

I would prefer simpler and shorter less verbose changes. It's a function name not also a description of the function.

The name should be descriptive though. Also length doesn't matter if you take advantage of the autocompletion (which is built-in in the Godot editor).

I mentioned it once on IRC and didn't get a reply, but TextureRect has a stretch mode called "Scale On Expand (Compat)". This sounds like something that could be removed.

KoBeWi picture KoBeWi on 5 Nov 2020
👍4 👎1

"Less verbose" is definitely not on the menu if we want to have robust codebases across our Godot projects. Modern coding tools provide autocompletion and other intelligent features that allow you to type quickly even if the name is long. Plus, if you read the argumentations for those changes, there is a goal to make those functions less ambiguous to developers using them. Short name might be sweet, but confusing and less discoverable.

And always remember: typing code is the quick part of the software development. Reading and understanding code afterwards is much more important. It's like conceiving and raising a child respectively.

pycbouh picture pycbouh on 5 Nov 2020
👍6 👎1

I disagree with both of you. I am a Godot user and I use Godot specifically because GDScript is terse and quick to write. If you gonna make them twice as long then the speed advantage is lost as I will be forced two write twice as much and would have to scroll twice as far to see the entire line of code horizontally.

When you are coding you don't wanna be typing incredibly long function or variable names. Some of these proposed changes changes add extremely long function and variable names for the sake "clarity" at the expense of everything else. You can read the built in help if you have any doubts. Plus programming is about learning an API. You will always look up a function first time you use it regardless of the name. Take printf in C is terse and simple. In Godot you would name it send_formatted_string_to_standard_out. Not only you're forcing everyone to re-learn the api but some of these changes don't even have a unified vision. Seems to me like a whole bunch of people got together and each changed one parts of the engine.

Take Array for example
remove -> remove_at What does _at add here? You already have remove_value. What else can you remove?
erase -> remove_value Still not clear enough. From documents "Removes the first occurrence of a value from the array." Also people might think you can remove one single value from the entire array. For clarity it should actually be remove_first_occurance_of_value because that's what the function actually does. So you made the function longer and equally confusing just longer.

You have remove and erase two different functions but you turn them both in the same variation on remove with extra letters. But they function work differently. Remove removes from a specific index where as erase removes the first instance of a value found.

These are okay just not really useful other than forcing people to update their code.
invert -> reverse
duplicate -> clone
empty -> is_empty

If is not broken don't fix it.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020
👎5

@CarlGustavAlbertDwarfsteinYung but you are not going to type twice as much. After first 3 letters you type autocopletion is going to kick in and you select what you need rather than type whole words.

As for other names for example when you take a look at empty -> is_empty change is needed to provide clarification of what it does. When you look at empty can you tell if this is a method that empties something, is it a book checking if something is empty, is it something else? With is_empty you can see at glance that this is a bool that is true if something is empty and false when it's not. You should know what function or variables do just by reading their name. You can't do that with name like empty

Feniks-Gaming picture Feniks-Gaming on 5 Nov 2020

@Feniks-Gaming I can tell you empty or is_empty are equally confusing just one is longer than the other.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020
👎3

@CarlGustavAlbertDwarfsteinYung empty can be an action if it's read as a verb. is_empty is definitely a quality. Of course that confusion may depend on your level of English.

Also, even if a function was called send_formatted_string_to_standard_out in a modern code editor it can be typed as sfstso, or any other combination of the intermediate letters, if you so desire and the autocomplete will give you the only option.

pycbouh picture pycbouh on 5 Nov 2020
😕1 👍1

@pycbouh People have been using this engine for how many years now? And I haven't heard one person come to me saying you know what is the biggest issue with this engine . The fact that arrays have empty instead of is_empty.

You guys are sitting here fixing things that nobody wanted or asked for. Yes the wording is confusing but is not really an issue and has never been an issue since I started to use this engine in 2015. Some of the proposed changes are quite welcome and to be fair I do use is_ . Is empty would be okay. But some changes are way too long and equally confusing. I was specifically talking about those changes not all.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020

The whole existence of this megathread is the evidence that people are asking for it. These problems may be insignificant for you or someone else, but people do have problems with understanding the API because of bad naming. And this is the type of problems this issue collects. As for the significance of these changes overall, believe it or not, but tracking and implementing these is not taking away from the other development time.

And look at the example you gave and tried to explain:

Remove removes from a specific index where as erase removes the first instance of a value found.

You write that and see no reason to improve naming to be at least a bit more clear for all current and future Godot users?

pycbouh picture pycbouh on 5 Nov 2020
👍1

@pycbouh And in fact I even gave the example of array remove_at and remove_value. remove_value is not same as the description of erase and is still equally confusing. Remove value from where? Remove value from end, rmeove from beginning? Remove all occurrences of a value from array?

if you really need to change things use remove and remove_first_occurence which makes it both terse and descriptive.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020

@pycbouh I am not asking for it. The existence of this thread is because some people are over engineering and fixing things that are not broken classical programmer fashion. What about future users? What about them? I was a future user once and I learned the api. I had no issue with function naming. 50% of comments are people disagreeing with proposed changes. It seems like most of these changes are driven by a small number of community members pushing these changes on everyone. Can we vote on the proposed changes?

In fact here is a proposal. Any changes to the API naming should include contributors/donators/users opinions. If they all agree then I'm also on board. Have polls and see what everyone things. Don't try to guess what other people want. What's good for you might not be good for someone else.

I am against about 50% of the changes proposed here from what I have seen.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020

50% of comments are people disagreeing with proposed changes.

Can we leave the made up statistics at the door, please?

Can we vote on the proposed changes?

That's what the discussion and the reactions are for.

I am against about 50% of the changes proposed here from what I have seen.

If you are only against them because "I've learned it this way, so I want everyone after me to suffer the same", then it's an invalid critique of the proposition and is going to be ignored.

What's good for you might not be good for someone else.

Ditto.

pycbouh picture pycbouh on 5 Nov 2020
😕1

Can we leave the made up statistics at the door, please?

Like this claim about the entire community?

These problems may be insignificant for you or someone else, but people do have problems with understanding the API because of bad naming.

How do you know what people have problems with? Did you ask them? Was there a poll or study or any questionnaire about this? How did you come by this information?

I am one such user started from zero and learned the API. Had no issues with naming. All APIs have some strange naming conventions. All functions need to be somewhat terse so you can write them in code.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020

@pycbouh If you're trying to convince me that all the naming suggestions here are good I will have to respectfully disagree with you. This is a thread that discusses changes and I am here to say that some of the proposes changes are not only not necessary/or asked for but just longer and equally confusing. Some are indeed good and are welcome . Let's not all mass rename everything just because a few people such as yourself think the entire community has a problem with API names. I don't and never had and I started as a beginner. And I know handful of other people that don't either. I believe some of these changes are significant and should be peer reviewed by the entire community or at least by contributors previous to a full release.

CarlGustavAlbertDwarfsteinYung picture CarlGustavAlbertDwarfsteinYung on 5 Nov 2020

How do you know what people have problems with? Did you ask them?

Most of the entries in this thread are spawned by people coming with problems, be it here on GitHub (and in those cases issues are linked) or using other community or personal channels. Don't assume we are just sitting here licking our own private parts because we have nothing better to do but to ponder what other function or property to rename in the engine.

Besides, a lot of the proposed changes here haven't even been acted upon, there were no PRs or any other activity. They are listed and that's it. Keep an eye of the PRs and if one offending you appears don't hesitate to comment on it. After that it's up to the PM of Godot to approve and merge the changes. Be constructive and you may prevent some unwanted modifications, but don't forget what you yourself once said:

What's good for you might not be good for someone else.

So even if you had no issues with the API up until this point, it doesn't mean that others hadn't neither or that they wouldn't have any issues in future.

All APIs have some strange naming conventions.

That's good if there is a convention to speak of. But some of the APIs in Godot were named long before it went open source and may not be as thought through as one would hope. Once again I ask you not to assume that people here suggest changes for the heck of it.

pycbouh picture pycbouh on 5 Nov 2020

Please don't have these kinds of discussions here. If you want to discuss specific API changes, that's fine, but blanket statements that you don't like the suggestions here aren't helpful.

The core devs will review each of these suggestions one by one. It is likely that many will end up not being accepted.

Temporarily locking, will unlock later.

aaronfranke picture aaronfranke on 5 Nov 2020

Could we have the following points added to the list?

  • AnimationPlayer: Rename stop() to pause() (https://github.com/godotengine/godot/issues/16863#issuecomment-562363660, godotengine/godot-proposals#287)
  • AnimatedSprite: Rename stop() to pause() (#31168) (already added)
  • AnimatedSprite3D: Rename stop() to pause() (#31168) (already added)
  • Tween: Rename stop() to pause(), stop_all() to pause_all() (PR #41794, #43442)
opl- picture opl- on 10 Nov 2020

Tween: Rename stop() to pause(), stop_all() to pause_all() (#43442)

This is covered by #41794

KoBeWi picture KoBeWi on 10 Nov 2020
👍1

@opl- this is also discussed here: https://github.com/godotengine/godot-proposals/issues/287

golddotasksquestions picture golddotasksquestions on 14 Nov 2020
👍1

A couple of renames that might make it clearer what these global scope RNG functions do:

  • seed() -> set_seed() (maybe add get_seed() too to match RandomNumberGenerator) – Currently, it's not clear that this is a setter function.
  • rand_seed() -> rand_from_seed() – currently, it sounds like it might randomize the seed or something, when it actually gives a random number based on the provided seed.
name-here picture name-here on 15 Nov 2020

rand_seed() -> rand_from_seed() – currently, it sounds like it might randomize the seed or something, when it actually gives a random number based on the provided seed.

Except it does randomize the seed. Read the documentation.

KoBeWi picture KoBeWi on 15 Nov 2020

rand_seed() -> rand_from_seed() – currently, it sounds like it might randomize the seed or something, when it actually gives a random number based on the provided seed.

Except it does randomize the seed. Read the documentation.

Sorry. What I meant is that it sounds like it only randomizes the seed. Maybe it should be rand_int_from_seed(), since it returns an int? Actually, the docs don't specify what type it returns, mentioning only a "Random…number". Is it an int?

name-here picture name-here on 15 Nov 2020

So it generates a new seed based on a seed you pass it and then generates a new number based on that new seed? Not sure about a rename, but the description can use some rework there, if that's what is happening.

pycbouh picture pycbouh on 15 Nov 2020

If that what it does it sounds like this method should be split into 2 smaller methods tharmt do one thing rather than renamed

Feniks-Gaming picture Feniks-Gaming on 15 Nov 2020

Control::pending_resize

Unused.

EricEzaM picture EricEzaM on 15 Nov 2020

Object::is_class(name) → Object::inherits_class(name).

I understand now that this method is mostly equivalent to GDScript is (which was extends btw), but C++ requires more explicitness.

The confusion I ran into is checking whether particular object is of existing type (without inheritance):

// Use case: we're only interested in editing "Node2D" classes directly.
node = Object::cast_to<Node2D>(p_edited);
if (!node) {
    return;
}
if (node->get_class() != "Node2D") {
    return; // OK, any class other than "Node2D" is not edited.
}
// vs.
if (!node->is_class("Node2D")) {
    return; // ERROR: derived classes like "Sprite" will also be edited...
}

The underlying implementation uses "inherits" macros/templates all over the place, so it makes sense to me to rename this to inherits_class.

See also https://github.com/godotengine/godot/issues/21461#issuecomment-416155187:

get_class() and is_class() are confusing to me in general

Xrayez picture Xrayez on 18 Nov 2020
👍2
Was this page helpful?
0 / 5 - 0 ratings

Related issues

EdwardAngeles picture EdwardAngeles  Â·  3Comments
blurymind picture blurymind  Â·  3Comments
gonzo191 picture gonzo191  Â·  3Comments
bojidar-bg picture bojidar-bg  Â·  3Comments
timoschwarzer picture timoschwarzer  Â·  3Comments