ContentCAO: Fix broken attachments on join (#8701)

What happened:
1) Object data is received. Client begins to read the data
2) Client initializes all its children (gob_cmd_update_infant)
3) Children try to attach to parent (yet not added)
4) Parent initializes, is added to the environment

And somewhere in between, Irrlicht wrecks up the attachments due to the missing matrix node.

The solution here is to:
1) Use the same structure as ServerActiveObject
2) Attach all children after the parent is really initialized

diff --git a/src/activeobject.h b/src/activeobject.h
index b6a0e67af37e51603c0a53ef584f6ff04f163346..a319ef904de4ed64037e1440891b5da7622096ec 100644 (file)
--- a/src/activeobject.h
+++ b/src/activeobject.h
@@ -20,8 +20,10 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #pragma once
 
 #include "irr_aabb3d.h"
+#include "irr_v3d.h"
 #include <string>
 
+
 enum ActiveObjectType {
        ACTIVEOBJECT_TYPE_INVALID = 0,
        ACTIVEOBJECT_TYPE_TEST = 1,
@@ -98,6 +100,16 @@ public:
 
 
        virtual bool collideWithObjects() const = 0;
+
+
+       virtual void setAttachment(int parent_id, const std::string &bone, v3f position,
+                       v3f rotation) {}
+       virtual void getAttachment(int *parent_id, std::string *bone, v3f *position,
+                       v3f *rotation) const {}
+       virtual void clearChildAttachments() {}
+       virtual void clearParentAttachment() {}
+       virtual void addAttachmentChild(int child_id) {}
+       virtual void removeAttachmentChild(int child_id) {}
 protected:
        u16 m_id; // 0 is invalid, "no id"
 };

diff --git a/src/client/clientenvironment.cpp b/src/client/clientenvironment.cpp
index a788c93c2b58089472a627ab646f9a4a2a2349e1..11dbcc35b1b422bd03bf4c1ae3dcc418bb9bfb95 100644 (file)
--- a/src/client/clientenvironment.cpp
+++ b/src/client/clientenvironment.cpp
@@ -47,8 +47,6 @@ ClientEnvironment::ClientEnvironment(ClientMap *map,
        m_texturesource(texturesource),
        m_client(client)
 {
-       char zero = 0;
-       memset(attachement_parent_ids, zero, sizeof(attachement_parent_ids));
 }
 
 ClientEnvironment::~ClientEnvironment()
@@ -392,7 +390,17 @@ void ClientEnvironment::addActiveObject(u16 id, u8 type,
                        <<std::endl;
        }
 
-       addActiveObject(obj);
+       u16 new_id = addActiveObject(obj);
+       // Object initialized:
+       if ((obj = getActiveObject(new_id))) {
+               // Final step is to update all children which are already known
+               // Data provided by GENERIC_CMD_SPAWN_INFANT
+               const auto &children = obj->getAttachmentChildIds();
+               for (auto c_id : children) {
+                       if (auto *o = getActiveObject(c_id))
+                               o->updateAttachments();
+               }
+       }
 }
 
 void ClientEnvironment::processActiveObjectMessage(u16 id, const std::string &data)

diff --git a/src/client/clientenvironment.h b/src/client/clientenvironment.h
index 4fa3f4848f2aab1625be181bf8c191a57b43b1d9..f182b5951864aae35c789db5a4fbe1bc4bef673a 100644 (file)
--- a/src/client/clientenvironment.h
+++ b/src/client/clientenvironment.h
@@ -138,8 +138,6 @@ public:
                std::vector<PointedThing> &objects
        );
 
-       u16 attachement_parent_ids[USHRT_MAX + 1];
-
        const std::list<std::string> &getPlayerNames() { return m_player_names; }
        void addPlayerName(const std::string &name) { m_player_names.push_back(name); }
        void removePlayerName(const std::string &name) { m_player_names.remove(name); }

diff --git a/src/client/clientobject.h b/src/client/clientobject.h
index 5e34177e4ba68c20ddddb626a564b617d3d95c9b..9870057306473b016d0c5acfb1bb9074d3339da3 100644 (file)
--- a/src/client/clientobject.h
+++ b/src/client/clientobject.h
@@ -22,6 +22,8 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "irrlichttypes_extrabloated.h"
 #include "activeobject.h"
 #include <unordered_map>
+#include <unordered_set>
+
 
 class ClientEnvironment;
 class ITextureSource;
@@ -51,8 +53,12 @@ public:
        virtual scene::ISceneNode *getSceneNode() { return NULL; }
        virtual scene::IAnimatedMeshSceneNode *getAnimatedMeshSceneNode() { return NULL; }
        virtual bool isLocalPlayer() const {return false;}
+
        virtual ClientActiveObject *getParent() const { return nullptr; };
-       virtual void setAttachments() {}
+       virtual const std::unordered_set<int> &getAttachmentChildIds() const
+       { static std::unordered_set<int> rv; return rv; }
+       virtual void updateAttachments() {};
+
        virtual bool doShowSelectionBox(){return true;}
 
        // Step object in time

diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp
index 388a71873d8010740b445df235c443cb580a1c46..d2ab0631a3213f2e8b04fbcdac7077f4f0db5510 100644 (file)
--- a/src/client/content_cao.cpp
+++ b/src/client/content_cao.cpp
@@ -17,36 +17,35 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */
 
+#include "content_cao.h"
+#include <IBillboardSceneNode.h>
 #include <ICameraSceneNode.h>
 #include <ITextSceneNode.h>
-#include <IBillboardSceneNode.h>
 #include <IMeshManipulator.h>
 #include <IAnimatedMeshSceneNode.h>
-#include "content_cao.h"
-#include "util/numeric.h" // For IntervalLimiter & setPitchYawRoll
-#include "util/serialize.h"
-#include "util/basic_macros.h"
+#include "client/client.h"
+#include "client/renderingengine.h"
 #include "client/sound.h"
 #include "client/tile.h"
-#include "environment.h"
+#include "util/basic_macros.h"
+#include "util/numeric.h" // For IntervalLimiter & setPitchYawRoll
+#include "util/serialize.h"
+#include "camera.h" // CameraModes
 #include "collision.h"
-#include "settings.h"
-#include "serialization.h" // For decompressZlib
-#include "clientobject.h"
-#include "mesh.h"
-#include "itemdef.h"
-#include "tool.h"
 #include "content_cso.h"
-#include "sound.h"
-#include "nodedef.h"
+#include "environment.h"
+#include "itemdef.h"
 #include "localplayer.h"
 #include "map.h"
-#include "camera.h" // CameraModes
-#include "client.h"
+#include "mesh.h"
+#include "nodedef.h"
+#include "serialization.h" // For decompressZlib
+#include "settings.h"
+#include "sound.h"
+#include "tool.h"
 #include "wieldmesh.h"
 #include <algorithm>
 #include <cmath>
-#include "client/renderingengine.h"
 
 class Settings;
 struct ToolCapabilities;
@@ -305,6 +304,7 @@ void TestCAO::processMessage(const std::string &data)
 */
 
 #include "genericobject.h"
+#include "clientobject.h"
 
 GenericCAO::GenericCAO(Client *client, ClientEnvironment *env):
                ClientActiveObject(0, client, env)
@@ -372,6 +372,7 @@ void GenericCAO::processInitData(const std::string &data)
        m_position = readV3F32(is);
        m_rotation = readV3F32(is);
        m_hp = readU16(is);
+
        const u8 num_messages = readU8(is);
 
        for (int i = 0; i < num_messages; i++) {
@@ -443,7 +444,7 @@ scene::IAnimatedMeshSceneNode* GenericCAO::getAnimatedMeshSceneNode()
 
 void GenericCAO::setChildrenVisible(bool toset)
 {
-       for (u16 cao_id : m_children) {
+       for (u16 cao_id : m_attachment_child_ids) {
                GenericCAO *obj = m_env->getGenericCAO(cao_id);
                if (obj) {
                        obj->setVisible(toset);
@@ -451,43 +452,79 @@ void GenericCAO::setChildrenVisible(bool toset)
        }
 }
 
-void GenericCAO::setAttachments()
+void GenericCAO::setAttachment(int parent_id, const std::string &bone, v3f position, v3f rotation)
 {
+       int old_parent = m_attachment_parent_id;
+       m_attachment_parent_id = parent_id;
+       m_attachment_bone = bone;
+       m_attachment_position = position;
+       m_attachment_rotation = rotation;
+
+       ClientActiveObject *parent = m_env->getActiveObject(parent_id);
+
+       if (parent_id != old_parent) {
+               if (auto *o = m_env->getActiveObject(old_parent))
+                       o->removeAttachmentChild(m_id);
+               if (parent)
+                       parent->addAttachmentChild(m_id);
+       }
+
        updateAttachments();
 }
 
-ClientActiveObject* GenericCAO::getParent() const
+void GenericCAO::getAttachment(int *parent_id, std::string *bone, v3f *position,
+       v3f *rotation) const
+{
+       *parent_id = m_attachment_parent_id;
+       *bone = m_attachment_bone;
+       *position = m_attachment_position;
+       *rotation = m_attachment_rotation;
+}
+
+void GenericCAO::clearChildAttachments()
 {
-       ClientActiveObject *obj = NULL;
+       // Cannot use for-loop here: setAttachment() modifies 'm_attachment_child_ids'!
+       while (!m_attachment_child_ids.empty()) {
+               int child_id = *m_attachment_child_ids.begin();
 
-       u16 attached_id = m_env->attachement_parent_ids[getId()];
+               if (ClientActiveObject *child = m_env->getActiveObject(child_id))
+                       child->setAttachment(0, "", v3f(), v3f());
 
-       if ((attached_id != 0) &&
-                       (attached_id != getId())) {
-               obj = m_env->getActiveObject(attached_id);
+               removeAttachmentChild(child_id);
        }
-       return obj;
 }
 
-void GenericCAO::removeFromScene(bool permanent)
+void GenericCAO::clearParentAttachment()
 {
-       // Should be true when removing the object permanently and false when refreshing (eg: updating visuals)
-       if((m_env != NULL) && (permanent))
-       {
-               for (u16 ci : m_children) {
-                       if (m_env->attachement_parent_ids[ci] == getId()) {
-                               m_env->attachement_parent_ids[ci] = 0;
-                       }
-               }
-               m_children.clear();
+       if (m_attachment_parent_id)
+               setAttachment(0, "", m_attachment_position, m_attachment_rotation);
+       else
+               setAttachment(0, "", v3f(), v3f());
+}
 
-               m_env->attachement_parent_ids[getId()] = 0;
+void GenericCAO::addAttachmentChild(int child_id)
+{
+       m_attachment_child_ids.insert(child_id);
+}
 
-               LocalPlayer* player = m_env->getLocalPlayer();
-               if (this == player->parent) {
-                       player->parent = nullptr;
-                       player->isAttached = false;
-               }
+void GenericCAO::removeAttachmentChild(int child_id)
+{
+       m_attachment_child_ids.erase(child_id);
+}
+
+ClientActiveObject* GenericCAO::getParent() const
+{
+       return m_attachment_parent_id ? m_env->getActiveObject(m_attachment_parent_id) :
+                       nullptr;
+}
+
+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();
+               clearParentAttachment();
        }
 
        if (m_meshnode) {
@@ -711,6 +748,7 @@ void GenericCAO::addToScene(ITextureSource *tsrc)
                updateTextures(m_current_texture_modifier);
 
        scene::ISceneNode *node = getSceneNode();
+
        if (node && !m_prop.nametag.empty() && !m_is_local_player) {
                // Add nametag
                v3f pos;
@@ -736,7 +774,7 @@ void GenericCAO::updateLight(u8 light_at_pos)
        updateLightNoCheck(light_at_pos);
 
        // Update light of all children
-       for (u16 i : m_children) {
+       for (u16 i : m_attachment_child_ids) {
                ClientActiveObject *obj = m_env->getActiveObject(i);
                if (obj) {
                        obj->updateLightNoCheck(light_at_pos);
@@ -871,12 +909,8 @@ void GenericCAO::step(float dtime, ClientEnvironment *env)
 
                // Attachments, part 1: All attached objects must be unparented first,
                // or Irrlicht causes a segmentation fault
-               for (auto ci = m_children.begin(); ci != m_children.end();) {
-                       if (m_env->attachement_parent_ids[*ci] != getId()) {
-                               ci = m_children.erase(ci);
-                               continue;
-                       }
-                       ClientActiveObject *obj = m_env->getActiveObject(*ci);
+               for (u16 cao_id : m_attachment_child_ids) {
+                       ClientActiveObject *obj = m_env->getActiveObject(cao_id);
                        if (obj) {
                                scene::ISceneNode *child_node = obj->getSceneNode();
                                // The node's parent is always an IDummyTraformationSceneNode,
@@ -884,18 +918,16 @@ void GenericCAO::step(float dtime, ClientEnvironment *env)
                                if (child_node)
                                        child_node->getParent()->setParent(m_smgr->getRootSceneNode());
                        }
-                       ++ci;
                }
 
                removeFromScene(false);
                addToScene(m_client->tsrc());
 
                // Attachments, part 2: Now that the parent has been refreshed, put its attachments back
-               for (u16 cao_id : m_children) {
-                       // Get the object of the child
+               for (u16 cao_id : m_attachment_child_ids) {
                        ClientActiveObject *obj = m_env->getActiveObject(cao_id);
                        if (obj)
-                               obj->setAttachments();
+                               obj->updateAttachments();
                }
        }
 
@@ -916,7 +948,6 @@ void GenericCAO::step(float dtime, ClientEnvironment *env)
                {
                        LocalPlayer *player = m_env->getLocalPlayer();
                        player->overridePosition = getParent()->getPosition();
-                       m_env->getLocalPlayer()->parent = getParent();
                }
        } else {
                rot_translator.translate(dtime);
@@ -1296,6 +1327,14 @@ void GenericCAO::updateBonePosition()
 void GenericCAO::updateAttachments()
 {
        ClientActiveObject *parent = getParent();
+
+       m_attached_to_local = parent && parent->isLocalPlayer();
+
+       if (!parent && m_attachment_parent_id) {
+               //m_is_visible = false; maybe later. needs better handling
+               return;
+       }
+
        if (!parent) { // Detach or don't attach
                if (m_matrixnode) {
                        v3f old_pos = m_matrixnode->getAbsolutePosition();
@@ -1303,10 +1342,6 @@ void GenericCAO::updateAttachments()
                        getPosRotMatrix().setTranslation(old_pos);
                        m_matrixnode->updateAbsolutePosition();
                }
-               if (m_is_local_player) {
-                       LocalPlayer *player = m_env->getLocalPlayer();
-                       player->isAttached = false;
-               }
        }
        else // Attach
        {
@@ -1325,10 +1360,11 @@ void GenericCAO::updateAttachments()
                        getPosRotMatrix().setRotationDegrees(m_attachment_rotation);
                        m_matrixnode->updateAbsolutePosition();
                }
-               if (m_is_local_player) {
-                       LocalPlayer *player = m_env->getLocalPlayer();
-                       player->isAttached = true;
-               }
+       }
+       if (m_is_local_player) {
+               LocalPlayer *player = m_env->getLocalPlayer();
+               player->isAttached = parent;
+               player->parent = parent;
        }
 }
 
@@ -1488,31 +1524,15 @@ void GenericCAO::processMessage(const std::string &data)
                updateBonePosition();
        } else if (cmd == GENERIC_CMD_ATTACH_TO) {
                u16 parent_id = readS16(is);
-               u16 &old_parent_id = m_env->attachement_parent_ids[getId()];
-               if (parent_id != old_parent_id) {
-                       if (GenericCAO *old_parent = m_env->getGenericCAO(old_parent_id)) {
-                               old_parent->m_children.erase(std::remove(
-                                       m_children.begin(), m_children.end(),
-                                       getId()), m_children.end());
-                       }
-                       if (GenericCAO *new_parent = m_env->getGenericCAO(parent_id))
-                               new_parent->m_children.push_back(getId());
-
-                       old_parent_id = parent_id;
-               }
+               std::string bone = deSerializeString(is);
+               v3f position = readV3F32(is);
+               v3f rotation = readV3F32(is);
 
-               m_attachment_bone = deSerializeString(is);
-               m_attachment_position = readV3F32(is);
-               m_attachment_rotation = readV3F32(is);
+               setAttachment(parent_id, bone, position, rotation);
 
                // localplayer itself can't be attached to localplayer
-               if (!m_is_local_player) {
-                       m_attached_to_local = getParent() != NULL && getParent()->isLocalPlayer();
-                       // Objects attached to the local player should be hidden by default
+               if (!m_is_local_player)
                        m_is_visible = !m_attached_to_local;
-               }
-
-               updateAttachments();
        } else if (cmd == GENERIC_CMD_PUNCHED) {
                u16 result_hp = readU16(is);
 
@@ -1539,6 +1559,12 @@ void GenericCAO::processMessage(const std::string &data)
                                        m_reset_textures_timer += 0.05 * damage;
                                updateTextures(m_current_texture_modifier + "^[brighten");
                        }
+               } else {
+                       // Same as 'Server::DiePlayer'
+                       clearParentAttachment();
+                       // Same as 'ObjectRef::l_remove'
+                       if (!m_is_player)
+                               clearChildAttachments();
                }
        } else if (cmd == GENERIC_CMD_UPDATE_ARMOR_GROUPS) {
                m_armor_groups.clear();
@@ -1561,13 +1587,10 @@ void GenericCAO::processMessage(const std::string &data)
                }
        } else if (cmd == GENERIC_CMD_SPAWN_INFANT) {
                u16 child_id = readU16(is);
-               u8 type = readU8(is);
+               u8 type = readU8(is); // maybe this will be useful later
+               (void)type;
 
-               if (GenericCAO *childobj = m_env->getGenericCAO(child_id)) {
-                       childobj->processInitData(deSerializeLongString(is));
-               } else {
-                       m_env->addActiveObject(child_id, type, deSerializeLongString(is));
-               }
+               addAttachmentChild(child_id);
        } else {
                warningstream << FUNCTION_NAME
                        << ": unknown command or outdated client \""

diff --git a/src/client/content_cao.h b/src/client/content_cao.h
index ca1518fb22ccc99fddbb9ad74fb774922281c65f..aef1f1296afa3f1d44301716f35f51cd96437345 100644 (file)
--- a/src/client/content_cao.h
+++ b/src/client/content_cao.h
@@ -102,10 +102,14 @@ private:
        bool m_animation_loop = true;
        // 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;
+       std::unordered_set<int> m_attachment_child_ids;
        std::string m_attachment_bone = "";
        v3f m_attachment_position;
        v3f m_attachment_rotation;
        bool m_attached_to_local = false;
+
        int m_anim_frame = 0;
        int m_anim_num_frames = 1;
        float m_anim_framelength = 0.2f;
@@ -122,8 +126,6 @@ private:
        bool m_is_visible = false;
        s8 m_glow = 0;
 
-       std::vector<u16> m_children;
-
 public:
        GenericCAO(Client *client, ClientEnvironment *env);
 
@@ -199,10 +201,17 @@ public:
        }
 
        void setChildrenVisible(bool toset);
-
+       void setAttachment(int parent_id, const std::string &bone, v3f position, v3f rotation);
+       void getAttachment(int *parent_id, std::string *bone, v3f *position,
+                       v3f *rotation) const;
+       void clearChildAttachments();
+       void clearParentAttachment();
+       void addAttachmentChild(int child_id);
+       void removeAttachmentChild(int child_id);
        ClientActiveObject *getParent() const;
-
-       void setAttachments();
+       const std::unordered_set<int> &getAttachmentChildIds() const
+       { return m_attachment_child_ids; }
+       void updateAttachments();
 
        void removeFromScene(bool permanent);
 
@@ -235,8 +244,6 @@ public:
 
        void updateBonePosition();
 
-       void updateAttachments();
-
        void processMessage(const std::string &data);
 
        bool directReportPunch(v3f dir, const ItemStack *punchitem=NULL,

diff --git a/src/content_sao.cpp b/src/content_sao.cpp
index 336f8df18f0a92db0fd93681cb0729fce2aed9ed..41b1aec18794c65db0bea633ccd167368f4cd3e4 100644 (file)
--- a/src/content_sao.cpp
+++ b/src/content_sao.cpp
@@ -200,7 +200,7 @@ void UnitSAO::setAttachment(int parent_id, const std::string &bone, v3f position
 }
 
 void UnitSAO::getAttachment(int *parent_id, std::string *bone, v3f *position,
-       v3f *rotation)
+       v3f *rotation) const
 {
        *parent_id = m_attachment_parent_id;
        *bone = m_attachment_bone;
@@ -242,7 +242,7 @@ void UnitSAO::removeAttachmentChild(int child_id)
        m_attachment_child_ids.erase(child_id);
 }
 
-const std::unordered_set<int> &UnitSAO::getAttachmentChildIds()
+const std::unordered_set<int> &UnitSAO::getAttachmentChildIds() const
 {
        return m_attachment_child_ids;
 }
@@ -575,6 +575,8 @@ std::string LuaEntitySAO::getClientInitializationData(u16 protocol_version)
                        (ii != m_attachment_child_ids.end()); ++ii) {
                if (ServerActiveObject *obj = m_env->getActiveObject(*ii)) {
                        message_count++;
+                       // TODO after a protocol bump: only send the object initialization data
+                       // to older clients (superfluous since this message exists)
                        msg_os << serializeLongString(gob_cmd_update_infant(*ii, obj->getSendType(),
                                obj->getClientInitializationData(protocol_version)));
                }

diff --git a/src/content_sao.h b/src/content_sao.h
index 9fbddbc5ad274904adfdb33c9f19a1139af673e3..0c503b4cbb1903cf93998b3423236ae36a9da1b1 100644 (file)
--- a/src/content_sao.h
+++ b/src/content_sao.h
@@ -56,12 +56,13 @@ public:
        void setBonePosition(const std::string &bone, v3f position, v3f rotation);
        void getBonePosition(const std::string &bone, v3f *position, v3f *rotation);
        void setAttachment(int parent_id, const std::string &bone, v3f position, v3f rotation);
-       void getAttachment(int *parent_id, std::string *bone, v3f *position, v3f *rotation);
+       void getAttachment(int *parent_id, std::string *bone, v3f *position,
+                       v3f *rotation) const;
        void clearChildAttachments();
        void clearParentAttachment();
        void addAttachmentChild(int child_id);
        void removeAttachmentChild(int child_id);
-       const std::unordered_set<int> &getAttachmentChildIds();
+       const std::unordered_set<int> &getAttachmentChildIds() const;
        ServerActiveObject *getParent() const;
        ObjectProperties* accessObjectProperties();
        void notifyObjectPropertiesModified();

diff --git a/src/serverobject.h b/src/serverobject.h
index 4a9430107fbc6c08b4e562333f9912e8382f7567..42eed071051ed85178429575e55689bfe969bef2 100644 (file)
--- a/src/serverobject.h
+++ b/src/serverobject.h
@@ -161,17 +161,7 @@ public:
        {}
        virtual void getBonePosition(const std::string &bone, v3f *position, v3f *lotation)
        {}
-       virtual void setAttachment(int parent_id, const std::string &bone, v3f position, v3f rotation)
-       {}
-       virtual void getAttachment(int *parent_id, std::string *bone, v3f *position, v3f *rotation)
-       {}
-       virtual void clearChildAttachments() {}
-       virtual void clearParentAttachment() {}
-       virtual void addAttachmentChild(int child_id)
-       {}
-       virtual void removeAttachmentChild(int child_id)
-       {}
-       virtual const std::unordered_set<int> &getAttachmentChildIds()
+       virtual const std::unordered_set<int> &getAttachmentChildIds() const
        { static std::unordered_set<int> rv; return rv; }
        virtual ServerActiveObject *getParent() const { return nullptr; }
        virtual ObjectProperties* accessObjectProperties()