godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.26k stars 99 forks source link

Add `is_valid()` method to server APIs #12392

Open tvenclovas96 opened 1 month ago

tvenclovas96 commented 1 month ago

Describe the project you are working on

RTS-y game that manages a lot of data using the servers via ECS

Describe the problem or limitation you are having in your project

It's pretty straightforward to manually create entities in the Physics or Rendering server API and working with them directly via the provided RIDs while skipping the Node middleman layer. But accordingly, the held RIDs need to be freed manually, which can be error-prone.

While it's possible to create a setup where RIDs are automatically freed when no longer needed, it's likely to still be finnicky and at a non-negligible performance cost, which undermines the value of using the server API directly. As such, it would be better to constrain it to debug scenarios, and used to validate, rather than replace, manual freeing.

Unfortunately, currently there is no way to check if a RID handle is valid, besides trying to do a server operation and hoping it won't or will give an error.

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

Add a bool is_valid(RID p_rid) method to servers, which allows checking if a RID represents an existing server entity or not. This could potentially return some type info, like an enum signifying what kind of entity is represented.

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

Example:

bool GodotPhysicsServer2D::is_valid(RID p_rid) const {
    return shape_owner.owns(p_rid) ||
            body_owner.owns(p_rid) ||
            area_owner.owns(p_rid) ||
            space_owner.owns(p_rid) ||
            joint_owner.owns(p_rid);
}

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

It's very Core

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

It's very Core

clayjohn commented 1 month ago

I understand the problem and agree it is something worth solving, but the proposed solution is a total non-starter. The Physics and Rendering Servers can run on a separate thread for performance reasons. Querying them every time you check the validity of an RID would cause a thread synchronization and mostly remove the performance benefit you get from multithreading. In other words, you would be forcing the servers to run in lockstep with your game.

The best practice right now is to set your RID to RID() immediately after freeing then just check is_valid() on the RID itself before using it.

While this approach is not foolproof, it is significantly simpler than accessing the Server APIs, and does not hurt performance.

for example:

var my_rid
...
RenderingServer.free(my_rid)
my_rid = RID()

...
if my_rid.is_valid():
    # do something with my_rid
tvenclovas96 commented 1 month ago

Yes, querying for data would cause a sync, but if done during server "sync" periods, e.g. during _physics_process() for the physics server, there should be little to no perf impact. The servers already expose a variety of different get methods that cause full server sync as it is. I think it's reasonable enough to expect users who are interfacing with the servers directly to be advanced enough to understand the implications of what they're doing.

But I do understand reservations about adding a method with these kinds of performance implications while providing comparatively little information back to the user.