Move shared parameters sending to UnitSAO (#9968)

Better header sorting by topic
Make UnitSAO-specific parameters private
Skip redundant recursive entity sending code (since ~5.2.0)

src/client/content_cao.cpp

@@ -475,6 +475,7 @@ void GenericCAO::setAttachment(int parent_id, const std::string &bone, v3f posit
 			parent->addAttachmentChild(m_id);
 	}
 
+
 	updateAttachments();
 }
 

src/server/luaentity_sao.cpp

@@ -123,12 +123,11 @@ void LuaEntitySAO::step(float dtime, bool send_recommended)
 	}
 
 	// If attached, check that our parent is still there. If it isn't, detach.
-	if(m_attachment_parent_id && !isAttached())
+	if (m_attachment_parent_id && !isAttached()) {
-	{
+		// This is handled when objects are removed from the map
-		m_attachment_parent_id = 0;
+		warningstream << "LuaEntitySAO::step() id=" << m_id <<
-		m_attachment_bone = "";
+			" is attached to nonexistent parent. This is a bug." << std::endl;
-		m_attachment_position = v3f(0,0,0);
+		clearParentAttachment();
-		m_attachment_rotation = v3f(0,0,0);
 		sendPosition(false, true);
 	}
 
@@ -217,43 +216,7 @@ void LuaEntitySAO::step(float dtime, bool send_recommended)
 		}
 	}
 
-	if (!m_armor_groups_sent) {
+	sendOutdatedData();
-		m_armor_groups_sent = true;
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, generateUpdateArmorGroupsCommand());
-	}
-
-	if (!m_animation_sent) {
-		m_animation_sent = true;
-		std::string str = generateUpdateAnimationCommand();
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, str);
-	}
-
-	if (!m_animation_speed_sent) {
-		m_animation_speed_sent = true;
-		std::string str = generateUpdateAnimationSpeedCommand();
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, str);
-	}
-
-	if (!m_bone_position_sent) {
-		m_bone_position_sent = true;
-		for (std::unordered_map<std::string, core::vector2d<v3f>>::const_iterator
-				ii = m_bone_position.begin(); ii != m_bone_position.end(); ++ii){
-			std::string str = generateUpdateBonePositionCommand((*ii).first,
-					(*ii).second.X, (*ii).second.Y);
-			// create message and add to list
-			m_messages_out.emplace(getId(), true, str);
-		}
-	}
-
-	if (!m_attachment_sent) {
-		m_attachment_sent = true;
-		std::string str = generateUpdateAttachmentCommand();
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, str);
-	}
 }
 
 std::string LuaEntitySAO::getClientInitializationData(u16 protocol_version)
@@ -273,20 +236,19 @@ std::string LuaEntitySAO::getClientInitializationData(u16 protocol_version)
 	msg_os << serializeLongString(getPropertyPacket()); // message 1
 	msg_os << serializeLongString(generateUpdateArmorGroupsCommand()); // 2
 	msg_os << serializeLongString(generateUpdateAnimationCommand()); // 3
-	for (std::unordered_map<std::string, core::vector2d<v3f>>::const_iterator
+	for (const auto &bone_pos : m_bone_position) {
-			ii = m_bone_position.begin(); ii != m_bone_position.end(); ++ii) {
+		msg_os << serializeLongString(generateUpdateBonePositionCommand(
-		msg_os << serializeLongString(generateUpdateBonePositionCommand((*ii).first,
+			bone_pos.first, bone_pos.second.X, bone_pos.second.Y)); // m_bone_position.size
-				(*ii).second.X, (*ii).second.Y)); // m_bone_position.size
 	}
 	msg_os << serializeLongString(generateUpdateAttachmentCommand()); // 4
+
 	int message_count = 4 + m_bone_position.size();
-	for (std::unordered_set<int>::const_iterator ii = m_attachment_child_ids.begin();
+
-			(ii != m_attachment_child_ids.end()); ++ii) {
+	for (const auto &id : getAttachmentChildIds()) {
-		if (ServerActiveObject *obj = m_env->getActiveObject(*ii)) {
+		if (ServerActiveObject *obj = m_env->getActiveObject(id)) {
 			message_count++;
-			// TODO after a protocol bump: only send the object initialization data
+			msg_os << serializeLongString(obj->generateUpdateInfantCommand(
-			// to older clients (superfluous since this message exists)
+				id, protocol_version));
-			msg_os << serializeLongString(obj->generateUpdateInfantCommand(*ii, protocol_version));
 		}
 	}
 
@@ -294,7 +256,8 @@ std::string LuaEntitySAO::getClientInitializationData(u16 protocol_version)
 	message_count++;
 
 	writeU8(os, message_count);
-	os.write(msg_os.str().c_str(), msg_os.str().size());
+	std::string serialized = msg_os.str();
+	os.write(serialized.c_str(), serialized.size());
 
 	// return result
 	return os.str();

src/server/player_sao.cpp

@@ -120,24 +120,26 @@ std::string PlayerSAO::getClientInitializationData(u16 protocol_version)
 	msg_os << serializeLongString(getPropertyPacket()); // message 1
 	msg_os << serializeLongString(generateUpdateArmorGroupsCommand()); // 2
 	msg_os << serializeLongString(generateUpdateAnimationCommand()); // 3
-	for (std::unordered_map<std::string, core::vector2d<v3f>>::const_iterator
+	for (const auto &bone_pos : m_bone_position) {
-			ii = m_bone_position.begin(); ii != m_bone_position.end(); ++ii) {
+		msg_os << serializeLongString(generateUpdateBonePositionCommand(
-		msg_os << serializeLongString(generateUpdateBonePositionCommand((*ii).first,
+			bone_pos.first, bone_pos.second.X, bone_pos.second.Y)); // m_bone_position.size
-			(*ii).second.X, (*ii).second.Y)); // m_bone_position.size
 	}
 	msg_os << serializeLongString(generateUpdateAttachmentCommand()); // 4
 	msg_os << serializeLongString(generateUpdatePhysicsOverrideCommand()); // 5
+
 	int message_count = 5 + m_bone_position.size();
-	for (std::unordered_set<int>::const_iterator ii = m_attachment_child_ids.begin();
+
-			ii != m_attachment_child_ids.end(); ++ii) {
+	for (const auto &id : getAttachmentChildIds()) {
-		if (ServerActiveObject *obj = m_env->getActiveObject(*ii)) {
+		if (ServerActiveObject *obj = m_env->getActiveObject(id)) {
 			message_count++;
-			msg_os << serializeLongString(obj->generateUpdateInfantCommand(*ii, protocol_version));
+			msg_os << serializeLongString(obj->generateUpdateInfantCommand(
+				id, protocol_version));
 		}
 	}
 
 	writeU8(os, message_count);
-	os.write(msg_os.str().c_str(), msg_os.str().size());
+	std::string serialized = msg_os.str();
+	os.write(serialized.c_str(), serialized.size());
 
 	// return result
 	return os.str();
@@ -227,10 +229,10 @@ void PlayerSAO::step(float dtime, bool send_recommended)
 
 	// If attached, check that our parent is still there. If it isn't, detach.
 	if (m_attachment_parent_id && !isAttached()) {
-		m_attachment_parent_id = 0;
+		// This is handled when objects are removed from the map
-		m_attachment_bone = "";
+		warningstream << "PlayerSAO::step() id=" << m_id <<
-		m_attachment_position = v3f(0.0f, 0.0f, 0.0f);
+			" is attached to nonexistent parent. This is a bug." << std::endl;
-		m_attachment_rotation = v3f(0.0f, 0.0f, 0.0f);
+		clearParentAttachment();
 		setBasePosition(m_last_good_position);
 		m_env->getGameDef()->SendMovePlayer(m_peer_id);
 	}
@@ -290,40 +292,13 @@ void PlayerSAO::step(float dtime, bool send_recommended)
 		m_messages_out.emplace(getId(), false, str);
 	}
 
-	if (!m_armor_groups_sent) {
-		m_armor_groups_sent = true;
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, generateUpdateArmorGroupsCommand());
-	}
-
 	if (!m_physics_override_sent) {
 		m_physics_override_sent = true;
 		// create message and add to list
 		m_messages_out.emplace(getId(), true, generateUpdatePhysicsOverrideCommand());
 	}
 
-	if (!m_animation_sent) {
+	sendOutdatedData();
-		m_animation_sent = true;
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, generateUpdateAnimationCommand());
-	}
-
-	if (!m_bone_position_sent) {
-		m_bone_position_sent = true;
-		for (std::unordered_map<std::string, core::vector2d<v3f>>::const_iterator
-				ii = m_bone_position.begin(); ii != m_bone_position.end(); ++ii) {
-			std::string str = generateUpdateBonePositionCommand((*ii).first,
-					(*ii).second.X, (*ii).second.Y);
-			// create message and add to list
-			m_messages_out.emplace(getId(), true, str);
-		}
-	}
-
-	if (!m_attachment_sent) {
-		m_attachment_sent = true;
-		// create message and add to list
-		m_messages_out.emplace(getId(), true, generateUpdateAttachmentCommand());
-	}
 }
 
 std::string PlayerSAO::generateUpdatePhysicsOverrideCommand() const

src/server/serveractiveobject.cpp

@@ -57,7 +57,12 @@ std::string ServerActiveObject::generateUpdateInfantCommand(u16 infant_id, u16 p
 	// parameters
 	writeU16(os, infant_id);
 	writeU8(os, getSendType());
+	if (protocol_version < 38) {
+		// Clients since 4aa9a66 so no longer need this data
+		// Version 38 is the first bump after that commit.
+		// See also: ClientEnvironment::addActiveObject
 		os << serializeLongString(getClientInitializationData(protocol_version));
+	}
 	return os.str();
 }
 

src/server/unit_sao.cpp

@@ -88,6 +88,39 @@ void UnitSAO::getBonePosition(const std::string &bone, v3f *position, v3f *rotat
 	*rotation = m_bone_position[bone].Y;
 }
 
+// clang-format off
+void UnitSAO::sendOutdatedData()
+{
+	if (!m_armor_groups_sent) {
+		m_armor_groups_sent = true;
+		m_messages_out.emplace(getId(), true, generateUpdateArmorGroupsCommand());
+	}
+
+	if (!m_animation_sent) {
+		m_animation_sent = true;
+		m_animation_speed_sent = true;
+		m_messages_out.emplace(getId(), true, generateUpdateAnimationCommand());
+	} else if (!m_animation_speed_sent) {
+		// Animation speed is also sent when 'm_animation_sent == false'
+		m_animation_speed_sent = true;
+		m_messages_out.emplace(getId(), true, generateUpdateAnimationSpeedCommand());
+	}
+
+	if (!m_bone_position_sent) {
+		m_bone_position_sent = true;
+		for (const auto &bone_pos : m_bone_position) {
+			m_messages_out.emplace(getId(), true, generateUpdateBonePositionCommand(
+				bone_pos.first, bone_pos.second.X, bone_pos.second.Y));
+		}
+	}
+
+	if (!m_attachment_sent) {
+		m_attachment_sent = true;
+		m_messages_out.emplace(getId(), true, generateUpdateAttachmentCommand());
+	}
+}
+// clang-format on
+
 void UnitSAO::setAttachment(
 		int parent_id, const std::string &bone, v3f position, v3f rotation)
 {

src/server/unit_sao.h

@@ -29,6 +29,11 @@ public:
 	UnitSAO(ServerEnvironment *env, v3f pos);
 	virtual ~UnitSAO() = default;
 
+	u16 getHP() const { return m_hp; }
+	// Use a function, if isDead can be defined by other conditions
+	bool isDead() const { return m_hp == 0; }
+
+	// Rotation
 	void setRotation(v3f rotation) { m_rotation = rotation; }
 	const v3f &getRotation() const { return m_rotation; }
 	v3f getRadRotation() { return m_rotation * core::DEGTORAD; }
@@ -36,26 +41,28 @@ public:
 	// Deprecated
 	f32 getRadYawDep() const { return (m_rotation.Y + 90.) * core::DEGTORAD; }
 
-	u16 getHP() const { return m_hp; }
+	// Armor groups
-	// Use a function, if isDead can be defined by other conditions
-	bool isDead() const { return m_hp == 0; }
-
-	inline bool isAttached() const { return getParent(); }
-
 	inline bool isImmortal() const
 	{
 		return itemgroup_get(getArmorGroups(), "immortal");
 	}
-
 	void setArmorGroups(const ItemGroupList &armor_groups);
 	const ItemGroupList &getArmorGroups() const;
+
+	// Animation
 	void setAnimation(v2f frame_range, float frame_speed, float frame_blend,
 			bool frame_loop);
 	void getAnimation(v2f *frame_range, float *frame_speed, float *frame_blend,
 			bool *frame_loop);
 	void setAnimationSpeed(float frame_speed);
+
+	// Bone position
 	void setBonePosition(const std::string &bone, v3f position, v3f rotation);
 	void getBonePosition(const std::string &bone, v3f *position, v3f *rotation);
+
+	// Attachments
+	ServerActiveObject *getParent() const;
+	inline bool isAttached() const { return getParent(); }
 	void setAttachment(int parent_id, const std::string &bone, v3f position,
 			v3f rotation);
 	void getAttachment(int *parent_id, std::string *bone, v3f *position,
@@ -65,10 +72,13 @@ public:
 	void addAttachmentChild(int child_id);
 	void removeAttachmentChild(int child_id);
 	const std::unordered_set<int> &getAttachmentChildIds() const;
-	ServerActiveObject *getParent() const;
+
+	// Object properties
 	ObjectProperties *accessObjectProperties();
 	void notifyObjectPropertiesModified();
+	void sendOutdatedData();
 
+	// Update packets
 	std::string generateUpdateAttachmentCommand() const;
 	std::string generateUpdateAnimationSpeedCommand() const;
 	std::string generateUpdateAnimationCommand() const;
@@ -77,21 +87,36 @@ public:
 			const v3f &velocity, const v3f &acceleration, const v3f &rotation,
 			bool do_interpolate, bool is_movement_end, f32 update_interval);
 	std::string generateSetPropertiesCommand(const ObjectProperties &prop) const;
-	void sendPunchCommand();
 	static std::string generateUpdateBonePositionCommand(const std::string &bone,
 			const v3f &position, const v3f &rotation);
+	void sendPunchCommand();
 
 protected:
 	u16 m_hp = 1;
 
 	v3f m_rotation;
 
+	ItemGroupList m_armor_groups;
+
+	// Object properties
 	bool m_properties_sent = true;
 	ObjectProperties m_prop;
 
-	ItemGroupList m_armor_groups;
+	// Stores position and rotation for each bone name
+	std::unordered_map<std::string, core::vector2d<v3f>> m_bone_position;
+
+	int m_attachment_parent_id = 0;
+
+private:
+	void onAttach(int parent_id);
+	void onDetach(int parent_id);
+
+	std::string generatePunchCommand(u16 result_hp) const;
+
+	// Armor groups
 	bool m_armor_groups_sent = false;
 
+	// Animation
 	v2f m_animation_range;
 	float m_animation_speed = 0.0f;
 	float m_animation_blend = 0.0f;
@@ -99,20 +124,13 @@ protected:
 	bool m_animation_sent = false;
 	bool m_animation_speed_sent = false;
 
-	// Stores position and rotation for each bone name
+	// Bone positions
-	std::unordered_map<std::string, core::vector2d<v3f>> m_bone_position;
 	bool m_bone_position_sent = false;
 
-	int m_attachment_parent_id = 0;
+	// Attachments
 	std::unordered_set<int> m_attachment_child_ids;
 	std::string m_attachment_bone = "";
 	v3f m_attachment_position;
 	v3f m_attachment_rotation;
 	bool m_attachment_sent = false;
-
-private:
-	void onAttach(int parent_id);
-	void onDetach(int parent_id);
-
-	std::string generatePunchCommand(u16 result_hp) const;
 };