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 support for exporting resource script constants and script classes. #22641

Closed
willnationsdev opened this issue Oct 2, 2018 · 17 comments
Closed

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented Oct 2, 2018

Godot version:
3.1 Alpha 1

Issue description:
I would expect the following to work:

# my_resource.gd
extends Resource
class_name MyResource

# node.gd
extends Node
export(MyResource) var my_res # ERROR: Expected a constant expression

I can actually confirm it isn't a constant by creating a derived version of MyResource, class naming it as SuperResource, and then literally re-assigning it, which I really don't think I should be able to do:

extends Node
func _ready():
    MyResource = SuperResource
    print(MyResource.resource_path) # prints "res://super_resource.gd"

To ME, this seems like a bug that shouldn't be allowed, and which consequently would allow the export of the type name to work in 3.1.

It also seems like this just flat-out wasn't ever a feature, even in 3.0.

const MyResource = preload("res://my_resource.gd")
export(MyResource) var my_res # Error: Export hint not a resource type (even though it is!)

Steps to reproduce:
Recreate the code examples above for Resource-extending scripts.

3.0: create a constant and try to export it. Fails.
3.1: Same as 3.0 + create a class name and try to export it. Fails.

@williamd1k0
Copy link
Contributor

Related: #6763

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 2, 2018

Just taking a quick look, it seems to me like part of the solution might lie in going to _type_from_variant in gdscript_parser.cpp and changing the logic here to this:

if (scr.is_valid()) {
    String name = GDScriptLanguage::get_singleton()->get_global_class_name(scr->get_path());
    if (ScriptServer::is_global_class(name)) {
        result.is_constant = true;
    }
    ...
}

That would at least make accessing a variant from the global_map not result in an editable value.

@willnationsdev
Copy link
Contributor Author

There's probably also merit in simplifying the GDScriptLanguage's implementation of that method since we don't need to parse the ENTIRE file just to determine what it's class name is.

@akien-mga
Copy link
Member

CC @vnen

@vnen
Copy link
Member

vnen commented Oct 4, 2018

we don't need to parse the ENTIRE file just to determine what it's class name is.

It's not only about the class name. If you are accessing methods and members of a variable typed with a custom class, the parser checks if those are valid as well, and for that it needs to parse the whole script. For this case it would also need to know if it's a resource.

Now about the OP, the problem is making the de/serialization work properly, and expose this to the editor as well. The GDScript part should be only setting the proper hints for that system once it's in place.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 4, 2018

@vnen Ah, that makes more sense.

I don't know if that's a significant "problem" per se. In the PR I've linked that closes this, my tests show that I can safely serialize and deserialize both types of resource scripts, all from the Inspector. I can even have a script class's ScriptInstance as a sub resource within a preloaded constant resource script's ScriptInstance. Here's the test I have:

# my_resource.gd
extends Resource
export var health: int = 5
export(SubResource) sub_resource

# sub_resource.gd
extends Resource
class_name SubResource
export var count = 2

# node.gd
extends Node
const MyResource = preload("res://my_resource.gd")
export(MyResource) var my_res
func _ready():
    print(my_res.health)
    print(my_res.sub_resource.count)

If I try to add the MyResource or SubResource to the node's script properties in the Inspector, there's a bug with a massive list showing that doesn't display a scrollbar, so I can't access them, but if I create them as new resources in the Inspector first, save them (which works), and then drag n' drop them from the FileSystem dock to the Inspector's script properties on the node script, THEN the instances show up properly. If I then edit the health or count properties, the relevant resource files are updated.

There's also a bug here where the scripted resources don't show the right icon (they show a "Node" icon if a custom icon isn't defined and it should be a "Resource" icon) and their typename isn't displayed (probably a factor in the changes my PR made). However, the functionality parts of it are all there, what with de/serialization, safely exporting, and editing the data.

So, all of the bugs I've found are related to the Inspector's user interface malfunctioning or it not finding the right class name / icon to display in the UI - none of which actually break the functionality. It's just slightly poorer usability at the moment.

@vnen
Copy link
Member

vnen commented Oct 4, 2018

What I think is that this should be solved in the editor and probably with a new type of PROPERTY_HINT. Then GDScript can just use it, as other languages would be able to use it as well.

@willnationsdev
Copy link
Contributor Author

@vnen What do you mean by "solved in the editor"? And wouldn't you still need to use PROPERTY_HINT_RESOURCE_TYPE with the hint_string being the base type? You could then just use the class_name property as the script class's name, and the accessing side can just check whether that value is in the ClassDB or ScriptServer. In that way, you can already have a variant represent a scripted resource type, regardless of platform. It's not like script class's names can collide with engine types'. Unless I'm missing something?

@vnen
Copy link
Member

vnen commented Oct 6, 2018

And wouldn't you still need to use PROPERTY_HINT_RESOURCE_TYPE with the hint_string being the base type? You could then just use the class_name property as the script class's name, and the accessing side can just check whether that value is in the ClassDB or ScriptServer.

@lobesoftllc
Copy link

Yes please, this will be very useful for a lot of reasons :D

@Reneator
Copy link

Would this feature change also have the effect, that you could edit the custom types you drag into the export fields in the inspector, be edited like Built-In types like Textures etc.?

@willnationsdev
Copy link
Contributor Author

@Reneator You can already do this, technically. If you create a resource script, then create a resource manually in the Inspector, and then set its script property to be equal to the resource script you made, the custom properties of that resource will appear in the Inspector. If you then save that resource and click+drag that resource file onto the export(Resource) property of some class, the sub-Inspector that appears when you expand that resource will include the custom properties. I have observed this myself with some of the work done in godot-next.

@Reneator
Copy link

@willnationsdev Thank you a lot! Was struggling with this!

I achieved it by (If somebody else finds this issue while searching for a solution):

  • define export(Resource) field in Node script
  • create Script that extends Resource
  • define export var
  • define class_name ("World_Def" in my example)
  • after saving the script file i could now click on the empty Resource-field in the inspector of the Node that i defined the export
  • the "World_Def" appears in the list and after choosing it, it shows me the export var that i defined in the Resource Script

@willnationsdev
Copy link
Contributor Author

@Reneator Just to say, the process as it is is very cumbersome. The change proposed by this issue would allow you to directly export the type, without having to manually attach a script to the resource in the Inspector.

@Reneator
Copy link

@willnationsdev the steps i did was in a current 3.2 nightly build (from 2 weeks ago). I tested the same in 3.1 and the customResource wouldnt show up in the list of Resources after i defined a class_name.
So maybe they now can load all the custom resources in the project and add them to the Resource-Selection.

@willnationsdev
Copy link
Contributor Author

@Reneator Oh, maybe. Yeah, that's cool. Didn't know they'd added that.

@Calinou
Copy link
Member

Calinou commented May 16, 2020

Closing in favor of godotengine/godot-proposals#18, as feature proposals are now tracked in the Godot proposals repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants