Refactoring: The great API review

Question

Refactoring: The great API review

Closed this issue 8 years ago · 38 comments

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() (#3778 (comment)).
OS.get_fullscreen_mode_list() (#4624 (comment)).

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.

Answer 1 · 2016-07-14T15:50:13.000Z

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 notifications@github.com
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
#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
#4625, #5262
#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
#382, #3011
#3011, #4232
#4232).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#5696, or mute the thread
https://github.com/notifications/unsubscribe/AF-Z2z31QAEIb70bh26SEeaUDE5E2asHks5qVk6mgaJpZM4JMhcW
.

Answer 2 · 2016-07-14T16:22:52.000Z

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

Answer 3 · 2016-07-14T16:31:05.000Z

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.

Answer 4 · 2016-07-14T19:54:39.000Z

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

Answer 5 · 2016-07-14T22:34:22.000Z

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 (#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 :)

Answer 6 · 2016-07-14T22:51:21.000Z

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

Answer 7 · 2016-07-14T22:57:39.000Z

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

Answer 8 · 2016-07-14T23:26:31.000Z

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.

Answer 9 · 2016-07-14T23:30:41.000Z

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

Answer 10 · 2016-07-15T17:56:43.000Z

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

Answer 11 · 2016-07-15T19:43:35.000Z

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

Answer 12 · 2016-07-30T13:04:23.000Z

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

Answer 13 · 2016-07-30T13:37:07.000Z

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 notifications@github.com
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
#5696 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z207623uOEbGoSTywt57-lKsLGRiUks5qa0vlgaJpZM4JMhcW
.

Answer 14 · 2016-07-31T07:11:08.000Z

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.

Answer 15 · 2016-08-03T14:02:07.000Z

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

Answer 16 · 2016-08-03T15:38:45.000Z

ah, that is clearly a bug

On Wed, Aug 3, 2016 at 11:34 AM, Bojidar Marinov notifications@github.com
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
#5696 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z29RyjvUiXdJR8w3zTBKnk9vcO7TZks5qcKb_gaJpZM4JMhcW
.

Answer 17 · 2016-08-05T21:43:48.000Z

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

Answer 18 · 2016-09-17T20:19:47.000Z

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.

Answer 19 · 2016-09-17T20:50:08.000Z

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 notifications@github.com
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
#5696 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20JFmia54_U_ajOi0_Qk5Pp7YzbDks5qrEtwgaJpZM4JMhcW
.

Answer 20 · 2016-09-18T00:40:28.000Z

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

set_rot
set_rotation
set_rotation_deg
set_rotationd
set_rotd

Answer 21 · 2016-09-18T01:40:34.000Z

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

Answer 22 · 2016-09-18T01:45:48.000Z

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

On Sep 17, 2016 22:40, "supaiku-o" notifications@github.com 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
#5696 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2xSc9BlDEFfdtJ3rK5nm5NDz8xcLks5qrJaggaJpZM4JMhcW
.

Answer 23 · 2016-09-19T14:27:56.000Z

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.

Answer 24 · 2016-09-19T17:59:48.000Z

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 notifications@github.com 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
#5696 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2zJ3QVH-A75AYTq3u89fN2UZhjZ2ks5qrpv7gaJpZM4JMhcW
.

Answer 25 · 2016-09-19T18:55:22.000Z

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

Answer 26 · 2016-09-19T18:57:25.000Z

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

Answer 27 · 2016-09-19T19:00:53.000Z

@reduz

Area2D and PhysicsBody2D have:

set_collision_mask
set_layer_mask

I think that could be better:

set_collision_layer
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_layer
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.

Answer 28 · 2016-09-29T19:36:19.000Z

This one has just been uncovered: #6639 (comment)
Also, GridMap function names should probably be made similar to TileMap, since the principle is the same.

Answer 29 · 2016-11-21T19:08:38.000Z

In String we have

get_base_dir()
get_file()

Why not the same with

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

Answer 30 · 2016-11-21T20:51:34.000Z

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

Answer 31 · 2017-01-13T15:11:49.000Z

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.

Answer 32 · 2017-01-13T15:20:54.000Z

JSON was changed to built-in functions for GDScript already

…

On Fri, Jan 13, 2017 at 12:11 PM, Leonard Meagher ***@***.***> 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 <#5696 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z2wRx9qkvMeCt5WKYw8_lju4bnml-ks5rR5Q_gaJpZM4JMhcW> .

Answer 33 · 2017-01-13T15:59:33.000Z

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

Answer 34 · 2017-01-13T16:00:31.000Z

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

Answer 35 · 2017-01-13T18:09:28.000Z

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.

Answer 36 · 2017-01-14T00:15:48.000Z

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

Answer 37 · 2017-01-14T01:11:10.000Z

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" ***@***.***> 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 <#5696 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z29b-H1hcCg6ohyCt3tdesWg_8YkMks5rSBO9gaJpZM4JMhcW> .

Answer 38 · 2017-01-14T01:24:03.000Z

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