godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.97k stars 21.16k forks source link

Saving custom resources using the ResourceSaver.FLAG_BUNDLE_RESOURCES flag causes crash on load. #65393

Open DaGamingWolf opened 2 years ago

DaGamingWolf commented 2 years ago

Godot version

4.0 dev (106b68050) 4.0 dev (432b25d36)

System information

windows 10

Issue description

it appears loading a custom resource using ResourceLoader.load() on a file saved with the ResourceSaver.FLAG_BUNDLE_RESOURCES can result in a failure to load the file and an error " hides a global script class."

Steps to reproduce

create a custom Resource, save it with the ResourceSaver.FLAG_BUNDLE_RESOURCES flag, then use ResourceSaver.load()

Minimal reproduction project

Minimal reproducable project.zip

DaGamingWolf commented 2 years ago

The mrp continues to show an error on Godot 4.0 Beta 1 dev (22a09fef5)

aaronfranke commented 1 year ago

Please re-test in the latest master or a recent beta. PR #71142 has likely fixed this issue.

nezvers commented 1 year ago

Godot 4 RC2

Saving a resource with an Array[CollectableResource] of resources with a class_name CollectableResource: ResourceSaver.save(self, get_filename(), ResourceSaver.FLAG_BUNDLE_RESOURCES) Loading: var data:SaveResource = ResourceLoader.load(get_filename(), "SaveResource",ResourceLoader.CACHE_MODE_REPLACE)

Got: Parser Error: Class "CollectableResource" hides a global script class

Dunno if that's the correct way to save nested resources, but that's the error I run into.

nezvers commented 1 year ago

I created a little playground project. Nested Resource Saving.zip res://Resources/SaveGame.tres is the master resource with save/load flag options. On saving the .tres file content is displayed in a TextEdit. When FLAG_BUNDLE_RESOURCES is selected it embeds the script of nested resources (with class_name) and upon loading it triggers the bug. Any other option refers to the array of classes like this: resources = Array[Resource("res://Resources/CollectibleResource.gd")]([ExtResource("1_avxuu"), ExtResource("2_6my64"), ExtResource("3_oe2tv")]) But also they all are saved the same as without flags, is it supposed to be like that? FLAG_BUNDLE_RESOURCES is the only one that packs actual values of nested resources.

DaGamingWolf commented 1 year ago

Please re-test in the latest master or a recent beta. PR #71142 has likely fixed this issue.

retested in Godot 4 RC4, The issue is still present. The project still crashes on load, and it appears to be a namespace error of some sort. It's like Godot is saving the class name of the nested resource, then when it loads the resource it becomes incapable of recognizing that the nested resource is an instance of the class registered in godot via class_name, and thus throws an error. That'd be my guess without delving into the editor source code.

the problem pops up when var mrpresource = ResourceLoader.load(file_path) is run, so i'd look into the aspects of the ResourceLoader.load() function that are responsible for checking for namespace conflicts and processing the class of the saved resource.

SaracenOne commented 1 year ago

I'm somewhat curious whether this is technically a bug or not. It's been established it seems that the issue is that FLAG_BUNDLE_RESOURCES is bundling an embedded copy of the NodeData script, which is subsequently creating a naming conflict with the existing NodeData script. It's somewhat nuanced, a script is technically a resource, and it is technically bundling it, along with an error due to the fact the conflicting class_names are not allowed. I think this issue might be difficult to establish a quick fix for and should possibly be punted to allow for broader discussion of what is considered correct behaviour here (should the bundled resource flag simply not bundle scripts with class_name definitions, should this apply to all scripts, ect.?)

DaGamingWolf commented 1 year ago

I'm somewhat curious whether this is technically a bug or not. It's been established it seems that the issue is that FLAG_BUNDLE_RESOURCES is bundling an embedded copy of the NodeData script, which is subsequently creating a naming conflict with the existing NodeData script. It's somewhat nuanced, a script is technically a resource, and it is technically bundling it, along with an error due to the fact the conflicting class_names are not allowed. I think this issue might be difficult to establish a quick fix for and should possibly be punted to allow for broader discussion of what is considered correct behaviour here (should the bundled resource flag simply not bundle scripts with class_name definitions, should this apply to all scripts, ect.?)

Based on the usage of the save function, i would expect that the resource's data and class name is saved in the save file, and upon load, the engine would look for a class with the same name in the class registry then create an instance of that class to hold the data saved in the file. I wouldn't expect it to save an entire script and load it like it was a new class. i don't actually see the usefulness of that.

henriquelalves commented 1 year ago

The problem is that Godot 4.x GDScript favours a lot the use of class_name in the project, since type inference is much better and most cyclic dependency problems were solved - so ResourceSaver won't work most of the time the user creates a custom resource with nested resources within. I think there should at least be an option of bundling all nested resources with the exception of scripts, or have a resource setting to force specific subresources to be saved as paths.

muhuk commented 1 year ago

Ah, I was hoping this would be fixed by 4.1. Does discussion label mean more discussion is required?

andrewgbliss commented 1 year ago

Has there been any more discussion about this? I am getting the same error

drd-dev commented 1 year ago

still having this issue. Anyone figure out a workaround? Creating the subresources at runtime seems to work if you exclude the ResourceSaver.FLAG_BUNDLE_RESOURCES flag but it's a pain to do that.

AwesomeAxolotl commented 11 months ago

I thought of a way to mitigate the problem while it's still present in the engine. It works by changing bundled scripts with a class name to simply extending the class_name instead. Perhaps it helps one or the other. utils.zip

Saadies commented 7 months ago

It seems saving resources is still completely broken? I've been searching online and I've found many people with this issue with no solution. I am confused because saving resources should be something pretty much everyone does, how can it be not working? Is this way of saving just wrong? Is there another reccomended way to save nested resources, for example by seperating the data and the logic of your resource somehow without losing the conveniences of resources?

Saadies commented 6 months ago

I thought of a way to mitigate the problem while it's still present in the engine. It works by changing bundled scripts with a class name to simply extending the class_name instead. Perhaps it helps one or the other. utils.zip

I am trying to use this, together with: https://github.com/godotengine/godot/issues/74918#issuecomment-2045182554 to save and load resources that contain an Array with other Resources.

It does not work unfortunately:

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): Attempted to assign an object into a TypedArray, that does not inherit from 'GDScript'.
  <C++ Error>    Condition "!other_script->inherits_script(script)" is true. Returning: false
  <C++ Source>   core/variant/container_type_validate.h:140 @ validate_object()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()
        var loadfile = ResourceLoader.load(SAVEDIR,"GameStats", 0)

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): unsupported format character
  <C++ Error>    Condition "error" is true. Returning: String()
  <C++ Source>   ./core/variant/variant.h:834 @ vformat()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): Method/function failed.
  <C++ Source>   core/variant/array.cpp:222 @ assign()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): Attempted to assign an object into a TypedArray, that does not inherit from 'GDScript'.
  <C++ Error>    Condition "!other_script->inherits_script(script)" is true. Returning: false
  <C++ Source>   core/variant/container_type_validate.h:140 @ validate_object()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): unsupported format character
  <C++ Error>    Condition "error" is true. Returning: String()
  <C++ Source>   ./core/variant/variant.h:834 @ vformat()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): Method/function failed.
  <C++ Source>   core/variant/array.cpp:222 @ assign()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

Has anyone managed to save and load resources that contain an Array of another resources?

AwesomeAxolotl commented 6 months ago

@Saadies if you use the bundle resources flag I think it already saves all contained subresources (no matter if they are in an array or not), at least it works for me. So it may not be strictly necessary to duplicate. Perhaps inspect the saved resource file to see if there's any mistakes (missing parts, missing quotes, whatever). One user commented they had to use a different delimiter to detect the end of a script in my utility script... it was because their scripts had no trailing empty line at the end of the script files (that Godot normally adds, but if you use another editor, it may not happen).

Saadies commented 6 months ago

@Saadies if you use the bundle resources flag I think it already saves all contained subresources (no matter if they are in an array or not), at least it works for me. So it may not be strictly necessary to duplicate. Perhaps inspect the saved resource file to see if there's any mistakes (missing parts, missing quotes, whatever). One user commented they had to use a different delimiter to detect the end of a script in my utility script... it was because their scripts had no trailing empty line at the end of the script files (that Godot normally adds, but if you use another editor, it may not happen).

There are multiple issues regarding Resources so it get's a bit confusing. I am not using the "BaseResource" class for my resources to fix the saving but for actually duplicating resources at runtime to create new entities in my game. The nested ressources do not get duplicated automatically but stay a reference otherwise.

Edit: Okay so, replacing the whole script just with "extends class" is actually the desired behaviour, I understand now. It is saving scripts that are not a resource and not inside any resources as far as I can see, maybe that is the problem, still investigating.

Edit 2: I tried to save a PackedScene that I didn't even use. Everything looks okay now but I still cannot load the file, with a few less errors now:

E 0:00:09:0735   SaveManager.gd:17 @ loadAll(): Attempted to assign an object into a TypedArray, that does not inherit from 'GDScript'.
  <C++ Error>    Condition "!other_script->inherits_script(script)" is true. Returning: false
  <C++ Source>   core/variant/container_type_validate.h:140 @ validate_object()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0736   SaveManager.gd:17 @ loadAll(): unsupported format character
  <C++ Error>    Condition "error" is true. Returning: String()
  <C++ Source>   ./core/variant/variant.h:834 @ vformat()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0736   SaveManager.gd:17 @ loadAll(): Method/function failed.
  <C++ Source>   core/variant/array.cpp:222 @ assign()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

Edit 3: I removed the custom ResourceBase class from my Resources (from the other script) and I still get the same errors, so It doesn't seem to have any influence on this saving fix.

IGNORE BELOW:

You are right that something is messed, I can see inside the save file that the scripts are completely replace by just "extends ClassName".

I cannot find out why though. When I check the variable "skipped_lines", it contains the whole script and "store_line(skipped)" is called for all lines but the save file is still missing the lines after saving.

My resources start like this:

class_name DungeonStats
extends ResourceBase

@export var mobLimit : int
Saadies commented 6 months ago

Godot 4 RC2

Saving a resource with an Array[CollectableResource] of resources with a class_name CollectableResource: ResourceSaver.save(self, get_filename(), ResourceSaver.FLAG_BUNDLE_RESOURCES) Loading: var data:SaveResource = ResourceLoader.load(get_filename(), "SaveResource",ResourceLoader.CACHE_MODE_REPLACE)

Got: Parser Error: Class "CollectableResource" hides a global script class

Dunno if that's the correct way to save nested resources, but that's the error I run into.

I just understood. You do NOT use the Flag to save nested resources, it is not necessary. I created a minimum project where nested resources (or even Arrays with resources) save and load perfectly fine without this Flag.

I do not know what we are all doing wrong in our actual games for this not to work but this is not the problem.

EDIT: The problem comes up when your subresources are instances, then it does not work anymore. 4-2_NestedResourcesTest.zip

AwesomeAxolotl commented 6 months ago

The problem is when the subresource exists in the project files, just a reference to that resource is saved, even if you duplicate the resource holding the subresource. So if you changed the value of those like say an that won't work. It can be avoided by duplicating them before putting them in the array, or going through the arrays and manually changing them to a duplicate, which is good as long as you are aware of it... or the flag to bundle resources can be used. And that has the problem with the class_names, hence my workaround (even if there are some errors for some cases in it apparently).

Another issue is loading savegames, where there's a possibility for arbitrary code execution upon load() if those have scripts, so I end up only using scripts with class name for resources I intend to load from the user:// directory and turn all scripts to a single line to extends ClassName or extends Resource.

TheWolfWizard64 commented 4 months ago

I'm somewhat curious whether this is technically a bug or not. It's been established it seems that the issue is that FLAG_BUNDLE_RESOURCES is bundling an embedded copy of the NodeData script, which is subsequently creating a naming conflict with the existing NodeData script. It's somewhat nuanced, a script is technically a resource, and it is technically bundling it, along with an error due to the fact the conflicting class_names are not allowed. I think this issue might be difficult to establish a quick fix for and should possibly be punted to allow for broader discussion of what is considered correct behaviour here (should the bundled resource flag simply not bundle scripts with class_name definitions, should this apply to all scripts, ect.?)

why not have it to where you have one for and one for not, like use a true false statement? Ex. ResourceSaver.save(self, get_filename(), ResourceSaver.FLAG_BUNDLE_RESOURCES, true)

or perhaps give the option to change the name the class_name of the bundled script

actually after some thought maybe have it to where false doesn't bundle with resource name but true lets you give it a new one

Ex. ResourceSaver.save(self, get_filename(), ResourceSaver.FLAG_BUNDLE_RESOURCES, true, new_class_name)

not entirely sure how it works on the engine side, just came across this when trying to find a shortcut to saving resources

deadeagle63 commented 4 months ago

So found a temporary solution which is a bit wonky: Steps: Create shadowed scripts of the resource giving a headache e.g

# file is: dish_shadowed.gd
extends Dish

If you have actual .res/.tres files drag and drop the new shadowed script under the RefCounted section e.g image

For the actual instantion this is where things become a bit wonky, instead of doing what you would normally do you would do e.g

# OLD WAY CAUSING THE ISSUE
dish = Dish.new()

# shadowing the class that is causing the breakage
dish = load("res://dish_shadowed.gd").new()

You will need to delete the saves and have your save system rerun

DriftWare07 commented 1 month ago

are we any closer to fixing this issue?

eldondanonino commented 1 week ago

This is crazy that a core feature like ressource bundling for saves still doesn't work