godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.09k stars 69 forks source link

Allow circular dependencies/references for resources in export var #7363

Open rossunger opened 1 year ago

rossunger commented 1 year ago

Describe the project you are working on

I'm working on a game with many characters where the data about their personal lives is stored in resources which need to be accessed from a variety of .tscn files. There are many times different characters need to reference each other e.g. I have an export var which is an array of family members, and all the family members have references to each other.

Describe the problem or limitation you are having in your project

Godot does not allow circular references in resources in export vars.

I apologise if this is the wrong place to post about this, it seemed the most appropriate to me, as this isn't a bug (it seems intentional), and I would like to see if it's possible/desirable to change this.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I'm not understanding the reason why resources can't have circular dependencies. I've looked through the forums and the issues on github and I see several thread about making the editor prevent circular dependencies instead of crashing, but I haven't been able to find an explanation for why this happens in the first place. I'm hoping someone can point me to a discussion thread, or even some of the godot source code

I understood the ref-counted issue, but it seems to have been resolved in #67714,

and I was looking at 68281 and I'm not sure if this made it into 4.1.1, or whether this solves my problem...

I would like to try to propose a solution to allow circular dependencies, but I'm hoping someone can help me understand the problem first.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I don't understand the problem well enough to propose a solution, but I wasn't sure where else to post about it because it's not really a bug... it seems intentional.

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

there are some hacky work-arounds involving referencing the file-path instead of the resource itself, but this requires you to load the resource to access them, and it doesn't allow you to edit the resource in the editor's inspector

I could also not use resources at all, and instead create my own tool which allows me to edit data and then serialize it for saving, but this is basically recreating Resources just to add support for circular dependency

Is there a reason why this should be core and not an add-on in the asset library?

Resources are a core component of the engine

aqwalnut commented 1 year ago

I ran into the same problem just now. The problem appears to be due to export in particular, and not because circular dependencies are not supported? 68281 is still open, so it might be a work in progress...

A structure like this might be convenient, for example, if I want to serialize a linked list of Resources, or if I want to make and edit a directed graph of places to traverse. For simple problems like those, one can use an ID system, but if things get more complicated (for example if the vertices have types and restrictions on types of vertices they can be linked to because they trigger certain behaviors on the neighboring vertices), you may need a different ID system for every subtype or interface and that gets messy.

Here's an example script in C# that causes Godot (latest version) to freeze and crash upon clicking the checkbox for Generate and then trying to inspect the resources in the exampleVariants array (one needs to click on another node in the scene and come back to it to see the items in the array), as well as a test to see if things are working.

using Godot;
using Godot.Collections;
using System;

[Tool]
public partial class VariantTest : Node
{
    //Only exported structures are kept in scene
    [Export]
    private Array<ExampleVariant> exampleVariants = new Array<ExampleVariant>();

    [ExportGroup("Editor")]
    [Export]
    public bool Generate
    {
        set
        {
            if (value == true)
            {
                GD.Print("Tool: Generate microtest");
                exampleVariants.Add(new ExampleVariant(1));
                exampleVariants.Add(new ExampleVariant(2));
                exampleVariants.Add(new ExampleVariant(3));
                exampleVariants[0].A = exampleVariants[1];
                exampleVariants[0].B = exampleVariants[2];
                exampleVariants[1].B = exampleVariants[2];
                exampleVariants[2].B = exampleVariants[2];
                exampleVariants[2].A = new ExampleVariant(4);
            }
        }
        get
        {
            return false;
        }
    }
    [ExportGroup("Editor")]
    [Export]
    public bool Test
    {
        set
        {
            if (value == true)
            {
                GD.Print(exampleVariants[0].A.B.B.ID);
                exampleVariants[0].A.B.B.ID = 5;
                GD.Print(exampleVariants[0].B.ID);
                GD.Print("meow");
                GD.Print(exampleVariants[0].A.B.B.A.ID);
                exampleVariants[0].A.B.B.A.ID = 5;
                GD.Print(exampleVariants[0].B.A.ID);
            }
        }
        get
        {
            return false;
        }
    }
}
public partial class ExampleVariant : Resource
{
    public ExampleVariant(int ID)
    {
        this.ID = ID;
    }
    [Export] public int ID = 0;
    [Export] public ExampleVariant A;//the test works fine after removing [Export] here and in the next line.
    [Export] public ExampleVariant B;//
}

But without the [Export] attribute in A and B, the engine serializes the cyclic references fine and nothing crashes. Clicking the Test checkbox works as expected even after closing and reloading the scene.

The export/inspector problem seems like it would require design decisions... it might involve displaying the same resource multiple times in the nested mess (which run into issues with other copies of the same object not updating when you change one of them), or it might involve stopping upon detecting an cyclic reference with some priority scheme.

AThousandShips commented 1 year ago

What is the reason you want circular references? You shouldn't have them, you need to use weak references or you will have memory leaks and all sorts of problems, see here

68281 is still open

It is not open it was closed two weeks ago

aqwalnut commented 1 year ago

@AThousandShips That's helpful and makes my life somewhat easier, but it doesn't fix the crash in my example. The engine still accumulates errors and crashes if I replace the ExampleVariant class with weak references like so:

public partial class ExampleVariant : Resource
{
    public ExampleVariant(int ID)
    {
        this.ID = ID;
    }
    [Export] public int ID = 0;

    [Export]
    public ExampleVariant A
    {
        get
        {
            return (ExampleVariant) _A.GetRef();
        }
        set
        {
            _A = WeakRef(value);
        }
    }
    private WeakRef _A;

    [Export]
    public ExampleVariant B
    {
        get
        {
            return (ExampleVariant)_B.GetRef();
        }
        set
        {
            _B = WeakRef(value);
        }
    }
    private WeakRef _B;
}
rossunger commented 1 year ago

I would be very happy to use weakref if it serialized for saving and worked as an export var type... but I don't think that's going to happen :p

I'm hoping someone can explain what happens in the saving process or the export var process that makes circular references a problem. I was thinking about trying to look it up in the c++ codebase but it's very overwhelming and I'm not sure where to start.

I'm also not clear if Resources disallowing circular references is intentional, or a temporary bandaid because a solution hasn't been made yet. To me this feels like a very common use case for resources and I want to understand what problems this causes. Godot seems to serialize packed scenes with circular references excellently.

I'm considering rewriting all my resources as nodes and saving them as packed scenes, but then I'm stuck using nodepaths, which means that they all have to be in the same scene in order to work predictably.

On Tue, Jul 25, 2023, 3:54 p.m. aqwalnut @.***> wrote:

@AThousandShips https://github.com/AThousandShips That's helpful and makes my life somewhat easier, but it doesn't fix the crash in my example. The engine still accumulates errors and crashes if I replace the ExampleVariant class with weak references like so:

public partial class ExampleVariant : Resource { public ExampleVariant(int ID) { this.ID = ID; } [Export] public int ID = 0;

[Export]
public ExampleVariant A
{
    get
    {
        return (ExampleVariant) _A.GetRef();
    }
    set
    {
        _A = WeakRef(value);
    }
}
private WeakRef _A;

[Export]
public ExampleVariant B
{
    get
    {
        return (ExampleVariant)_B.GetRef();
    }
    set
    {
        _B = WeakRef(value);
    }
}
private WeakRef _B;

}

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-proposals/issues/7363#issuecomment-1650448168, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGZ2R3R4UEPCHG3WWYU5QDXSAP65ANCNFSM6AAAAAA2VT3R5I . You are receiving this because you authored the thread.Message ID: @.***>

Calandiel commented 11 months ago

I'm working on a project with similar problems to the ones @rossunger encountered (a graph of relationships between humans). Poor support for cyclic dependencies in exported values (for both resources and scenes) has been my largest issue with the engine for the past year or so. "You shouldn't have them" isn't a solution, some problems require bidirectional graphs and currently they have to be handled either by not using engine features altogether or representing graphs with strings/as an adjacency matrix (as to the best of my information, WeakRef isn't handled in exports).

HeavensSword commented 9 months ago

I believe I'm running into this issue as well. I'm attempting to port my Unity game into Godot, and I have a Scriptable Object for Jobs that I've translated into a Resource.

Each job can reference a Prereq Job and Advanced Job object of the same type. I can't link them in Godot though because it says "Recursion Detected" when attempting to set the reference. Meanwhile it works perfecting fine in Unity.

Calandiel commented 9 months ago

I believe I'm running into this issue as well. I'm attempting to port my Unity game into Godot, and I have a Scriptable Object for Jobs that I've translated into a Resource.

Each job can reference a Prereq Job and Advanced Job object of the same type. I can't link them in Godot though because it says "Recursion Detected" when attempting to set the reference. Meanwhile it works perfecting fine in Unity.

You should use uIDs instead and get the typed resources with load or preload from the uID. Those allow circular references. It should be mentioned somewhere in the docs for people coming in from Unity but I don't think it is.

rossunger commented 9 months ago

I believe I'm running into this issue as well. I'm attempting to port my Unity game into Godot, and I have a Scriptable Object for Jobs that I've translated into a Resource. Each job can reference a Prereq Job and Advanced Job object of the same type. I can't link them in Godot though because it says "Recursion Detected" when attempting to set the reference. Meanwhile it works perfecting fine in Unity.

You should use uIDs instead and get the typed resources with load or preload from the uID. Those allow circular references. It should be mentioned somewhere in the docs for people coming in from Unity but I don't think it is.

How would this work @export vars in the inspector?

HeavensSword commented 9 months ago

I believe I'm running into this issue as well. I'm attempting to port my Unity game into Godot, and I have a Scriptable Object for Jobs that I've translated into a Resource. Each job can reference a Prereq Job and Advanced Job object of the same type. I can't link them in Godot though because it says "Recursion Detected" when attempting to set the reference. Meanwhile it works perfecting fine in Unity.

You should use uIDs instead and get the typed resources with load or preload from the uID. Those allow circular references. It should be mentioned somewhere in the docs for people coming in from Unity but I don't think it is.

Where can you find the uID of a resource?

Go-Ba commented 9 months ago

This is an issue that I ran into and is seriously making me consider leaving the engine and returning to unity until this is fixed. Making bi-directional/cyclical graphs with ScriptableObjects is a common use case for me for a variety of systems, and I have not found an alternative in godot which does not feel like a hacky workaround (such as using string references) which would be terrible to work with at scale.

For a quick example, if I wanted to make a dialogue system in unity and wanted a character to repeat themselves, I could very simply just have a DialogueData ScriptableObject reference the first DialogueData as the next one to go to when the player chooses the "please repeat that" option. It seems that godot cannot match this functionality and it makes me concerned that I will not be able to build robust and extensible systems for larger games.

Calandiel commented 9 months ago

This is an issue that I ran into and is seriously making me consider leaving the engine and returning to unity until this is fixed. Making bi-directional/cyclical graphs with ScriptableObjects is a common use case for me for a variety of systems, and I have not found an alternative in godot which does not feel like a hacky workaround (such as using string references) which would be terrible to work with at scale.

For a quick example, if I wanted to make a dialogue system in unity and wanted a character to repeat themselves, I could very simply just have a DialogueData ScriptableObject reference the first DialogueData as the next one to go to when the player chooses the "please repeat that" option. It seems that godot cannot match this functionality and it makes me concerned that I will not be able to build robust and extensible systems for larger games.

Use uids, they serialize to something similar to what unity stores internally for their references. Alternatively, something like the Pandora plugin if you want a pretty ui for it.

I'm working on a first person open world rpg and was in a similar spot to you some time ago (see my posts earlier in this thread) but its been smooth sailing overall. Work with the tool, not against it.

That being said, with Clyde and Dialogic existing, using scriptable objects / resources for dialogue data seems really odd to me.

Go-Ba commented 9 months ago

Use uids, they serialize to something similar to what unity stores internally for their references. Alternatively, something like the Pandora plugin if you want a pretty ui for it.

uIDs are cryptic strings, it seems impossible to see which resource that uID refers to from the editor which will make it extremely difficult to navigate your reference graph. I'll check out that addon, but I'd rather not have to rely on the development and upkeep of a random person's alpha addon for something as fundamental as being able to structure my data in a bidirectional graph

Calandiel commented 9 months ago

Use uids, they serialize to something similar to what unity stores internally for their references. Alternatively, something like the Pandora plugin if you want a pretty ui for it.

uIDs are cryptic strings, it seems impossible to see which resource that uID refers to from the editor which will make it extremely difficult to navigate your reference graph. I'll check out that addon, but I'd rather not have to rely on the development and upkeep of a random person's alpha addon for something as fundamental as being able to structure my data in a bidirectional graph

If you want both bidirectional references and human readability, overwrite the way the field is displayed and display it directly. This is also what you had to do in Unity until version 5.2

See pages on customizing the editor with plugins and/or getters/setters.

A quick way to do it is to have a get/set for your uuid, then a second exported string under it (say, dialogue_uid and dialogue_name below it). Then, the setter for uid can retrieve the name of the file (or an even more semantically meaningful name, if your data has one) and write it to dialogue_name, keeping it both human readable and allowing bidirectional references.

dreadicon commented 5 months ago

Going to add my 2cents here - while it is generally not a best practice, circular references are a perfectly legitimate tool that are fine if used sparingly and correctly. Also, saying that a work-around exists doesn't mean a smoother implementation should be passed on. In this case, uID strings are prone to breaking silently, and requires a plugin to even be human readable. It may be an easy plugin, but it still requires the user to devote time and effort into learning the plugin system.

I ran into it with a network based map. The map has nodes the player can move between. Each node can connect to other nodes. It generally makes sense that each node would have a bi-directional reference to the nodes it connects to. Or lets take biomes; the nodes belong to certain biomes. When setting up a node's data as a resource, it should know what biome it's a part of, right? But biomes also need to know what nodes they can spawn. Thus, another circular reference. I could have made the biome keep track of the spawned nodes, but now the biome is stateful when that was entirely unnecessary if each node minded its own business.

Sure you can hack around it with things like brittle uIDs or asset path references. Or with a factory pattern. But what if you're not using a factory pattern because it doesn't suit the rest of what you're doing? The more high level architectures you mash together for particular cases, the less clear the architecture is and the less benefits you can reap. Also, because resources are not typically meant to be created and destroyed during runtime, is there really a problem with the objects being held onto in memory? At what point is it no longer the engine's design's job to police memory if that's such a concern in the edge case where resources COULD cause a memory leak?

Is this a must-have feature? Nope, not even close. But given how easy a solution is, and that it's a fairly common edge case, I still think this is a good feature request. +1 from me :)

AThousandShips commented 5 months ago

So how do you intend to handle the memory leaks of circular references? That applies regardless if it's stored or not, you simply shouldn't have circular references, they're not just "not a best practice" they're actually bad practice and don't work, they always cause memory leaks, that's the whole thing of them

If you mean using weak references that's a whole other thing, that's not circular references

I haven't seen a concrete idea here of how to accomplish this without actual circular references in the resulting data, the resulting Resource is RefCounted so it will not work by circular references

Will the engine somehow insert a WeakRef at the right point? How does the data in this get accessed?

This issue is something you should work around in building your structures, not in making the engine work it out for you IMO

My suggestion for an improvement would simply be to allow serialising weak references, that'd solve these edge cases

But in general this to me stems from actually misusing the system, resources shouldn't link to the resource containing them, that's just not how resources should work, they should be shared, and not care where they are in the relationship

dreadicon commented 5 months ago

So how do you intend to handle the memory leaks of circular references? That applies regardless if it's stored or not, you simply shouldn't have circular references, they're not just "not a best pracitce" they're actually bad practice and don't work

If you mean using weak references that's a whole other thing, that's not circular references

I've used circular references all the time in data holders; like in the example I gave, each mapNodeDefinition resource has a reference to a biomeDefinition resource. They are static data that isn't meant to change at any point during gameplay, nor should they go away. What's more, at least in .NET's case, it detects small circular references (a loop consisting of 2-3 objects) without much trouble during its mid-depth collection passes.

I love weak references, but as you noted the engine doesn't understand them. There's no way to have a [WeakExport] option afaik (which would be a fantastic solution btw), and till someone approves the PR to support it for C# at least, there's not even Interface based support, which is Design By Contract 101 and a pillar of modern coding.

Really, it's not the end of the world, like I said. But there's perfectly legitimate use cases where the architectural work-around would introduce as much or more code complexity as prohibiting all forms of circular reference saves. A critical feature this is not, but a useful one it is, regardless of if it's restricted to Resources, a wrapper on uIDs with some added error-checking for broken references, or an implementation of WeakRef in some form - all of those would work, with the latter being probably the best solution.

dreadicon commented 5 months ago

But in general this to me stems from actually misusing the system, resources shouldn't link to the resource containing them, that's just not how resources should work, they should be shared, and not care where they are in the relationship

How then would you suggest to store the game's read-only data for biomes which must know their map nodes so that map nodes can be generated, and a method for identifying the map node's biome is also required?

AThousandShips commented 5 months ago

Don't use a resource? Use some other data that isn't designed to be shared, resources are supposed to be shared and be location agnostic, that's their whole design and purpose

Or just use an identifier, I'm not sure about your specific game design, but the simple fact is that resources are not designed to be in this kind of direct relationship, you probably are using them for something they aren't designed for

Having:

Are IMO likely indicators that you're using a resource for something it's not designed for, and you should instead use just an Object to handle your data, that saves you a lot of overhead anyway

Go-Ba commented 5 months ago

and you should instead use just an Object to handle your data, that saves you a lot of overhead anyway

What is the method to instantiate an Object in the filesystem like a resource and edit it within the engine's inspector like a resource?

dreadicon commented 5 months ago

Don't use a resource? Use some other data that isn't designed to be shared, resources are supposed to be shared and be location agnostic, that's their whole design and purpose

Or just use an identifier, I'm not sure about your specific game design, but the simple fact is that resources are not designed to be in this kind of direct relationship, you probably are using them for something they aren't designed for

Having:

  • A resource that knows of the Node it's used by
  • A resource that knows what resource it's used in

Are IMO likely indicators that you're using a resource for something it's not designed for, and you should instead use just an Object to handle your data, that saves you a lot of overhead anyway

Problem is POCOs and JSONs have no gui, no drag-drop, no visual wizard unless you build it, serialization and all. And as I outlined earlier, just because you CAN work around something doesn't mean it would be nice to NOT have to work around it.

A resource not knowing what nodes it's used in is sensible; I view resources as your ROM - stateless; data configured before the game runs, or maybe deriving some results from a pre-set configuration, while Nodes hold state.

But if Resources are stateless, why is it a problem to have circular references? I'm still a bit lost on that. I just can't picture a use for Resources which allows for unbound memory leaks; even if you clone one, it'll reference the original other half of the pair, which references the original that was cloned, not the new clone.

If Resources aren't for ROM data to be easily configured by designers and developers without any arbitrary limitations to that purpose, then what is? Tweaking stats and values, configuring sets and groups, this stuff is like 40% of what designers do in the editor. And for that matter, what use ARE resources useful for then from a designer perspective? Nothing about editor-time set circular dependencies with other resources makes them less share-able or location agnostic; why would it? If any resource can reference another resource, making that relationship reciprocal is only significant for garbage collection, which as I mentioned is only relevant to a slightly increased footprint, not an ongoing leak. Unless Resources are loaded and unloaded in the background when asked for by some process I'm not aware of?

aqwalnut commented 5 months ago

@dreadicon Can you link the PR for WeakExport? I want to follow it but can't find it.

AThousandShips commented 5 months ago

You get a memory leak with a circular reference, that's not an unbounded leak

Just that if A references B which references A they can never be freed as they are reference counted, see the page I linked above, it's basic memory management for these purposes, also the C# side might likely not work well with this either as the resources are handled on the engine side not in C#, so the memory handling isn't guaranteed to resolve circular dependencies

caimantilla commented 5 months ago

My notifications are blowing up so just to chime in with an example from a shipped game of why this would be useful: Cassette Beasts mostly uses resource references, with paths specifically being reserved for edge cases like a monster's reference to a loot table (which can potentially contain monsters). But one case which would cause a lot of pain made itself clear very quickly as I dug around: monsters store evolution data within themselves, as they should... using resource exports. If the developers ever want to add any kind of backwards evolution, this would need to be reworked, and mods might become incompatible. Circular references might not be necessary from a code standpoint, but it's inevitable that they come up when designing a complex game as the only way to avoid convoluted solutions. I've personally had to swear off these kinds of references, instead toying with _get_property_list and static methods which return an enum-style hint for files in a directory, but this is still only appropriate for database-style use cases like a list of items, not level data like points in a map, and direct object refs would be much easier to manage... I still end up holding the ref in a member variable anyways for performance and convenience.

AThousandShips commented 5 months ago

To clarify a bit what I meant by how resources "should" be used, because I might have been a bit unclear, isn't that it's bad to use them in some ways, but that it's important to consider how they were designed to work

It's not bad in itself to have a two-way relationship with resources or objects, but resources weren't designed with that in mind or designed around that, so it's an unusual feature that you need to handle in your own way, because it doesn't fit the wider, main usage of resources

Now for true circular references, i.e. "RefCounted all the way down", I'd say there won't and shouldn't be any system for handling that, as they simply are not valid, you should always break the circle somewhere with a WeakRef

I'd say that adding support for serializing WeakRef references would be great, they'd simply be the same as the normal way but be deferred and handle just connecting it, possibly with some syntactic sugar to make it easier to use them without having the get_ref method

But for the "true" ones they simply aren't something that should be done, because they are bad code, because they don't work and leak memory

Note that a RefCounted referencing a non-RefCounted, like a Node, is not a circular reference and is perfectly valid

dreadicon commented 5 months ago

@dreadicon Can you link the PR for WeakExport? I want to follow it but can't find it.

Sorry if my comment was a bit meandering, but I'm not aware of any implementation of a WeakExport or such - it was just an idea. The PR I was talking about is to add support for C# Interfaces, which are imho even more important than circular references designs.

To clarify a bit what I meant by how resources "should" be used, because I might have been a bit unclear, isn't that it's bad to use them in some ways, but that it's important to consider how they were designed to work

It's not bad in itself to have a two-way relationship with resources or objects, but resources weren't designed with that in mind or designed around that, so it's an unusual feature that you need to handle in your own way, because it doesn't fit the wider, main usage of resources

Now for true circular references, i.e. "RefCounted all the way down", I'd say there won't and shouldn't be any system for handling that, as they simply are not valid, you should always break the circle somewhere with a WeakRef

I'd say that adding support for serializing WeakRef references would be great, they'd simply be the same as the normal way but be deferred and handle just connecting it, possibly with some syntactic sugar to make it easier to use them without having the get_ref method

But for the "true" ones they simply aren't something that should be done, because they are bad code, because they don't work and leak memory

Note that a RefCounted referencing a non-RefCounted, like a Node, is not a circular reference and is perfectly valid

I do get that, and I'll concede that even if a memory leak is or could be bound in scope it shouldn't really be supported in the core engine. But as myself and others have mentioned, Godot currently lacks a solution for circular references in data structures, and since Resources were designed with IMO a bit overly-broad use case in mind it may be appropriate to consider making a new type, either a sub-type of Resource or a new type all together that can handle circular references. Again, I agree WeakRef is ideal from a technical perspective in its simplicity and elegance, but I believe the implementation of that would be challenging for a variety of reasons (different languages handle (or dont handle) weak references differently, WeakRefs often require wrapping/unboxing every time they are accessed, etc).

I do in fact know how memory leaks work; I previously worked on a custom in-house C# engine where memory leaks were a constant issue to track down and solve, and most recently built a custom event bus which uses weakref to eliminate all the boilerplate besides a parameterless ".subscribe()" call and has no way to leak memory (I love Godot's signals, but I want my architecture to be as platform agnostic as possible for reasons).

So, all that to say I don't think we necessarily disagree on the practicalities of the design need or the practical hurdles in the end here - wrapping back to my original conclusion: Do I think this feature is the biggest bang for development buck in Godot? Nope. Would it be nice, practical, and useful in a number of cases? Yep. One thing I DONT want to see from my time in Unity is 'Sorry, that's the design we picked a long time ago so that feature is never happening' - imho Unity's unwillingness to moderately break backwards compatibility every 2-5 years in order to iterate on core elements was a major part of why it's so convoluted.

AThousandShips commented 5 months ago

Circular reference detection is error prone and expensive so I'd say it's better to just rely on users using references properly, over adding a lot of overhead to solve incorrect usage IMO (it would also almost certainly not work with extensions as they do their own handling of data and therefore wouldn't be analyzeable)

Sorry, that's the design we picked a long time ago so that feature is never happening

And that's a valid desire, but it's not something to fear here, that's not what I'm saying :)

Go-Ba commented 5 months ago

It doesn't seem helpful to continually return to "it wasn't designed for that", "people just shouldn't do things that way" and "that's an incorrect usage of references" when it's been explained multiple times that:

  1. Bidirectional graphs are a common and very elegant structure that allows you to do important things without much overhead
  2. Resources and export fields are the only native tool for storing data about the game with proper integration in the engine's inspector
  3. Other engines (i.e. Unity) already allow this pattern and it's very useful and intuitive.

If resources were specifically designed to have such limitations then they shouldn't be presented as the main and sole in-engine tool for storing data and a new one should be implemented. But I don't see what specific use a structure which only allows directed acyclic graphs has thus a discussion about changing the functionality of resources is instead taking place.

"users can just write their own solution" is not a valid answer to a lack of functionality in an engine. Especially when this is an error which a lot of beginners face because having two things reference each other is a normal and intuitive way to approach a lot of problems.

AThousandShips commented 5 months ago

SO I'll just leave a final comment here and then leave this discussion, and that is that this needs to be clarified a bit: If we are talking about simply supporting actual circular references in resource exports, then there needs to be a dedicated proposal for adding some form of garbage collector or other management system to the engine core or GDScript first, because if you are wanting support for "Resource A has a reference to Resource B which has a reference to Resource A" then that's not something we will, or should, support with the current way the engine works, those circular references are bad practice and do not work and will leak memory. There's no nice way around it, those are not opinions but facts about how the engine works. That needs its own discussion, which is going to involve weighing the costs (because this kind of GC isn't free, nor is it perfect), and then we can discuss how that should work with resources.

Currently resources (at least text based ones) traverse down resource exports and exports the deepest resources first, this is because no resource should have a circular reference, so no need to have an elaborate system for this, it simply works because we assume people use references properly.

Now for the second option, which I fully support, namely to add support for serializing WeakRef properly, this wouldn't be too complicated IMO, and would work and wouldn't break things or be bad code.

With this you could then have either an automatic system exposing this property as if it was a non-weak reference, hiding that it is underneath, or you could use little helper exports to do it for you, like so:

@export var _my_export_weak : WeakRef

var my_export: MyResource:
    get():
        if is_instance_valid(_my_export_weak):
            return _my_export_weak.get_ref()
        else:
            return null
    set(value):
        _my_export_weak = weakref(value)

And you'd just indirect the access to ensure it works correctly

That's all I have to say on this, hope you have a good day

AThousandShips commented 5 months ago

Did some experimentation with adding weak reference serialization and while I made good headway I've come to the view that it's less critical and that the relevant use cases are few, and that the cases when you really do need to have weak references to break chains they can be done with other means than built-in serialization support, if there's a real demand for the weak reference export support I can pick that work back up, but it'd involve breaking compatibility with the file formats and other issues that I feel aren't worth it and wouldn't be approved by reviewers

Instead I'd say that this should be handled by code, for example if you have a resource A that contains a resource B property and you need B to know A you should probably solve that by having A have a:

set(val): 
    my_val = val
    if is_instance_valid(my_val):
        my_val.my_parent = weakref(self)

To handle the relationship management, that would solve the need and would work just as well, without any complex changes to the resource writing and loading

Adding a system for weak references to serialization would, I believe, be possible, and not too difficult in my experimentation so far, but it would be potentially fragile, and I do believe this is a niche feature and can (at least in most cases) be worked around

The one area that is a bit weak currently is exporting arrays of resources, that is a case where you need some custom management to handle the relationship, as at least with the inspector you don't (currently) have specific set/get features to handle that

This can be handled in a few different ways, including custom array exports that use _set/_get to handle the actual data by paths, like my_sub_resources/name, etc., but other than that area I'd say that using setters to handle the parent relationship is a sufficient workaround for this, using WeakRef to manage it

If there's further interest in that part, or issues with using any of these workarounds, I'd be interested in looking back into this, but I don't currently think such an improvement would be approved, and I don't have the time to work more on it right now as it's slightly involved (though entirely possible)

Tl;dr; There are workarounds for setting WeakRefs on assigning properties, proven to work as I literally used them to work with my attempts to write a system for serializing WeakRef

anvilfolk commented 5 months ago

Oooh, this one looks difficult! I just caught up on the discussion and would like to provide an overview. Hopefully it's not too out of my lane, and people find it helpful, since I think there's a couple of entrenched positions here that may be having a hard time connecting.

Wanting a system in Godot where you can define data with circular references and editor UI support is super legitimate! Especially for teams with folks who aren't programmers. Maybe it isn't what resources were designed for, but it's what's available and it's frustrating that it doesn't work out of the box. Heck, it's probably what I'd try to do as well! 😅

And it's also frustrating that for Godot developers who are intimately familiar with the Resource design goals (and therefore its implementation limitations) to see people use them for what they weren't intended, then complaining it doesn't work and being insistent a "fix" be made 😞

So I think it's important to acknowledge these dual frustrations, which also indicate there may not actually be a perfect solution here: 1) The RefCounted system intentionally underpins much of Godot's core, including Resources, and thus avoids many evils. These include memory leaks, error-prone manual memory management, maintaining complex garbage collection code and therefore the infamous stuttering problems that are direct consequence of having to memory-manage data with circular references. 2) This also means there are fundamental technical limitations to this system, and it may not even be possible to adapt it to allow for first-class circular references. This is why the proposed solutions must be second-class solutions like WeakRefs and indexing with UIDs, which is why they feel unintuitive and insufficient to many, especially when other engines support it (and thus often its associated issues, like memory-management stutters).

As the saying goes, you can't shove a (circularly referenced) square peg into a (refcounted) round hole! At least not without breaking a lot of things 😬

So the team either decides to implement a completely new, non-RefCounted resource type, which would need new serialization/deserialization code, editor support, and dedicated memory management, which is a big effort with a huge cost to maintainability and a lot of code surface for bugs...

... or we live with second-class solutions like WeakRef which isn't intuitive for directed cyclic graphs with no parent-child relationship indicating the direction of the WeakRef; or with UIDs which are prone to manual errors are harder to work with due to the indirection.

This is my understanding of the situation, and I hope it helps folks engage with this issue in a more nuanced and less antagonistic way. We ultimately truly are all trying to make Godot better :)

HeavensSword commented 5 months ago

In the short-term I'd be happy with just a usability enhancement such as:

Again, I'm not a Godot expert, but a Unity refugee; so if there is a quick way to do this myself or if it's possible to do this already in some manner, please share!

aqwalnut commented 5 months ago

Speaking of uid drag and drop.... is there a particular reason the "shows up in inspector" functionality of Export, and the "serialization and memory is managed this way" functionality of Export are tied together?

Just being able to separate these functionalities would make it easy to do a lot of things without needing to write a custom inspector... including the thing mentioned above.

Natthapolmnc commented 5 months ago

Hi, Idk if I try this correctly but I have made this code for the array of resource that support exporting.

@export var connectedStates:Array[Resource]:
    set(new_value):
        var val=[]
        for i in new_value:
            val.append(weakref(i))
        if is_instance_valid(connectedStates):
            connectedStates=val
    get:
        return connectedStates

it resulted in many lines of error like

E 0:00:00:0702   _parse_ext_resource: res://scripts/class/Movement/Move/PlayerMove/sprintState.tres:40 - Parse Error: [ext_resource] referenced non-existent resource at: res://scripts/class/Movement/Move/PlayerMove/walkState.tres
  <C++ Source>   scene/resources/resource_format_text.cpp:162 @ _parse_ext_resource()
E 0:00:00:0702   load: res://scripts/class/Movement/Move/PlayerMove/sprintState.tres:40 - Parse Error: [ext_resource] referenced non-existent resource at: res://scripts/class/Movement/Move/PlayerMove/walkState.tres
  <C++ Source>   scene/resources/resource_format_text.cpp:706 @ load()
E 0:00:00:0702   _load: Failed loading resource: res://scripts/class/Movement/Move/PlayerMove/sprintState.tres. Make sure resources have been imported by opening the project in the editor at least once.
  <C++ Error>    Condition "found" is true. Returning: Ref<Resource>()
  <C++ Source>   core/io/resource_loader.cpp:273 @ _load()

I don't know if I'm using the weakRef class correctly or I don't know the detail of is_instance_valid. Like I know it to check if the Object is in-memory but Idk when will the Object be deleted from the memory or instantiate in the memory.

Did some experimentation with adding weak reference serialization and while I made good headway I've come to the view that it's less critical and that the relevant use cases are few, and that the cases when you really do need to have weak references to break chains they can be done with other means than built-in serialization support, if there's a real demand for the weak reference export support I can pick that work back up, but it'd involve breaking compatibility with the file formats and other issues that I feel aren't worth it and wouldn't be approved by reviewers

Instead I'd say that this should be handled by code, for example if you have a resource A that contains a resource B property and you need B to know A you should probably solve that by having A have a:

set(val): 
    my_val = val
    if is_instance_valid(my_val):
        my_val.my_parent = weakref(self)

To handle the relationship management, that would solve the need and would work just as well, without any complex changes to the resource writing and loading

Adding a system for weak references to serialization would, I believe, be possible, and not too difficult in my experimentation so far, but it would be potentially fragile, and I do believe this is a niche feature and can (at least in most cases) be worked around

The one area that is a bit weak currently is exporting arrays of resources, that is a case where you need some custom management to handle the relationship, as at least with the inspector you don't (currently) have specific set/get features to handle that

This can be handled in a few different ways, including custom array exports that use _set/_get to handle the actual data by paths, like my_sub_resources/name, etc., but other than that area I'd say that using setters to handle the parent relationship is a sufficient workaround for this, using WeakRef to manage it

If there's further interest in that part, or issues with using any of these workarounds, I'd be interested in looking back into this, but I don't currently think such an improvement would be approved, and I don't have the time to work more on it right now as it's slightly involved (though entirely possible)

Tl;dr; There are workarounds for setting WeakRefs on assigning properties, proven to work as I literally used them to work with my attempts to write a system for serializing WeakRef

AThousandShips commented 5 months ago

That won't work, look at my code, you need a backing structure of WeakRef, further you can't export the non-weak one, as it'll still have circular references, I'd suggest:

var _connectedStatesWeak : Array[WeakRef]

var connectedStates:Array[Resource]:
    set(new_value):
        var val : Array[WeakRef]
        for i in new_value:
            val.append(weakref(i))
        _connectedStatesWeak = val
    get:
        var ret : Array[Resource]
        for i in _connectedStatesWeak:
            if is_instance_valid(i):
                ret.append(i.get_ref())
        return ret

But haven't tested

Natthapolmnc commented 5 months ago

I see so if I wanted it to be exported I have to export the array of string instead and used it to update the value of real array??

I have this as solution. Pls share if anyone has a way to improve it. It is actually just the uid method mentioned before. I just wanted to put a milestone of how to solve the issue and have both array and exportable property

@export var connectedStatesRf:Array[String]:
    set(new_value):
        connectedStatesRf=new_value
        var val:Array[Resource]
        for i in new_value:
            val.append(load(i))
        connectedStates=val

var connectedStates:Array[Resource]
robbertzzz commented 2 months ago

I get that this is an issue in the editor due to how nested resources are displayed, but is there any reason why the file format itself doesn't allow for circular references? The .tscn format allows circular references since they're really just UID and/or NodePath strings, it seems odd that .tres sets the value to null. The editor not displaying a resource shouldn't stop the file format from storing a resource.

Note, this exact thing has a bug report at https://github.com/godotengine/godot/issues/89961, but I was referred here to discuss it.

AThousandShips commented 2 months ago

See above:

Currently resources (at least text based ones) traverse down resource exports and exports the deepest resources first, this is because no resource should have a circular reference, so no need to have an elaborate system for this, it simply works because we assume people use references properly.

It would require reading and writing text resources in a non-linear way which is much more complicated, I've tried implementing it with weak resources but it was very fragile and relied on some hacks, so it's not something that's necessarily easy to implement

The reason it isn't supported, and no one has written a non-linear storing of them in the text format is because they're not encouraged or supported in the system, so the serialisation can be made much cheaper and faster

If we either implement support for serialising weak references (what I strongly recommended) or add some way to handle circular references in the engine itself (which would be needed, as they're not currently supported safely, they cause serious memory and resource leaks) with a dedicated way to serialise them, though again this would be complex

This was discussed some time ago in the contributor chat, when I was testing out a weak reference feature, but the support was low so I abandoned the idea as the core team didn't support the feature at the time

There are workarounds if you want either to use weak references or full reference cycles by storing paths or using initialization, some examples are available above in the thread

robbertzzz commented 2 months ago

Circular references just turn a tree structure into a graph structure, which aren't hard to serialise at all. The main difference is that you can no longer serialise from the bottom up as is the current approach as there's no clear "bottom". What you do have is a clear starting point, namely the resource being serialised, and from there on it's a matter of keeping track of which resources have already been serialised and which still need to be addressed. This is a very common problem with a straightforward solution, simply keep an open and closed set, loop over the open set, add unaddressed resources to the open set and move anything to closed when it has been addressed.

This approach is common in all kinds of graph traversals, but for an easy-to-find implementation of it I'd recommend the A* algorithm.

AThousandShips commented 2 months ago

If you have a workable solution you're welcome to open a PR for it, but I'd suggest using WeakRefs to break the cycles so it doesn't introduce circular references just by loading a resource, as I don't think that would be accepted, but the interest in this from the team has been low when previously raised

But again in my testing integrating this into the existing system without introducing weak points or more overhead is not trivial

lazyvainglory commented 1 month ago

I’ve been grappling with resource-related issues recently. While working on a dialogue system, I encountered frequent missing dependencies especially with my ‘conversation resources’ which references other 'conversation resources.' To address this, I transitioned to using .json files. Today, I attempted to create a state machine for my enemies using ‘state resources’ that can reference other ‘state resources,’ but it broke again. I’m genuinely puzzled. And here I thought Resources = Scriptable Object