From 63271016075405f621efe210efaf7ab21fe8f3ef Mon Sep 17 00:00:00 2001 From: HybridDog Date: Wed, 24 Jan 2024 22:03:11 +0100 Subject: [PATCH 1/2] Refactor server-side node placement or dig prediction handling The code for handling `TOSERVER_INTERACT` packets has some style-related problems: * There are five places with mapblock-sending code to undo predictions in the large `handleCommand_Interact` method in `serverpackethandler.cpp`, so these can be abstracted away into a function to shorten the code. * In `networkprotocol.h`, below `TOSERVER_INTERACT`, there is an outdated copy of the `InteractAction` enum documentation and the player position information is not mentioned. * In `serverpackethandler.cpp`, there is a (newer) copy of the `TOSERVER_INTERACT` documentation from `networkprotocol.h`. Furthermore, the way the server undoes or confirms client-side node placement or dig predictions looks inconsistent, thus erroneous: * If a player tries to place a node in a distance too far away or if he/she is dead, only the mapblock containing `pointed.node_undersurface` is triggered to be sent although the player may have placed the node at `pointed.node_abovesurface` and this position may be in a different mapblock. * Similarly, if a player without interact privilege tries to place a node, only the mapblock containing `pointed.node_abovesurface` is triggered to be sent although the player may have placed the node at `pointed.node_undersurface` and this position may be in a different mapblock. This change addresses the problems listed above: * Fix the documentation comments related to the TOSERVER_INTERACT package content * Move mapblock sending code related to predictions to ClientInterface * If the player tries to place but has no interact privilege, is dead, or exceeds the permitted distance, send both the mapblock corresponding to `pointed.node_undersurface` and `pointed.node_abovesurface` if these mapblocks differ --- src/network/networkprotocol.h | 25 +++++---- src/network/serverpackethandler.cpp | 78 ++++++----------------------- src/server/clientiface.cpp | 33 +++++++++--- src/server/clientiface.h | 16 ++++-- 4 files changed, 64 insertions(+), 88 deletions(-) diff --git a/src/network/networkprotocol.h b/src/network/networkprotocol.h index e3618042fe..761d96b7ce 100644 --- a/src/network/networkprotocol.h +++ b/src/network/networkprotocol.h @@ -1006,17 +1006,20 @@ enum ToServerCommand : u16 TOSERVER_INTERACT = 0x39, /* - [0] u16 command - [2] u8 action - [3] u16 item - [5] u32 length of the next item - [9] serialized PointedThing - actions: - 0: start digging (from undersurface) or use - 1: stop digging (all parameters ignored) - 2: digging completed - 3: place block or item (to abovesurface) - 4: use item + u16 command + u8 action (InteractAction) + u16 item + u32 length of the next item + u8[length] serialized PointedThing + v3s32 player position multiplied by 100 and converted to integers + v3s32 player speed multiplied by 100 and converted to integers + s32 player pitch multiplied by 100 and converted to an integer + s32 player yaw multiplied by 100 and converted to an integer + u32 keyPressed + u8 fov + u8 wanted drawing range + optional: + u8 flags; it is 0x01 if the camera is inverted */ TOSERVER_REMOVED_SOUNDS = 0x3a, diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp index c1ac821d3e..10d8e5ac64 100644 --- a/src/network/serverpackethandler.cpp +++ b/src/network/serverpackethandler.cpp @@ -921,15 +921,6 @@ static inline void getWieldedItem(const PlayerSAO *playersao, std::optionalisDead()) { actionstream << "Server: " << player->getName() << " tried to interact while dead; ignoring." << std::endl; - if (pointed.type == POINTEDTHING_NODE) { - // Re-send block to revert change on client-side - RemoteClient *client = getClient(peer_id); - v3s16 blockpos = getNodeBlockPos(pointed.node_undersurface); - client->SetBlockNotSent(blockpos); - } - // Call callbacks + getClient(peer_id)->respondToInteraction(action, pointed, false); m_script->on_cheat(playersao, "interacted_while_dead"); return; } @@ -1012,22 +997,7 @@ void Server::handleCommand_Interact(NetworkPacket *pkt) if (!checkPriv(player->getName(), "interact")) { actionstream << player->getName() << " attempted to interact with " << pointed.dump() << " without 'interact' privilege" << std::endl; - - if (pointed.type != POINTEDTHING_NODE) - return; - - // Re-send block to revert change on client-side - RemoteClient *client = getClient(peer_id); - // Digging completed -> under - if (action == INTERACT_DIGGING_COMPLETED) { - v3s16 blockpos = getNodeBlockPos(pointed.node_undersurface); - client->SetBlockNotSent(blockpos); - } - // Placement -> above - else if (action == INTERACT_PLACE) { - v3s16 blockpos = getNodeBlockPos(pointed.node_abovesurface); - client->SetBlockNotSent(blockpos); - } + getClient(peer_id)->respondToInteraction(action, pointed, false); return; } @@ -1055,12 +1025,7 @@ void Server::handleCommand_Interact(NetworkPacket *pkt) float d = playersao->getEyePosition().getDistanceFrom(target_pos); if (!checkInteractDistance(player, d, pointed.dump())) { - if (pointed.type == POINTEDTHING_NODE) { - // Re-send block to revert change on client-side - RemoteClient *client = getClient(peer_id); - v3s16 blockpos = getNodeBlockPos(pointed.node_undersurface); - client->SetBlockNotSent(blockpos); - } + getClient(peer_id)->respondToInteraction(action, pointed, false); return; } } @@ -1213,14 +1178,12 @@ void Server::handleCommand_Interact(NetworkPacket *pkt) if (is_valid_dig && n.getContent() != CONTENT_IGNORE) m_script->node_on_dig(p_under, n, playersao); - v3s16 blockpos = getNodeBlockPos(p_under); - RemoteClient *client = getClient(peer_id); - // Send unusual result (that is, node not being removed) - if (m_env->getMap().getNode(p_under).getContent() != CONTENT_AIR) - // Re-send block to revert change on client-side - client->SetBlockNotSent(blockpos); - else - client->ResendBlockIfOnWire(blockpos); + // For whatever reason we assume that the client always predicts that a + // dug node is air irrespective of the node's node_dig_prediction + bool prediction_success = + m_env->getMap().getNode(p_under).getContent() == CONTENT_AIR; + getClient(peer_id)->respondToInteraction(action, pointed, + prediction_success); return; } // action == INTERACT_DIGGING_COMPLETED @@ -1264,24 +1227,11 @@ void Server::handleCommand_Interact(NetworkPacket *pkt) SendInventory(player, true); } - if (pointed.type != POINTEDTHING_NODE) - return; - - // If item has node placement prediction, always send the - // blocks to make sure the client knows what exactly happened - RemoteClient *client = getClient(peer_id); - v3s16 blockpos = getNodeBlockPos(pointed.node_abovesurface); - v3s16 blockpos2 = getNodeBlockPos(pointed.node_undersurface); - if (had_prediction) { - client->SetBlockNotSent(blockpos); - if (blockpos2 != blockpos) - client->SetBlockNotSent(blockpos2); - } else { - client->ResendBlockIfOnWire(blockpos); - if (blockpos2 != blockpos) - client->ResendBlockIfOnWire(blockpos2); - } - + // Since we do not known if the client has predicted the node at + // pointed.above or pointed.under, + // we assume that a prediction is always wrong. + getClient(peer_id)->respondToInteraction(action, pointed, + !had_prediction); return; } // action == INTERACT_PLACE diff --git a/src/server/clientiface.cpp b/src/server/clientiface.cpp index e5c07b3d80..4ff9df1a11 100644 --- a/src/server/clientiface.cpp +++ b/src/server/clientiface.cpp @@ -31,6 +31,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "server/luaentity_sao.h" #include "server/player_sao.h" #include "log.h" +#include "util/pointedthing.h" #include "util/srp.h" #include "util/string.h" #include "face_position_cache.h" @@ -77,14 +78,6 @@ RemoteClient::RemoteClient() : { } -void RemoteClient::ResendBlockIfOnWire(v3s16 p) -{ - // if this block is on wire, mark it for sending again as soon as possible - if (m_blocks_sending.find(p) != m_blocks_sending.end()) { - SetBlockNotSent(p); - } -} - static LuaEntitySAO *getAttachedObject(PlayerSAO *sao, ServerEnvironment *env) { ServerActiveObject *ao = sao; @@ -469,6 +462,30 @@ void RemoteClient::SetBlocksNotSent(const std::vector &blocks) } } +void RemoteClient::respondToInteraction(InteractAction action, + const PointedThing &pointed, bool prediction_success) +{ + if ((action != INTERACT_PLACE && action != INTERACT_DIGGING_COMPLETED) + || pointed.type != POINTEDTHING_NODE) + // The client has not predicted not node changes + return; + + // The client may have an outdated mapblock if the placement or dig + // prediction was wrong or if an old mapblock is still being sent to it. + v3s16 blockpos = getNodeBlockPos(pointed.node_undersurface); + if (!prediction_success + || m_blocks_sending.find(blockpos) != m_blocks_sending.end()) { + SetBlockNotSent(blockpos); + } + if (action != INTERACT_PLACE) + return; + v3s16 blockpos2 = getNodeBlockPos(pointed.node_abovesurface); + if (blockpos2 != blockpos && (!prediction_success + || m_blocks_sending.find(blockpos2) != m_blocks_sending.end())) { + SetBlockNotSent(blockpos2); + } +} + void RemoteClient::notifyEvent(ClientStateEvent event) { std::ostringstream myerror; diff --git a/src/server/clientiface.h b/src/server/clientiface.h index ac41b00cae..b345827218 100644 --- a/src/server/clientiface.h +++ b/src/server/clientiface.h @@ -38,6 +38,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include class MapBlock; +class PointedThing; class ServerEnvironment; class EmergeManager; @@ -270,12 +271,17 @@ public: void SetBlocksNotSent(const std::vector &blocks); /** - * tell client about this block being modified right now. - * this information is required to requeue the block in case it's "on wire" - * while modification is processed by server - * @param p position of modified block + * Trigger block sending to undo client-side predicted node changes + * + * \param action The interact action to determine which predictions the + * client could have made + * \param pointed The positions where the client has interacted + * \param prediction_success If true, assume that the client has made a + * correct prediction and send the mapblock only if an outdated mapblock + * is currently "on wire", which can erroneously override the prediction */ - void ResendBlockIfOnWire(v3s16 p); + void respondToInteraction(InteractAction action, + const PointedThing &pointed, bool prediction_success); u32 getSendingCount() const { return m_blocks_sending.size(); } From 78a79b8ae9705bdce81e3db2c7c6842b8153698f Mon Sep 17 00:00:00 2001 From: HybridDog Date: Fri, 16 Feb 2024 16:03:30 +0100 Subject: [PATCH 2/2] try it without forward declaration --- src/server/clientiface.cpp | 1 - src/server/clientiface.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/server/clientiface.cpp b/src/server/clientiface.cpp index 4ff9df1a11..4c9e01bc1e 100644 --- a/src/server/clientiface.cpp +++ b/src/server/clientiface.cpp @@ -31,7 +31,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "server/luaentity_sao.h" #include "server/player_sao.h" #include "log.h" -#include "util/pointedthing.h" #include "util/srp.h" #include "util/string.h" #include "face_position_cache.h" diff --git a/src/server/clientiface.h b/src/server/clientiface.h index b345827218..11cf9512e9 100644 --- a/src/server/clientiface.h +++ b/src/server/clientiface.h @@ -29,6 +29,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "porting.h" #include "threading/mutex_auto_lock.h" #include "clientdynamicinfo.h" +#include "util/pointedthing.h" #include #include @@ -38,7 +39,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include class MapBlock; -class PointedThing; class ServerEnvironment; class EmergeManager;