From 3504c98c1233bbd2506e89ce46509bc79afaec17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Wed, 31 Jul 2024 22:46:56 +0200 Subject: [PATCH] Revert "[MP] Handle cleanup of "scene cache" nodes" This reverts commit 8544106b7e4d9add7905da08e3cd3bf720f3cb20. This caused regression #90908. --- modules/multiplayer/scene_cache_interface.cpp | 164 ++++++++---------- modules/multiplayer/scene_cache_interface.h | 27 +-- 2 files changed, 82 insertions(+), 109 deletions(-) diff --git a/modules/multiplayer/scene_cache_interface.cpp b/modules/multiplayer/scene_cache_interface.cpp index 33b05d4cc21..56cd0bec18b 100644 --- a/modules/multiplayer/scene_cache_interface.cpp +++ b/modules/multiplayer/scene_cache_interface.cpp @@ -35,61 +35,25 @@ #include "core/io/marshalls.h" #include "scene/main/node.h" #include "scene/main/window.h" -#include "scene/scene_string_names.h" - -SceneCacheInterface::NodeCache &SceneCacheInterface::_track(Node *p_node) { - const ObjectID oid = p_node->get_instance_id(); - NodeCache *nc = nodes_cache.getptr(oid); - if (!nc) { - nodes_cache[oid] = NodeCache(); - p_node->connect(SceneStringNames::get_singleton()->tree_exited, callable_mp(this, &SceneCacheInterface::_remove_node_cache).bind(oid), Object::CONNECT_ONE_SHOT); - } - return nodes_cache[oid]; -} - -void SceneCacheInterface::_remove_node_cache(ObjectID p_oid) { - NodeCache *nc = nodes_cache.getptr(p_oid); - if (!nc) { - return; - } - for (KeyValue &E : nc->recv_ids) { - PeerInfo *pinfo = peers_info.getptr(E.key); - ERR_CONTINUE(!pinfo); - pinfo->recv_nodes.erase(E.value); - } - for (KeyValue &E : nc->confirmed_peers) { - PeerInfo *pinfo = peers_info.getptr(E.key); - ERR_CONTINUE(!pinfo); - pinfo->sent_nodes.erase(p_oid); - } - nodes_cache.erase(p_oid); -} void SceneCacheInterface::on_peer_change(int p_id, bool p_connected) { if (p_connected) { - peers_info.insert(p_id, PeerInfo()); + path_get_cache.insert(p_id, PathGetCache()); } else { - PeerInfo *pinfo = peers_info.getptr(p_id); - ERR_FAIL_NULL(pinfo); // Bug. - for (KeyValue E : pinfo->recv_nodes) { - NodeCache *nc = nodes_cache.getptr(E.value); - ERR_CONTINUE(!nc); - nc->recv_ids.erase(E.key); + // Cleanup get cache. + path_get_cache.erase(p_id); + // Cleanup sent cache. + // Some refactoring is needed to make this faster and do paths GC. + for (KeyValue &E : path_send_cache) { + E.value.confirmed_peers.erase(p_id); } - for (const ObjectID &oid : pinfo->sent_nodes) { - NodeCache *nc = nodes_cache.getptr(oid); - ERR_CONTINUE(!nc); - nc->confirmed_peers.erase(p_id); - } - peers_info.erase(p_id); } } void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_packet, int p_packet_len) { - ERR_FAIL_COND(!peers_info.has(p_from)); // Bug. - ERR_FAIL_COND_MSG(p_packet_len < 38, "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); + ERR_FAIL_COND_MSG(p_packet_len < 38, "Invalid packet received. Size too small."); int ofs = 1; String methods_md5; @@ -99,13 +63,15 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac int id = decode_uint32(&p_packet[ofs]); ofs += 4; - ERR_FAIL_COND_MSG(peers_info[p_from].recv_nodes.has(id), vformat("Duplicate remote cache ID %d for peer %d", id, p_from)); - String paths; paths.parse_utf8((const char *)(p_packet + ofs), p_packet_len - ofs); const NodePath path = paths; + if (!path_get_cache.has(p_from)) { + path_get_cache[p_from] = PathGetCache(); + } + Node *node = root_node->get_node(path); ERR_FAIL_NULL(node); const bool valid_rpc_checksum = multiplayer->get_rpc_md5(node) == methods_md5; @@ -113,9 +79,10 @@ 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()); - NodeCache &cache = _track(node); - cache.recv_ids.insert(p_from, id); + PathGetCache::NodeInfo ni; + ni.path = node->get_path(); + + path_get_cache[p_from].nodes[id] = ni; // Encode path to send ack. CharString pname = String(path).utf8(); @@ -155,15 +122,15 @@ void SceneCacheInterface::process_confirm_path(int p_from, const uint8_t *p_pack 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."); + PathSentCache *psc = path_send_cache.getptr(node->get_instance_id()); + ERR_FAIL_NULL_MSG(psc, "Invalid packet received. Tries to confirm a path which was not found in cache."); - 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."); - *confirmed = true; + HashMap::Iterator E = psc->confirmed_peers.find(p_from); + ERR_FAIL_COND_MSG(!E, "Invalid packet received. Source peer was not found in cache for the given path."); + E->value = true; } -Error SceneCacheInterface::_send_confirm_path(Node *p_node, NodeCache &p_cache, const List &p_peers) { +Error SceneCacheInterface::_send_confirm_path(Node *p_node, PathSentCache *psc, const List &p_peers) { // Encode function name. const CharString path = String(multiplayer->get_root_path().rel_path_to(p_node->get_path())).utf8(); const int path_len = encode_cstring(path.get_data(), nullptr); @@ -181,7 +148,7 @@ Error SceneCacheInterface::_send_confirm_path(Node *p_node, NodeCache &p_cache, ofs += encode_cstring(methods_md5.utf8().get_data(), &packet.write[ofs]); - ofs += encode_uint32(p_cache.cache_id, &packet.write[ofs]); + ofs += encode_uint32(psc->id, &packet.write[ofs]); ofs += encode_cstring(path.get_data(), &packet.write[ofs]); @@ -195,74 +162,80 @@ Error SceneCacheInterface::_send_confirm_path(Node *p_node, NodeCache &p_cache, err = multiplayer->send_command(peer_id, packet.ptr(), packet.size()); ERR_FAIL_COND_V(err != OK, err); // Insert into confirmed, but as false since it was not confirmed. - p_cache.confirmed_peers.insert(peer_id, false); - ERR_CONTINUE(!peers_info.has(peer_id)); - peers_info[peer_id].sent_nodes.insert(p_node->get_instance_id()); + psc->confirmed_peers.insert(peer_id, false); } return err; } bool SceneCacheInterface::is_cache_confirmed(Node *p_node, int p_peer) { ERR_FAIL_NULL_V(p_node, false); - const ObjectID oid = p_node->get_instance_id(); - NodeCache *cache = nodes_cache.getptr(oid); - bool *confirmed = cache ? cache->confirmed_peers.getptr(p_peer) : nullptr; - return confirmed && *confirmed; + const PathSentCache *psc = path_send_cache.getptr(p_node->get_instance_id()); + ERR_FAIL_NULL_V(psc, false); + HashMap::ConstIterator F = psc->confirmed_peers.find(p_peer); + ERR_FAIL_COND_V(!F, false); // Should never happen. + return F->value; } int SceneCacheInterface::make_object_cache(Object *p_obj) { Node *node = Object::cast_to(p_obj); ERR_FAIL_NULL_V(node, -1); - NodeCache &cache = _track(node); - if (cache.cache_id == 0) { - cache.cache_id = last_send_cache_id++; + const ObjectID oid = node->get_instance_id(); + // See if the path is cached. + PathSentCache *psc = path_send_cache.getptr(oid); + if (!psc) { + // Path is not cached, create. + path_send_cache[oid] = PathSentCache(); + psc = path_send_cache.getptr(oid); + psc->id = last_send_cache_id++; } - return cache.cache_id; + return psc->id; } bool SceneCacheInterface::send_object_cache(Object *p_obj, int p_peer_id, int &r_id) { Node *node = Object::cast_to(p_obj); ERR_FAIL_NULL_V(node, false); + const ObjectID oid = node->get_instance_id(); // See if the path is cached. - NodeCache &cache = _track(node); - if (cache.cache_id == 0) { - cache.cache_id = last_send_cache_id++; + PathSentCache *psc = path_send_cache.getptr(oid); + if (!psc) { + // Path is not cached, create. + path_send_cache[oid] = PathSentCache(); + psc = path_send_cache.getptr(oid); + psc->id = last_send_cache_id++; } - r_id = cache.cache_id; + r_id = psc->id; bool has_all_peers = true; List peers_to_add; // If one is missing, take note to add it. if (p_peer_id > 0) { // Fast single peer check. - ERR_FAIL_COND_V_MSG(!peers_info.has(p_peer_id), false, "Peer doesn't exist: " + itos(p_peer_id)); - - bool *confirmed = cache.confirmed_peers.getptr(p_peer_id); - if (!confirmed) { + HashMap::Iterator F = psc->confirmed_peers.find(p_peer_id); + if (!F) { peers_to_add.push_back(p_peer_id); // Need to also be notified. has_all_peers = false; - } else if (!(*confirmed)) { + } else if (!F->value) { has_all_peers = false; } } else { // Long and painful. - for (KeyValue &E : peers_info) { - if (p_peer_id < 0 && E.key == -p_peer_id) { + for (const int &E : multiplayer->get_connected_peers()) { + if (p_peer_id < 0 && E == -p_peer_id) { continue; // Continue, excluded. } - bool *confirmed = cache.confirmed_peers.getptr(E.key); - if (!confirmed) { - peers_to_add.push_back(E.key); // Need to also be notified. + HashMap::Iterator F = psc->confirmed_peers.find(E); + if (!F) { + peers_to_add.push_back(E); // Need to also be notified. has_all_peers = false; - } else if (!(*confirmed)) { + } else if (!F->value) { has_all_peers = false; } } } if (peers_to_add.size()) { - _send_confirm_path(node, cache, peers_to_add); + _send_confirm_path(node, psc, peers_to_add); } return has_all_peers; @@ -271,23 +244,22 @@ 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); + HashMap::Iterator E = path_get_cache.find(p_from); + ERR_FAIL_COND_V_MSG(!E, nullptr, vformat("No cache found for peer %d.", p_from)); - 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(ObjectDB::get_instance(*oid)); - 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)); + HashMap::Iterator F = E->value.nodes.find(p_cache_id); + ERR_FAIL_COND_V_MSG(!F, nullptr, vformat("ID %d not found in cache of peer %d.", p_cache_id, p_from)); + + PathGetCache::NodeInfo *ni = &F->value; + Node *node = root_node->get_node(ni->path); + if (!node) { + ERR_PRINT("Failed to get cached path: " + String(ni->path) + "."); + } return node; } void SceneCacheInterface::clear() { - for (KeyValue &E : nodes_cache) { - Object *obj = ObjectDB::get_instance(E.key); - ERR_CONTINUE(!obj); - obj->disconnect(SceneStringNames::get_singleton()->tree_exited, callable_mp(this, &SceneCacheInterface::_remove_node_cache)); - } - peers_info.clear(); - nodes_cache.clear(); + path_get_cache.clear(); + path_send_cache.clear(); last_send_cache_id = 1; } diff --git a/modules/multiplayer/scene_cache_interface.h b/modules/multiplayer/scene_cache_interface.h index ab4a20c078d..e63beb5f84f 100644 --- a/modules/multiplayer/scene_cache_interface.h +++ b/modules/multiplayer/scene_cache_interface.h @@ -43,26 +43,27 @@ private: SceneMultiplayer *multiplayer = nullptr; //path sent caches - struct NodeCache { - int cache_id; - HashMap recv_ids; // peer id, remote cache id - HashMap confirmed_peers; // peer id, confirmed + struct PathSentCache { + HashMap confirmed_peers; + int id; }; - struct PeerInfo { - HashMap recv_nodes; // remote cache id, ObjectID - HashSet sent_nodes; + //path get caches + struct PathGetCache { + struct NodeInfo { + NodePath path; + ObjectID instance; + }; + + HashMap nodes; }; - HashMap nodes_cache; - HashMap peers_info; + HashMap path_send_cache; + HashMap path_get_cache; int last_send_cache_id = 1; - void _remove_node_cache(ObjectID p_oid); - NodeCache &_track(Node *p_node); - protected: - Error _send_confirm_path(Node *p_node, NodeCache &p_cache, const List &p_peers); + Error _send_confirm_path(Node *p_node, PathSentCache *psc, const List &p_peers); public: void clear();