Skip to content

Add GDScript resource export.#62411

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
willnationsdev:gdres-gdscript
Sep 18, 2022
Merged

Add GDScript resource export.#62411
akien-mga merged 1 commit into
godotengine:masterfrom
willnationsdev:gdres-gdscript

Conversation

@willnationsdev

@willnationsdev willnationsdev commented Jun 25, 2022

Copy link
Copy Markdown
Contributor

Adds support for GDScript to be able to export user-defined resource types.

Related Issues:

Related PRs:

Sibling PRs:

Considerations:

  • GDScript does not yet support exports for non-GDScript languages because there is no @export("<typename>") syntax yet.

@PoisonousGame

Copy link
Copy Markdown

image
image
image
image
image
image

@willnationsdev

Copy link
Copy Markdown
Contributor Author

If you were attempting to test out just this pull request, then the way to do that would more or less be to confirm whether the GDScript compiler gave you some sort of error when attempting to export a user-defined resource type (whether through static typing or not). I'm not really sure you tested this out correctly.

@willnationsdev

Copy link
Copy Markdown
Contributor Author

@vnen I was just getting C# working and made an attempt to export C# types in a GDScript property when I ran into some trouble. I have no way to export a global C# resource property from a GDScript node or resource. I would like clarification on which syntax below should be used. Neither of them are currently supported.

  1. There is no way to have a dynamically typed variable that supports exporting resources, of any kind. So this legacy syntax that used to work in 3.x does not work now.

    class_name MyNode
    extends Node
    
    # Godot used to let you have a dynamically typed variable
    # that would be presented in the Inspector as a global script class resource.
    # You could still assign alternative values within the script though, because it has no dynamic type.
    @export("MyResource") var my_data
  2. There is no way to statically export global script classes that are not GDScript types. This is because during GDScriptAnalyzer::resolve_datatype(), it detects that the variable is using a global script class, but it then parses that file as a GDScript even if it doesn't have a .gd file extension. It then gets a parse failed error during get_parser_for() and raise_status(), resulting in push_error(vformat(R"(Could not parse global class "%s" from "%s".)", ....); So, the following use case is not allowed either:

    class_name MyNode
    extends Node
    
    # `CSData` would be some C# resource global script class.
    @export var cs_data: CSData = null

@willnationsdev willnationsdev force-pushed the gdres-gdscript branch 2 times, most recently from 41e3b8b to 1465bd3 Compare July 30, 2022 23:31
@willnationsdev

willnationsdev commented Jul 31, 2022

Copy link
Copy Markdown
Contributor Author

I have made an update to this branch that allows GDScript to use global script classes from other languages (NOT GDScript) for static typing. So you should now be able to add a scripting language using GDExtension or add global script classes to VisualScript, and then use those global script classes as static type hints. Then the GDScript language will accurately perform static type checks, provide autocompletion/intellisense, etc. Exporting ones extending Resource also works (option 2 above in my previous comment).

@Zireael07

Copy link
Copy Markdown
Contributor

Is this going to make it intp 4.0?

@willnationsdev

Copy link
Copy Markdown
Contributor Author

@Zireael07 Yeah, it should, from what I've heard. There are just some hangups that have to get resolved before it and the sibling PRs can be merged.

@reduz reduz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks ok to me on the exported part, the analyzer part I have no idea, would appreciate more eyes there.

@reduz

reduz commented Sep 16, 2022

Copy link
Copy Markdown
Member

@willnationsdev Thanks hugely for the work in this area. We wanted to discuss and implement a cleaner API for you to rely on that would allow better information retrieval regarding to global classes, which is why we did not want to merge this right away.

Unfortunately, we did not have the time and resources to work on this, and given the demand there is for this feature, and given these PRs are already very good, it will do as-is and we can discuss cleaning up the global class registration in 4.1.

Again, thanks for working on this and apologies for the delay in getting this merged, but circumstances beyond our control made properly reviewing and merging this difficult.

@akien-mga

akien-mga commented Sep 16, 2022

Copy link
Copy Markdown
Member

Tested this PR together with #62413, it seems to work well!

One needs to reload the project after defining a new custom resource with class_name but AFAIK that's been the problem with class_name from the beginning.

@export var some_res: MyCustomRes

works fine to export it from GDScript.

It's properly filtered in the Inspector, with multi-level inheritance taken into account (MyCustomSubRes extends MyCustomRes):
image
image

Drag and drop of saved custom resources works from the FileSystem dock:
image

Just some testing fun :)

image
(That's an existing issue: #57286.)

I didn't test #62417 for now because it looks way hackier and far reaching into core internals and multiple editor components. We do want it to work in the QuickOpen eventually but I suggest focusing on getting #62411 and #62413 merged first, which IMO implement the missing feature in a quite satisfactory way already.

Comment thread modules/gdscript/gdscript_parser.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!ClassDB::is_parent_class(export_type.native_type, Resource::get_class_static())) {
if (!ClassDB::is_parent_class(export_type.native_type, SNAME("Resource"))) {

For consistency with the existing code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can definitely make this change, but it also kinda confuses me why this would be preferable implementation-wise (if addressed, it'd be in a totally different proposal/PR probably).

I get that using the cached StaticCString is better here, but if that's the case, why not arrange things so that get_class() and get_class_static() return a StringName that is likewise constructed from _scs_create(...), that way you can get performance benefits of SNAME(...) built into those methods? That would then allow you to clean up all of these stringly-typed class name references throughout the code.

Comment thread modules/gdscript/gdscript_parser.cpp Outdated
Comment thread modules/gdscript/gdscript_parser.cpp Outdated
Comment thread modules/gdscript/gdscript_analyzer.cpp Outdated

@YuriSizov YuriSizov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just have nitpicks, otherwise looks fine + I trust Akien's testing.

@willnationsdev

Copy link
Copy Markdown
Contributor Author

@akien-mga

One needs to reload the project after defining a new custom resource with class_name but AFAIK that's been the problem with class_name from the beginning.

Was it? I didn't think that was the case, even in Godot 3.x. Either way, that was already an issue in 4.0 before I implemented all of my stuff. I'm not entirely sure why it's happening, but I haven't yet had time to diagnose the cause. Perhaps something related to EditorData caching information about script class inheritance.

@YuriSizov

YuriSizov commented Sep 16, 2022

Copy link
Copy Markdown
Contributor

Was it? I didn't think that was the case, even in Godot 3.x.

Yes, there are some things that don't update and recognize class_name changes without an editor restart, so we kind of roll with it for now.

@willnationsdev

Copy link
Copy Markdown
Contributor Author

I've made the requested adjustments. Should be good to go for merge at this point afaik.

@akien-mga akien-mga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, and seems to work well.

@vnen might still want to give the analyzer changes a qualified review but that can be done after merge when he's available.

@akien-mga akien-mga merged commit 0aea7f2 into godotengine:master Sep 18, 2022
@akien-mga

Copy link
Copy Markdown
Member

Thanks a ton for your work and your patience with this series of PRs - you've officially solved the most upvoted Godot proposal ever 🎉

image

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants