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 first-class custom resource support #18

Closed
ghost opened this issue Sep 4, 2019 · 76 comments · Fixed by godotengine/godot#72619
Closed

Add first-class custom resource support #18

ghost opened this issue Sep 4, 2019 · 76 comments · Fixed by godotengine/godot#72619
Milestone

Comments

@ghost
Copy link

ghost commented Sep 4, 2019

Describe the project you are working on:
Data-heavy RPG

Describe how this feature / enhancement will help your project:
I have a lot of different structures in my game which represent various different things, such as healing items, equipment, party members, item/character stats, enemies, and so on. To make it easy for designers/modders to add and change this data, these are represented through extending Resource, which, combined with class_name, allows you to easily create .tres files containing exported variables that can be assigned to in the inspector. So to modify data, one can double-click some file in the file system, glance over to the inspector, and easily change properties this way, leveraging all the various options associated with export.

image

This works, but only up to a point. Let's say, I now want all of my party members, equipment, and enemies to have a set of stats associated with them. I can write a stats class like so:

extends Resource
class_name Stats

export var strength: float
export var defense: float
...more stats

And now I can just do the following in all the associated scripts, and get to modify that data as I would with any other resource:

export(Stats) var stats = Stats.new()

...except, unfortunately, that doesn't work, as Godot will throw an error about not recognizing Stats as a Resource, though we see that Stats, due to extending Resource, should count as one. This works with any resource the engine ships with:

export(Texture) var tex
export(AudioStream) var stream
export(Shape2D) var shape

And you even get a little menu to create a new resource:

image

But, sadly, the engine rejects Stats as being something entirely different. If there was support for this kind of resource, we should be able to see a menu something like this, after the menu to create a new Stats resource:

image

Which would alleviate all the work-arounds outlined below.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

export(Stats) var stats
or
export var stats: Stats

stats_mock

If this enhancement will not be used often, can it be worked around with a few lines of script?:

In short: no, just the opposite, it requires a lot more code to be written to work around this inhibition, or requires a tedious process to add a custom resource to the inspector.

The fundamental issue is the current inability for the user to simply export a resource of their own creation. We are left with the following options:

Brute Force

#unnamed and unstructured, no guarantees of anything
export(Array, float) var stats
export(Array, Array, String) var combat_messages
#this one's especially painful, where you need multiple types in one array
export(Array) var mixed_data
#...but nothing compares to this, besides the dictionary variation
export(Array, Array) var lots_of_mixed_data

#named but unstructured, and you have to name them for every dictionary added in the latter example, no autocomplete
export(Dictionary) var stats := {"strength" : 0.0, "defense" : 0.0, ...}
export(Array, Dictionary) var god_help_you_if_you_do_this

The OmniResource

#named, structured, autocompleted (if you cast)
#necessitates a process where you create the custom resource then copy it over (after scrolling through the ginormous list of resources this gives you), or save it to the system and drag and drop
export(Resource) var not_as_it_seems

image

Do Repeat Yourself

#player_character.gd
export var strength: float
export var defense: float
...

#enemy.gd
export var strength: float
export var defense: float
...

#equipment.gd
export var start_strength: float
export var start_defense: float
...
export var upgraded_strength: float
export var upgraded_defense: float
...

Forget About It!

#my_character.gd
func get_character() -> PlayerCharacter:
    var player_character = PlayerCharacter.new()
    player_character.stats = Stats.new()
    player_character.stats.strength = 7
    ...
    return player_character

#character_defs.gd
func get_characters() -> Array:
   ...

#loader.gd
func load_character(path_to_json) -> PlayerCharacter:
    ...turning json back into an object logic...

There's the opportunity to build an inspector plugin or use _get_property_list(), both of which require quite a bit of code and technical know-how, on a per-resource basis. Based on what people have told me in the discord, it's a painful process (at least with property list) which they seek to avoid in the future. We could also go into C++ land and build modules for the engine to understand our data types, but again, it's a lot of code, and beyond the skill level of many of us who haven't delved into the engine code (assuming we know C++ at all); even if it were easily understood by anyone, or you know what you're doing, it's still many magnitudes more work than export var my_res: MyRes working out of the box.

It takes a lot of code, or unsafe code, or bad programming practices, or a tiresome process to make up for functionality that Godot already provides, leaving many of us to reinvent the wheel time and time again if we want this functionality. If exporting a custom resource Just Worked, lots of time, energy and headaches would be saved.

Is there a reason why this should be core and not an add-on in the asset library?:
I don't believe first-class support for custom resources would constitute as bloat. I believe it's an essential but missing feature for all games which require any kind of user-created resource type, for anyone who needs to export a bundle of named data with type safety, for anyone who wants to associate their own resource type with their own node type, for anyone who needs complex but easily modified data saved to disk; since virtually every game made with Godot makes use of Resources, every developer would benefit from this, if not by a little, by a lot.

@reduz
Copy link
Member

reduz commented Sep 4, 2019

I have the feeling this may be a bug? would be nice if someone else give this a check as I don't see why this shouldn't work.

@vnen
Copy link
Member

vnen commented Sep 5, 2019

Related: godotengine/godot#26162

@willnationsdev
Copy link
Contributor

This gets a massive upvote from me. As someone who spends most of their time developing data-driven code and loves Godot's resource types, not being able to export user-defined ones is one of the biggest obstacles to working in Godot by far.

@reduz it doesn't work yet just because the logic in the GDScriptParser and the EditorPropertyResource were never updated to account for globally-recognizable script classes from 3.1.

I'll try to look into the PR more / test it tonight if I can find the time. Would love for this to be mergeable as soon as the 3.2 release is done.

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 5, 2019

So, I spent a little time during my lunch break to fetch the branch of the PR for this change, rebase it, merge changes, and fix some of the implementation so that it is based entirely around reducing constants, not referencing an identifier, and changing it so that it correctly iterates over the languages, asks the languages to return the name of the script in the constant, and conditionally exports it if the script both is a script class and has a base type that extends Resource.

The copy in my test project is working fully correctly as far as I can tell, and it also avoids all of the other export issues brought up in this comment of the PR. The only thing I haven't implemented yet is allowing any arbitrary preloaded constant to be able to export as a resource. I'm also not 100% sure we should even support that since the editor itself needs to be able to access the typename associated with the script in order for it to know what GUI to generate when you click on an empty exported user-defined script (e.g. "Create new MyResource").

If you use a preloaded constant, you would have to use export.class_name to store "Resource" and export.hint_string to store the script's path which isn't its intended use-case and a lot of other things would need to change to accommodate that potentially being a file path. Furthermore, only the exact script will have any awareness of the type's name and the name will even be able to change over time easily, without "script classes" updating, so I suspect you could even end up with desynchronizations between what the editor thinks the name is and what the object says the name is.

As such, I think we should, at least for now, only offer support for it with script classes. In which case, my branch is largely completed. I'll comment here again when it is cleaned up/done and either @vixelz can do a hard reset to make his branch equal to mine (he did most of the work and therefore deserves most of the credit for the merge) or, if he would prefer, I can just submit my own PR to be merged. The branch in question is my gdres branch, if anyone wants to take a look at it pre-emptively.

@erodozer
Copy link

erodozer commented Sep 6, 2019

Is this something that's already supported by C# custom resources, or does this functionality need to be implemented there too or made note of?

@vixelz
Copy link

vixelz commented Sep 6, 2019

@willnationsdev I've not been able to give time to my PR in a while, and I have no need for credit for what parts survived the fixing. If your branch is looking more viable I'm happy to close my PR for my branch to make way for yours to get merged

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 6, 2019

@nhydock I suspect it is NOT supported for C# custom resources because it operates off the script class system which VisualScript and CSharpScript have yet to get support for. See #22 for more details.

@Reneator
Copy link

Reneator commented Sep 7, 2019

Related, but not as well formulated as this one, but wants the exact same thing!

godotengine/godot#30694

With what this proposal would offer i could much more easily edit certain things in editor, which i currently do via code or file-loading from directory.

I think this is something unity already has working but im not sure if its exactly the same functionality. I once saw this kind of custom ressource in unity in a video by Anton from H3VR where he showed in editor, how he defined the classes for his take and hold mode. So in this way you could just export an array with your custom ressources and then you could just create as many as you want and you can dig as deep into the objects as you like and create other custom ressources within those sub-variables (like with particles2D and shaders in godot currently)

@samdze

This comment has been minimized.

@willnationsdev
Copy link
Contributor

@samdze The pull request I submitted implements this feature. Thus far, no one has mentioned any further problems with the associated changes. I'm guessing it will be merged sometime soon after the 3.2 release, at which point, anyone on the master branch will be able to use custom resources out-of-the-box (finally).

Still need to get #22 fully implemented though so that all languages can benefit from the custom resource support. I've finished a VisualScript implementation (but haven't merged it yet) and I have a WIP C# implementation, but I hit a roadblock and neikeq hasn't gotten back to me on it on Discord (probably because he's been busy with everything else in C# exports).

@Reneator

Yes, Unity has a ScriptableObject class that is similar to Godot's user-defined Resource scripts. The only difference being that we do not yet have support for rendering those resources in the Inspector via export commands. You can still do it, but you have to manually assign the script to the resource instance by editing its script property. Possible, but with really bad UX. The pull request I mentioned above solves the UX issue afaict though.

@Reneator
Copy link

Reneator commented Dec 5, 2019

@samdze Im currently using these changes in my project. I built the project according to @willnationsdev s instructions and now i have a project with custom Resource support. I can send you those instructions as well, if you want to.
The approach of working with resources (at least the way i do) holds some "problems" because resources are meant mainly to be used as data-containers and its rather just tolerated if you use them in a more functional way (giving the resources their own methods). But for this, there are easy workarounds. So the main challenge lies in understanding how resources can be best utilized.

But using the CustomRecources to define GameObjects for my rpg (mainly idle game that is played via menues) helped me slim down a lot of the overhead in the project, where i usually would load and handle data.
By having resources and duplicating them, i can use them as if i created a new gameobject (keep in mind, duplicate(true) is still bugged (godotengine/godot#33079)

@willnationsdev I recently had a bug regarding the debugging context (the UI that shows the variables of the current context and the global singletons) where it died with a "parser bug" because of a custom resource. I could solve the problem (by triald and error), but i couldnt yet fully reproduce the issue in an independent project. Will post it here as soon as its possible.

@willnationsdev
Copy link
Contributor

Huh, I'm not really familiar with the debugger's code. Hopefully someone else has an idea of what the problem is. I'm guessing it would have something to do with the PropertyInfo's class_name and/or hint_string values being the name of a script class and it just not knowing to check the ScriptServer for a possible class name when inspecting the property. That's the only thing I could think of that it might notice differently given the changes.

@Reneator
Copy link

Reneator commented Dec 5, 2019

@willnationsdev I think its not a too high priority yet. But i will try to create a reproduction project that isolates the problem. Should i post it here or make a new ticket and reference this issue?

@willnationsdev
Copy link
Contributor

@Reneator if it has to do with the commits specifically in the pull request, then please post all information in the pull request.

@Reneator
Copy link

Reneator commented Dec 10, 2019

@willnationsdev I found a "problem" that arose with the custom resources, but i think it may be happening somewhere else in the engine entirely:

I declare a Custom Resource:
MyCustomResource

and then add an export of itself:

extends Resource
class_name MyCustomResource

export (MyCustomResource) var blubb

Now the editor will complain about: Expected a constant or identifier expression
and Resources that are created with that resource-script dont show any fields in the inspector.

So far, relatable behaviour

But if the class gets a bit bigger the behaviour gets weird:
Now I change that export to another type, so that it compiles successfully.
If i now change it back to the original "self-export" I can create Resources within itself.

image

here my expectation would be, that it either doesnt work entirely or (hopefully) actually allows for me to do it without error.

At this point restarting the project or changing other code would sometimes let me keep working with the "self-exporting" Resource and sometimes not.

I could use the ability to put a Resource of the same type inside a resource for my current project.

If you think this is unrelated I will create a different ticket for this.

Thanks in advance!

@willnationsdev
Copy link
Contributor

I feel like this is related to the GDScript parser just not explicitly checking for the current script's class name when it is checking exported resource types. Because, if you get the script in a compiling state, then it suddenly becomes a valid class type again. But the moment you have an error of some kind, then suddenly the exports don't work anymore.

I could add an exception that would enable you to export a class name of the same file, and that would at least get the GDScript to not complain, but you'd have to be aware that exported fields would only actually have data in them when the script is compiling. In fact, any exported data you edit in the Inspector might even be lost the moment you edit it into a non-compiling state (not sure if that would happen though).

It's a tricky issue to get around. I'll try investigating that when I get a chance.

@dioptryk
Copy link

dioptryk commented Jan 21, 2020

@somnivore , I'm in the same boat as you but I implemented this differently. My idea is that, simply, do not use custom resources for the data - use Nodes for each custom property. For example, your player character would have child nodes called: Strength, Dexterity etc., all of them extending Attribute.gd, which extends Node. This makes the entire mechanism much more dynamic than just using a resource. This approach can be used for everything, like skills, items (children of Inventory node), custom effects, etc. Try it - it works nicely for me. I'd not worry about performance unless your game is about whole armies of creatures clashing. If it's more of a dungeon crawler, with not that many NPCs, the performance impact will be next to none.

I've used custom resources for something different - some entity metadata which is supposed to be indexed without loading the entire creature with all the media, for example. So, Vampire.tscn has a MonsterData property, which is a custom resource (saved as external .tres), containing: name, where can it appear, rarity etc. When starting the game I'm loading all these .tres files to create a dictionary, which I use when needed to generate monsters and items for a level. If I stored everything in a .tscn, I'd have to load everything for indexing (which would make the game load much slower) or do some preprocessing to save this somewhere before building the release version. Custom resources deal with this problem nicely.

@YuriSizov
Copy link
Contributor

YuriSizov commented Feb 13, 2020

Is it related to this issue/proposal that custom resources are not properly initialized when loaded from disk?

It seems, that ClassDB does not recognize custom resources by their class_name and therefore cannot initialize them. For a custom resource get_class() returns a simple "Resource" string, and using their actual class name in ClassDB.instance() errs. This means, that custom resources loaded from disk are treated as a generic Resource, their initialization scripts are skipped, setter functions never called. At least, I think that's the reason. Should it be fixed as well?

@willnationsdev Does your PR cover it?

@willnationsdev
Copy link
Contributor

@pycbouh That is an unrelated non-bug (probably a documentation issue). ClassDB deals exclusively with engine C++ classes. So, you could use ClassDB.instance("GradiantTexture") or something, but you can't use a script. Script class support doesn't change that.

To get an initialized Object with an initialized ScriptInstance attached to it, you have to instantiate the content from the Script (e.g. with.new()) or create an Object first and then assign it the Script resource using set_script().

For serialization, if you create a Resource instance this way with a script attached, and then you save that Resource/ScriptInstance pair using ResourceSaver or the Inspector, then you'll get a resource file (.tres) which, when loaded, comes with all the ScriptInstance's properties setup and initialized properly.

My PR just makes it so that you'll be able to export the resources to the Inspector, so that you can view custom resources as properties on another Object, rather than having to directly view a Resource in the Inspector and manually attach a script to it in order to get custom properties to show up. It will work more out-of-the-box.

@YuriSizov
Copy link
Contributor

YuriSizov commented Feb 13, 2020

For serialization, if you create a Resource instance this way with a script attached, and then you save that Resource/ScriptInstance pair using ResourceSaver or the Inspector, then you'll get a resource file (.tres) which, when loaded, comes with all the ScriptInstance's properties setup and initialized properly.

@willnationsdev It does not, though. The issue I am talking about is exactly in the fact the ResourceLoader does not properly initialize a custom resource, as it uses ClassDB::instance to create instances of your serialized resources, and ClassDB is oblivious to your custom resource's class_name and type. So it creates a generic Resource. And while ScriptInstance may be loaded correctly, _init function is never called and neither are setters.

It may be a missing feature from the engine standpoint, but from usability standpoint it's a bug. I ran into this problem because sanity checks I put into setters were never called, when I loaded resources from disk. And there was no way to run any code on instance creation, as _init was not called either. This creates problems when dealing with custom resources, and something should be changed about handling deserialization.

@willnationsdev
Copy link
Contributor

willnationsdev commented Jun 27, 2022

For those tracking this issue and wanting to assist in testing the development of this feature, please re-visit godotengine/godot#48201 as I have updated its contents to incorporate a variety of small/lean child PRs that will incrementally tackle this issue alongside a demo project (maybe another for C#, when I get to it). Please report any bugs on the pull request and I'll get to fixing them as soon as I can.

@viktor-ferenczi
Copy link

Godot 4.0.0 alpha 14

Attempted to use custom resources as described in the description.

Exporting a variable with a custom Resource subclass as its type still results in this error:
Export type can only be built-in, a resource, or an enum.

@dploeger merged the PR, but I don't see it working in the public alpha.

Have I missed something?

@dploeger
Copy link

I didn't merge it. Before it could be merged, the core devs wanted to split the different parts into separated PRs. @willnationsdev is currently busy sorting everything out. As far as I understood, it should come with 4.0.0, but it's still not done.

@YuriSizov
Copy link
Contributor

Yes, we want it in 4.0, and preferably before the beta. Will did a great job separating this volume of work, it just needs reviews from our responsible maintainers 🙃

@willnationsdev
Copy link
Contributor

willnationsdev commented Aug 25, 2022

If one looks at the main PR for this feature, they will see that it has several listed "child PRs". Those are the ones that will actually be merged, but none of them have been yet. They should all be good to go aside from the C# one which.....seems to be wholly irrelevant/inapplicable to the various changes made in the recent move to .NET 6.

I'm not even really sure yet whether it's necessary to do the effort of investigating and refactoring it since I've been told that C# will start becoming ClassDB types rather than scripts, in which case, my global script class changes wouldn't even be needed since they should already be properly recognized as exported resources.

The only other PR I have that is loosely related but not ready yet is a compatibility-breaking PR that makes the class_name keyword no longer expose a class to the editor by default, preferring instead to require you to add an @export annotation to indicate one's desire to do so. My changes to the parsing logic are resulting in some segfault in the unit tests that I still need to investigate and fix.

@viktor-ferenczi
Copy link

Thank you for the really quick response. Once it is merged I'm ready to test in GDScript. I would take part in the code reviews as well, but I'm quite new to Godot, so would not be of much help (yet).

@akien-mga
Copy link
Member

Implemented by godotengine/godot#62411, godotengine/godot#62413 and godotengine/godot#62417.

This will be available in Godot 4.0 beta 2 within a couple of weeks.

Repository owner moved this from Seems Consensual to Discussion Stalled in Proposals shortlist for review Sep 18, 2022
Repository owner moved this from Ready for Implementation to Implemented in Godot Proposal Metaverse Sep 18, 2022
@SeijiStory
Copy link

SeijiStory commented Dec 7, 2022

Any chance of the PR getting merged and backported to 3.x? I have a project I can't update to Godot 4, so having this in 3 would be great.

@YuriSizov
Copy link
Contributor

@SeijiStory While Will had a working branch for 3.x, it's a pretty significant change that needs some of the more experienced maintainers to validate it. With most of the efforts being put into 4.0, and the codebases being significantly different at this point, a backport of this may not be very likely. It's possible, though.

@Shadowblitz16
Copy link

@akien-mga does this work with C# classes in 4.0?

@MetRiko
Copy link

MetRiko commented Nov 29, 2023

@YuriSizov
If porting this to 3.x would be too complicated could we at least have a simplified version of that?
I'm thinking about updating export hint for the Resource type to allow something like this:

export(Resource, MyResourceA, MyResourceB, ...) var res : Resource 

In this case this would just filter out all the unnecessary resources on the list inside the inspector for the res property, leaving only the "MyResourceA" and "MyResourceB" on the list when creating a new resource.
I'm curious if that would be hard to implement because this would already make things much easier for the 3.x users.

@YuriSizov
Copy link
Contributor

@MetRiko As I've said, the main obstacle would be getting someone to review it. We don't have many active maintainers for the 3.x branch, although we can of course try to find someone.

For this reason I can't recommend anyone putting more effort into bringing this feature to Godot 3 right now. But if you really want to try, it needs to happen before 3.6 is finalized because there are currently no plans for 3.7.

Note that the syntax is not the issue. The internal rework is. The necessary changes to recognize scripted types within the engine and the editor are the core of it.

@ricolantern
Copy link

ricolantern commented Mar 17, 2024

@akien-mga does this work with C# classes in 4.0?

To make your custom resource appear in the editor, you must annotate the class with [GlobalClass].
It's explained here briefly, the results of which can be found at the bottom of that page.
The annotation is basically your C# equivalent of GDScript's class_name.

Without the annotation, your class won't be visible in the editor.

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