Properties tagged PROPERTY_USAGE_INTERNAL are usable from GDscript

[clayjohn]

I started out reporting that properties tagged PROPERTY_USAGE_INTERNAL were not showing in the docs, but then I saw this PR and realized that they were not even supposed to be usable in GDScript.

I noticed with Multimesh which has three arrays that are tagged PROPERTY_USAGE_INTERNAL, but can all be used from GDScript.

godot/scene/resources/multimesh.cpp

Lines 267 to 269 in c19840d

	ADD_PROPERTY(PropertyInfo(Variant::POOL_VECTOR3_ARRAY, "transform_array", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NOEDITOR | PROPERTY_USAGE_INTERNAL), "_set_transform_array", "_get_transform_array");
	ADD_PROPERTY(PropertyInfo(Variant::POOL_COLOR_ARRAY, "color_array", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NOEDITOR | PROPERTY_USAGE_INTERNAL), "_set_color_array", "_get_color_array");
	ADD_PROPERTY(PropertyInfo(Variant::POOL_COLOR_ARRAY, "custom_data_array", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NOEDITOR | PROPERTY_USAGE_INTERNAL), "_set_custom_data_array", "_get_custom_data_array");

They don't appear in the docs (which I guess is good?) but they do autocomplete in GDScript and work fine in GDScript.

@akien-mga You may want to look at this.

[dominiks]

At the moment this is effectively a flag for "undocumented feature" since they are accessible via gdscript. Can the property made private or hidden from the autocomplete in any way?

If that is not possible I think all items flagged with PROPERTY_USAGE_INTERNAL should appear in the documentation but with a note that tells the reader the property is not intended for use and might get changed or moved around unexpectedly.

[Skullfurious]

I don't agree that hiding them from autocomplete is necessary. If anything they should be, at the very least, documented.

[clayjohn]

@Skullfurious the tag is for properties that are used internally/ for serialization. Accessing them from script is likely to break the nodes. The only possible documentation we could write would be "danger! Do not use!"

[dalexeev]

I don't think this is a bug. Unregistered classes, unexposed properties, methods, signals, and constants must not be accessible from GDScript. See godotengine/godot-proposals#2285.

But PROPERTY_USAGE_INTERNAL is probably not the case. If a property is exposed using ADD_PROPERTY, then it must be available for scripting. This is useful for properties that should not documented but are displayed in the editor and can be changed via GDScript. For example layout_mode (see this comment).

Probably important internal APIs should be better documented, since it is not always possible to determine the function of a particular flag by its name.

[YuriSizov]

It's not a bug, but it's a poorly designed feature. I think these properties should not be available for scripting, neither should they be generated for C# or other language bindings. So far the feature only hides them from the documentation, which means that we expose a purposefully undocumented API. Which doesn't make much sense to do. And if these properties are added to the documentation, then this feature becomes moot, since this is currently the only difference from any other property.

So since there is clearly a need for internal properties, then they need to be correctly handled by scripting languages. We did discuss it a bit recently with the .NET contributors btw.

[vnen]

So since there is clearly a need for internal properties, then they need to be correctly handled by scripting languages.

This more of a core issue, at least for GDScript. It cannot really check if a property is internal when accessing, it just dispatches the setting/getting to the core methods (well, technically it could check but it sounds like too much for me adding an extra condition in every property access based on the off-chance it might be an internal one).

If those are not to be used for scripting languages, they shouldn't be exposed in the first place. IMO the set/get dispatch in the core side should handle this and treat as non-existing properties when used from non-internal code. Otherwise the mistake is likely to happen with every language using the API.

[YuriSizov]

Well, you know, if we need it to be a property-like entity for some internal purposes, then we have to add it to ClassDB. As far a scripting languages are concerned it's no different from other editor only property types, like categories and groups, so it can probably be handled similarly. Though I don't know if it's core that handles that or script server, or a little bit of both.

I can take a look.

[vnen]

Well, you know, if we need it to be a property-like entity for some internal purposes, then we have to add it to ClassDB. As far a scripting languages are concerned it's no different from other editor only property types, like categories and groups, so it can probably be handled similarly. Though I don't know if it's core that handles that or script server, or a little bit of both.

The ScriptServer does not get involved in this process at all BTW.

Groups, for instance, are in the property list but since they don't have setter or getter, they are treated as non-existing properties. This is handled by ClassDB::{g,s}et_property(). IMO it should do the same with internal properties, and then have another path to get/set internal properties when they are relevant.

Offloading this to scripting languages means one of them will miss this detail and allow access to those. The ones that do handle it will have to do extra checks (at runtime for dynamic languages like GDScript). It is pretty complicated to get the usage flags of some property at runtime since it involves many checks and lookups, making things worse. Doing this in core can skip all of those checks and avoid some language not doing it properly.

[YuriSizov]

That makes sense, thanks for the pointers!

[adamscott]

Pushed the milestone to 4.x
This issue will certainly not be resolved for 4.1