search

godotengine / godot

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

Refactoring: The great API review #5696

Closed vnen closed 7 years ago

vnen commented 8 years ago

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

Singletons/Classes to split

reduz commented 8 years ago

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

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

— 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 commented 8 years ago

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

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

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

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

@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 commented 8 years ago

@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 commented 8 years ago

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

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

neikeq commented 8 years ago

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

Zylann commented 8 years ago

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

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

reduz commented 8 years ago

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::getaspect 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 commented 8 years ago

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

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

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 https://github.com/godotengine/godot/issues/5696#issuecomment-237245397, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z29RyjvUiXdJR8w3zTBKnk9vcO7TZks5qcKb_gaJpZM4JMhcW .

leonkrause commented 8 years ago

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:

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

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

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

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

spkjp commented 8 years ago

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

reduz commented 8 years ago

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 https://github.com/godotengine/godot/issues/5696#issuecomment-247818665, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z2xSc9BlDEFfdtJ3rK5nm5NDz8xcLks5qrJaggaJpZM4JMhcW .

eon-s commented 8 years ago

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

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 https://github.com/godotengine/godot/issues/5696#issuecomment-248008980, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z2zJ3QVH-A75AYTq3u89fN2UZhjZ2ks5qrpv7gaJpZM4JMhcW .

Sslaxx commented 8 years ago

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

vnen commented 8 years ago

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

eon-s commented 8 years ago

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

Zylann commented 8 years ago

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

In String we have

get_base_dir()
get_file()

Why not the same with

basename() -> get_basename()
extension() -> get_extension()
Marqin commented 7 years ago

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

LeonardMeagher2 commented 7 years ago

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

JSON was changed to built-in functions for GDScript already

On Fri, Jan 13, 2017 at 12:11 PM, Leonard Meagher notifications@github.com 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 commented 7 years ago

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

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

akien-mga commented 7 years ago

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

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

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" notifications@github.com 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 commented 7 years ago

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