Godot: Refactoring: The great API review

[vnen]

Since for 3.0 a big compatibility breakage is expected, it will be a perfect time to review the API to rename confusing stuff, create/remove singletons, and also standardizing names across the different classes.

Confusing names

• set_hidden() from CanvasItem and the corresponding property, which is visibility/visible (#5531).
• is_visible()/is_hidden(), which sound the same but do different things.
• pressed/released signals from BaseButton, which does not really do what it sounds like. They should be renamed to button_{down,up} (#4625, #5262).
• CheckBox inherits Button, which means it has is_pressed() method instead of a more natural is_checked().
• Input.get_mouse_speed() returns the speed of the _last non-null_ mouse motion, not the current one. It could be renamed to Input.get_last_mouse_speed() or similar.
• Tween.set_speed() is confusing as it returns a scale, could be renamed to set_speed_scale() (#2060) Same with the property (#6935).

Singletons/Classes to split

• OS, which has generic functions.

  • Move to Engine singleton:
  • get_main_loop()
  • is_debug_build()
  • dump_{memory,resources}_to_file()
  • {get,set}_iterations_per_second()
  • {get,set}_target_fps()
  • {get,set}_time_scale()
  • get_engine_version()
    

    Functions to move

• Dictionary.parse_json() -> String.parse_json(), since JSON can be dictionaries, arrays, or single values (#382, #3011, #4232).

  Functions to change

• DirAccess::list_dir_begin should return Error like other functions of this kind.

  Functions to remove

• OS.set_video_mode() (https://github.com/godotengine/godot/issues/3778#issuecomment-233447010).

• OS.get_fullscreen_mode_list() (https://github.com/godotengine/godot/pull/4624#issuecomment-218515473).

  Properties to remove

• Expand from TextureFrame. The Strech Mode property is much more flexible and can do some things that already override the Expand settings.

[reduz]

I actually like how show() / hide() / and set_hidden() works. I know it's
confusing, but it needs to be this way because is_visible() is not the same
as is_hidden().
You will naturally go and try to find a set_visible() function, and when
you don't find it you learn how it actually works.

An Engine singleton does indeed make sense.

I am collecting stuff that needs to change in this document:

https://docs.google.com/spreadsheets/d/1SqLGKpF5B5KzYnY2JzuuP71tsR8WeXZn1imMvHkoKDc/edit?usp=sharing

On Thu, Jul 14, 2016 at 11:58 AM, George Marques [email protected]
wrote:

Since for 3.0 a big compatibility breakage is expected, it will be a
perfect time to review the API to rename confusing stuff, create/remove
singletons, and also standardizing names across the different classes.
Confusing names

• set_hidden() from CanvasItem and the corresponding property, which
  is visibility/visible (#5531
  https://github.com/godotengine/godot/issues/5531).
• show()/hide() also from CanvasItem which does the same thing as
  set_hidden().
• pressed/released signals from BaseButton, which does not really do
  what it sounds like. They should be renamed to button_{down,up} (#4625
  https://github.com/godotengine/godot/issues/4625, #5262
  https://github.com/godotengine/godot/pull/5262).

Sigletons/Classes to split

• OS, which has generic functions.
  
  
  
  • Move to Engine singleton:
  
  
  • get_main_loop()
  
  
  • is_debug_build()
  
  
  • dump_{memory,resources}_to_file()
  
  
  • {get,set}_iterations_per_second()
  
  
  • {get,set}_target_fps()
  
  
  • {get,set}_time_scale()
  
  
  • get_engine_version() (not yet on OS, but proposed by @akien-mga
    
    https://github.com/akien-mga)
  
  

Functions to move

• Dictionary.parse_json() -> String.parse_json(), since JSON can be
  dictionaries, arrays, or single values (#382
  https://github.com/godotengine/godot/pull/382, #3011
  https://github.com/godotengine/godot/issues/3011, #4232
  https://github.com/godotengine/godot/issues/4232).

—
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/5696, or mute the thread
https://github.com/notifications/unsubscribe/AF-Z2z31QAEIb70bh26SEeaUDE5E2asHks5qVk6mgaJpZM4JMhcW
.

[akien-mga]

I agree about set_hiddenbeing better than set_visible for the reasons already discussed in other issues. What was proposed however to make the API clearer is to change the parameter to visibility/hidden, which clears the confusion (and is more accurate).

[vnen]

set_hidden() is OK for me too (with the change of the corresponding property). Also, show()/hide() can stay too. The biggest problem with is_visible()/is_hidden(). Those look like they do the same thing. The latter may be changed by is_hidden_enabled() to make more clear that it's a toggle property.

[WalasPrime]

SceneTree.call_group() (and notify_group()) requires flags as the first argument, which pretty much just results in many zeros in the code, I'd suggest an alternative call_group_flags() or something to mitigate this, and remove the flags argument from call_group().

[Zylann]

At first it didn't agreed with set_hidden(), then I understdood that is_visible could be confused with the fact the object is actually in view. But actually, I would have solved this by naming the other functions is_in_sight, is_in_view or is_culled (unless the reason is different?).
EDIT: ok I see it has to do with parents being visible or not. In that case, you could also have is_visible_self and is_visible, or is_visible_in_hierarchy, as Unity3D did with enabled/disabled GameObjects. But it's a proposal, not a personal preference :)
I don't like is_hidden_enabled, because it has enabled in something supposed to remove something from view, which is kinda the reverse way of common thinking IMO.

Between 2D and 3D, function names doing same things sometimes differ, for example set_pos in 2D, is set_translation in 3D, although they do the same thing with just one additional coordinate (https://github.com/godotengine/godot/issues/4311)

Some constants have redundant naming, for example in VisibilityEnabler2D, all identifiers have the ENABLER_ prefix, but it seem useless since they already are scoped in the class.
On the opposite side, there are a lot of consts that are outside of any class, and it can be confusing. For example, the first time I looked for button codes I was trying to get them from Input, or InputEvent, to no avail. So I eventually used the raw numbers, to finally find they are in global scope like KEY_A or BUTTON_LEFT.

I agree for call_group() to not have the flags as first parameter, because I never use it anyways. But I also see it should be first because what follows are the arguments of the call. So I agree there should be another version of the function having the flags as first arg.

OS.get_ticks_msec() => should be named get_time_msec(), the name makes it hard to find it (it was for me) because no one would search for tick to get the current ms-precise time.

About String.parse_json() and encode_json(): the String type already has lots of various things in it, could we instead have this in a dedicated JSON class, as in Javascript? (there is also an XMLParser class which is neither in String nor in Dictionary, which is great, but doesn't seems to be made only for reading).

Why File.get/store_***(content) instead of File.read/write_***(content) as any other I/O API?

That's all I recall at the moment :)

[vnen]

@Zylann thanks for the lots of insights. About JSON class I'm not so sure, as it would have parse(), encode(), maybe escape() and what else? Seems to much of a push for a class without much functionality. I prefer to add this to an existing class, and String seems the best fit, since JSON is just a string.

[Zylann]

@vnen I don't think the number of functions in a class really matters (unless it's only one of course, would be weird), it's just a matter of having parser consistently have their class, and avoid to bloat the String type. Also, there is a JSON class already in core, it should just be a matter of making it "official", instead of having that in unrelated objects.

[vnen]

My problem with making a JSON class is that it needs to be a singleton (or have static methods, if that's possible). Otherwise you'd have to do what you already have to for some functions like File.get_md5(): create a dummy object just to call the functions on it.

And it's pretty much one function to add in String: parse_json(). Dictionary already has to_json() and Array can have one too. Unless you want to unbloat String and move some of its functions to another class.

[Zylann]

I'm talking about static methods, it doesn't makes sense to call this on a JSON object.

[neikeq]

I think the methods API always needs an instance. That's why classes like Input and OS are singletons.

[Zylann]

Oh, right. Well in that case JSON could be a singleton, but it doesn't changes much how it would be called in GDScript.

[bojidar-bg]

Vector2::get_aspect should probably become Vector2::aspect, since none of the other methods in the class have the get_ prefix.

[reduz]

For reference this is the list I'm keeping:
https://docs.google.com/spreadsheets/d/1SqLGKpF5B5KzYnY2JzuuP71tsR8WeXZn1imMvHkoKDc/edit?usp=sharing

On Sat, Jul 30, 2016 at 10:04 AM, Bojidar Marinov [email protected]
wrote:

Vector2::get_aspect should probably become Vector2::aspect, since none of
the other methods in the class have the get_ prefix.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-236364163,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z207623uOEbGoSTywt57-lKsLGRiUks5qa0vlgaJpZM4JMhcW
.

[Marqin]

http://docs.godotengine.org/en/latest/classes/class_packetpeerudp.html?highlight=udp

set_send_address should be set_destination_address

When I first saw this function I thought I will be able to set packet origin IP/Port (so I can later listen on that port - I'm trying to do UDP hole punching). Sadly it's something different.

[bojidar-bg]

Many nodes override inherited methods, which makes the said methods uncallable from GDScript.
Here is a small list

Viewport::get_viewport
CanvasItem::get_viewport
EditorPlugin::get_name

[reduz]

ah, that is clearly a bug

On Wed, Aug 3, 2016 at 11:34 AM, Bojidar Marinov [email protected]
wrote:

Many nodes override inherited methods, which makes the said methods
uncallable from GDScript.
Here is a small list

Viewport::get_viewport
CanvasItem::get_viewport
EditorPlugin::get_name

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-237245397,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z29RyjvUiXdJR8w3zTBKnk9vcO7TZks5qcKb_gaJpZM4JMhcW
.

[leonkrause]

As explained in #6048, the infamous visibility property is not only confusing due to its propagating nature, but also since it controls more than just visibility. A few examples that come to mind:

• whether a Control node can be focused
• whether a CollisionObject2D is pickable

Maybe it's just those, but I suggest either renaming visibility and related names to enabled or something similar, or to move this behavior out of visible to some other property

[pkowal1982]

I think it's good to implement String.parse_json() and String.to_json(Object) in 2.2 and leave deprecated Dictionary.parse_json() until 3.0 will come.

[reduz]

there's no hurry, let's do things properly and do all the changes in 3.0

On Sat, Sep 17, 2016 at 5:20 PM, pkowal1982 [email protected]
wrote:

I think it's good to implement String.parse_json() and
String.to_json(Object) in 2.2 and leave deprecated Dictionary.parse_json()
until 3.0 will come.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-247805169,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20JFmia54_U_ajOi0_Qk5Pp7YzbDks5qrEtwgaJpZM4JMhcW
.

[djrm]

rotation setters and getters must be standarized:, currently there are many variations like

• set_rot
• set_rotation
• set_rotation_deg
• set_rotationd
• set_rotd

[supaiku0]

String and Array should get an alias to size()/length() respectively or one of them should be renamed.

[reduz]

I took this from C++ STL but it's true it makes not much sense

On Sep 17, 2016 22:40, "supaiku-o" [email protected] wrote:

String and Array should get an alias to size()/length() respectively or
one of them should be renamed.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-247818665,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2xSc9BlDEFfdtJ3rK5nm5NDz8xcLks5qrJaggaJpZM4JMhcW
.

[eon-s]

Collision layers and masks methods have many confusing names like set_collision_mask set_collision_layer set_layer_mask (some in bodies, others in tilemaps, etc.), also the parameters used on similar methods varies (some use bit mask, others layer number).

I think these should be unified, it will simplify collision checks.

[reduz]

The collision layer and mask are really useful, but I have no idea how to
make the names more obvious. Any idea welcome.

On Mon, Sep 19, 2016 at 11:28 AM, eon-s [email protected] wrote:

Collision layers and masks have many confusing names like set_collision_mask
set_collision_layer set_layer_mask (some in bodies, others in tilemaps,
etc.), also the parameters used on similar methods varies (some use bit
mask, others layer number).

I think these should be unified, it will simplify collision checks.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-248008980,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2zJ3QVH-A75AYTq3u89fN2UZhjZ2ks5qrpv7gaJpZM4JMhcW
.

[Sslaxx]

Just giving them the same names throughout the nodes they're used would be a start?

[vnen]

The different polygons need a consistent API too. Here's a major divergence for about the same functionality:

• ConvexPolygonShape2D has set_points()
• ConcavePolygonShape2D has set_segments()
• NavigationPolygon has set_vertices()
• OccluderPolygon2D has set_polygon()

[eon-s]

@reduz

Area2D and PhysicsBody2D have:

set_layer_mask

I think that could be better:

set_collision_mask

That way is clear that you are talking about collisions.
Tilemap has these names but lacks of the bit set part, which is useful too, so I propose:

set_collision_mask
set_collision_layer_bit
set_collision_mask_bit

For everything that is related to collision detection (physics, area, tilemap)

ps: Not sure if the occluders and 2D lights have clear/matching names for the layers too.

[Zylann]

This one has just been uncovered: https://github.com/godotengine/godot/issues/6639#issuecomment-250567911
Also, GridMap function names should probably be made similar to TileMap, since the principle is the same.

[AlexHolly]

In String we have

get_base_dir()
get_file()

Why not the same with

basename() -> get_basename()
extension() -> get_extension()

[Marqin]

Yeah, all inconsistencies with choosen style (#3916) should be removed.

[LeonardMeagher2]

I think the JSON singletob makes sense only because you can encode a single Variant (I think?) But Dictionary and Array are the only to_json methods. A JSON singleton makes it so you know where to look, and you dont assume the data allowed. Previously you could only serialize a Dictionary, so it made sense for it to be a method of that class.

[reduz]

JSON was changed to built-in functions for GDScript already

On Fri, Jan 13, 2017 at 12:11 PM, Leonard Meagher [e-mail]
wrote:

I think the JSON singletob makes sense only because you can encode a
single Variant (I think?) But Dictionary and Array are the only to_json
methods. A JSON singleton makes it so you know where to look, and you dont
assume the data allowed. Previously you could only serialize a Dictionary,
so it made sense for it to be a method of that class.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-272465932,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2wRx9qkvMeCt5WKYw8_lju4bnml-ks5rR5Q_gaJpZM4JMhcW
.

[reduz]

ConvexPolygonShape2D has set_points()
Makes sense, you supply points to it, because any point cloud will work

ConcavePolygonShape2D has set_segments()
This is segments, not points

NavigationPolygon has set_vertices()
This is named vertices because there are indices involved to form a polygon

OccluderPolygon2D has set_polygon()
This is specifically a polygon

[reduz]

Everything in this post has been taken care of, so with a lot of joy, will proceed to close it!

[akien-mga]

Everything in this post has been taken care of, so with a lot of joy, will proceed to close it!

Not exactly :D Most things evoked here and not fixed yet should have been moved to #7516 IINM.

[vnen]

@reduz I don't really get the difference between the polygons. Aren't points the same as vertices? And the occluder one is "specifically a polygon" but it's also set as Vector2Array with "points", what's the difference? For ConvexPolygonShape2D, does it get the convex hull now? (as per 2.x API, it has set_point_cloud which states to be not implemented and set_points which requires an order, so as stated it's not different from segments).

[reduz]

A point cloud is not a polygon, a polygon is generatd from its convex hull.

Segments are individual a-b segments, they are unconnected.

Vertices do not make up a polygon, the indices do.

On Jan 13, 2017 21:15, "George Marques" [e-mail] wrote:

@reduz https://github.com/reduz I don't really get the difference
between the polygons. Aren't points the same as vertices? And the occluder
one is "specifically a polygon" but it's also set as Vector2Array with
"points", what's the difference? For ConvexPolygonShape2D, does it get the
convex hull now? (as per 2.x API, it has set_point_cloud which states to
be not implemented and set_points which requires an order, so as stated
it's not different from segments).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/5696#issuecomment-272582409,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z29b-H1hcCg6ohyCt3tdesWg_8YkMks5rSBO9gaJpZM4JMhcW
.

[vnen]

Okay, I guess what's missing is documentation then.