Allow exporting custom Resource variables from GDscript

[JFonS]

This PR is based on the work by Will Nations (@willnationsdev) in
https://github.com/willnationsdev/godot/commits/gdres-3.2

It includes some extra changes to make it more robust to cyclic script
references and a minor bugfix. These changes were sponsored by IMVU.

In order to export a custom Resource:

1. Create a script that extends Resource (or any other Resource type) and give it a class_name
   

2. You are done! You can now export variables using the class name as a hint.
   
   

3. Inheritance works too :)
   
   

Thanks again to @willnationsdev for the awesome work!

[JFonS]

@willnationsdev It would be nice if you could review this PR, I didn't make that many changes but I'd rather be safe :)

[YuriSizov]

I don't see this implementation as user friendly.

• Why is it so explicit? Why do you have to name it twice, using the class_name directive and the argument of add_custom_type?
• Why is there an editor plugin at all for something that should just work outright when you extend the Resource class? Are we expecting all users to create editor plugins now to be able to export their Resource-based properties?

[willnationsdev]

@JFonS

1. I'm not seeing where in your changes the use of EditorPlugin::add_custom_type suddenly becomes a mandatory requirement. The only reference to CustomType or custom_type I saw in your changes was updating the editor popup menus in the Inspector to take into account the custom type's inheritance more accurately (which makes sense). Did you just make a mistake in detailing the usage of your changes? Or is there something I missed? Ideally, we want script classes to have absolutely nothing to do with custom types.
2. Ultimately, I'd like to see Custom Types be completely removed in the upcoming 4.0 release since it's a chance at breaking compatibility. Unfortunately, the only way to successfully do that is to add script class support for C# which I've had trouble doing successfully in my local testing (but others have succeeded, so I'm not sure if it's just something I'm doing wrong somehow). See Add cross-language script class support and expose the ScriptServer singleton #40147 for more details. It doesn't necessarily affect anything for this PR, but just know that the Custom Type changes may end up being a moot point anyway.

It includes some extra changes to make it more robust to cyclic script
references and a minor bugfix. These changes were sponsored by IMVU.

Maybe you could just outline what problems you specifically wanted to fix, and I can work together with you to update my own PR to properly handle those cases? I simply haven't had a chance to mess with mine recently, so it's outdated since GDScript 2.0.

Edit: Also, gdres-3.2 is a branch I have slightly maintained specifically for adding the features to the older 3.2 codebase. It may be missing various considerations in the gdres branch which I have been trying to keep up-to-date with the latest master when I can.

[willnationsdev]

I have some confusion about these lines.

1. If member.data_type.native_type is a StringName containing the native class type, then why would it ever become a script class name? I'm not sure of what the intent here is. I was looking in the source for where native_type gets assigned a value, but I couldn't seem to find anything in gdscript_parser.h/.cpp, so I'm not sure how a script class name is supposed to end up there. I did see that parse_class_name() directly assigns the parsed identifier to current_class->identifier, so that is likely what you would want to pass to the ScriptServer, unless I'm mistaken.

2. Based on the above point, I think you should gather the class_name variable's value from something different. As far as I can tell with how it is used, native_type is supposed to be an actual C++ engine class.

3. I don't think you can simply check if the native type of the class is "Resource". After all, someone might choose to extend a different class other than Resource and make a script class out of that type. So, you should first grab the native type, and then compare it to the ClassDB in full.

    if (ClassDB::is_parent_class(native_type, "Resource")) { ... }
   

[JFonS]

1. In 3.2 stable the parser sets the native_type to the identifier's string here:
   https://github.com/godotengine/godot/blob/3.2/modules/gdscript/gdscript_parser.cpp#L5723-L5731

I agree it doesn't make much sense semantically, but I just took the information I had available, I didn't change any of the type parsing code.

3. Totally agree, I will update the PR soon.

[willnationsdev]

1. Wouldn't this check need to be made the other way around? Here, you are checking to see if the container is a script and the expression is a native class, e.g. var my_node: MyNode = Node.new(). However, the scripted type would have to extend the native type, so you would be assigning a base type to a derived type member. But that would break polymorphism, allowing you to assign incompatible values to variables.

2. Even if you move it, you still need to get what the base type is and then query the ClassDB for inheritance with is_parent_class() rather than making a direct comparison to the base type. After all, the script could extend a class deriving the member's expected type, e.g.

    var a_control: Control = MyTextureRect.new()
   

   where MyTextureRect is a script extending TextureRect rather than Control.

[JFonS]

Yes, this code doesn't seem to be right... but the whole type checking in 3.2 GDscript is a big mess. The type checking goes through so many code paths it's hard to keep track of everything.

I did some tests and my changes don't really affect any of the cases you mentioned (I still think my code is incorrect, but it doesn't really make things worse in these cases). Comparing This PR and 3.2.2 stable I get the exact same behavior in this test script:

So... I'm not really sure what is the best course of action here. I hope with the cleanup of GDscript 2.0 we get a type checker that is much easier to maintain because the 3.2 one is a bit of a nightmare.

[JFonS]

@willnationsdev Sorry, I did this changes a while back, so I kinda forgot how everything worked, I refreshed my mind now.

I double-checked and you are completely right, there is no need to add custom types through a plugin. I just did that because otherwise the Resource icons on the inspector menu didn't work properly, so it's just a minor issue.

Also, I am not a fan of custom types either, so I would also get rid of them if possible for 4.0

In general the only major problem I found was that the changes to the GDscript parser would load every script that was used as a class_name identifier (i.e. actually calling ResourceLoader::load() on them) creating a lot of dependency cycles that were not present before. My solution was to get rid of the changes to GDScriptParser::_parse_type() and instead grab the latest code from 3.2 (I now realized your branch was not up to date, sorry for the confusion). I left more details in your review comments.

Finally, if you want to continue this discussion and development on your own PR I'm all for it. This was my way to upstream the changes but you did the bulk of the work, I didn't mean to steal that from you :)

[Arrow-x]

will this be picked for 3.2.4? it should be ... @willnationsdev seems to update his own recently as well, the proposal for this feature is one the highest liked

[willnationsdev]

@JFonS I'm fine with you setting this up for upstream deployment. I have made some additional changes to my PR though, to fix a few things:

• usage of get_global_class_base / get_global_class_native_base (since the _native_base one was added later and _base used to just be the native base).
• add stuff for supporting drag-and-drop in EditorPropertyResource (don't think it was there before).

Feel free to consult the updated changes in my gdres_gd2 branch (NOT the gdres branch used for my pull request). I haven't actually updated the pull request directly yet because none of the GDScript parser changes can be done until more progress is made on GDScript 2.0.

[JFonS]

@willnationsdev awesome! I will check it out.

[Shadowblitz16]

so does this allow for something like so?

export(MyCustomResource) var whatever

or do we need to do it like so?

export var whatever : MyCustomResource

[jonbonazza]

Any update here? I'd LOVE to see this for 3.2.4! @akien-mga @Calinou @willnationsdev @vnen

[willnationsdev]

@jonbonazza Well, before anything else, commits must be fixed to not have conflicts. Aside from that, it might just be a question of when it comes up in a PR meeting, though I don't see the pr meeting label on this Issue, so idk.

[jonbonazza]

@JFonS have all of @willnationsdev concerns been addressed or are there any outstanding? Trying to push to see if this can be released in 3.2.4

[willnationsdev]

@jonbonazza I'll try to take another look at the PR tonight to evaluate the changes.

[JFonS]

Sorry, I've been focusing work on the CPU lightmapper which I would like to merge before 3.2.4 too.

@willnationsdev If you can allocate some time to work on this it would be great, otherwise I will do it eventually :)

[jonbonazza]

@JFonS what work do you know that is left to do? If I can find some time this week, I can help try to finish it.

[JFonS]

@jonbonazza If I remember correctly, everything was already working. All that's left to do is port the improvements that @willnationsdev did to his version of this PR (gdres_gd2 branch in his fork).

[vitorbalbio]

This is a must have improvement in UI/UX for Resources in Godot. Thanks all devs involved! 🥳🎉
Any chance the same support in C# soon ?

[willnationsdev]

@vitorbalbio maybe in 4.0, if C# gets script class support. WIP draft PR for it already exists.

[Zylann]

Just hit this problem, and I wonder, is class_name mandatory for this to work? None of my plugin uses class_name so far, and I was already able define custom types through add_custom_type, just the inspector is not able to recognize them.
Also I use the hint_string of _get_property_list to do this, instead of export.

[willnationsdev]

@Zylann defining script classes instead of custom types is required. Custom types do not actually exist at runtime - they are merely an editor tool to auto-add a script to a node or resource upon instantiation from the Editor GUI. Without the script class info in the project.godot file, there is nothing mapping a name to a script filepath; the ScriptServer never deserializes the information and makes it available to scripting languages. The intent is actually to add script class support for all languages in 4.0 and completely deprecate and remove custom types.

[willnationsdev]

@JFonS Do you have a sample test project you use to confirm that the features are working? I've developed a gdres-all-3.2 branch, built on top of my P22/script-classes-3.2 branch, that has script class support for all languages and enables you to define and export resources for all scripting languages from any scripting language. It would presumably replace this PR for 3.2, but I don't quite know if I'm missing anything from changes you made to my code. I would just want to make sure that I'm guaranteeing I don't lose any of your work in my own updated PR.

[JFonS]

Sorry for the delayed response, back from holidays now :)

Before the holiday break, we found some more issues in the GDscript parser. Specifically the fact that the parser was loading (and parsing IIRC) the scripts of the custom types used as export hints. This was causing trouble in exported projects the original .gd file was not present (it was a compiled .gdc file) and the parser didn't take this into account.

@RandomShaper did a clean-up of the parsing code and added a parsing mode that, instead of loading the exported script types, keeps the class names in the parser tokens. This still allows for checking if the used type is a Resource (using ClassDB) but gets rid of all the cyclic reference and compiled scripts shenanigans.
(please correct me if I'm wrong @RandomShaper)

@willnationsdev I can dig up the patch for those changes, but at this point it would be a patch on top of a patch on top of a patch. So it might be easier just to get the general idea of the change and apply it manually on your branch.

[RandomShaper]

Yes, I extended the boolean that tells whether to parse to get a constant to a three modes enum: constant, non-constant (both work as the old boolean), and a new constant-but-keep-classnames-as-identifiers. That one avoids having to lookup the classname from the script object when dealing with custom resource types.

[willnationsdev]

@JFonS Yeah, I'd really like a copy of that patch, just so I can review the changes and compare it with what I have. The strategy I was previously using to handle the static type hint parsing wasn't working well, so I switched over to your technique, and so far everything has been fine in my demo project.

@RandomShaper Where is this "extended the boolean that tells whether to parse to get a constant"?

Could both of you provide an example of what sort of exported project script code caused your issue? Cause I just tried it with mine and it worked just fine.

[RandomShaper]

@willnationsdev, my patch was done to some fork of Godot, which is older than 3.2. I've gotten a diff of the patch, here: https://pastebin.com/GWBLRtgK

It's quite hard to merge now because of conflicts. Maybe it's better or worse than the current approach.

Regarding the error itself, I'm making an effort to remember. I think that get_global_class_name() was trying to load the .gd file, instead of the .gdc. That could have been solved, but I decided to take a different route which avoided having to resolve class name from path in the first place.

[Calinou]

Superseded by #44879.