RPC with QueueFree Gives "Node Not Found" on Server

lbragile commented 10 months ago

Tested versions

Godot Engine v4.2.stable.mono.official.46dc27791

System information

Windows 10 - Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 1650

Issue description

Similar issue is outlined here.

This error did not occur in Godot 4.1, only since upgrading to 4.2.

However, I am not sure how to implement the potential resolution:

deactivate the node on the server and delay freeing for a bit. But only on the server.

I have a server & client project for an Agario Clone. When the player node overlaps with a "pellet" (food) on the server I use an RPC to delete the node on the client. I am not using a MultiplayerSpawner due to the fact that the pellets spawn on the server before clients connect and are "synced" to each client via another RPC call.

Server (before client joins):

Server (after client joins):

In the RPC functions, I use QueueFree to remove the nodes from both the server and client, respectively. The nodes get removed from both sides, however I see the following error (only on the server):

Steps to reproduce

add a moveable player to the game scene
spawn "pellet" scenes (with below scripts) randomly in the game scene
move player over any pellet and observe that server gets an error, as outlined above)
- above happens even though the pellet is removed correctly & the RPC function is only executed once

Minimal reproduction project (MRP)

// client
using Godot;

namespace AgarioClone.Scripts.Entities;

public partial class Food : Area2D
{
    [Rpc(MultiplayerApi.RpcMode.Authority, CallLocal = true)]
    private void OnEatenByPlayer(NodePath path)
    {
        QueueFree();
    }
}

// server
using Godot;

namespace AgarioCloneServer.Scripts.Entities;

public partial class Food : Area2D
{
    public override void _Ready()
    {
        base._Ready();

        AddToGroup("Pellet");

        BodyEntered += OnBodyEntered;
    }

    private void OnBodyEntered(Node2D body)
    {
        if (body is not Player player)
        {
            return;
        }

        Rpc(MethodName.OnEatenByPlayer, player.GetPath());
    }

    [Rpc(MultiplayerApi.RpcMode.Authority, CallLocal = true)]

    private void OnEatenByPlayer(NodePath path)
    {
        var player = GetNode<Player>(path);

               // some logic (not relevant)

        QueueFree();
    }
}

lbragile commented 10 months ago

@Faless any chance you can have a look?

AThousandShips commented 10 months ago

Please provide a proper minimum reproduction project, not just code:

A small Godot project which reproduces the issue, with no unnecessary files included. Be sure to not include the .godot folder in the archive (but keep project.godot).
Having an MRP is very important for contributors to be able to reproduce the bug in the same way that you are experiencing it. When testing a potential fix for the issue, contributors will use the MRP to validate that the fix is working as intended.
If the reproduction steps are not project dependent (e.g. the bug is visible in a brand new project), you can write "N/A" in the field.
Drag and drop a ZIP archive to upload it (max 10 MB). Do not select another field until the project is done uploading.
Note for C# users: If your issue is not C#-specific, please upload a minimal reproduction project written in GDScript. This will make it easier for contributors to reproduce the issue locally as not everyone has a .NET setup available.

Please try convert the code to GDScript as well, it'll help getting this tested faster

Without a project we can't know things like spawners and other very important network details 🙂

Faless commented 10 months ago

Like #85883 , the err print is a side effect of #82777 which no longer use NodePaths for indexing (and soon won't keep the path cache lingering indefinitely).

The problem is that, the first RPC from a node, needs to "simplify" the path.

This means:

RPC sender sends a "path simplify" message, with the node path, and an ID
RPC sender also sends the RPC with the full path (until path simplification is complete)
The remote then sends a "path simplify ack" message, with the received ID
The remote then execute the RPC.

The problem is that when the sender received the "path simplify ack" message, the node is gone because it was queue_freed, hence the error.

I don't think there's much we can do tbh, beside using a spawner in the project, or simply ignoring the error.

lbragile commented 10 months ago

Got it, thank you for the detailed explanation!

AFAICT the plan is to fix this in v4.3, so I will just ignore the error for now.

lbragile commented 9 months ago

@Faless I ended up implementing a spawner for the "pellets" as follows:

server starts without pellets
client joins, which triggers random pellet creation on server (only when number of peers is 1)
spawner replicates the spawned pellets on the client
spawner replicates the pellets from server to other clients upon connection

This works well and eliminates the above issue I was facing. I can now replicate despawning pellets on all peers using the spawner!

However, I am running into a strange error that I cannot wrap my head around since it only pops up in a specific situation (which appears unrelated). I am hoping the following can sufficiently explain:

When a client connects, a Player is spawned on the server:

// Game.cs

[Rpc(MultiplayerApi.RpcMode.AnyPeer)]
private void SpawnPlayer(string playerName)
{
    var peerId = Multiplayer.GetRemoteSenderId();
    var player = _playerScene.Instantiate<Player>();
    player.Name = $"{playerName}-{peerId}";
    player.SetPeerId(peerId);
    _playerContainer.AddChild(player, true);
}

and then an initial SubPlayer is spawned in the Player's respective container:

// Player.cs

public override void _Ready()
{
    base._Ready();
    var subPlayerContainer = GetNode("SubPlayerContainer");
    var subPlayerScene = ResourceLoader.Load<PackedScene>("res://Scenes/SubPlayer.tscn");
    var subPlayer = subPlayerScene.Instantiate<SubPlayer>();
    subPlayer.Init();
    subPlayerContainer.AddChild(subPlayer, true);
    subPlayer.Owner = this;
}

Again, this shows up well on both server & client. That is, upon connection I see the same replicated Player on both. This also works with multiple client connections. For example, if I connect with 2 clients, I see both Players replicated correctly on each peer. Eating pellets also works correctly and replicates the "despawned" pellets across all peers.

However, when I join with client A and eat a single pellet with it, then join with client B, I don't see client B on all peers! Instead I get the following error (ff-* is the Player):

What I cannot understand:

why does the pellet's QueueFree impact the SubPlayerSpawner "visibility" when a subsequent client joins?

I am also not able to debug this with breakpoints since I haven't figured out a way to do so with VSCode for multiple client Godot Engine instances.

The interesting thing is that the pellet despawning (and corresponding synchronized properties based on this event) is correctly replicated on all peers.

This does not happen if I don't eat a pellet or eat 2+ pellets on client A before joining with client B. In such cases, again everything works as expected with respect to the multiplayer replication.

For reference, my updated pellet script (no longer using RPC call due to spawner):

using Godot;

namespace AgarioCloneServer.Scripts.Entities;

public partial class Food : Area2D
{
    public override void _Ready()
    {
        base._Ready();

        AddToGroup("Pellet");

        BodyEntered += OnBodyEntered;
    }

    private void OnBodyEntered(Node2D body)
    {
        if (body is not SubPlayer subPlayer)
        {
            return;
        }

        subPlayer.ScoreManager.IncreaseScore(100.0f);

        QueueFree();
    }
}

I came across this post which appears to be related, but I don't think that the proposed solution applies in my case.

Any ideas would be greatly appreciated as this is a blocker for my project.

Feel free to re-open this issue if it is indeed a bug.

lbragile commented 9 months ago

If my understanding is correct, this https://github.com/godotengine/godot/pull/71534#issue-1535726185 appears to suggest that nested spawners replicate scenes on the _enter_tree() method (instead of on _ready()). Wouldn't this cause the above to happen frequently?

I also found this comment:

// If we are a nested spawn, we need to wait until the parent is ready.

It is found in the following method:

void SceneReplicationInterface::_node_ready(const ObjectID &p_oid)

Shouldn't a node only be ready when it's children are ready (ref)?

Called when the node is "ready", i.e. when both the node and its children have entered the scene tree. If the node has children, their _ready callbacks get triggered first, and the parent node will receive the ready notification afterwards.

theromis commented 7 months ago

@lbragile may I ask how you "despawning" objects , because I'm facing same issue on 4.3-dev5 I'm just queue_free on collision

godotengine / godot