Attachments: Fix attachments to temporary removed objects (#8989)

Does not clear the parent's attachment information when the child is deleted locally.
Either it was removed permanently, or just temporary - we don't know, but it's up to the server to send a *detach from child" packet for the parent.

6 files changed, 70 insertions, 31 deletions

diff --git a/src/client/clientenvironment.cpp b/src/client/clientenvironment.cpp
index e1b20ec84..5eb033302 100644
--- a/src/client/clientenvironment.cpp
+++ b/src/client/clientenvironment.cpp
@@ -402,6 +402,23 @@ void ClientEnvironment::addActiveObject(u16 id, u8 type,
 	}
 }
 
+
+void ClientEnvironment::removeActiveObject(u16 id)
+{
+	// Get current attachment childs to detach them visually
+	std::unordered_set<int> attachment_childs;
+	if (auto *obj = getActiveObject(id))
+		attachment_childs = obj->getAttachmentChildIds();
+
+	m_ao_manager.removeObject(id);
+
+	// Perform a proper detach in Irrlicht
+	for (auto c_id : attachment_childs) {
+		if (ClientActiveObject *child = getActiveObject(c_id))
+			child->updateAttachments();
+	}
+}
+
 void ClientEnvironment::processActiveObjectMessage(u16 id, const std::string &data)
 {
 	ClientActiveObject *obj = getActiveObject(id);
diff --git a/src/client/clientenvironment.h b/src/client/clientenvironment.h
index f182b5951..864496a41 100644
--- a/src/client/clientenvironment.h
+++ b/src/client/clientenvironment.h
@@ -104,10 +104,7 @@ public:
 	u16 addActiveObject(ClientActiveObject *object);
 
 	void addActiveObject(u16 id, u8 type, const std::string &init_data);
-	void removeActiveObject(u16 id)
-	{
-		m_ao_manager.removeObject(id);
-	}
+	void removeActiveObject(u16 id);
 
 	void processActiveObjectMessage(u16 id, const std::string &data);
 
diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp
index d7ab8e945..716468402 100644
--- a/src/client/content_cao.cpp
+++ b/src/client/content_cao.cpp
@@ -523,7 +523,9 @@ void GenericCAO::removeFromScene(bool permanent)
 	// Should be true when removing the object permanently
 	// and false when refreshing (eg: updating visuals)
 	if (m_env && permanent) {
-		clearChildAttachments();
+		// The client does not know whether this object does re-appear to
+		// a later time, thus do not clear child attachments.
+
 		clearParentAttachment();
 	}
 
@@ -1330,10 +1332,17 @@ void GenericCAO::updateAttachments()
 
 	m_attached_to_local = parent && parent->isLocalPlayer();
 
-	if (!parent && m_attachment_parent_id) {
-		//m_is_visible = false; maybe later. needs better handling
-		return;
-	}
+	/*
+	Following cases exist:
+		m_attachment_parent_id == 0 && !parent
+			This object is not attached
+		m_attachment_parent_id != 0 && parent
+			This object is attached
+		m_attachment_parent_id != 0 && !parent
+			This object will be attached as soon the parent is known
+		m_attachment_parent_id == 0 && parent
+			Impossible case
+	*/
 
 	if (!parent) { // Detach or don't attach
 		if (m_matrixnode) {
diff --git a/src/client/content_cao.h b/src/client/content_cao.h
index 3a071101f..2c2d11077 100644
--- a/src/client/content_cao.h
+++ b/src/client/content_cao.h
@@ -156,6 +156,11 @@ public:
 
 	const v3f getPosition() const;
 
+	void setPosition(const v3f &pos)
+	{
+		pos_translator.val_current = pos;
+	}
+
 	inline const v3f &getRotation() const { return m_rotation; }
 
 	const bool isImmortal();
diff --git a/src/content_sao.cpp b/src/content_sao.cpp
index d6baa1580..87e6466a9 100644
--- a/src/content_sao.cpp
+++ b/src/content_sao.cpp
@@ -1107,14 +1107,14 @@ void PlayerSAO::step(float dtime, bool send_recommended)
 	if (!send_recommended)
 		return;
 
-	// If the object is attached client-side, don't waste bandwidth sending its
-	// position or rotation to clients.
-	if (m_position_not_sent && !isAttached()) {
+	if (m_position_not_sent) {
 		m_position_not_sent = false;
 		float update_interval = m_env->getSendRecommendedInterval();
 		v3f pos;
-		if (isAttached()) // Just in case we ever do send attachment position too
-			pos = m_env->getActiveObject(m_attachment_parent_id)->getBasePosition();
+		// When attached, the position is only sent to clients where the
+		// parent isn't known
+		if (isAttached())
+			pos = m_last_good_position;
 		else
 			pos = m_base_position;
 
diff --git a/src/server.cpp b/src/server.cpp
index 0e677199a..4aa8375c8 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -694,19 +694,33 @@ void Server::AsyncRunStep(bool initial_step)
 		// Route data to every client
 		for (const auto &client_it : clients) {
 			RemoteClient *client = client_it.second;
+			PlayerSAO *player = getPlayerSAO(client->peer_id);
 			std::string reliable_data;
 			std::string unreliable_data;
 			// Go through all objects in message buffer
 			for (const auto &buffered_message : buffered_messages) {
-				// If object is not known by client, skip it
+				// If object does not exist or is not known by client, skip it
 				u16 id = buffered_message.first;
-				if (client->m_known_objects.find(id) == client->m_known_objects.end())
+				ServerActiveObject *sao = m_env->getActiveObject(id);
+				if (!sao || client->m_known_objects.find(id) == client->m_known_objects.end())
 					continue;
 
 				// Get message list of object
 				std::vector<ActiveObjectMessage>* list = buffered_message.second;
 				// Go through every message
 				for (const ActiveObjectMessage &aom : *list) {
+					// Send position updates to players who do not see the attachment
+					if (aom.datastring[0] == GENERIC_CMD_UPDATE_POSITION) {
+						if (sao->getId() == player->getId())
+							continue;
+
+						// Do not send position updates for attached players
+						// as long the parent is known to the client
+						ServerActiveObject *parent = sao->getParent();
+						if (parent && client->m_known_objects.find(parent->getId()) !=
+								client->m_known_objects.end())
+							continue;
+					}
 					// Compose the full new data with header
 					std::string new_data;
 					// Add object id
@@ -1907,14 +1921,17 @@ void Server::SendActiveObjectRemoveAdd(RemoteClient *client, PlayerSAO *playersa
 	while (!added_objects.empty()) {
 		// Get object
 		u16 id = added_objects.front();
-		ServerActiveObject* obj = m_env->getActiveObject(id);
+		ServerActiveObject *obj = m_env->getActiveObject(id);
+		added_objects.pop();
+
+		if (!obj) {
+			warningstream << FUNCTION_NAME << ": NULL object id="
+				<< (int)id << std::endl;
+			continue;
+		}
 
 		// Get object type
-		u8 type = ACTIVEOBJECT_TYPE_INVALID;
-		if (!obj)
-			warningstream << FUNCTION_NAME << ": NULL object" << std::endl;
-		else
-			type = obj->getSendType();
+		u8 type = obj->getSendType();
 
 		// Add to data buffer for sending
 		writeU16((u8*)buf, id);
@@ -1922,19 +1939,13 @@ void Server::SendActiveObjectRemoveAdd(RemoteClient *client, PlayerSAO *playersa
 		writeU8((u8*)buf, type);
 		data.append(buf, 1);
 
-		if (obj)
-			data.append(serializeLongString(
-				obj->getClientInitializationData(client->net_proto_version)));
-		else
-			data.append(serializeLongString(""));
+		data.append(serializeLongString(
+			obj->getClientInitializationData(client->net_proto_version)));
 
 		// Add to known objects
 		client->m_known_objects.insert(id);
 
-		if (obj)
-			obj->m_known_by_count++;
-
-		added_objects.pop();
+		obj->m_known_by_count++;
 	}
 
 	NetworkPacket pkt(TOCLIENT_ACTIVE_OBJECT_REMOVE_ADD, data.size(), client->peer_id);