Skip to content

Commit

Permalink
[MP] Gracefully handle cache confirmation of deleted nodes
Browse files Browse the repository at this point in the history
It's possible that after sending a cached node reference (e.g. RPC or
static MultiplayerSynchronizer) the reference node is removed from tree
before the remote peer(s) can confirm the referenced path.

To better detect that case, and avoid spamming errors when it happens,
this commit modifies the multiplayer API caching protocol, to send the
received ID instead of the Node path when sending the confirmation
packet.

**This is a breaking change** because it makes the runtime multiplayer
protocol incompatible with previous versions of Godot.
  • Loading branch information
Faless committed Mar 29, 2024
1 parent 29b3d9e commit 4b973f4
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 19 deletions.
38 changes: 20 additions & 18 deletions modules/multiplayer/scene_cache_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ void SceneCacheInterface::_remove_node_cache(ObjectID p_oid) {
if (!nc) {
return;
}
if (nc->cache_id) {
assigned_ids.erase(nc->cache_id);
}
for (KeyValue<int, int> &E : nc->recv_ids) {
PeerInfo *pinfo = peers_info.getptr(E.key);
ERR_CONTINUE(!pinfo);
Expand Down Expand Up @@ -117,16 +120,12 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac
NodeCache &cache = _track(node);
cache.recv_ids.insert(p_from, id);

// Encode path to send ack.
CharString pname = String(path).utf8();
int len = encode_cstring(pname.get_data(), nullptr);

// Send ack.
Vector<uint8_t> packet;

packet.resize(1 + 1 + len);
packet.resize(1 + 1 + 4);
packet.write[0] = SceneMultiplayer::NETWORK_COMMAND_CONFIRM_PATH;
packet.write[1] = valid_rpc_checksum;
encode_cstring(pname.get_data(), &packet.write[2]);
encode_uint32(id, &packet.write[2]);

Ref<MultiplayerPeer> multiplayer_peer = multiplayer->get_multiplayer_peer();
ERR_FAIL_COND(multiplayer_peer.is_null());
Expand All @@ -137,26 +136,26 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac
}

void SceneCacheInterface::process_confirm_path(int p_from, const uint8_t *p_packet, int p_packet_len) {
ERR_FAIL_COND_MSG(p_packet_len < 3, "Invalid packet received. Size too small.");
ERR_FAIL_COND_MSG(p_packet_len != 6, "Invalid packet received. Size too small.");
Node *root_node = SceneTree::get_singleton()->get_root()->get_node(multiplayer->get_root_path());
ERR_FAIL_NULL(root_node);

const bool valid_rpc_checksum = p_packet[1];
int id = decode_uint32(&p_packet[2]);

String paths;
paths.parse_utf8((const char *)&p_packet[2], p_packet_len - 2);

const NodePath path = paths;
const ObjectID *oid = assigned_ids.getptr(id);
if (oid == nullptr) {
return; // May be trying to confirm a node that was removed.
}

if (valid_rpc_checksum == false) {
ERR_PRINT("The rpc node checksum failed. Make sure to have the same methods on both nodes. Node path: " + path);
const Node *node = Object::cast_to<Node>(ObjectDB::get_instance(*oid));
ERR_FAIL_NULL(node); // Bug.
ERR_PRINT("The rpc node checksum failed. Make sure to have the same methods on both nodes. Node path: " + node->get_path());
}

Node *node = root_node->get_node(path);
ERR_FAIL_NULL(node);

NodeCache *cache = nodes_cache.getptr(node->get_instance_id());
ERR_FAIL_NULL_MSG(cache, "Invalid packet received. Tries to confirm a node which was not requested.");
NodeCache *cache = nodes_cache.getptr(*oid);
ERR_FAIL_NULL(cache); // Bug.

bool *confirmed = cache->confirmed_peers.getptr(p_from);
ERR_FAIL_NULL_MSG(confirmed, "Invalid packet received. Tries to confirm a node which was not requested.");
Expand Down Expand Up @@ -216,6 +215,7 @@ int SceneCacheInterface::make_object_cache(Object *p_obj) {
NodeCache &cache = _track(node);
if (cache.cache_id == 0) {
cache.cache_id = last_send_cache_id++;
assigned_ids[cache.cache_id] = p_obj->get_instance_id();
}
return cache.cache_id;
}
Expand All @@ -227,6 +227,7 @@ bool SceneCacheInterface::send_object_cache(Object *p_obj, int p_peer_id, int &r
NodeCache &cache = _track(node);
if (cache.cache_id == 0) {
cache.cache_id = last_send_cache_id++;
assigned_ids[cache.cache_id] = p_obj->get_instance_id();
}
r_id = cache.cache_id;

Expand Down Expand Up @@ -289,5 +290,6 @@ void SceneCacheInterface::clear() {
}
peers_info.clear();
nodes_cache.clear();
assigned_ids.clear();
last_send_cache_id = 1;
}
3 changes: 2 additions & 1 deletion modules/multiplayer/scene_cache_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class SceneCacheInterface : public RefCounted {

//path sent caches
struct NodeCache {
int cache_id;
int cache_id = 0;
HashMap<int, int> recv_ids; // peer id, remote cache id
HashMap<int, bool> confirmed_peers; // peer id, confirmed
};
Expand All @@ -55,6 +55,7 @@ class SceneCacheInterface : public RefCounted {
};

HashMap<ObjectID, NodeCache> nodes_cache;
HashMap<int, ObjectID> assigned_ids;
HashMap<int, PeerInfo> peers_info;
int last_send_cache_id = 1;

Expand Down

0 comments on commit 4b973f4

Please sign in to comment.