Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve a property list's mapping to internal organization of scripts for better Inspector UX. #2803

Closed
willnationsdev opened this issue May 31, 2021 · 5 comments

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented May 31, 2021

Describe the project you are working on

Wanted to add categories for each named script class in the Inspector.

Describe the problem or limitation you are having in your project

Was introduced by godotengine/godot#32428, but had to be reverted later. I had attempted to use Script.get_script_property_list() to isolate only the properties which belonged to each script in the inheritance hierarchy and insert category faux-properties accordingly.

The only reason I resorted to Script.get_script_property_list() was because calling get_property_list() returned all properties and I needed to know which ones were specifically coming from the attached Script. However, the Object/Script API makes no effort to understand how the given Script internally organizes itself relative to other scripts in the same language. That is, given a Base script and a Derived script that extends Base, Derived.get_script_property_list() will return properties that belong to both Base and Derived.

As a result, to differentiate which properties belong to which actual script files, one must get N lists (one for each class in the hierarchy) and iterate through, each time comparing names (at a minimum, N names, assuming each class has at least one property) to map the properties to each Script accordingly. This is unnecessary, tedious, and slow.

In addition, this ignores all dynamic properties from instance-specific _get_property_list() calls so they don't end up included in the generated set of properties. That's why the original PR was reverted in the first place.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Amend the scripting language's implementation of get_property_list() so that the instance-specific return value already incorporates PROPERTY_HINT_CATEGORY faux properties to separate each sub-collection of properties from each script as the ScriptInstance generates the list. This allows each language to dictate how its internal organization translates to a flat generated list of properties with CATEGORY subsections without the core Object/Script API having to be involved. Then the Inspector simply has to use those categories as they appear rather than adding the Script Variables category.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  1. Remove the inclusion of Script Variables category in Object::get_property_list(...).
  2. Update each language to parse its own inheritance hierarchy, if any, and establish category properties each step of the way.
    • If the script is a global script class, then that name is used. Otherwise, the file name is used.
    • Duplicate category names shouldn't matter since it is just a generated list. Each list need not correspond 1-to-1 with the actual members of the ScriptInstance.
  3. If duplicate category names exist in the final output, the Inspector can force subsequent names to adopt a numeric suffix (although, not sure if that would even be necessary).

If this enhancement will not be used often, can it be worked around with a few lines of script?

While categories can be added via scripts, it is quite verbose to do so. There are proposals to make it more concise, e.g. #1255, but that would still require users to add an @category annotation to every property just to make it part of the correct script. This is too cumbersome to expect users to deal with.

Is there a reason why this should be core and not an add-on in the asset library?

It's a matter of good UX that properties belonging to each class are displayed as such, so it really should be handled internally.

@willnationsdev
Copy link
Contributor Author

Any PR for this would be related, to some extent, to godotengine/godot#44879 and godotengine/godot#48201 since the generated names for GDScript, C#, VisualScript, NativeScript, and PluginScript would need to include global script class information if present. Furthermore, it would be a good idea to confine the name generation logic to a single method in the ScriptServer.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 9, 2021

I recently built Godot 4 and noticed that the Inspector is actually already rendering the script classes' categories. I will have to look at the code and test it with script types other than GDScript, but it's promising that it already is handling inherited properties' differences automatically, unlike how things were in 3.x. Might not even need this proposal.

@raulsntos
Copy link
Member

As far as I know godotengine/godot#32428 was only reverted in 3.2, so it's still in master but it does only seem to work with GDScript.

@raulsntos
Copy link
Member

Also implemented for C# in godotengine/godot#63712.

Since this already seems to be in master, should the proposal be closed as implemented? The related proposal #1255 has also been implemented.

@YuriSizov
Copy link
Contributor

Yes, I think we're in a good place regarding categories based on scripts right now in master, and it's implemented on the language level, not on the inspector level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants