Nodes added via EditorPlugin shouldn't have properties under Script Variables

[willnationsdev]

If a node's script is added to the engine via EditorPlugin::add_custom_type, the exported properties of that node should not be listed in the Inspector under Script Variables. Instead, it should be labeled whatever the name of the new node type is. For example, with a new node called MyNode inheriting Control, I might have in the Inspector...

MyNode
* My Var
Control
* etc.

Then, if I add a script to that node, I should see...

Script Variables
* A Script Var
MyNode
* My Var
Control
* etc.

[vnen]

Not sure if that is useful since a custom type will already have a script attached and you can't attach another one.

#4378 would solve this though.

[willnationsdev]

? No, I just did it in my own project.

1. Use an EditorPlugin to add_custom_type a "MyNode" type at "res://addons/my_node/MyNode.gd".
2. Create a new scene with a root node that is of type MyNode.
3. Attach a script that extends "res://addons/my_node/MyNode.gd".

^ this totally compiles and runs just fine. It is possible to have a custom type's variables AND a script's variables both present on a node. But currently the custom type's variables are mixed in with the script's.

[AllanDaemon]

A good example for this is this custom node in the Godot Next plugin:
https://github.com/willnationsdev/godot-next/blob/master/addons/godot-next/gui/navigation/ControlSwitcher/ControlSwitcher.gd

The ControlSwitcher extends the BaseSwitcher that also export variables.

I found this discussion before to post my proposal, that is displaying the nome name instead of "Scrips variables" always when it's possible.

The current behaviour seems to me non-core nodes are first citizen class nodes and the others second citizen class nodes. So, if I create a Scene+GDScript called Enemy, and create many instances in another scene, I think it's more informative to see "Enemy" instead "Script variables". In the same way, if I extend the Enemy as BigBoss, I would like to see the BigBoss category for exported variables followed by Enemy class with the respective exported variables, just like the internal Godot classes. And I think this should be the behaviour even for nodes not meant for plugins (ie, that extends the EditorPluging class and register it).

[willnationsdev]

@AllanDaemon I've discovered since originally posting this issue that even the custom types are themselves really just scripts on top of nodes. They aren't actually "custom types" at all.

Not sure of all of the details, but it appears that EditorPlugin::add_custom_type merely does the following:

• adds a shortcut to creating the base node with the associated script already attached to the "add a node" window. This is the reason that the nodes are shown to inherit one of the 1st class nodes, and also why, at this point in time, you don't have inheritance hierarchies shown for custom types in this window. So, adding the custom type is merely removing the manual step of adding the script.

• An editor icon is bound into the __meta (or is it _meta_? can't recall...) property on the node, allowing the editor to recognize and display the custom image in the node hierarchy dock.

...and that's pretty much it. It's actually REALLY lax as far as interaction goes, and I can kinda get why, based on that reality, there has been some confusion/debate between users and the core devs regarding how to handle and interpret custom types, and really any type that derives from a scripted type.

But as vnen said, the issue he mentioned would help to alleviate this problem if people could customize what category is associated with the variables. It just sucks that, for now, you can't create categories and sub-categories for properties (like if I had category each for BaseSwitcher/ControlSwitcher, and then sub-categories for sets of properties that were related).

Glad to hear someone is actually looking at that repo though. It's still pretty barren at the moment. XD

[AllanDaemon]

As the time to change this in the Godot 3 is running out, I've made an implementation to test it. It's in my fork:
https://github.com/AllanDaemon/godot/tree/propname

It basically uses the name of the node instance instead "Script Variables", if the object extends Node.

This is an example:

This is my first time with Godot code and I still getting familiar with it, so there is room for improvements.
I don't know how to proceed now. Should I open a PR and let the ones who decide discuss it there or just keep the discussion here and PR if approved?

Glad to hear someone is actually looking at that repo though. It's still pretty barren at the moment. XD

I actually saw this repo is your @willnationsdev after you said it in the last post.

[willnationsdev]

Judging from the code changes you made, it looks like all you have to do is a push into the list a PROPERTY_USAGE_CATEGORY PropertyInfo object with the proper string name parameter to select the name.

If I have 3 levels of hierarchy of script:

# base_script.gd
export(bool) var base_bool = true

# DerivedScript.gd
inherits "base_script.gd"
export(bool) var DerivedBool = true

# SUPER_DERIVED_SCRIPT.gd
inherits "DerivedScript.gd"
export(bool) var SUPER_DERIVED_BOOL = true

If you then selected a node with the SUPER_DERIVED_SCRIPT.gd attached, I would expect to see in the Inspector...

Category block with text "Super Derived Script"
Check button with label "Super Derived Bool"
Category block with text "Derived Script"
Check button with label "Derived Bool"
Category block with text "Base Script"
Check button with label "Base Bool"

There is supposed to be a map SOMEWHERE (the ClassDB maybe?) that tracks the inheritance hierarchies between scripts by their path(?) or name or something. You would need to use that map in order to look up the additional labels that would need to be added. I'm not sure if it would even be possible to isolate which properties are associated with each layer of the hierarchy though, cause you'd have to know at what point in the list to insert the new category block.

I know String has a function to convert snake case to title case, but you'll have to do some extra magic to accommodate PascalCase names when people wanna do that.

P.S. Yes, I am the maker of that repository, hehe. It's pretty early on in its lifetime, so I don't expect it to be expanded all that much until after 3.0 and people start making all sorts of things like character controllers, basic camera tools, etc. Godot-NExt is meant to just consolidate stuff that doesn't really fit into a repository all by its lonesome.

[willnationsdev]

@AllanDaemon Looking at the source code, it seems like every Script object has a get_base_script() function that returns a Ref<GDScript> object or, if it's null, a Ref<Script>(). You can then use that to go up the hierarchy. As for the name, you'll probably need to use the get_path() method to retrieve the full file path of the resource, slice out just the last portion, remove the file extension(s), and then use the String's capitalize() method to get it into a readable format. I've already submitted a PR to improve the capitalize function (#12927).

Gotta thank karroffel for giving me a lead on where to get the base script (don't wanna ping him though).

[bojidar-bg]

In 2.x, I used PROPERTY_USAGE_CATEGORY to do it:

func _get_property_list():
	return [
		{usage = PROPERTY_USAGE_CATEGORY, type = TYPE_NIL, name = "CircularContainer"},
		#...
	]

I guess we might automatically create a category for each script, based on its filename. Seems like a simpler thing to do than implementing categories ( #4378 ), in any case...

[willnationsdev]

@bojidar-bg do you know if there is an easy way to check whether a script has an exported property, maybe in the ClassDB?

[bojidar-bg]

You cannot look a script up in ClassDB, other than that, you might instance the script and call has_property on the instance...

[willnationsdev]

@AllanDaemon Looking at the source code, it seems like every Script object has a get_base_script() function that returns a Ref<GDScript> object or, if it's null, a Ref<Script>(). You can then use that to go up the hierarchy. As for the name, you'll probably need to use the get_path() method to retrieve the full file path of the resource, slice out just the last portion, remove the file extension(s), and then use the String's capitalize() method to get it into a readable format. I've already submitted a PR to improve the capitalize function (#12927).

Gotta thank karroffel for giving me a lead on where to get the base script (don't wanna ping him though).

[bojidar-bg]

In 2.x, I used PROPERTY_USAGE_CATEGORY to do it:

func _get_property_list():
	return [
		{usage = PROPERTY_USAGE_CATEGORY, type = TYPE_NIL, name = "CircularContainer"},
		#...
	]

I guess we might automatically create a category for each script, based on its filename. Seems like a simpler thing to do than implementing categories ( #4378 ), in any case...

[willnationsdev]

@bojidar-bg do you know if there is an easy way to check whether a script has an exported property, maybe in the ClassDB?

[bojidar-bg]

You cannot look a script up in ClassDB, other than that, you might instance the script and call has_property on the instance...

[willnationsdev]

@bojidar-bg Are you working on this now then, based on what I read in that PR? Cause I was gonna start trying to do it.

[bojidar-bg]

@willnationsdev I might, depends on what the community votes on.

[willnationsdev]

btw, I've been working on this since yesterday. I'm adding get_property_count and get_script_property_count methods for all ScriptInstance and Script types in the engine so that we can use it to efficiently jump along the "Script Variables" property list for the current instance and insert the appropriate CATEGORY PropertyInfos.

[willnationsdev]

Well, I wasted a whole bunch of time on this.

Apparently, manually inserting multiple PROPERTY_USAGE_CATEGORY PropertyInfo elements into the property list simply doesn't work(?). Only the first insertion will actually make it through. Just did the following...

//object.cpp, Object::get_property_list
if (script_instance && p_reversed) {

    print_line("TEST");
    List<PropertyInfo> plist;
    script_instance->get_property_list(&plist);
    int i = 0;
    for (auto E = plist.front(); E; E = E->next()) {
        String title;
        if (i == 0) title = "TEST 0";
        if (i == 1) title = "TEST 1";
        if (i == 2) title = "TEST 2";
        if (i == 3) title = "TEST 3";
        print_line("Inserting before: " + E->get().name);
        plist.insert_before(E, PropertyInfo(Variant::NIL, title, PROPERTY_HINT_NONE, String(), PROPERTY_USAGE_CATEGORY));
        i++;
    }
    for (auto E = plist.front(); E; E = E->next()) {
        print_line(E->get().name);
    }
    print_line("END TEST");

Ended up with...

TEST
Inserting before: base_bool
Inserting before: derived_bool
Inserting before: derived_bool2
TEST 0
base_bool
derived_bool
derived_bool2
END TEST

[Calinou]

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!