Ignore class's property array when generating extension_api.json (not… #64428

touilleMan · 2022-08-15T09:45:27Z

I've copied the code from the mono's bindings_generator.cpp (I'm not sure why the F.type == Variant::NIL part is needed given PropertyInfo::add_property_array always set the type to Variant::NIL)

In the long run, we should probably replace this by a proper factorization (for instance adding a PropertyInfo::is_fake_property method) as there is multiple places where this E.usage & PROPERTY_USAGE_CATEGORY || ... pattern is used (sometime without the check to PROPERTY_USAGE_ARRAY and no comment to easily understand if it is a desired behavior ^^)

EDIT: I've also included a commit that filter out the properties with / in there name (this also comes from mono's bindings_generator.cpp ) as they are also not real properties (if you ask me, in the long run we should have a dedicated PROPERTY_... for this kind of "editor only" property). Tell me you'd rather have it as a separate PR ;-)

raulsntos · 2022-08-15T13:50:17Z

I'm not sure why the F.type == Variant::NIL part is needed given PropertyInfo::add_property_array always set the type to Variant::NIL

Probably because it can be Variant::INT when using add_property_array_count which adds the same PROPERTY_USAGE_ARRAY flag so we need to disambiguate. This way the array count is exposed, although it may not be very useful by itself.

By the way, when I added that check to mono I copied it from test_class_db (this was added in the original PR that implements the array property feature):

godot/tests/test_class_db.h

Line 530 in 4bd7700

    
           if (property.usage & PROPERTY_USAGE_GROUP || property.usage & PROPERTY_USAGE_SUBGROUP || property.usage & PROPERTY_USAGE_CATEGORY || (property.type == Variant::NIL && property.usage & PROPERTY_USAGE_ARRAY)) {

touilleMan · 2022-08-15T15:43:57Z

This way the array count is exposed, although it may not be very useful by itself.

According to #63066 PROPERTY_USAGE_ARRAY on Variant::INT also cause issues in extension_api.json

@raulsntos I'd rather updated this PR to ignore all PROPERTY_USAGE_ARRAY (As you say it is not very useful and this array flag seems pretty fragile). But if you prefer I can implement instead the fix you proposed

raulsntos · 2022-08-15T17:51:58Z

One example of something that would break if we remove/unexpose the count properties: in C# we use the ItemList.ItemCount property in BuildOutputView.

touilleMan · 2022-08-15T21:14:00Z

@raulsntos Thanks for the feedback ! I've updated my PR with your fix then ;-)

akien-mga · 2022-08-22T08:32:19Z

Since the changes seem related to each other it might be best to squash them, and use the commit description to clarify what the changes do instead of separate commits.

BTW it seems like you opened this PR by creating a branch on the upstream Godot repo instead of your fork. That means double the amount of CI runs (when pushing to the branch, and in the PR). To note for future PRs :)

akien-mga · 2022-08-22T08:37:20Z

core/extension/extension_api_dump.cpp

+					if (F.name.find("/") >= 0) {
+						// Ignore properties with '/' (slash) in the name. These are only meant for use in the inspector.
+						continue;
+					}


I'm not sure if that's always true.

At least some of them seem to be real but badly named properties:

doc/classes/AudioEffectDelay.xml: <member name="feedback/active" type="bool" setter="set_feedback_active" getter="is_feedback_active" default="false"> doc/classes/AudioEffectDelay.xml: <member name="feedback/delay_ms" type="float" setter="set_feedback_delay_ms" getter="get_feedback_delay_ms" default="340.0"> doc/classes/AudioEffectDelay.xml: <member name="feedback/level_db" type="float" setter="set_feedback_level_db" getter="get_feedback_level_db" default="-6.0"> doc/classes/AudioEffectDelay.xml: <member name="feedback/lowpass" type="float" setter="set_feedback_lowpass" getter="get_feedback_lowpass" default="16000.0"> doc/classes/AudioEffectDelay.xml: <member name="tap1/active" type="bool" setter="set_tap1_active" getter="is_tap1_active" default="true"> doc/classes/AudioEffectDelay.xml: <member name="tap1/delay_ms" type="float" setter="set_tap1_delay_ms" getter="get_tap1_delay_ms" default="250.0"> doc/classes/AudioEffectDelay.xml: <member name="tap1/level_db" type="float" setter="set_tap1_level_db" getter="get_tap1_level_db" default="-6.0"> doc/classes/AudioEffectDelay.xml: <member name="tap1/pan" type="float" setter="set_tap1_pan" getter="get_tap1_pan" default="0.2"> doc/classes/AudioEffectDelay.xml: <member name="tap2/active" type="bool" setter="set_tap2_active" getter="is_tap2_active" default="true"> doc/classes/AudioEffectDelay.xml: <member name="tap2/delay_ms" type="float" setter="set_tap2_delay_ms" getter="get_tap2_delay_ms" default="500.0"> doc/classes/AudioEffectDelay.xml: <member name="tap2/level_db" type="float" setter="set_tap2_level_db" getter="get_tap2_level_db" default="-12.0"> doc/classes/AudioEffectDelay.xml: <member name="tap2/pan" type="float" setter="set_tap2_pan" getter="get_tap2_pan" default="-0.4"> doc/classes/CharacterBody2D.xml: <member name="collision/safe_margin" type="float" setter="set_safe_margin" getter="get_safe_margin" default="0.08"> doc/classes/CharacterBody3D.xml: <member name="collision/safe_margin" type="float" setter="set_safe_margin" getter="get_safe_margin" default="0.001"> doc/classes/Joint3D.xml: <member name="collision/exclude_nodes" type="bool" setter="set_exclude_nodes_from_collision" getter="get_exclude_nodes_from_collision" default="true"> doc/classes/Joint3D.xml: <member name="nodes/node_a" type="NodePath" setter="set_node_a" getter="get_node_a" default="NodePath("")"> doc/classes/Joint3D.xml: <member name="nodes/node_b" type="NodePath" setter="set_node_b" getter="get_node_b" default="NodePath("")"> doc/classes/Joint3D.xml: <member name="solver/priority" type="int" setter="set_solver_priority" getter="get_solver_priority" default="1">

See #17558.

I'm fixing those in #44876.

In extension_api.json we want to expose properties that are meant to access a class attribute from script (i.e. `Node2D.position`). However property system is also used in Godot to declare attributes accessible from the node editor: - property with '/' in their name - property array with NIL type that represents an array

touilleMan · 2022-08-22T20:17:46Z

@akien-mga thanks for the review !

I've squashed and rebase the commits and improved the commit message as you asked

BTW it seems like you opened this PR by creating a branch on the upstream Godot repo

Sorry for this (I can't even say it's the first time, shame on me !), I'll try to be more careful next time ;-)

I'm fixing those in #44876.

So do you want me to remove the check on / properties now ?

akien-mga · 2022-08-22T20:19:49Z

I'm fixing those in #44876.

So do you want me to remove the check on / properties now ?

No I think it's ok. Those properties with / are not actually usable in GDScript so it makes sense to ignore them in GDExtension too. After #44876, the remaining ones should all be stuff that is indeed not meant to be accessed as properties.

akien-mga · 2022-08-23T05:58:31Z

Thanks!

touilleMan requested a review from a team as a code owner August 15, 2022 09:45

Calinou added bug topic:gdextension labels Aug 15, 2022

Calinou added this to the 4.0 milestone Aug 15, 2022

akien-mga approved these changes Aug 22, 2022

View reviewed changes

akien-mga reviewed Aug 22, 2022

View reviewed changes

akien-mga mentioned this pull request Aug 22, 2022

Fix properties still using a slash delimiter #44876

Closed

touilleMan force-pushed the gdextension-ignore-property-array branch from 488fc63 to b68ae22 Compare August 22, 2022 20:14

touilleMan force-pushed the gdextension-ignore-property-array branch from b68ae22 to a696332 Compare August 22, 2022 20:17

akien-mga approved these changes Aug 22, 2022

View reviewed changes

akien-mga merged commit f97f5a6 into master Aug 23, 2022

akien-mga deleted the gdextension-ignore-property-array branch August 23, 2022 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore class's property array when generating extension_api.json (not… #64428

Ignore class's property array when generating extension_api.json (not… #64428

touilleMan commented Aug 15, 2022 •

edited by akien-mga

Loading

raulsntos commented Aug 15, 2022

touilleMan commented Aug 15, 2022

raulsntos commented Aug 15, 2022

touilleMan commented Aug 15, 2022

akien-mga commented Aug 22, 2022

akien-mga Aug 22, 2022 •

edited

Loading

akien-mga Aug 22, 2022

touilleMan commented Aug 22, 2022

akien-mga commented Aug 22, 2022

akien-mga commented Aug 23, 2022

Ignore class's property array when generating extension_api.json (not… #64428

Ignore class's property array when generating extension_api.json (not… #64428

Conversation

touilleMan commented Aug 15, 2022 • edited by akien-mga Loading

raulsntos commented Aug 15, 2022

touilleMan commented Aug 15, 2022

raulsntos commented Aug 15, 2022

touilleMan commented Aug 15, 2022

akien-mga commented Aug 22, 2022

akien-mga Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

akien-mga Aug 22, 2022

Choose a reason for hiding this comment

touilleMan commented Aug 22, 2022

akien-mga commented Aug 22, 2022

akien-mga commented Aug 23, 2022

touilleMan commented Aug 15, 2022 •

edited by akien-mga

Loading

akien-mga Aug 22, 2022 •

edited

Loading