Script properties coming from `_get_property_list` are no longer shown in inspector

[Zylann]

Godot 3.2.4 adf2dabbde23fdd89803871e26dd1d33ca20bf77 Windows 10 64 bits

From report https://github.com/Zylann/godot_heightmap_plugin/issues/216#issuecomment-726427174 My plugin makes extensive use of _get_property_list to add specific hints and categories to its exported variables and make a few dynamic ones appear, which worked in Godot 3.2.3. However, it appears the commit 8a02f221b468056345b7506a463a8c8b529bfb29 now prevents all these properties from appearing.

Godot3 MRP: CustomPropertyList.zip Open main scene, select root node. The first property is the only one exported with the export keyword. There are many others supposed to be visible, but in master they aren't showing up anymore.

Godot4 MRP: https://github.com/godotengine/godot/issues/43491#issuecomment-1001666589

[willnationsdev]

Oh gosh, lovely. Guess I'll have to take a look at that when I get a chance. Thanks for the sample project.

[willnationsdev]

Looks like this happens because get_script_property_list() doesn't include generated properties. The same appears to be the case for all of the scripting languages, that I can see. get_property_list() includes it of course, but if I were to switch to using that, it would require me to have an actual Object with the Script attached in order to get a ScriptInstance from which to pull the data, for each script in the inheritance hierarchy. And I don't think it's a good idea to instantiate and later free whatever the underlying Object type is just for the sake of rendering some properties.

Seems to me like the only good solution is to modify implementations of get_script_property_list() in every language to ensure that it properly includes the generated properties from get_script_property_list() if an implementation of the method exists on the script.

[willnationsdev]

So, I have an updated version of the sample project where I make a base.gd that is also a tool script, make main.gd inherit from base.gd, make a _props property in there, change main.gd's _props to _props2, and then finally re-implement all the functions, but with shader_param/test having been moved to the base class instead of kept with the others in main.

CustomPropertyList_V2.zip

That then results in a naturally-generated list that looks like this:

Script Variables (category)
base_regular_export
_props
regular_export
_props2
directory
chunk_size
Collision (group)
collision_enabled
shader_params/test

We want it to become this:

main.gd (category)
regular_export
directory
chunk_size
Collision (group)
collision_enabled
base.gd (category)
base_regular_export
shader_params/test

Unfortunately, it doesn't seem like there was ever support for having generated properties be mixed together with the regular properties of each script, so they are always appended to the end, regardless of which script actually implemented the logic to generate them.

When I initially wrote this change, I wanted to find a way to implement all of the logic for fixing the order in a single place and just let scripts do what they want, but if you look carefully, the generated properties are in a reverse-class-order compared to their regular counterparts. In addition, because they don't show up in get_script_property_list(), I have no way of identifying, post creation, which script the properties belong to.

As such, the only way to know which properties go where is to modify how each respective scripting language builds each of its get_property_list contributions. There's no silver bullet solution from what I can tell.

[willnationsdev]

I suppose, at least for now, to fix it in stable, I can do a half-measure where I create a Generated section at the bottom of the scripted content and just stick them all in there. That'd be the easy solution anyway, without the risk of breaking any other logic in Godot that depends on the current scripting languages' get_property_list() implementations.

Edit: Additionally, I could add support for class/file name prefixed properties to automatically be added to that class's property section. So, if you have base.gd, then a property with the generated name base.gd_my_prop would appear as my_prop at the bottom of the base.gd category. And if base.gd is a script class with the name Base, then it'd have to be Base_my_prop for the property name.

[Zylann]

So... the PR you did only added the name of the scripts properties come from right? And just because of that... we can't get properties from _get_property_list? Man that's wild :( Having a Generated category is going to be very confusing... it seems your feature just needs proper support for these backing functions rather than requiring such workaround.

[willnationsdev]

@Zylann well, we can get the properties. They are there. It's just that the way I was building the new property list was by looking up the properties by name from whatever came back from get_script_property_list(). Which works, but only for statically defined properties which the Script, not the ScriptInstance, knows about. I can see the generated properties in the property list, but I am accidentally deleting them atm since they exist between the generated-and-reordered properties vs the base C++ class's original properties. So, I can get the properties, but I wouldn't know which script they belong to without more information.

Edit: I guess, it may be better to just not have a Generated section at all, and just mandate that if someone wants their generated properties to show up, then they need to prefix them with the class name / file name. That would look better in the end, and make it easier for plugin creators to make their documentation.

[Zylann]

they need to prefix them with the class name / file name.

If I do that it's going to break everything, because that's an API breakage as well, while also being awkward...

If the problem is only about generated properties, if possible, I would just not have that feature trigger if some of them exist. But if even that is not possible, I would not have the feature at all until it can be properly implemented.

[willnationsdev]

@Zylann

If the problem is only about generated properties, if possible, I would just not have that feature trigger if some of them exist

I believe I could do that, although it'd be a little weird on the logic side. You'd have to get the names of all properties, then get names of properties from individual scripts and systematically remove them all from the master list of properties. Then check if any properties are remaining and if so, exit early.

[Zylann]

Well that first idea was just for a first workaround until better is implemented, but if that's not ok (seems dirty to me as well) then this feature needs to get reverted IMO, and rework the internals if needed, so that this can be properly handled just like the core classes. The info is definitely there, we just need a clean way to get it.

[willnationsdev]

Yeah, I think doing the first solution for 3.2.x would be good, and then a better solution for 4.x can be done.

[willnationsdev]

@Zylann Then again, I think the idea of having properties just suddenly appear sorted and then not appear sorted if you happen to implement a certain method is even more jarring of a user experience. So, @aaronfranke @akien-mga I think it may be a good idea to revert my 8a02f22 until a more in-depth implementation can be made, both in 3.2 and in master.

[aaronfranke]

but if I were to switch to using that, it would require me to have an actual Object with the Script attached in order to get a ScriptInstance from which to pull the data

Can you try getting the ScriptInstance, and then check if it's null before using it? Sorry if that seems naïve, I haven't looked into this in depth.

Maybe this should be reverted in 3.2, but I think that it's fine to keep this in master for now (we'll just need to keep this issue open to remind us to either revert it later or make a proper solution).

[willnationsdev]

Well, I just tried to test it out in the master branch, but the unaltered List<PropertyInfo> appears to have properties all with properties containing names with only a single starting character. So "S" from "Script Variables", "r" from "regular_export", etc. It appears to be happening during the PropertyInfo constructor, when assigning to the name field. That goes to the String's copy constructor where a CowData references the given strings CowData. But what results from that is "Script Variables" going in and "S" going out. I'm not really sure how the data loss is happening.

Happens here: https://github.com/godotengine/godot/blob/master/core/string/ustring.h#L438

[akien-mga]

Reverted the 3.2 cherry-pick with 755ee768718f9137c2dae62ae4e906871d59ef50 until this is fixed, as suggested.

As I understand the regression also affects master? So it can still be debugged and fixed there, and then #32428 could be cherry-picked for 3.2 again with the bugfix.

[immccc]

I was about to open an issue about this, but first I found this one. In my case I don't need a fancy use of _get_property_list so far, but would be great to have proper group of properties.

Please find the issue in the gif below.

[DarioDaF]

Since it's probably going to be breaking why not simply use _get_property_list() for extra properties etc that are just appended to the property data, while also allowing _get_property_list(gen_props) to be called with the props pregenerated and returning itself (default implementation obviously return gen_props)

[willnationsdev]

There's a deeper issue at root here. Namely that the manner in which I was trying to generate the titles of scripts was using statically defined information only rather than runtime information available to the Object/ScriptInstance. And since each scripting language devises its own manner of organizing properties, the appropriate solution would be to have an internal solution for each language that independently pre-inserts the necessary property groups/categories where they should appear in the inheritance history (godotengine/godot-proposals#2803).

However, I noticed in a recent build that Godot is now already showing the class names in 4.0 - at least, with GDScript, not sure about others - and that kind of kills the need for this issue / my PR in the first place unless it is something specific to GDScript (would have to investigate further).

[AnidemDex]

@willnationsdev are there any updates on this issue? I was having a similar issue with export vars in 3.4, and @crystalwarrior was able to replicate it on its side using the same files as I was using.

The error in the console is related to custom classes registered with class_name and the script not being able to find them (maybe because the script is loaded before classes registered with class_name are added to global scope?), but everything works when you play. Modifying the script and saving it seems to fix this but no clue what should I debug to find where's the real issue.

This is the most related issue to this, since other nodes/resources that this scene is using appends a lot of properties in _get_property_list

https://user-images.githubusercontent.com/7025991/145495783-4c382600-3b1e-4122-9ecb-8587e3e47768.mp4

[Zylann]

I believe I'm still hitting that problem in master 28174d531b7128f0281fc2b88da2f4962fd3513e, I am unable to use some editor plugins after porting them to Godot4 because of this.

GetPropertyList.zip

[rosshadden]

I believe this may be related to #54410. The custom properties do actually show up (for me and those in that issue) if they are in a category. But otherwise they do not.