From 0e504e9ec668a95551a7360916b62b4c2778295e Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 9 Feb 2017 13:58:28 -0800 Subject: [PATCH 1/6] make methods private: avoid sanity check logic --- libraries/avatars/src/AvatarData.cpp | 18 +++++++----------- libraries/avatars/src/AvatarData.h | 7 ++++--- .../entities/src/EntityEditPacketSender.cpp | 15 ++------------- .../entities/src/EntityEditPacketSender.h | 8 ++++---- 4 files changed, 17 insertions(+), 31 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index af060429af..a51221b6bd 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/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) { @@ -2245,17 +2243,15 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { return; } _avatarEntitiesLock.withWriteLock([&] { - if (_avatarEntityData != avatarEntityData) { - // keep track of entities that were attached to this avatar but no longer are - AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); + // keep track of entities that were attached to this avatar but no longer are + AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); - _avatarEntityData = avatarEntityData; - setAvatarEntityDataChanged(true); + _avatarEntityData = avatarEntityData; + setAvatarEntityDataChanged(true); - foreach (auto entityID, previousAvatarEntityIDs) { - if (!_avatarEntityData.contains(entityID)) { - _avatarEntityDetached.insert(entityID); - } + foreach (auto entityID, previousAvatarEntityIDs) { + if (!_avatarEntityData.contains(entityID)) { + _avatarEntityDetached.insert(entityID); } } }); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 01cab8b93a..189f6432a9 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -538,7 +538,6 @@ public: glm::vec3 getGlobalBoundingBoxCorner() { return _globalPosition + _globalBoundingBoxOffset - _globalBoundingBoxDimensions; } Q_INVOKABLE AvatarEntityMap getAvatarEntityData() const; - Q_INVOKABLE void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); void setAvatarEntityDataChanged(bool value) { _avatarEntityDataChanged = value; } AvatarEntityIDs getAndClearRecentlyDetachedIDs(); @@ -552,7 +551,7 @@ public: int getJointCount() { return _jointData.size(); } - QVector getLastSentJointData() { + QVector getLastSentJointData() { QReadLocker readLock(&_jointDataLock); _lastSentJointData.resize(_jointData.size()); return _lastSentJointData; @@ -576,6 +575,8 @@ public slots: void resetLastSent() { _lastToByteArray = 0; } protected: + Q_INVOKABLE void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); + void lazyInitHeadData(); float getDistanceBasedMinRotationDOT(glm::vec3 viewerPosition); @@ -614,7 +615,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 }; diff --git a/libraries/entities/src/EntityEditPacketSender.cpp b/libraries/entities/src/EntityEditPacketSender.cpp index c8a14c40be..00f85f5078 100644 --- a/libraries/entities/src/EntityEditPacketSender.cpp +++ b/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; } diff --git a/libraries/entities/src/EntityEditPacketSender.h b/libraries/entities/src/EntityEditPacketSender.h index 9150748a68..9190a8296a 100644 --- a/libraries/entities/src/EntityEditPacketSender.h +++ b/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 message, SharedNodePointer sendingNode); +private: + void queueEditAvatarEntityMessage(PacketType type, EntityTreePointer entityTree, + EntityItemID entityItemID, const EntityItemProperties& properties); + private: AvatarData* _myAvatar { nullptr }; QScriptEngine _scriptEngine; From ee702f945fae900223460d65a670f4cd420fc9fe Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 9 Feb 2017 13:59:03 -0800 Subject: [PATCH 2/6] cleanup around when to send identityPacket --- interface/src/Application.cpp | 4 ---- interface/src/avatar/MyAvatar.cpp | 6 ++++-- interface/src/avatar/MyAvatar.h | 2 ++ 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 1441ae9001..dea6d9f5d5 100644 --- a/interface/src/Application.cpp +++ b/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(argv); QString concurrentDownloadsStr = getCmdOption(argc, constArgv, "--concurrent-downloads"); bool success; diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 1915046f72..c46d87607f 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/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) { diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index 18774c8719..0bae2ea390 100644 --- a/interface/src/avatar/MyAvatar.h +++ b/interface/src/avatar/MyAvatar.h @@ -507,6 +507,8 @@ private: std::mutex _holdActionsMutex; std::vector _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 }; From 563bf65e89299c3ba134e8bf59c88ffb3b2f7ca5 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 9 Feb 2017 13:59:39 -0800 Subject: [PATCH 3/6] fix bug: performance problem for avatarEntities --- interface/src/avatar/Avatar.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/interface/src/avatar/Avatar.cpp b/interface/src/avatar/Avatar.cpp index ab97f563f6..ad3b615549 100644 --- a/interface/src/avatar/Avatar.cpp +++ b/interface/src/avatar/Avatar.cpp @@ -228,7 +228,6 @@ void Avatar::updateAvatarEntities() { return; } - bool success = true; QScriptEngine scriptEngine; entityTree->withWriteLock([&] { AvatarEntityMap avatarEntities = getAvatarEntityData(); @@ -268,17 +267,15 @@ void Avatar::updateAvatarEntities() { EntityItemPointer entity = entityTree->findEntityByEntityItemID(EntityItemID(entityID)); + // NOTE: we don't bother checking whether updateEntity or addEntity succeed here: + // if not we'll try again next time the data is changed. This is to close a performance DOS vector + // whereby invalid entity data is given to the AvatarMixer and constant retries kill performance. if (entity) { if (entityTree->updateEntity(entityID, properties)) { entity->updateLastEditedFromRemote(); - } else { - success = false; } } else { entity = entityTree->addEntity(entityID, properties); - if (!entity) { - success = false; - } } } @@ -295,9 +292,7 @@ void Avatar::updateAvatarEntities() { } }); - if (success) { - setAvatarEntityDataChanged(false); - } + setAvatarEntityDataChanged(false); } bool Avatar::shouldDie() const { @@ -364,6 +359,9 @@ void Avatar::simulate(float deltaTime, bool inView) { measureMotionDerivatives(deltaTime); simulateAttachments(deltaTime); updatePalms(); + } + { + PROFILE_RANGE(simulation, "entities"); updateAvatarEntities(); } } From c551a41e0ab37621b86da260d1f51b1539290342 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 10 Feb 2017 10:55:54 -0800 Subject: [PATCH 4/6] restore exposure of setAvatarEntityData() to JS --- libraries/avatars/src/AvatarData.cpp | 16 +++++++++------- libraries/avatars/src/AvatarData.h | 3 +-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index a51221b6bd..b6839ee049 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -2243,15 +2243,17 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { return; } _avatarEntitiesLock.withWriteLock([&] { - // keep track of entities that were attached to this avatar but no longer are - AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); + if (_avatarEntityData != avatarEntityData) { + // keep track of entities that were attached to this avatar but no longer are + AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); - _avatarEntityData = avatarEntityData; - setAvatarEntityDataChanged(true); + _avatarEntityData = avatarEntityData; + setAvatarEntityDataChanged(true); - foreach (auto entityID, previousAvatarEntityIDs) { - if (!_avatarEntityData.contains(entityID)) { - _avatarEntityDetached.insert(entityID); + foreach (auto entityID, previousAvatarEntityIDs) { + if (!_avatarEntityData.contains(entityID)) { + _avatarEntityDetached.insert(entityID); + } } } }); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 189f6432a9..b28501eead 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -538,6 +538,7 @@ public: glm::vec3 getGlobalBoundingBoxCorner() { return _globalPosition + _globalBoundingBoxOffset - _globalBoundingBoxDimensions; } Q_INVOKABLE AvatarEntityMap getAvatarEntityData() const; + Q_INVOKABLE void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); void setAvatarEntityDataChanged(bool value) { _avatarEntityDataChanged = value; } AvatarEntityIDs getAndClearRecentlyDetachedIDs(); @@ -575,8 +576,6 @@ public slots: void resetLastSent() { _lastToByteArray = 0; } protected: - Q_INVOKABLE void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); - void lazyInitHeadData(); float getDistanceBasedMinRotationDOT(glm::vec3 viewerPosition); From d2315c15a12077ea681cc9049b7c9123a5038a7d Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 10 Feb 2017 11:13:05 -0800 Subject: [PATCH 5/6] avoid duplicate parse of AvatarEntityData entries --- interface/src/avatar/Avatar.cpp | 55 ++++++++++++++++++++++++++++----- interface/src/avatar/Avatar.h | 10 ++++++ 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/interface/src/avatar/Avatar.cpp b/interface/src/avatar/Avatar.cpp index ad3b615549..0b59100bcb 100644 --- a/interface/src/avatar/Avatar.cpp +++ b/interface/src/avatar/Avatar.cpp @@ -231,13 +231,40 @@ void Avatar::updateAvatarEntities() { 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(jsonByteArray); + QJsonDocument jsonProperties = QJsonDocument::fromBinaryData(data); if (!jsonProperties.isObject()) { - qCDebug(interfaceapp) << "got bad avatarEntity json" << QString(jsonByteArray.toHex()); + qCDebug(interfaceapp) << "got bad avatarEntity json" << QString(data.toHex()); continue; } @@ -265,18 +292,22 @@ void Avatar::updateAvatarEntities() { properties.setScript(noScript); } + // try to build the entity EntityItemPointer entity = entityTree->findEntityByEntityItemID(EntityItemID(entityID)); - - // NOTE: we don't bother checking whether updateEntity or addEntity succeed here: - // if not we'll try again next time the data is changed. This is to close a performance DOS vector - // whereby invalid entity data is given to the AvatarMixer and constant retries kill performance. + bool success = true; if (entity) { if (entityTree->updateEntity(entityID, properties)) { entity->updateLastEditedFromRemote(); + } else { + success = false; } } else { entity = entityTree->addEntity(entityID, properties); + if (!entity) { + success = false; + } } + stateItr.value().success = success; } AvatarEntityIDs recentlyDettachedAvatarEntities = getAndClearRecentlyDetachedIDs(); @@ -289,6 +320,14 @@ void Avatar::updateAvatarEntities() { } } }); + + // remove stale data hashes + foreach (auto entityID, recentlyDettachedAvatarEntities) { + MapOfAvatarEntityDataHashes::iterator stateItr = _avatarEntityDataHashes.find(entityID); + if (stateItr != _avatarEntityDataHashes.end()) { + _avatarEntityDataHashes.erase(stateItr); + } + } } }); diff --git a/interface/src/avatar/Avatar.h b/interface/src/avatar/Avatar.h index 5c05702e92..80d387fd33 100644 --- a/interface/src/avatar/Avatar.h +++ b/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; + MapOfAvatarEntityDataHashes _avatarEntityDataHashes; + uint64_t _lastRenderUpdateTime { 0 }; int _leftPointerGeometryID { 0 }; int _rightPointerGeometryID { 0 }; From 66a315cb9cfb38efe906c0f014c98793887e863b Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 10 Feb 2017 13:13:46 -0800 Subject: [PATCH 6/6] cap number of avatar entities --- libraries/avatars/src/AvatarData.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index b6839ee049..0da2ee0282 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -2200,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([&] { - _avatarEntityData.insert(entityID, entityData); - _avatarEntityDataLocallyEdited = true; + 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; + } }); } @@ -2238,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;