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

Add script class categories to EditorInspector. #32428

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Sep 29, 2019

Closes godotengine/godot-proposals#109.

There isn't an icon showing beside DerivedB, but that is a bug that is unrelated to the mentioned Issue.

inspector_script_class_hierarchy

extends Node
class_name DerivedA
export var health := 0
export var can_heal := true

extends DerivedA
class_name DerivedB
export var power := 0
export var duration := 0.0
export var superpower := @""

Here is an updated version that does away with the "Script Variables" section entirely and which always uses the script class name (if it's available) or the file name (capitalized, without spaces) so that they usually appear as a valid identifier (unless it's a filename that starts with a number or something).

In the example below, I have a non-script-class script in between two script classes:

inspector_script_class_hierarchy_with_files

extends Node
class_name DerivedA
export var health := 0
export var can_heal := true

extends DerivedA
export var power := 0
export var duration := 0.0
export var superpower := @""

extends "res://derived_b.gd"
class_name DerivedC
export var experience := 0
export var level := 0

Okay, here is the same code, but now I've changed it to show the file name and I went ahead and updated the icon rendering code so that it will fetch the most recent script class icon / base type icon PER individual script in the hierarchy.

Note that the code now anticipates that if it is a category, and the category's name does not match an existing ClassDB or ScriptServer record, AND the hint_string contains a valid file path, then it will attempt to load that file path as a script so that it can load the script and fetch the appropriate icon from its information.

inspector_script_class_hierarchy_with_files_and_icons

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Sep 29, 2019

Oh wait, I just realized that the current implementation omits the Script Variables section, even if the user is currently working on an unnamed script. That hasn't been accounted for.

Also, we need to decide on what to do about properties that belong to intermediate scripts. Please see the Issue in the post for reference.

Edit: Updated it. Went with the second option I suggested in the issue.

@groud
Copy link
Member

groud commented Sep 29, 2019

or the file name (capitalized, without spaces)

I think that if there are no class_name in the script, we should not make the user believe there is one. So I think that in this situation the file name should be used as the section header.

@willnationsdev
Copy link
Contributor Author

@groud Updated to show file names AND include proper icons.

@akien-mga akien-mga added this to the 4.0 milestone Sep 30, 2019
@akien-mga akien-mga requested a review from reduz February 11, 2020 08:27
@willnationsdev willnationsdev force-pushed the script-var-order branch 2 times, most recently from 1dc0f25 to 87193d5 Compare May 14, 2020 07:19
@willnationsdev
Copy link
Contributor Author

Rebased

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR and it seems to work properly, really glad script classes are getting improvements 🎉

editor/editor_inspector.cpp Show resolved Hide resolved
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _update_script_class_properties function look a little bit complex for what it does, but well, there might not be better way to implement it.
In any case, since it's inside a single function, it's likely ok to merge anyway.

@willnationsdev
Copy link
Contributor Author

@aaronfranke @groud I realized that code was re-fetching the name/icon-path from the script rather than from the cached data in the EditorData. I've modified two more lines of code to pull from there instead (an optimization).

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jul 1, 2020

@groud The only way that I can think of that you could improve the performance of it would be if the generic Script interface was self-aware that scripts can support inheritance when it comes to generating its property list, and so you would have the option of allowing the scripting languages to return multiple lists of properties, broken down by class. That way, my code wouldn't have to systematically get each of the base scripts' properties and cross-check them with which properties have already been added to the main property list. But to do that, you'd need to add a new method to core Script (or change the existing get_script_property_list() function) and then re-implement the logic for each specific scripting language to actually break down their content on a per-class basis. It'd be a lot more complicated overall. Rather than just having one small, somewhat slower and more complicated function. I could certainly try to alter the implementation to go that route, if people would prefer it.

editor/editor_inspector.cpp Show resolved Hide resolved
editor/editor_inspector.cpp Show resolved Hide resolved
editor/editor_inspector.cpp Show resolved Hide resolved
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, it even works with multiple inherited script classes!
Screenshot from 2020-07-01 15-46-53

Two notes:

  • The icon doesn't show up for nodes added with add_custom_type, but AFAIK this is going to be removed eventually in favor of the script class system, so this is fine.

  • While testing, I noticed that changing the values of a property of an inherited script in the inspector doesn't call the getters and setters for that property (ex: changing Spatial Position in the above image does not work, but it works when not inherited). However, this bug also exists on master, so it's not a bug with this PR. I'll open a separate bug report for this.

@willnationsdev
Copy link
Contributor Author

@aaronfranke Also, as you can see in the OP, it even works if you have a script, then inherit a non-script-class, and then have a script class inherit that script. It works quite well.

@akien-mga akien-mga merged commit 938e2c5 into godotengine:master Jul 2, 2020
@akien-mga
Copy link
Member

Thanks!

@willnationsdev willnationsdev deleted the script-var-order branch July 2, 2020 14:08
@@ -3661,8 +3661,6 @@ Ref<Texture2D> EditorNode::get_class_icon(const String &p_class, const String &p
if (icon.is_null()) {
icon = gui_base->get_theme_icon(ScriptServer::get_global_class_base(name), "EditorIcons");
}

return icon;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is removed, what is the whole point of this if block?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, lines 3661-3663 are useless code now.

@groud
Copy link
Member

groud commented Jul 6, 2020

@willnationsdev Just a question, may #40161 allow you to simplify the implementation ?

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jul 6, 2020

@groud No since ClassDB can only be aware of C++ level engine code. I'd need something like get_individual_script_property_list() which would return the list of properties associated with a single Script and not the Scripts that it inherits.

@Jummit
Copy link
Contributor

Jummit commented Oct 28, 2020

Is there any chance for this to be cherry-picked for 3.2? Only if it's trivial of course.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 29, 2020

Everything I did here should be compatible with 3.2.x, afaik. So, it should work out of the box for the most part if cherry-picked (very few, if any, conflicts).

@ghost
Copy link

ghost commented Oct 29, 2020

Not sure how up to date the commit I used is, but I can at least report that I've been using this as a cherry-pick in a 3.2 custom build for some time without issue.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 29, 2020
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 11, 2020
@capttheo
Copy link

capttheo commented Nov 13, 2020

@akien-mga in 3.2, this commit seems to break editor plugin tools, i.e. Zylann's HTerrain now missing vital properties in UI
This is probably due to usage of other flags such as PROPERTY_USAGE_EDITOR, PROPERTY_USAGE_STORAGE
(should I open an issue?)

hterr

@Zylann
Copy link
Contributor

Zylann commented Nov 13, 2020

I posted an issue #43491

@akien-mga
Copy link
Member

3.2 cherry-pick reverted with 755ee76 until #43491 is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have class_name classes properly show in the inspector
8 participants