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

Export support for gdscript resource types #26162

Closed

Conversation

vixelz
Copy link
Contributor

@vixelz vixelz commented Feb 22, 2019

Allow GDScript export statements to export global class types.

  • Updates gdscriptparser to accept identifiers as export types, and resolve them at around the same time as export-type / type-hint checking.
  • Adds a method to editordata to be able to check if one class is equal to, or a parent of another, regardless of whether the class being checked is a native class or a script
  • Updated editorpropertyresource to handle the above changes:

globalclassexport

@vixelz vixelz force-pushed the export-gdscript-resource-types branch 3 times, most recently from aca178b to 4e62432 Compare February 22, 2019 17:06
@Zylann
Copy link
Contributor

Zylann commented Feb 22, 2019

Note that if the script you export extends a node type, it will leak unless you add it to the tree in code or free it.

@Zireael07
Copy link
Contributor

@Zylann I believe that bit it worth adding to the docs?

@vixelz
Copy link
Contributor Author

vixelz commented Feb 22, 2019

@Zylann if I'm understanding correctly, I think this shouldn't be an issue because the type resolution step should ensure that the exported type derives from Resource, either directly or through other global script classes.

@hpvb hpvb added this to the 3.2 milestone Feb 22, 2019
@vixelz vixelz force-pushed the export-gdscript-resource-types branch 2 times, most recently from 2d12065 to dc87d74 Compare February 23, 2019 09:57
@samgreen
Copy link
Contributor

This is wonderful

@vixelz vixelz force-pushed the export-gdscript-resource-types branch from dc87d74 to 450b34c Compare February 26, 2019 21:26
vixelz added 2 commits March 5, 2019 09:21
Much like how script defined global classes can be created from the
"New Resource..." button in the FileSystem panel, this allows the
creation of script defined resources to be embedded.
@vixelz vixelz force-pushed the export-gdscript-resource-types branch from 450b34c to 2b7b64f Compare March 5, 2019 09:56
@vixelz
Copy link
Contributor Author

vixelz commented Mar 11, 2019

This has broken and conflicts with other changes. I'm going to wait till 3.1 is branched and I'll fix it up for review in to 3.2

@akien-mga
Copy link
Member

This needs a rebase (and a review, ping @vnen @bojidar-bg).

@bojidar-bg
Copy link
Contributor

Solves same issue as #22660?

@akien-mga akien-mga removed the request for review from reduz April 30, 2019 12:44
@akien-mga
Copy link
Member

Still needs a rebase. Should be compared with #22660 which seems to address a similar feature, to see which implementation is the most interesting (if they do overlap). CC @willnationsdev

@willnationsdev
Copy link
Contributor

@akien-mga Mine stopped working quite a ways back and I never went back to fix it (and it was buggy to begin with), so I'd opt to go for this solution over the one I wrote.

Copy link
Contributor

@willnationsdev willnationsdev left a comment

Choose a reason for hiding this comment

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

@vixelz Out of curiosity, does the implementation work with resources declared as constants too and/or arrays of custom resources?

@@ -2754,6 +2770,20 @@ void EditorPropertyResource::set_use_sub_inspector(bool p_enable) {
use_sub_inspector = p_enable;
}

String EditorPropertyResource::_get_file_script_name_or_default(const RES &p_resource) const {
Ref<Script> rscript = p_resource->get_script();
if (rscript != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency/clarity, I would recommend making this be if (rscript.is_valid()) or something.

@xDGameStudios
Copy link
Contributor

Ohh I can’t wait for this to be rebased and merged! Please let it be soon! ;)

@ClementRivaille
Copy link

I've encountered an error while testing the branch. If I try to type an exported var, I get the error:

export hint type (GDScriptNativeClass) doesn't match the variable type ({Class})

This will appear on any type of export, whether they're custom, native, or even primitive.

# None of those work
export(Texture) var texture: Texture
export(int) var index: int
export(String) var text := "Hello"
export(Array, AudioStream) var streams: Array

The error can be avoided by not typing exported variables. This is not convenient though, as it prevents auto-completion, safe lines… Well, everything that comes with typed variables.

I see the branch needs to be rebased, so maybe this has already been fixed on master. I hope this feature will soon be included in a release, because it might become indispensable for plugins! :)

@akien-mga
Copy link
Member

We discussed this briefly on IRC with @vnen and @bojidar-bg, some comments:

16:05 <Akien> Next: #26162
16:05 <IssueBot> #26162: Export support for gdscript resource types | https://git.io/fhF7v
16:08 <bojidar_bg> I feel like reducing constants might be a better idea than checking for identifiers separatelly
16:09 <bojidar_bg> .. we are already reducing the constant though D:
16:12 <vnen> constant reduction does not always happen with current implementation
16:16 <bojidar_bg> it uses global script names exclusively, which might cause trouble later on
16:17 <vnen> why would it cause trouble?
16:17 <bojidar_bg> `const X = preload("res://X.gd)`, `export var x: X`
16:18 <bojidar_bg> also, if the global class names start getting resolved to a constant, it will break
16:19 <vnen> not sure if you can declare a constant if the name is already in the global scope
16:19 <vnen> in any case, the implementation is complex and may need to be reviewed properly (code for export vars is one of the most complex in the parser)
16:20 <vnen> and there's a valid issue raised in the last comment

As we're in feature freeze for 3.2 and this PR is not merge-ready for now, moving to the 4.0 milestone. It's a good feature proposal though, but some updates and a thorough review are needed.

@@ -582,6 +582,8 @@ class EditorPropertyResource : public EditorProperty {
void _button_input(const Ref<InputEvent> &p_event);
void _open_editor_pressed();
void _fold_other_editors(Object *p_self);

Copy link
Contributor

Choose a reason for hiding this comment

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

Just clean whitespaces in empty line to pass clang-format in Travis

@aaronfranke
Copy link
Member

@vixelz Is this still desired? If so, it needs to be rebased on the latest master branch. Additionally, the above feedback needs to be addressed.

If not, abandoned pull requests will be closed in the future as announced here.

@rcorre
Copy link
Contributor

rcorre commented May 13, 2020

@aaronfranke I'm still interested and happy to finish the work if @vixelz does not.

@GalaxyCr8r
Copy link

For what its worth, I am very much interested in this feature - working on a space station economy simulator and this would make the data-driven exports soooo much cleaner.

@willnationsdev
Copy link
Contributor

@aaronfranke @rcorre I'd thought that my PR was further along in this as it is derived from vixelz's initial work. I am planning to rebase and fixup that PR too. #32018

@aaronfranke
Copy link
Member

This PR has not received any new commits for over a year and is abandoned by the author, closing.

This PR can be salvaged, but it seems that @willnationsdev is already doing so.

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.