Allow method binds to take Object subclasses as arguments #57205

TechnoPorg · 2022-01-25T15:53:37Z

Using some new C++ features, this PR adds a condition to VariantCaster that casts Variants of type OBJECT to any type T, if T is derived from Object.
This change enables a fair bit of code cleanup. First, the Variant implicit cast operators for Node and Control can be removed, which allows for some of the invalid includes mentioned in #53295 to be removed. Second, helper methods in Tree, whose sole purpose was to cast arguments to TreeItem *, are no longer necessary.
A few small changes also had to be made to other files, due to the changes cascading down all the includes.

Fixes #21160, closes #20538, supersedes #57169

This commit adds a condition to VariantCaster that casts Variants of type OBJECT to any type T, if T is derived from Object. This change enables a fair bit of code cleanup. First, the Variant implicit cast operators for Node and Control can be removed, which allows for some invalid includes to be removed. Second, helper methods in Tree whose sole purpose was to cast arguments to TreeItem * are no longer necessary. A few small changes also had to be made to other files, due to the changes cascading down all the includes.

akien-mga · 2022-01-25T16:12:13Z

That's pretty cool! There might be quite a few other occurrences of bindings where return types or arguments have been limited to Object *, Node * or Control * when they could be more specific (I'd suggest searching for them with regex on the class reference XML), but it's probably better to wait for the Variant change to be approved and merged before changing further bindings in a follow-up PR.

Zireael07 · 2022-01-25T16:19:26Z

@akien-mga; Just to be clear, you recommend that OP do not change any more bindings in this PR?

aaronfranke · 2022-01-26T03:11:20Z

Does this mean that we can bind LineEdit as the type of the method I changed in #48920? Awesome! But yes, it's probably best to get the Variant changes in first before we start changing things around the engine.

Xrayez · 2022-01-26T10:13:18Z

My take on this was #42060, @reduz was not happy with implementation because that would require using VARIANT_OBJECT_CAST(Object) for each class.

So yeah... I confirm this would be useful, even from the user perspective (I'd also remove specific casts from the Goost module in two places (https://github.com/goostengine/goost):

Therefore, this PR supersedes #42060 as well.

TechnoPorg · 2022-01-26T19:36:52Z

~~VARIANT_ENUM_CAST could probably even be removed with the same technique, but using std::is_enum. That change should go in a separate PR if it's desired, though.~~

EDIT: Never mind, there's no way to get the name of an enum in string format, without using a macro.

reduz · 2022-01-27T08:29:36Z

Would still be good if @neikeq gives it a review

neikeq · 2022-01-27T14:37:59Z

Second, helper methods in Tree, whose sole purpose was to cast arguments to TreeItem *, are no longer necessary.

Why were they needed? Is it because of DEFVALs?

YuriSizov · 2022-01-27T14:42:41Z

Second, helper methods in Tree, whose sole purpose was to cast arguments to TreeItem *, are no longer necessary.

Why were they needed? Is it because of DEFVALs?

Because you could not bind methods with Object derivatives as arguments/return value with ClassDB before this PR. So we use Object everywhere even if there is a way narrower type expected, and then cast and check inside of every method. I guess whoever made Tree just added a wrapper to save a few lines.

akien-mga · 2022-01-27T15:59:38Z

Yeah, see https://github.com/godotengine/godot/pull/57205/files#diff-73e583f1bfffebc5e30a925726bf40f32d72daaea940bf229e71479e98f8d1f7R339 - prior to this PR the only types that could be used in bindings were Object *, Control * and Node * because they had dedicated operators implemented in Variant (and this made Variant depend on node.h and control.h).

neikeq · 2022-01-27T19:10:06Z

Ah, I see. I never realized we were using only those 3 object types in method signatures.
This looks fine then. Both PtrToArg (for ptrcall) and GetTypeInfo (for bindings generators) already handle this.

akien-mga

Looks great to me! I'm looking forward to improving the existing bindings with this more flexible system.

akien-mga · 2022-01-28T14:19:06Z

There might be quite a few other occurrences of bindings where return types or arguments have been limited to Object *, Node * or Control * when they could be more specific (I'd suggest searching for them with regex on the class reference XML)

So I had a look at occurrences for type="Object" in the class reference XML (rg -g'*.xml' 'type="Object"'), here's what I found.

Normal method bindings

Some cases that could be improved easily with the new feature:

TileSetAtlasSource: Make get_tile_data return TileData * #57351
Improve some method bindings to use specific Object subtypes #57355
There might be more in physics code specifically, I didn't look in depth but there's a number of getters returning Object and some might be actually always handling CollisionObject2D/3D (albeit via their RID).

Virtual methods

Virtual methods are bound with GDVIRTUAL* macros and those don't seem to support types more specific than Object *. I don't know if this can be improved. Here's an example:

scene/gui/control.h
301:    GDVIRTUAL1RC(Object *, _make_custom_tooltip, String)

(This could return Control * in theory.)

PropertyInfo

I found a few under-specified object types for some signals declared like this:

scene/gui/base_button.cpp
506:    ADD_SIGNAL(MethodInfo("pressed", PropertyInfo(Variant::OBJECT, "button")));

This can be worked around via a stringly typed hint in PropertyInfo like this:

ADD_SIGNAL(MethodInfo("pressed", PropertyInfo(Variant::OBJECT, "button", PROPERTY_HINT_RESOURCE_TYPE, "BaseButton")));

(Even though it's not a Resource type but a Node here.)

This is used throughout the engine for signals and properties to specific their actual type in the bindings.
Might be worth looking into to see if a solution similar to the one used here could make it possible to detect these types properly without having to rely on manually specified strings.

Edit: This seems only needed for signals, for properties the type is inferred from the setter/getter.
Edit 2: For now #57358 fixes the few occurrences where PROPERTY_HINT_RESOURCE_TYPE was not used but could be.

Xrayez · 2022-02-06T16:24:29Z

This would be nice to implement in 3.x, if that doesn't break compatibility (I need this for custom modules and implementing custom data structures, such as graph: goostengine/goost#172).

TechnoPorg · 2022-02-06T17:05:55Z

I'm not sure if this can be implemented in 3.x, since if constexpr is a C++ 17 feature, and 3.x uses C++ 14. I couldn't get this to compile with a regular if statement, but maybe that could be worked around by using a constexpr bool first, and then testing that. Or maybe the template wizardry used in GetTypeInfo could work for VariantCaster as well?

TechnoPorg · 2022-02-11T02:45:39Z

@Xrayez I tried briefly to make this work on 3.x with a constexpr bool, but the compiler unfortunately didn't cooperate. I don't have any major projects with Godot ongoing, so I'd prefer to focus on 4.0 as much as possible. I would be glad to continue to try and get this working on 3.x if you want me to, but it may be most efficient for goost to use your VARIANT_OBJECT_CAST macro instead.

Xrayez · 2022-02-11T13:10:26Z

No worries, since it's possible to workaround, it's not a big deal. Sometimes I do forget to cast custom objects, though (just like VARIANT_ENUM_CAST).

nikitalita · 2023-12-29T08:12:51Z

core/variant/binder_common.h

+		using TStripped = std::remove_pointer_t<T>;
+		if constexpr (std::is_base_of<Object, TStripped>::value) {
+			Object *obj = p_variant;
+			return Object::cast_to<TStripped>(p_variant) || !obj;


Isn't this supposed to be !obj || Object::cast_to<TStripped>(p_variant)? Getting EA_BAD_ACCESS errors at this line

Yes, it is. Object::cast_to<T>() takes in an Object *, so both values will be the result of p_variant->operator Object*() and will be the same. So, if obj is a nullptr, we shouldn't attempt the cast.

Related #84858
Would your change fix the same issue? Maybe it's better solution.

No, it doesn't. This should be changed either way because it's semantically incorrect, but it doesn't fix the issues I was having in #86602

TechnoPorg requested review from a team as code owners January 25, 2022 15:53

TechnoPorg force-pushed the variant-template-cast branch from 424c782 to 051ef47 Compare January 25, 2022 16:04

YeldhamDev added enhancement topic:core labels Jan 25, 2022

YeldhamDev added this to the 4.0 milestone Jan 25, 2022

YuriSizov requested a review from reduz January 25, 2022 16:09

reduz approved these changes Jan 27, 2022

View reviewed changes

akien-mga requested a review from neikeq January 27, 2022 08:36

neikeq approved these changes Jan 27, 2022

View reviewed changes

akien-mga approved these changes Jan 27, 2022

View reviewed changes

akien-mga merged commit e6caaf4 into godotengine:master Jan 27, 2022

akien-mga mentioned this pull request Jan 28, 2022

TileSetAtlasSource: Make get_tile_data return TileData * #57351

Merged

akien-mga mentioned this pull request Jan 28, 2022

Improve some method bindings to use specific Object subtypes #57355

Merged

TechnoPorg deleted the variant-template-cast branch January 28, 2022 16:25

TechnoPorg mentioned this pull request Jan 28, 2022

Allow to cast custom objects for Variant #42060

Closed

akien-mga mentioned this pull request Feb 15, 2022

Change find_node to find_nodes and Add an type parameter. #56032

Merged

Faless mentioned this pull request Jan 10, 2023

Allow method binds to take Object subclasses as arguments godotengine/godot-cpp#989

Merged

nikitalita reviewed Dec 29, 2023

View reviewed changes

This was referenced Dec 29, 2023

Crash when closing scenes after interacting with the animation player #86602

Closed

Validate Object pointers on binder method calls #84858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow method binds to take Object subclasses as arguments #57205

Allow method binds to take Object subclasses as arguments #57205

TechnoPorg commented Jan 25, 2022

akien-mga commented Jan 25, 2022

Zireael07 commented Jan 25, 2022

aaronfranke commented Jan 26, 2022

Xrayez commented Jan 26, 2022

TechnoPorg commented Jan 26, 2022 •

edited

reduz commented Jan 27, 2022

neikeq commented Jan 27, 2022

YuriSizov commented Jan 27, 2022

akien-mga commented Jan 27, 2022

neikeq commented Jan 27, 2022

akien-mga left a comment

akien-mga commented Jan 28, 2022 •

edited

Xrayez commented Feb 6, 2022 •

edited

TechnoPorg commented Feb 6, 2022 •

edited

TechnoPorg commented Feb 11, 2022 •

edited

Xrayez commented Feb 11, 2022

nikitalita Dec 29, 2023

nikitalita Dec 29, 2023

KoBeWi Dec 29, 2023

nikitalita Dec 29, 2023

Allow method binds to take Object subclasses as arguments #57205

Allow method binds to take Object subclasses as arguments #57205

Conversation

TechnoPorg commented Jan 25, 2022

akien-mga commented Jan 25, 2022

Zireael07 commented Jan 25, 2022

aaronfranke commented Jan 26, 2022

Xrayez commented Jan 26, 2022

TechnoPorg commented Jan 26, 2022 • edited

reduz commented Jan 27, 2022

neikeq commented Jan 27, 2022

YuriSizov commented Jan 27, 2022

akien-mga commented Jan 27, 2022

neikeq commented Jan 27, 2022

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Jan 28, 2022 • edited

Normal method bindings

Virtual methods

PropertyInfo

Xrayez commented Feb 6, 2022 • edited

TechnoPorg commented Feb 6, 2022 • edited

TechnoPorg commented Feb 11, 2022 • edited

Xrayez commented Feb 11, 2022

nikitalita Dec 29, 2023

Choose a reason for hiding this comment

nikitalita Dec 29, 2023

Choose a reason for hiding this comment

KoBeWi Dec 29, 2023

Choose a reason for hiding this comment

nikitalita Dec 29, 2023

Choose a reason for hiding this comment

TechnoPorg commented Jan 26, 2022 •

edited

akien-mga commented Jan 28, 2022 •

edited

Xrayez commented Feb 6, 2022 •

edited

TechnoPorg commented Feb 6, 2022 •

edited

TechnoPorg commented Feb 11, 2022 •

edited