From 1e0d56346715e14f01b0684584adae42d9b17699 Mon Sep 17 00:00:00 2001 From: Fabio Alessandrelli Date: Mon, 21 Feb 2022 19:05:04 +0100 Subject: [PATCH 1/2] [Net] Fix multi-peer path-only replication. It used to check if a net_id was ever assigned to that node to detect when to send the path confirm to the remote peer. This is wrong, because the same net_id is shared for all the remote peers, but sent one by one. Instead we now check if it's either not assigned or if the assigned net_id is a cache ID, and in that case ensure that the remote peer has been notified. This can be further improved by unifying the cache interface, but for now it's a fast fix to get path-only sync to work. --- scene/multiplayer/scene_replication_interface.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scene/multiplayer/scene_replication_interface.cpp b/scene/multiplayer/scene_replication_interface.cpp index 25a704b37e18..0764f136e407 100644 --- a/scene/multiplayer/scene_replication_interface.cpp +++ b/scene/multiplayer/scene_replication_interface.cpp @@ -350,11 +350,12 @@ void SceneReplicationInterface::_send_sync(int p_peer, uint64_t p_msec) { } if (size) { uint32_t net_id = rep_state->get_net_id(oid); - if (net_id == 0) { + if (net_id == 0 || (net_id & 0x80000000)) { // First time path based ID. NodePath rel_path = multiplayer->get_root_path().rel_path_to(sync->get_path()); int path_id = 0; multiplayer->send_object_cache(sync, rel_path, p_peer, path_id); + ERR_CONTINUE_MSG(net_id && net_id != (uint32_t(path_id) | 0x80000000), "This should never happen!"); net_id = path_id; rep_state->set_net_id(oid, net_id | 0x80000000); } From f1dc6cc9e441e4dc5a6e0c9764d583a668679e5c Mon Sep 17 00:00:00 2001 From: Fabio Alessandrelli Date: Mon, 21 Feb 2022 19:45:18 +0100 Subject: [PATCH 2/2] [Net] Optimize object cache notification send for single peer. It used to always cycle all the peers when checking which one needed to be notified, now it only does that when the target is multiple (i.e. broadcast or exclusion). --- scene/multiplayer/scene_cache_interface.cpp | 122 +++++++++++--------- scene/multiplayer/scene_cache_interface.h | 2 +- 2 files changed, 66 insertions(+), 58 deletions(-) diff --git a/scene/multiplayer/scene_cache_interface.cpp b/scene/multiplayer/scene_cache_interface.cpp index 2f278ed8646a..f05dc5a2da57 100644 --- a/scene/multiplayer/scene_cache_interface.cpp +++ b/scene/multiplayer/scene_cache_interface.cpp @@ -139,74 +139,46 @@ void SceneCacheInterface::process_confirm_path(int p_from, const uint8_t *p_pack E->get() = true; } -bool SceneCacheInterface::_send_confirm_path(Node *p_node, NodePath p_path, PathSentCache *psc, int p_target) { - bool has_all_peers = true; - List peers_to_add; // If one is missing, take note to add it. - - for (const Set::Element *E = multiplayer->get_connected_peers().front(); E; E = E->next()) { - if (p_target < 0 && E->get() == -p_target) { - continue; // Continue, excluded. - } - - if (p_target > 0 && E->get() != p_target) { - continue; // Continue, not for this peer. - } - - Map::Element *F = psc->confirmed_peers.find(E->get()); - - if (!F || !F->get()) { - // Path was not cached, or was cached but is unconfirmed. - if (!F) { - // Not cached at all, take note. - peers_to_add.push_back(E->get()); - } +Error SceneCacheInterface::_send_confirm_path(Node *p_node, NodePath p_path, PathSentCache *psc, const List &p_peers) { + // Encode function name. + const CharString path = String(p_path).utf8(); + const int path_len = encode_cstring(path.get_data(), nullptr); - has_all_peers = false; - } - } - - if (peers_to_add.size() > 0) { - // Those that need to be added, send a message for this. - - // Encode function name. - const CharString path = String(p_path).utf8(); - const int path_len = encode_cstring(path.get_data(), nullptr); + // Extract MD5 from rpc methods list. + const String methods_md5 = multiplayer->get_rpc_md5(p_node); + const int methods_md5_len = 33; // 32 + 1 for the `0` that is added by the encoder. - // Extract MD5 from rpc methods list. - const String methods_md5 = multiplayer->get_rpc_md5(p_node); - const int methods_md5_len = 33; // 32 + 1 for the `0` that is added by the encoder. - - Vector packet; - packet.resize(1 + 4 + path_len + methods_md5_len); - int ofs = 0; + Vector packet; + packet.resize(1 + 4 + path_len + methods_md5_len); + int ofs = 0; - packet.write[ofs] = MultiplayerAPI::NETWORK_COMMAND_SIMPLIFY_PATH; - ofs += 1; + packet.write[ofs] = MultiplayerAPI::NETWORK_COMMAND_SIMPLIFY_PATH; + ofs += 1; - ofs += encode_cstring(methods_md5.utf8().get_data(), &packet.write[ofs]); + ofs += encode_cstring(methods_md5.utf8().get_data(), &packet.write[ofs]); - ofs += encode_uint32(psc->id, &packet.write[ofs]); + ofs += encode_uint32(psc->id, &packet.write[ofs]); - ofs += encode_cstring(path.get_data(), &packet.write[ofs]); + ofs += encode_cstring(path.get_data(), &packet.write[ofs]); - Ref multiplayer_peer = multiplayer->get_multiplayer_peer(); - ERR_FAIL_COND_V(multiplayer_peer.is_null(), false); + Ref multiplayer_peer = multiplayer->get_multiplayer_peer(); + ERR_FAIL_COND_V(multiplayer_peer.is_null(), ERR_BUG); #ifdef DEBUG_ENABLED - multiplayer->profile_bandwidth("out", packet.size() * peers_to_add.size()); + multiplayer->profile_bandwidth("out", packet.size() * p_peers.size()); #endif - for (int &E : peers_to_add) { - multiplayer_peer->set_target_peer(E); // To all of you. - multiplayer_peer->set_transfer_channel(0); - multiplayer_peer->set_transfer_mode(Multiplayer::TRANSFER_MODE_RELIABLE); - multiplayer_peer->put_packet(packet.ptr(), packet.size()); - - psc->confirmed_peers.insert(E, false); // Insert into confirmed, but as false since it was not confirmed. - } + Error err = OK; + for (int peer_id : p_peers) { + multiplayer_peer->set_target_peer(peer_id); + multiplayer_peer->set_transfer_channel(0); + multiplayer_peer->set_transfer_mode(Multiplayer::TRANSFER_MODE_RELIABLE); + err = multiplayer_peer->put_packet(packet.ptr(), packet.size()); + ERR_FAIL_COND_V(err != OK, err); + // Insert into confirmed, but as false since it was not confirmed. + psc->confirmed_peers.insert(peer_id, false); } - - return has_all_peers; + return err; } bool SceneCacheInterface::is_cache_confirmed(NodePath p_path, int p_peer) { @@ -230,7 +202,43 @@ bool SceneCacheInterface::send_object_cache(Object *p_obj, NodePath p_path, int } r_id = psc->id; - return _send_confirm_path(node, p_path, psc, p_peer_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. + Map::Element *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 (!F->get()) { + has_all_peers = false; + } + } else { + // Long and painful. + for (const Set::Element *E = multiplayer->get_connected_peers().front(); E; E = E->next()) { + if (p_peer_id < 0 && E->get() == -p_peer_id) { + continue; // Continue, excluded. + } + if (p_peer_id > 0 && E->get() != p_peer_id) { + continue; // Continue, not for this peer. + } + + Map::Element *F = psc->confirmed_peers.find(E->get()); + if (!F) { + peers_to_add.push_back(E->get()); // Need to also be notified. + has_all_peers = false; + } else if (!F->get()) { + has_all_peers = false; + } + } + } + + if (peers_to_add.size()) { + _send_confirm_path(node, p_path, psc, peers_to_add); + } + + return has_all_peers; } Object *SceneCacheInterface::get_cached_object(int p_from, uint32_t p_cache_id) { diff --git a/scene/multiplayer/scene_cache_interface.h b/scene/multiplayer/scene_cache_interface.h index 91a53cb948b0..c709d26b5117 100644 --- a/scene/multiplayer/scene_cache_interface.h +++ b/scene/multiplayer/scene_cache_interface.h @@ -60,7 +60,7 @@ class SceneCacheInterface : public MultiplayerCacheInterface { int last_send_cache_id = 1; protected: - bool _send_confirm_path(Node *p_node, NodePath p_path, PathSentCache *psc, int p_target); + Error _send_confirm_path(Node *p_node, NodePath p_path, PathSentCache *psc, const List &p_peers); static MultiplayerCacheInterface *_create(MultiplayerAPI *p_multiplayer); public: