godotengine / godot

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

ResourceSaver.save() writes null for sub-resources that refer to each other #89961

Open PeterMacDonaldSpatula opened 7 months ago

PeterMacDonaldSpatula commented 7 months ago

Tested versions

Reproducible in v4.3.dev5.official [89f70e98d], v4.2.1-stable, v4.2-stable, v4.2-dev1

Does not exactly reproduce in v4.1.4-rc2, v4.1-stable, or v4.0-stable, but a different undesirable behavior is seen instead (resulting file is blank, no error message)

System information

Godot v4.2.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3070 (NVIDIA; 31.0.15.3699) - Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz (12 Threads)

Issue description

One object of type Alpha contains references to objects of type Beta. Beta itself also references to Beta objects. The same Beta objects are referred to in both; that is, the Beta objects in the Alpha reference each other.

If you use ResourceSaver.save() to serialize an Alpha object to disc, a number of the references in the arrays in the Beta objects will instead be written as 'null'. For each such instance, the following error message will be produced:

E 0:00:00:0542   test.gd:32 @ _ready(): Resource was not pre cached for the resource section, bug?
  <C++ Error>    Method/function failed. Returning: "null"
  <C++ Source>   scene/resources/resource_format_text.cpp:1853 @ _write_resource()
  <Stack Trace>  test.gd:32 @ _ready()

I dug into the resulting .tres file and what seems to be happening is that as the ResourceSaver is writing out the references held in the Beta object, it only correctly references the other Beta objects if it has already seen them in the Alpha object. Otherwise, it can't find them, and writes null.

I tried saving as both .tres and .res, and with and without FLAG_BUNDLE_RESOURCES set; the same behavior was seen in all cases.

Here's an example .tres file produced by my minimum reproduction project (renamed so github would accept it): file.txt

If you read it, you'll notice that the further down the file you go, the fewer null values you see. The first beta object's references are all null. Then for the next one, they're all null except for the first one that was written to the file. And for the third, all null except for the two we've already written. Etc. This is a simple example, but I've spent considerable time going over a much more complex real-world example (from when I first encountered this) and the same pattern holds.

It seems likely that this is caused by the circularity of the references, but I can't find any documentation on the ResourceSaver, ResourceFormatSaver, or Resource pages indicating that this is supposed to be invalid or a known issue.

I tested with versions going back to 4.0; prior to 4.2, the behavior was different, but also incorrect: it would silently fail, producing a blank file and no error at all.

Steps to reproduce

Open the Minimum Reproduction Project I've provided, and run the Project. You'll see the error messages appear, and the broken file will be generated at "user://file.tres".

Minimal reproduction project (MRP)

Bug Minimum Reproduction.zip

allenwp commented 7 months ago

I've trimmed down the minimum repro a bit:

Custom resource script:

class_name MyResource extends Resource
@export var my_resource_export : MyResource

Test script:

extends Node

func _ready() -> void:
    var res_1 : MyResource = MyResource.new()
    var res_2 : MyResource = MyResource.new()

    res_1.my_resource_export = res_2
    res_2.my_resource_export = res_1

    ResourceSaver.save(res_1, "user://file.tres")

This results in the error message:

E 0:00:00:0680   test.gd:10 @ _ready(): Resource was not pre cached for the resource section, bug?
  <C++ Error>    Method/function failed. Returning: "null"
  <C++ Source>   scene/resources/resource_format_text.cpp:1881 @ _write_resource()
  <Stack Trace>  test.gd:10 @ _ready()

And a saved file that looks something like this:

[gd_resource type="Resource" script_class="MyResource" load_steps=3 format=3]

[ext_resource type="Script" path="res://my_resource.gd" id="1_7vp5f"]

[sub_resource type="Resource" id="Resource_c1iwo"]
script = ExtResource("1_7vp5f")
my_resource_export = null

[resource]
script = ExtResource("1_7vp5f")
my_resource_export = SubResource("Resource_c1iwo")

4.3 dev 5 project

If these sort of cyclic references are not supported by ResourceSaver, then I believe that the error message Resource was not pre cached for the resource section, bug? is problematic because it does not make it clear to the developer that the issue is simply an unsupported cyclic reference. Additionally, as mentioned in the original issue, there may not be any documentation describing this limitation.

If these sort of cyclic references should be supported, then this is simply a ResourceSaver bug.

AThousandShips commented 7 months ago

Circular references are indeed not supported, and this could be indicated better, see also:

allenwp commented 7 months ago

I have taken a brief look at the source code to see if I could improve the error message to let the developer know that there might be a circular reference... But I noticed some code was introduced in 3ea04c1366eb83327b4552a8e84b68a8130f6bc1 to give these sorts of error messages, but it doesn't seem to be working for all cases.

I also noticed that no error message is given when saving with the binary format, which is different behaviour than the text format error message in the OP. Instead, the following warning is given when trying to load from a binary file:

W 0:00:03:0373   test.gd:24 @ load(): Couldn't load resource (no cache): local://Resource_bqh1u
  <C++ Source>   core\io\resource_format_binary.cpp:412 @ ResourceLoaderBinary::parse_variant()
  <Stack Trace>  test.gd:24 @ load()
robbertzzz commented 6 months ago

@AThousandShips That's false, circular references are supported by the engine. I've been using them in my project without issue. The thing that's not supported, and what your linked issue discusses, is an editor implementation that allows the user to view or edit those circular dependencies. This shouldn't affect ResourceSaver.save() in any way.

AThousandShips commented 6 months ago

That's false, circular references are supported by the engine. I've been using them in my project without issue.

No, you are wrong, they are not supported by tres and tscn files

It is simply a limitation, this is not a bug

robbertzzz commented 6 months ago

The are supported by Resources, so them not being supported by tres files is a bug.

robbertzzz commented 6 months ago

Or to put it differently, tres files should support everything that Resource supports. There is no reason why the file format couldn't support cyclic references, it works fine in tscn files which use the same structure.

AThousandShips commented 6 months ago

I'll say it a final time: This is a limitation, resources do not support circular references by design for a great number of reasons, this is not a bug, it is an intentional limitation as it's not supported by the underlying RefCounted type (which does not support circular references, it causes leaks, i.e. not supported), so there's no reason to support it without having systems for handling that

See, for example: https://github.com/godotengine/godot/blob/d4f726f3ef21cef3e7936b2c9770cdac6478b8ee/scene/resources/resource_format_text.cpp#L1900-L1911

So let's not argue in circles about this, as you can see in the code this is a limitation that is known and considered unavoidable in the current setup, and every system around it is designed around or to enforce this limitation, like how it's impossible to have circular resources in the editor

robbertzzz commented 6 months ago

I'll say it a final time

I'm trying to have a constructive discussion about this topic, not you telling me "the current state will never ever be changed".

This is a limitation, resources do not support circular references by design for a great number of reasons

But resources do support them. It's the file format that doesn't support them. The only real limitation is in the editor, which should not limit the file format itself since the file format can be used without ever editing the resource in the editor. Instead of making the editor limit the file format, the editor should be the place where warnings are implemented.

it's not supported by the underlying RefCounted type

This statement is false as well, like I said earlier I've been using Resources with circular references without any issues. I've double checked just now to confirm I'm not making this up, and I can indeed see the cyclic reference when looking at my project through the remote tab.

AThousandShips commented 6 months ago

I recognise where you're coming from, but not once have I said this will never change, all I've said is that it's a current limitation, please read what I've said again, you'll see I'm not misrepresenting the reality of the system:

So this is a limitation, not a bug, and the discussion on how to change that is in https://github.com/godotengine/godot-proposals/issues/7363, this issue report is for the problem of this producing clearer error messages

So please if you want to support circular references, which aren't currently supported, please discuss it on the issue linked above, otherwise please let's keep this focused on the right topic, this is not the place to discuss changing these things

Thank you and have a nice day

robbertzzz commented 5 months ago

@AThousandShips sounds like this issue can be closed then, because it's by design and a proposal to change the design already exists.

AThousandShips commented 5 months ago

I think we can try and improve the error message and ensure it happens reliably, if it doesn't already, haven't tested recently, it might not work correctly if you have multiple resource files referencing each other circularly, but within one file it should report correctly