Allow manually freeing instances of RefCounted

dev-mico commented 2 weeks ago

Describe the project you are working on

I'm building an open-world, top-down procedurally generated RPG.

Describe the problem or limitation you are having in your project

We can't free RefCounted's, which makes managing them unnecessarily complicated.

For instance, we're using synchronized RefCounted objects for some multiplayer stuff. However, managing the state of these refcounted's is exceedingly convoluted, because if I want a RefCounted to be "deleted,"I have to emit a synchronized signal on all peers to dereference the refcounted, or as far as I understand, use WeakRefs (haven't tried the latter yet - will return with my results).

An example use case from our game - suppose we have some Inventory RefCounted object as part of a shopkeeper. This inventory should be deleted when the shopkeeper is. However, if anything else has a reference to the inventory - for example, perhaps there is some script that needs to keep track of what items are sold in which shop for a map display - the inventory will persist in memory despite the shopkeeper being deleted. This is, again, particularly painful in multiplayer with strong coupling between a synchronized RefCounted object and some other object.

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

The feature is to allow free'ing refcounted's.

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

I'm not entirely sure how the low-level details will work, but presumably you could simply free the memory location of the reference in the same way you would with any other node & have it become 'previously freed' like with queue_free'd nodes.

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

Kind of? I'm going to try restructuring my infrastructure around returning WeakRefs where possible (or just use Objects). In either case, this sort of defeats the purpose of a RefCounted object - which exists for convenience.

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

It's a core modification to GDScript and it seems quite bizarre to me that this isn't allowed. I can't see a single benefit to having RefCounted's exist and also not be maually free-able.

AThousandShips commented 2 weeks ago

Are you sure you need RefCounted for these uses? Is a plain Object not better suited? It sounds like your use case means you don't actually need a RefCounted

dev-mico commented 2 weeks ago

@AThousandShips Thanks for the recommendation. That's what I concluded as well after some initial tests with WeakRef's, and was my original plan. With how RefCounted's are built currently - yes, objects are better. But my proposal is that it doesn't need to be this way.

If I may provide more information, a more concrete use case from my game is as follows:

We have a separate logical component for our serialized nodes that describes its state. This exists as a RefCounted.
These logical components are used as lightweight representations for our objects, so they may exist with or without the object. To this end, we have a singleton to manage them. This Singleton cannot free these refcounted's without first getting permission from everyone who references them, requiring rampant duplication or a refactor to use an Object and miss out on the 'niceness' of not having to worry about memory management for lightweight objects.
If we switch to objects, there also exist lots of other isolated bits of logic - e.g., unit tests, or other backend things that are too complex to explain concisely - on these components, which will now be reworked to need manual memory management.

I'd say none of this particularly needs a RefCounted. In fact, I'd argue you rarely - if ever - need a RefCounted. They're just very nice-to-have. But isn't that... kinda the point? lol

I guess I'm just not convinced that there's a good reason for not allowing the free'ing of these RefCounteds - or at minimum, supplying a more 'heavyweight' variant of the RefCounted that has this functionality.

Hopefully you see where I'm coming from.

AThousandShips commented 2 weeks ago

What more than just assigning null to a RefCounted would you need? Force freeing all the counts and forcing it to be freed would go against the fundamental idea of a RefCounted

dev-mico commented 2 weeks ago

@AThousandShips That may be true with the current definition of a RefCounted. Let me rephrase my proposal.

There is a strong argument to be made, I think, for objects that allow both reference counting and manual memory management. As I said, the only fundamental use case I can see for RefCounted's is convenience. If the idea is to provide convenience, why not extend this to objects in the same way?

In any conceivable case where we can use an Object, we could also use this 'RefCountedV2" (or whatever we'd call it). However, developers wouldn't have to worry about memory management edge cases. It's the best of both worlds, and even if there's some associated overhead, I think lots of developers would be willing to make that tradeoff to keep their code simpler.

That said, here's what the proposal would be:

Alter the 'fundamental idea' of the RefCounted, such that free'ing it is an option, OR
Create a new class - like a RefCounted - that can be also free'd manually.

If I may ask - both for me and for anyone who may request this feature in the future - why wouldn't we want something like this, since I suppose there is some reason it doesn't exist? Does it have some unseen computational complexity or memory overhead that I'm not aware of?

Thanks for your quick responses BTW.

Also thanks @Calinou for renaming the feature proposal.

AThousandShips commented 2 weeks ago

As a simple working use I'd suggest making your own class extending RefCounted and have it hold an Object reference and allow clearing it (and freeing it on the RefCounted being freed)

I'm not against this fundamentally, but the idea is extremely unorthodox and I've not seen it in any smart pointer/ref counted library anywhere, part of the core idea of it is that as long as you hold a RefCounted instance the object will be valid, it's a fundamental guarantee that's part of the concept itself, I'd be interested in seeing some example of a library that does provide this feature (the RefCounted seems to be heavily based on the general "shared pointer" concept of C++, which does not allow this, neither the standard library one or the Boost library one), that'd be an indication that this idea has been generally suggested before

The issue in my mind isn't overhead, there isn't really any extra cost to this that I can see with an implementation, the issue I have is safety and the violation of the guarantee above, and that would mean overhead, namely in having to add checks so that the RefCounted wasn't freed along the way, there'd also be some complexity added to the usage, because you'd have sort of a "three state" situation with the RefCounted in GDScript:

An empty RefCounted, i.e. it isn't shared with anything, it's equal to null
A valid RefCounted pointing to a freed object
A valid RefCounted pointing to a valid object

Currently the second option isn't possible (without messing with things), this change (if we don't create a new type that allows freeing as well, making it opt-in) would make existing code unsafe and would require some check on the RefCounted to tell the two options apart, the engine itself is also written with the assumption of safety here so we'd have to rewrite quite a lot of the engine if this was applied to the basic RefCounted system itself and allow you to free these, for example if you provide a Mesh to a MeshInstance3D and you decide to free the Mesh

Just some thoughts and ideas, and a suggestion for how to achieve this on your own as a simple workaround

Edit: Realized that there will be a non insignificant overhead for this generally as we can no longer rely on that the pointer in the Ref data itself is valid, so we need to use an ObjectID to fetch it on every access which is pretty expensive comparatively, this would make general access to these types slower if we allow it generally, unless we insert some kind of interface between it with a pointer in it, but that's pretty complicated and still has overhead

dev-mico commented 2 weeks ago

@AThousandShips thanks for the detailed response. I ended up using Objects in my project. And, funnily enough, I have a TODO to build the exact workaround you described with the layered "access reference."

I think there are other languages where objects are garbage-collected - including Python, which Godot seems to be inspired on - but Python also allows deleting objects before they are fully dereferenced.

I suppose this is more of a design philosophy thing. I think it would probably make sense to make this its own class, if it's implemented. If it's too much trouble, no worries, the workaround isn't that bad.

For anyone else experiencing this issue, just do what @AThousandShips suggested above. I'll leave it up to the Godot team to decide whether it's worth implementing at this time, unless other people come in and agree with it.

Thanks!

AThousandShips commented 2 weeks ago

Thank you for a productive conversation! It made me think deeper on the actual reasons for the way things are the way they are rather than give a "it just is that way" which is important!

Spartan322 commented 1 week ago

Not sure how much it contributes to this, but my two cents, a lot of the cases where I felt the desire to delete RefCounted objects it was specifically me trying to store a centralized place for an auto freeing reference and then weakref it almost everywhere kind of like a unique pointer in C++, having a "shared pointer" and functionality of a "raw pointer" in GDScript while lacking the capacity of a self-freeing owned reference 'smart pointer'-like object (unique_ptr) might be part of the issue. Granted you can write your own using RefCounted as mentioned to accomplish this but that can be annoying and the lack of any real generic support in the type system itself for these things, (like #9174 and #1207) as well as it is a bit of unnecessary overhead to do it via RefCounted but I suppose that's the least of concerns for this. I'm not exactly sure how that specific case could be solved in Godot or if it even should, and perhaps it could be argued to be a separate issue, but the description of this issue sort of feels like that's what was being attempted.

AThousandShips commented 1 week ago

I'd say that's very much a different issue, support for a unique pointer style thing is pretty distinct as they should never be shared, this involves having multiple smart_ptr and freeing all while still having references, not having the ability to force free a single instance

A unique_ptr equivalent would be useful, but since there's no move semantics in GDScript I don't see how it would be possible

dev-mico commented 6 days ago

@AThousandShips should we close this, leave it open, or move it to a feature proposal? Don't want to leave clutter around for you guys to handle lol

AThousandShips commented 6 days ago

This is a feature proposal, so nothing to move, but it's up to you, this hasn't gotten more attention or feedback from different teams, but if you feel you've got ideas or solutions from this or a different understanding that's a good reason to close

But other people in the development teams might disagree with me or have other ideas for how to approach this, and there could also be room for documenting tricks and ideas as well

godotengine / godot-proposals