Fix property/method filtering in extension_api_dump.cpp

[touilleMan]

fix #64429

Commit 24c79760e712b388a7bb12535f0e94db54742448 add a is_property_accessor attribute to extension_api.json in order to simplify the "don't expose the property accessors in your binding" rule @reduz was talking about

[touilleMan]

rebased ;-)

[YuriSizov]

These shouldn't be exposed, they have no use in scripting.

[dsnopek]

In principle, this makes sense to me!

However, testing this and looking at the extension_api.json, I'm a little worried we'll end up with a messier API than we'd like.

For example, looking at AudioStreamPlayer, with this PR we end with these two new methods: _set_playing() and _is_active(). They are are both property accessors, however:

• In a language binding without properties, we'd probably want to expose them as set_playing and is_active (without the leading underscore), so it doesn't feel quite so weird calling them. I suppose the binding generator could see that "is_virtual" is false, and then rename to not have the underscore (assuming the name isn't already taken)?
• These are both private methods on the C++ class on the Godot side. So, from a Godot module, we actually wouldn't be able to call them in the normal C++ way, which I think is probably intentional in this case? In a Godot module, you'd call play() or stop() rather than _set_playing(), and is_playing() rather than _is_active(). Maybe we also need an "is_private" property on methods, so we can treat the ones that were intentionally hidden from the C++ module API differently?

Before unleashing all these new methods on the GDExtension bindings, I think we need to make sure they have the tools to do something nice with them. Or, maybe even clean-up the names of some of them? Or, remove some? (Like in the case of AudioStreamPlayer::_is_active, it has the exact same implementation as AudioStreamPlayer::is_playing() - we could maybe just get rid of the former?)

[raulsntos]

AudioStreamPlayer::_is_active is not used as a property accessor, so from the perspective of GDExtension bindings I don't need it. I only need the private methods that are used as property accessors, so what if we only expose those?

List of private methods used as property accessors as of 4.3-dev.6.

• AnimationNode
  • _get_filters
  • _set_filters
• AnimationNodeBlendSpace2D
  • _get_triangles
  • _set_triangles
• AudioStreamPlayer
  • _set_playing
• AudioStreamPlayer2D
  • _set_playing
• AudioStreamPlayer3D
  • _set_playing
• Bitmap
  • _get_data
  • _set_data
• Control
  • _get_layout_mode
  • _set_layout_mode
  • _get_anchors_layout_preset
  • _set_anchors_layout_preset
  • _set_anchor
  • _set_size
  • _set_position
  • _set_global_position
• Image
  • _get_data
  • _set_data
• LightmapGIData
  • _get_user_data
  • _set_user_data
  • _get_probe_data
  • _set_probe_data
  • _get_light_textures_data
  • _set_light_textures_data
• MultiMesh
  • _get_transform_array
  • _set_transform_array
  • _get_transform_2d_array
  • _set_transform_2d_array
  • _get_color_array
  • _set_color_array
  • _get_custom_data_array
  • _set_custom_data_array
• NavigationMesh
  • _get_polygons
  • _set_polygons
• NavigationPolygon
  • _get_polygons
  • _set_polygons
  • _get_outlines
  • _set_outlines
• OptionButton
  • _select_int
• Polygon2D
  • _get_bones
  • _set_bones
• PolygonPathFinder
  • _get_data
  • _set_data
• ResourcePreloader
  • _get_resources
  • _set_resources
• ShapeCast2D
  • _get_collision_result
• ShapeCast3D
  • _get_collision_result
• SpriteFrames
  • _get_animations
  • _set_animations
• Translation
  • _get_messages
  • _set_messages
• VisualShaderNode
  • _get_output_ports_expanded
  • _set_output_ports_expanded
• VisualShaderNodeCustom
  • _is_initialized
  • _set_initialized
  • _get_properties
  • _set_properties
• VisualShaderNodeParameterRef
  • _get_parameter_type
  • _set_parameter_type

[dsnopek]

I only need the private methods that are used as property accessors, so what if we only expose those?

Yeah, I think exposing a targeted set of accessors (in a way that produces a friendly API) is a good way to go.

[raulsntos]

I've rebased this PR and modified it to only expose the methods that are used as property accessors of non-hidden properties, as discussed in the comments above.

I have also dropped commit f0ee3de since it doesn't seem wanted (#64825 (comment)). However, this means we now expose members like the Control::layout_mode property that use the type Control::LayoutMode which is an unexposed enum¹.

GDExtension language bindings can workaround this by using int, like we currently do in the C# bindings:

godot/modules/mono/editor/bindings_generator.cpp

Lines 3355 to 3358 in 6cd2876

	// Enum not found. Most likely because none of its constants were bound, so it's empty. That's fine. Use int instead.
	HashMap<StringName, TypeInterface>::ConstIterator int_match = builtin_types.find(name_cache.type_int);
	ERR_FAIL_NULL_V(int_match, nullptr);
	return &int_match->value;

Unfortunately, this means that if we ever expose the enum it could be a breaking change and I don't think our current compat checks handle this scenario. It's also weird to have members in extension_api.json with an unexposed type.

The rebased branch can be found on cbb151a.

And here's the before and after of the generated extension_api.json:

• extension_api.before.json
• extension_api.after.json

@touilleMan Are you able to continue working on this PR? Otherwise I'd be willing to take over for you.

Footnotes

1. Naturally, this makes the godot-cpp CI check fail: https://github.com/raulsntos/godot/actions/runs/10186088072/job/28177739185 ↩

[dsnopek]

I'm not convinced that we should remove the filtering out of functions that start with an underscore.

If there are functions that start with an underscore that we really want to be exposed, let's renamed them so they don't start with an underscore, and make sure that we like those functions being part of the public API, because:

1. In GDExtension bindings that don't have properties (like godot-cpp), just exposing these underscore functions means we end up with a public API full of function names that start with an underscore. This conflicts with common Godot naming conventions.
2. Like I pointed out above, some of these are just badly named or even unneeded (because they do the same thing as other functions that are better named).

But if we do decide to remove the filtering, at the very least, we're going to need some other way to bind a method that isn't exposed, because that is a use-case we still have. For example, with Control::_set_layout_mode(), it was explicitly decided that that shouldn't be part of the public API or used by code, it's only meant for use by the editor.

[raulsntos]

In GDExtension bindings that don't have properties (like godot-cpp), just exposing these underscore functions means we end up with a public API full of function names that start with an underscore.

Can't the godot-cpp bindings generator generate these methods as private/protected? Or just not generate them (moving the filter to godot-cpp) since we have no use for them.

My intention with "removing the filter" was just so that language bindings that need those methods to implement properties can use them. I didn't mean for these methods to be exposed in public API. That is to say, language bindings should probably avoid exposing methods that start with an underscore (thus moving the filter from Godot to the language bindings generator).

For example, in the C# GDExtension bindings I have no intention of exposing these methods, I'll only use them to implement the properties that need them.

For example, with Control::_set_layout_mode(), it was explicitly decided that that shouldn't be part of the public API or used by code, it's only meant for use by the editor.

Right, but it's used by the Control::layout_mode property so unless we expose the method somehow I can't expose the property in C#. This is a property that is already exposed in the C# scripting bindings, and it's also accessible from GDScript.

That said, the Control::layout_mode property has the PROPERTY_USAGE_INTERNAL flag so I agree it's not meant to be used by users. A better example would be Control::global_position, I'm pretty sure we want to expose this property but the setter is Control::_set_global_position so I can't generate the property in the C# GDExtension bindings.

[dsnopek]

Can't the godot-cpp bindings generator generate these methods as private/protected? Or just not generate them (moving the filter to godot-cpp) since we have no use for them.

Sure, it could. However, godot-cpp does actually need these methods too.

We don't need them in order to make properties, because we don't have a good way to do properties in C++, but these filtered methods mean there are properties on the Godot side that godot-cpp can't set, and we should have the ability to set any property that any other language binding can set.

So, what I really think we need to do, is go through the methods that start with an underscore, and determine which really should be part of the public API, and rename those. Then godot-cpp will have access to them under nice friendly names, and so will all the bindings that are going to use them as setters and getters for properties.

That said, the Control::layout_mode property has the PROPERTY_USAGE_INTERNAL flag so I agree it's not meant to be used by users. A better example would be Control::global_position, I'm pretty sure we want to expose this property but the setter is Control::_set_global_position so I can't generate the property in the C# GDExtension bindings.

Yeah, I think this needs to be evaluated on a case-by-case basis, to determine which should really be public.

[vnen]

Discussed at the GDExtension meeting and we agreed that the filtering should not be removed because there are cases that we don't want to expose the methods. If we want to expose some specific ones we should remove the underscore.

Otherwise we would have to create another filter to remove the ones that we really don't want to expose.

[raulsntos]

[...] these filtered methods mean there are properties on the Godot side that godot-cpp can't set, and we should have the ability to set any property that any other language binding can set.

Doesn't this mean we should expose all of them? For example, Control::_set_layout_mode can be used from GDScript throught the layout_mode property (although we discourage it by hidding it, but it's still available).

Discussed at the GDExtension meeting and we agreed that the filtering should not be removed because there are cases that we don't want to expose the methods.

My intention is not to remove the filtering entirely, only for property accessors. I was also thinking that keeping the underscore could be used as a convention for language bindings to treat these methods as internal and avoid exposing them (only use them for property accessors).

I thought this would be a good compromise for now, until we get around to decide whether to expose these methods properly or not. But since the team decided against it, let me take a look at the list of methods again and I'll see if I can find some low hanging fruits.

[dsnopek]

[...] these filtered methods mean there are properties on the Godot side that godot-cpp can't set, and we should have the ability to set any property that any other language binding can set.

Doesn't this mean we should expose all of them? For example, Control::_set_layout_mode can be used from GDScript throught the layout_mode property (although we discourage it by hidding it, but it's still available).

Hm, exposing layout_mode to GDScript feels like it might be an accident? Given that it uses PROPERTY_USAGE_INTERNAL.

@vnen Should properties with PROPERTY_USAGE_INTERNAL be accessible to scripts?

For godot-cpp: We want all the property accessor methods to be available as normal public methods, except for the ones for internal properties that only exist for storage or the editor. So, almost all of them. :-) But if they're going to be normal public methods that we want folks to call, they shouldn't start with an underscore.

[raulsntos]

We want all the property accessor methods to be available as normal public methods, except for the ones for internal properties that only exist for storage or the editor.

That's what I thought you'd say, so here's the list:

List

• Animation
  • _set_capture_included
  • PR: Make Animation::capture_included read-only #95377
• AudioStreamPlayer, AudioStreamPlayer2D, AudioStreamPlayer3D
  • _set_playing
  • PR: Expose AudioStreamPlayer{2D,3D}::set_playing and remove AudioStreamPlayer::{2D,3D}::_is_active #95375
• Control
  • _set_anchor
  • _set_position
  • _set_size
  • _set_global_position
• Image
  • _set_data
  • _get_data
• MultiMesh
  • _set_transform_array
  • _get_transform_array
  • _set_transform_2d_array
  • _get_transform_2d_array
  • _set_color_array
  • _get_color_array
  • _set_custom_data_array
  • _get_custom_data_array
• OptionButton
  • _select_int
  • PR: Make OptionButton::selected read-only #95378
• Polygon2D
  • _set_bones
  • _get_bones
  • PR: Make Polygon2D::bones property internal #95376
• ShapeCast2D, ShapeCast3D
  • _get_collision_result
  • PR: Expose ShapeCast{2D,3D}::get_collision_result #95374

In some of these, the internal method seems to only exist to workaround the lack of overload support in ClassDB. For example, a method Control::set_global_position already exists and it's exposed but it has additional optional parameters which I imagine is what prevents us from using it as a property accessor and why Control::_set_global_position exists.

It's also worth noting that making a method a property accessor seems to hide it. For example, in OptionButton there's already a select method we could use instead of _select_int as the property accessor for the selected property. But doing so hides the select method which is probably not what we want to do.

In the case of MultiMesh, all of those methods (and their respective properties) are inside a #ifndef DISABLE_DEPRECATED block, it seems they only exist for compatibility with Godot 3.

And then, for some of these the corresponding property may have been intended to be internal (e.g.: Image::data? Polygon2D::bones?).