From 90d5d260265a8092624ad3af91c12171bc1e9b7e Mon Sep 17 00:00:00 2001
From: Fabio Alessandrelli <fabio.alessandrelli@gmail.com>
Date: Tue, 30 Jul 2024 13:21:36 +0200
Subject: [PATCH] [MP] Partially revert cache cleanup, track paths as fallback

Cleaning up remote NodePath cache is not trivial since the visibility
API allows for certain nodes to be despawned (and re-spawned) on some
peers while being retained in the authority.

This means that from the server point of view, the node has not changed,
and the path simplification protocol won't be run again after
respawning.

While we can track this information for synchronizers via the
replication API, we can't easily track this information for potential
child nodes that use RPCs (I'm convinced it is doable, but we need to
track the whole dependency tree which would require some more complex
refactoring).

This commit partially reverts some of the cache cleanup logic to always
retain remote IDs, and adds a NodePath lookup fallback when the ObjectID
is invalid.
---
 modules/multiplayer/scene_cache_interface.cpp | 31 +++++++++++++------
 modules/multiplayer/scene_cache_interface.h   | 12 ++++++-
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/modules/multiplayer/scene_cache_interface.cpp b/modules/multiplayer/scene_cache_interface.cpp
index af43123b299..99c8930e922 100644
--- a/modules/multiplayer/scene_cache_interface.cpp
+++ b/modules/multiplayer/scene_cache_interface.cpp
@@ -54,11 +54,14 @@ void SceneCacheInterface::_remove_node_cache(ObjectID p_oid) {
 	if (nc->cache_id) {
 		assigned_ids.erase(nc->cache_id);
 	}
+#if 0
+	// TODO: Find a way to cleanup recv_nodes without breaking visibility and RPCs interactions.
 	for (KeyValue<int, int> &E : nc->recv_ids) {
 		PeerInfo *pinfo = peers_info.getptr(E.key);
 		ERR_CONTINUE(!pinfo);
 		pinfo->recv_nodes.erase(E.value);
 	}
+#endif
 	for (KeyValue<int, bool> &E : nc->confirmed_peers) {
 		PeerInfo *pinfo = peers_info.getptr(E.key);
 		ERR_CONTINUE(!pinfo);
@@ -73,9 +76,12 @@ void SceneCacheInterface::on_peer_change(int p_id, bool p_connected) {
 	} else {
 		PeerInfo *pinfo = peers_info.getptr(p_id);
 		ERR_FAIL_NULL(pinfo); // Bug.
-		for (KeyValue<int, ObjectID> E : pinfo->recv_nodes) {
-			NodeCache *nc = nodes_cache.getptr(E.value);
-			ERR_CONTINUE(!nc);
+		for (KeyValue<int, RecvNode> E : pinfo->recv_nodes) {
+			NodeCache *nc = nodes_cache.getptr(E.value.oid);
+			if (!nc) {
+				// Node might have already been deleted locally.
+				continue;
+			}
 			nc->recv_ids.erase(p_id);
 		}
 		for (const ObjectID &oid : pinfo->sent_nodes) {
@@ -115,7 +121,7 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac
 		ERR_PRINT("The rpc node checksum failed. Make sure to have the same methods on both nodes. Node path: " + path);
 	}
 
-	peers_info[p_from].recv_nodes.insert(id, node->get_instance_id());
+	peers_info[p_from].recv_nodes.insert(id, RecvNode(node->get_instance_id(), path));
 	NodeCache &cache = _track(node);
 	cache.recv_ids.insert(p_from, id);
 
@@ -269,14 +275,21 @@ bool SceneCacheInterface::send_object_cache(Object *p_obj, int p_peer_id, int &r
 }
 
 Object *SceneCacheInterface::get_cached_object(int p_from, uint32_t p_cache_id) {
-	Node *root_node = SceneTree::get_singleton()->get_root()->get_node(multiplayer->get_root_path());
-	ERR_FAIL_NULL_V(root_node, nullptr);
 	PeerInfo *pinfo = peers_info.getptr(p_from);
 	ERR_FAIL_NULL_V(pinfo, nullptr);
 
-	const ObjectID *oid = pinfo->recv_nodes.getptr(p_cache_id);
-	ERR_FAIL_NULL_V_MSG(oid, nullptr, vformat("ID %d not found in cache of peer %d.", p_cache_id, p_from));
-	Node *node = Object::cast_to<Node>(ObjectDB::get_instance(*oid));
+	RecvNode *recv_node = pinfo->recv_nodes.getptr(p_cache_id);
+	ERR_FAIL_NULL_V_MSG(recv_node, nullptr, vformat("ID %d not found in cache of peer %d.", p_cache_id, p_from));
+	Node *node = Object::cast_to<Node>(ObjectDB::get_instance(recv_node->oid));
+	if (!node) {
+		// Fallback to path lookup.
+		Node *root_node = SceneTree::get_singleton()->get_root()->get_node(multiplayer->get_root_path());
+		ERR_FAIL_NULL_V(root_node, nullptr);
+		node = root_node->get_node(recv_node->path);
+		if (node) {
+			recv_node->oid = node->get_instance_id();
+		}
+	}
 	ERR_FAIL_NULL_V_MSG(node, nullptr, vformat("Failed to get cached node from peer %d with cache ID %d.", p_from, p_cache_id));
 	return node;
 }
diff --git a/modules/multiplayer/scene_cache_interface.h b/modules/multiplayer/scene_cache_interface.h
index 73d6bde6eff..fbe618f4ad1 100644
--- a/modules/multiplayer/scene_cache_interface.h
+++ b/modules/multiplayer/scene_cache_interface.h
@@ -49,8 +49,18 @@ private:
 		HashMap<int, bool> confirmed_peers; // peer id, confirmed
 	};
 
+	struct RecvNode {
+		ObjectID oid;
+		NodePath path;
+
+		RecvNode(const ObjectID &p_oid, const NodePath &p_path) {
+			oid = p_oid;
+			path = p_path;
+		}
+	};
+
 	struct PeerInfo {
-		HashMap<int, ObjectID> recv_nodes; // remote cache id, ObjectID
+		HashMap<int, RecvNode> recv_nodes; // remote cache id, (ObjectID, NodePath)
 		HashSet<ObjectID> sent_nodes;
 	};