Merge pull request #9642 from AndrewMeadows/beware-humbletim

fix performance problem for invalid "avatarEntityData"

interface/src/Application.cpp

@@ -869,10 +869,6 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo
     // connect to the packet sent signal of the _entityEditSender
     connect(&_entityEditSender, &EntityEditPacketSender::packetSent, this, &Application::packetSent);
 
-    // send the identity packet for our avatar each second to our avatar mixer
-    connect(&identityPacketTimer, &QTimer::timeout, myAvatar.get(), &MyAvatar::sendIdentityPacket);
-    identityPacketTimer.start(AVATAR_IDENTITY_PACKET_SEND_INTERVAL_MSECS);
-
     const char** constArgv = const_cast<const char**>(argv);
     QString concurrentDownloadsStr = getCmdOption(argc, constArgv, "--concurrent-downloads");
     bool success;

interface/src/avatar/Avatar.cpp

@@ -228,17 +228,43 @@ void Avatar::updateAvatarEntities() {
         return;
     }
 
-    bool success = true;
     QScriptEngine scriptEngine;
     entityTree->withWriteLock([&] {
         AvatarEntityMap avatarEntities = getAvatarEntityData();
-        for (auto entityID : avatarEntities.keys()) {
+        AvatarEntityMap::const_iterator dataItr = avatarEntities.begin();
+        while (dataItr != avatarEntities.end()) {
+            // compute hash of data.  TODO? cache this?
+            QByteArray data = dataItr.value();
+            uint32_t newHash = qHash(data);
+
+            // check to see if we recognize this hash and whether it was already successfully processed
+            QUuid entityID = dataItr.key();
+            MapOfAvatarEntityDataHashes::iterator stateItr = _avatarEntityDataHashes.find(entityID);
+            if (stateItr != _avatarEntityDataHashes.end()) {
+                if (stateItr.value().success) {
+                    if (newHash == stateItr.value().hash) {
+                        // data hasn't changed --> nothing to do
+                        ++dataItr;
+                        continue;
+                    }
+                } else {
+                    // NOTE: if the data was unsuccessful in producing an entity in the past
+                    // we will try again just in case something changed (unlikely).
+                    // Unfortunately constantly trying to build the entity for this data costs
+                    // CPU cycles that we'd rather not spend.
+                    // TODO? put a maximum number of tries on this?
+                }
+            } else {
+                // remember this hash for the future
+                stateItr = _avatarEntityDataHashes.insert(entityID, AvatarEntityDataHash(newHash));
+            }
+            ++dataItr;
+
             // see EntityEditPacketSender::queueEditEntityMessage for the other end of this.  unpack properties
             // and either add or update the entity.
-            QByteArray jsonByteArray = avatarEntities.value(entityID);
+            QJsonDocument jsonProperties = QJsonDocument::fromBinaryData(data);
-            QJsonDocument jsonProperties = QJsonDocument::fromBinaryData(jsonByteArray);
             if (!jsonProperties.isObject()) {
-                qCDebug(interfaceapp) << "got bad avatarEntity json" << QString(jsonByteArray.toHex());
+                qCDebug(interfaceapp) << "got bad avatarEntity json" << QString(data.toHex());
                 continue;
             }
 
@@ -266,8 +292,9 @@ void Avatar::updateAvatarEntities() {
                 properties.setScript(noScript);
             }
 
+            // try to build the entity
             EntityItemPointer entity = entityTree->findEntityByEntityItemID(EntityItemID(entityID));
-
+            bool success = true;
             if (entity) {
                 if (entityTree->updateEntity(entityID, properties)) {
                     entity->updateLastEditedFromRemote();
@@ -280,6 +307,7 @@ void Avatar::updateAvatarEntities() {
                     success = false;
                 }
             }
+            stateItr.value().success = success;
         }
 
         AvatarEntityIDs recentlyDettachedAvatarEntities = getAndClearRecentlyDetachedIDs();
@@ -292,13 +320,19 @@ void Avatar::updateAvatarEntities() {
                     }
                 }
             });
+
+            // remove stale data hashes
+            foreach (auto entityID, recentlyDettachedAvatarEntities) {
+                MapOfAvatarEntityDataHashes::iterator stateItr = _avatarEntityDataHashes.find(entityID);
+                if (stateItr != _avatarEntityDataHashes.end()) {
+                    _avatarEntityDataHashes.erase(stateItr);
+                }
+            }
         }
     });
 
-    if (success) {
     setAvatarEntityDataChanged(false);
 }
-}
 
 bool Avatar::shouldDie() const {
     const qint64 AVATAR_SILENCE_THRESHOLD_USECS = 5 * USECS_PER_SECOND;
@@ -364,6 +398,9 @@ void Avatar::simulate(float deltaTime, bool inView) {
         measureMotionDerivatives(deltaTime);
         simulateAttachments(deltaTime);
         updatePalms();
+    }
+    {
+        PROFILE_RANGE(simulation, "entities");
         updateAvatarEntities();
     }
 }

interface/src/avatar/Avatar.h

@@ -269,6 +269,16 @@ protected:
 
 
 private:
+    class AvatarEntityDataHash {
+    public:
+        AvatarEntityDataHash(uint32_t h) : hash(h) {};
+        uint32_t hash { 0 };
+        bool success { false };
+    };
+
+    using MapOfAvatarEntityDataHashes = QMap<QUuid, AvatarEntityDataHash>;
+    MapOfAvatarEntityDataHashes _avatarEntityDataHashes;
+
     uint64_t _lastRenderUpdateTime { 0 };
     int _leftPointerGeometryID { 0 };
     int _rightPointerGeometryID { 0 };

interface/src/avatar/MyAvatar.cpp

@@ -376,7 +376,9 @@ void MyAvatar::update(float deltaTime) {
         Q_ARG(glm::vec3, (getPosition() - halfBoundingBoxDimensions)),
         Q_ARG(glm::vec3, (halfBoundingBoxDimensions*2.0f)));
 
-    if (_avatarEntityDataLocallyEdited) {
+    uint64_t now = usecTimestampNow();
+    if (now > _identityPacketExpiry || _avatarEntityDataLocallyEdited) {
+        _identityPacketExpiry = now + AVATAR_IDENTITY_PACKET_SEND_INTERVAL_MSECS;
         sendIdentityPacket();
     }
 
@@ -1212,7 +1214,7 @@ void MyAvatar::useFullAvatarURL(const QUrl& fullAvatarURL, const QString& modelN
         setSkeletonModelURL(fullAvatarURL);
         UserActivityLogger::getInstance().changedModel("skeleton", urlString);
     }
-    sendIdentityPacket();
+    _identityPacketExpiry = 0; // triggers an identity packet next update()
 }
 
 void MyAvatar::setAttachmentData(const QVector<AttachmentData>& attachmentData) {

interface/src/avatar/MyAvatar.h

@@ -507,6 +507,8 @@ private:
     std::mutex _holdActionsMutex;
     std::vector<AvatarActionHold*> _holdActions;
 
+    uint64_t _identityPacketExpiry { 0 };
+
     float AVATAR_MOVEMENT_ENERGY_CONSTANT { 0.001f };
     float AUDIO_ENERGY_CONSTANT { 0.000001f };
     float MAX_AVATAR_MOVEMENT_PER_FRAME { 30.0f };

libraries/avatars/src/AvatarData.cpp

@@ -63,7 +63,6 @@ AvatarData::AvatarData() :
     _handState(0),
     _keyState(NO_KEY_DOWN),
     _forceFaceTrackerConnected(false),
-    _hasNewJointData(true),
     _headData(NULL),
     _displayNameTargetAlpha(1.0f),
     _displayNameAlpha(1.0f),
@@ -703,7 +702,6 @@ int AvatarData::parseDataFromBuffer(const QByteArray& buffer) {
     bool hasFaceTrackerInfo      = HAS_FLAG(packetStateFlags, AvatarDataPacket::PACKET_HAS_FACE_TRACKER_INFO);
     bool hasJointData            = HAS_FLAG(packetStateFlags, AvatarDataPacket::PACKET_HAS_JOINT_DATA);
 
-
     quint64 now = usecTimestampNow();
 
     if (hasAvatarGlobalPosition) {
@@ -2202,14 +2200,24 @@ void AvatarData::setAttachmentsVariant(const QVariantList& variant) {
     setAttachmentData(newAttachments);
 }
 
+const int MAX_NUM_AVATAR_ENTITIES = 42;
+
 void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& entityData) {
     if (QThread::currentThread() != thread()) {
         QMetaObject::invokeMethod(this, "updateAvatarEntity", Q_ARG(const QUuid&, entityID), Q_ARG(QByteArray, entityData));
         return;
     }
     _avatarEntitiesLock.withWriteLock([&] {
+        AvatarEntityMap::iterator itr = _avatarEntityData.find(entityID);
+        if (itr == _avatarEntityData.end()) {
+            if (_avatarEntityData.size() < MAX_NUM_AVATAR_ENTITIES) {
                 _avatarEntityData.insert(entityID, entityData);
                 _avatarEntityDataLocallyEdited = true;
+            }
+        } else {
+            itr.value() = entityData;
+            _avatarEntityDataLocallyEdited = true;
+        }
     });
 }
 
@@ -2240,6 +2248,11 @@ AvatarEntityMap AvatarData::getAvatarEntityData() const {
 }
 
 void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) {
+    if (avatarEntityData.size() > MAX_NUM_AVATAR_ENTITIES) {
+        // the data is suspect
+        qCDebug(avatars) << "discard suspect AvatarEntityData with size =" << avatarEntityData.size();
+        return;
+    }
     if (QThread::currentThread() != thread()) {
         QMetaObject::invokeMethod(this, "setAvatarEntityData", Q_ARG(const AvatarEntityMap&, avatarEntityData));
         return;

libraries/avatars/src/AvatarData.h

@@ -614,7 +614,7 @@ protected:
     KeyState _keyState;
 
     bool _forceFaceTrackerConnected;
-    bool _hasNewJointData; // set in AvatarData, cleared in Avatar
+    bool _hasNewJointData { true }; // set in AvatarData, cleared in Avatar
 
     HeadData* _headData { nullptr };
 

libraries/entities/src/EntityEditPacketSender.cpp

@@ -38,19 +38,7 @@ void EntityEditPacketSender::queueEditAvatarEntityMessage(PacketType type,
                                                           EntityTreePointer entityTree,
                                                           EntityItemID entityItemID,
                                                           const EntityItemProperties& properties) {
-    if (!_shouldSend) {
-        return; // bail early
-    }
-
-    if (properties.getOwningAvatarID() != _myAvatar->getID()) {
-        return; // don't send updates for someone else's avatarEntity
-    }
-
-    assert(properties.getClientOnly());
-
-    // this is an avatar-based entity.  update our avatar-data rather than sending to the entity-server
     assert(_myAvatar);
-
     if (!entityTree) {
         qCDebug(entities) << "EntityEditPacketSender::queueEditEntityMessage null entityTree.";
         return;
@@ -93,7 +81,8 @@ void EntityEditPacketSender::queueEditEntityMessage(PacketType type,
         return; // bail early
     }
 
-    if (properties.getClientOnly()) {
+    if (properties.getClientOnly() && properties.getOwningAvatarID() == _myAvatar->getID()) {
+        // this is an avatar-based entity --> update our avatar-data rather than sending to the entity-server
         queueEditAvatarEntityMessage(type, entityTree, entityItemID, properties);
         return;
     }

libraries/entities/src/EntityEditPacketSender.h

@@ -27,10 +27,6 @@ public:
     AvatarData* getMyAvatar() { return _myAvatar; }
     void clearAvatarEntity(QUuid entityID) { assert(_myAvatar); _myAvatar->clearAvatarEntity(entityID); }
 
-    void queueEditAvatarEntityMessage(PacketType type, EntityTreePointer entityTree,
-                                      EntityItemID entityItemID, const EntityItemProperties& properties);
-
-
     /// Queues an array of several voxel edit messages. Will potentially send a pending multi-command packet. Determines
     /// which voxel-server node or nodes the packet should be sent to. Can be called even before voxel servers are known, in
     /// which case up to MaxPendingMessages will be buffered and processed when voxel servers are known.
@@ -48,6 +44,10 @@ public:
 public slots:
     void processEntityEditNackPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer sendingNode);
 
+private:
+    void queueEditAvatarEntityMessage(PacketType type, EntityTreePointer entityTree,
+                                      EntityItemID entityItemID, const EntityItemProperties& properties);
+
 private:
     AvatarData* _myAvatar { nullptr };
     QScriptEngine _scriptEngine;