From 5bf075457c36242a64fbe7133fa77198e68f5530 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 19 Jun 2018 10:21:30 -0700 Subject: [PATCH 01/13] Add avatar identity request packet --- assignment-client/src/avatars/AvatarMixer.cpp | 31 +++++++++++++++++++ assignment-client/src/avatars/AvatarMixer.h | 1 + .../networking/src/udt/PacketHeaders.cpp | 2 +- libraries/networking/src/udt/PacketHeaders.h | 1 + 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 9b5c4d4f30..4e8f8b932d 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -53,6 +53,7 @@ AvatarMixer::AvatarMixer(ReceivedMessage& message) : packetReceiver.registerListener(PacketType::NodeIgnoreRequest, this, "handleNodeIgnoreRequestPacket"); packetReceiver.registerListener(PacketType::RadiusIgnoreRequest, this, "handleRadiusIgnoreRequestPacket"); packetReceiver.registerListener(PacketType::RequestsDomainListData, this, "handleRequestsDomainListDataPacket"); + packetReceiver.registerListener(PacketType::AvatarIdentityRequest, this, "handleAvatarIdentityRequestPacket"); packetReceiver.registerListenerForTypes({ PacketType::ReplicatedAvatarIdentity, @@ -602,6 +603,36 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes _handleAvatarIdentityPacketElapsedTime += (end - start); } +void AvatarMixer::handleAvatarIdentityRequestPacket(QSharedPointer message, SharedNodePointer senderNode) { + if (message->getSize() < NUM_BYTES_RFC4122_UUID) { + qCDebug(avatars) << "Malformed AvatarIdentityRequest received from" << message->getSenderSockAddr().toString(); + return; + } + + QUuid avatarID(QUuid::fromRfc4122(message->getMessage()) ); + if (!avatarID.isNull()) { + auto nodeList = DependencyManager::get(); + + nodeList->eachNode([&](SharedNodePointer node) { + QMutexLocker lock(&node->getMutex()); + if (node->getType() == NodeType::Agent && node->getLinkedData()) { + AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); + if (avatarClientData) { + const AvatarData& avatarData = avatarClientData->getAvatar(); + if (avatarData.getID() == avatarID) { + QByteArray serializedAvatar = avatarData.identityByteArray(); + auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); + identityPackets->write(serializedAvatar); + nodeList->sendPacketList(std::move(identityPackets), *senderNode); + } + } + } + } + ); + + } +} + void AvatarMixer::handleKillAvatarPacket(QSharedPointer message, SharedNodePointer node) { auto start = usecTimestampNow(); handleAvatarKilled(node); diff --git a/assignment-client/src/avatars/AvatarMixer.h b/assignment-client/src/avatars/AvatarMixer.h index 9ef5903eec..c27ca33729 100644 --- a/assignment-client/src/avatars/AvatarMixer.h +++ b/assignment-client/src/avatars/AvatarMixer.h @@ -54,6 +54,7 @@ private slots: void handleRequestsDomainListDataPacket(QSharedPointer message, SharedNodePointer senderNode); void handleReplicatedPacket(QSharedPointer message); void handleReplicatedBulkAvatarPacket(QSharedPointer message); + void handleAvatarIdentityRequestPacket(QSharedPointer message, SharedNodePointer senderNode); void domainSettingsRequestComplete(); void handlePacketVersionMismatch(PacketType type, const HifiSockAddr& senderSockAddr, const QUuid& senderUUID); void start(); diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index c342ecffc5..02d59674d6 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -93,7 +93,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::AvatarQuery: return static_cast(AvatarQueryVersion::ConicalFrustums); default: - return 21; + return 22; } } diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 64054fd080..0f30ba402c 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -61,6 +61,7 @@ public: AssignmentClientStatus, NoisyMute, AvatarIdentity, + AvatarIdentityRequest, NodeIgnoreRequest, DomainConnectRequest, DomainServerRequireDTLS, From c8c135a98578a8419e5f32381745fbd9dbb82c6d Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 19 Jun 2018 12:01:27 -0700 Subject: [PATCH 02/13] Use eachMatchingNode instead of eachNode --- assignment-client/src/avatars/AvatarMixer.cpp | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 4e8f8b932d..9658ed8908 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -613,23 +613,36 @@ void AvatarMixer::handleAvatarIdentityRequestPacket(QSharedPointer(); - nodeList->eachNode([&](SharedNodePointer node) { + nodeList->eachMatchingNode( + // Predicate: + [&](SharedNodePointer node) -> bool { QMutexLocker lock(&node->getMutex()); if (node->getType() == NodeType::Agent && node->getLinkedData()) { AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); if (avatarClientData) { const AvatarData& avatarData = avatarClientData->getAvatar(); if (avatarData.getID() == avatarID) { - QByteArray serializedAvatar = avatarData.identityByteArray(); - auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); - identityPackets->write(serializedAvatar); - nodeList->sendPacketList(std::move(identityPackets), *senderNode); + return true; } } } + return false; + }, + // Action: + [&](SharedNodePointer node) { + QMutexLocker lock(&node->getMutex()); + AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); + if (avatarClientData) { + const AvatarData& avatarData = avatarClientData->getAvatar(); + if (avatarData.getID() == avatarID) { + QByteArray serializedAvatar = avatarData.identityByteArray(); + auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); + identityPackets->write(serializedAvatar); + nodeList->sendPacketList(std::move(identityPackets), *senderNode); + } + } } ); - } } From d638bdd9861660f3c9178519c2821031e39fb56f Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 20 Jun 2018 12:21:39 -0700 Subject: [PATCH 03/13] First pass at requesting ID for unknown data Keep list of AvatarData that was created from a data packet without a matching ID. If items get old enough send an ID request and remove them. --- assignment-client/src/avatars/AvatarMixer.cpp | 1 + interface/src/avatar/AvatarManager.cpp | 17 +++++++++++++++++ interface/src/avatar/AvatarManager.h | 2 ++ libraries/avatars/src/AvatarData.cpp | 13 +++++++++++++ libraries/avatars/src/AvatarData.h | 2 +- libraries/avatars/src/AvatarHashMap.cpp | 18 +++++++++++++++--- libraries/avatars/src/AvatarHashMap.h | 4 +++- 7 files changed, 52 insertions(+), 5 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 9658ed8908..b567119181 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -639,6 +639,7 @@ void AvatarMixer::handleAvatarIdentityRequestPacket(QSharedPointerwrite(serializedAvatar); nodeList->sendPacketList(std::move(identityPackets), *senderNode); + ++_sumIdentityPackets; } } } diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 4d133706e6..3ac0d01a8c 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -54,6 +54,9 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / // We add _myAvatar into the hash with all the other AvatarData, and we use the default NULL QUid as the key. const QUuid MY_AVATAR_KEY; // NULL key +// For an unknown avatar-data packet, wait this long before requesting the identity (in µs). +const quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 500 * 1000; + AvatarManager::AvatarManager(QObject* parent) : _avatarsToFade(), _myAvatar(new MyAvatar(qApp->thread()), [](MyAvatar* ptr) { ptr->deleteLater(); }) @@ -118,6 +121,7 @@ void AvatarManager::updateMyAvatar(float deltaTime) { _lastSendAvatarDataTime = now; _myAvatarSendRate.increment(); } + } @@ -276,6 +280,19 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { simulateAvatarFades(deltaTime); + const quint64 now = usecTimestampNow(); + QWriteLocker writeLock(&_hashLock); + for (auto avatarData = _pendingAvatars.begin(); avatarData != _pendingAvatars.end(); ++avatarData) { + Avatar* pendingAvatar = dynamic_cast(avatarData->get()); + if (now - pendingAvatar->getLastRenderUpdateTime() >= REQUEST_UNKNOWN_IDENTITY_DELAY) { + pendingAvatar->sendIdentityRequest(); + avatarData = _pendingAvatars.erase(avatarData); + if (avatarData == _pendingAvatars.end()) { + break; + } + } + } + _avatarSimulationTime = (float)(usecTimestampNow() - startTime) / (float)USECS_PER_MSEC; } diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 6a3d0355f6..12446ef41a 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -194,6 +194,8 @@ private: int _numAvatarsNotUpdated { 0 }; float _avatarSimulationTime { 0.0f }; bool _shouldRender { true }; + + static const quint64 REQUEST_UNKNOWN_IDENTITY_DELAY; }; #endif // hifi_AvatarManager_h diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index b5186ba8f4..f21c6c78c5 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1919,6 +1919,19 @@ void AvatarData::sendIdentityPacket() { _identityDataChanged = false; } +void AvatarData::sendIdentityRequest() const { + auto nodeList = DependencyManager::get(); + auto packet = NLPacket::create(PacketType::AvatarIdentityRequest, NUM_BYTES_RFC4122_UUID, true); + packet->write(getID().toRfc4122()); + nodeList->eachMatchingNode( + [&](const SharedNodePointer& node)->bool { + return node->getType() == NodeType::AvatarMixer && node->getActiveSocket(); + }, + [&](const SharedNodePointer& node) { + nodeList->sendPacket(std::move(packet), *node); + }); +} + void AvatarData::updateJointMappings() { { QWriteLocker writeLock(&_jointDataLock); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 51b3257ba2..e1372076ac 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -401,7 +401,7 @@ class AvatarData : public QObject, public SpatiallyNestable { Q_PROPERTY(float sensorToWorldScale READ getSensorToWorldScale) public: - + void sendIdentityRequest() const; virtual QString getName() const override { return QString("Avatar:") + _displayName; } static const QString FRAME_NAME; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 974ae92432..fa70a64bbf 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -89,11 +89,15 @@ AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWe return avatar; } -AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { +AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer, + bool& isNew) { QWriteLocker locker(&_hashLock); auto avatar = _avatarHash.value(sessionUUID); if (!avatar) { avatar = addAvatar(sessionUUID, mixerWeakPointer); + isNew = true; + } else { + isNew = false; } return avatar; } @@ -125,8 +129,13 @@ AvatarSharedPointer AvatarHashMap::parseAvatarData(QSharedPointer(); + bool isNewAvatar; if (sessionUUID != _lastOwnerSessionUUID && (!nodeList->isIgnoringNode(sessionUUID) || nodeList->getRequestsDomainListData())) { - auto avatar = newOrExistingAvatar(sessionUUID, sendingNode); + auto avatar = newOrExistingAvatar(sessionUUID, sendingNode, isNewAvatar); + if (isNewAvatar) { + QWriteLocker locker(&_hashLock); + _pendingAvatars.insert(sessionUUID, avatar); + } // have the matching (or new) avatar parse the data from the packet int bytesRead = avatar->parseDataFromBuffer(byteArray); @@ -157,6 +166,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer { QReadLocker locker(&_hashLock); + _pendingAvatars.remove(identityUUID); auto me = _avatarHash.find(EMPTY); if ((me != _avatarHash.end()) && (identityUUID == me.value()->getSessionUUID())) { // We add MyAvatar to _avatarHash with an empty UUID. Code relies on this. In order to correctly handle an @@ -168,7 +178,8 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer if (!nodeList->isIgnoringNode(identityUUID) || nodeList->getRequestsDomainListData()) { // mesh URL for a UUID, find avatar in our list - auto avatar = newOrExistingAvatar(identityUUID, sendingNode); + bool isNewAvatar; + auto avatar = newOrExistingAvatar(identityUUID, sendingNode, isNewAvatar); bool identityChanged = false; bool displayNameChanged = false; bool skeletonModelUrlChanged = false; @@ -189,6 +200,7 @@ void AvatarHashMap::processKillAvatar(QSharedPointer message, S void AvatarHashMap::removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason) { QWriteLocker locker(&_hashLock); + _pendingAvatars.remove(sessionUUID); auto removedAvatar = _avatarHash.take(sessionUUID); if (removedAvatar) { diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index ef6f7845eb..219d5ac1a4 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -145,13 +145,15 @@ protected: virtual AvatarSharedPointer parseAvatarData(QSharedPointer message, SharedNodePointer sendingNode); virtual AvatarSharedPointer newSharedAvatar(); virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); - AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer, + bool& isNew); virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID) const; // uses a QReadLocker on the hashLock virtual void removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason = KillAvatarReason::NoReason); virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason); AvatarHash _avatarHash; + AvatarHash _pendingAvatars; mutable QReadWriteLock _hashLock; private: From 6f15e699076187ce9ab8a2be5dfef654d2446db6 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 20 Jun 2018 13:55:34 -0700 Subject: [PATCH 04/13] The QA discussion reminded me that I should add the new type to the dissectors --- tools/dissectors/1-hfudt.lua | 49 ++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tools/dissectors/1-hfudt.lua b/tools/dissectors/1-hfudt.lua index fa491723fb..b4826997fc 100644 --- a/tools/dissectors/1-hfudt.lua +++ b/tools/dissectors/1-hfudt.lua @@ -91,31 +91,32 @@ local packet_types = { [27] = "AssignmentClientStatus", [28] = "NoisyMute", [29] = "AvatarIdentity", - [30] = "AvatarBillboard", - [31] = "DomainConnectRequest", - [32] = "DomainServerRequireDTLS", - [33] = "NodeJsonStats", + [30] = "AvatarIdentityRequest", + [31] = "AvatarBillboard", + [32] = "DomainConnectRequest", + [33] = "DomainServerRequireDTLS", + [34] = "NodeJsonStats", [34] = "OctreeDataNack", - [35] = "StopNode", - [36] = "AudioEnvironment", - [37] = "EntityEditNack", - [38] = "ICEServerHeartbeat", - [39] = "ICEPing", - [40] = "ICEPingReply", - [41] = "EntityData", - [42] = "EntityQuery", - [43] = "EntityAdd", - [44] = "EntityErase", - [45] = "EntityEdit", - [46] = "DomainServerConnectionToken", - [47] = "DomainSettingsRequest", - [48] = "DomainSettings", - [49] = "AssetGet", - [50] = "AssetGetReply", - [51] = "AssetUpload", - [52] = "AssetUploadReply", - [53] = "AssetGetInfo", - [54] = "AssetGetInfoReply" + [36] = "StopNode", + [37] = "AudioEnvironment", + [38] = "EntityEditNack", + [39] = "ICEServerHeartbeat", + [40] = "ICEPing", + [41] = "ICEPingReply", + [42] = "EntityData", + [43] = "EntityQuery", + [44] = "EntityAdd", + [45] = "EntityErase", + [46] = "EntityEdit", + [47] = "DomainServerConnectionToken", + [48] = "DomainSettingsRequest", + [49] = "DomainSettings", + [50] = "AssetGet", + [51] = "AssetGetReply", + [52] = "AssetUpload", + [53] = "AssetUploadReply", + [54] = "AssetGetInfo", + [55] = "AssetGetInfoReply" } local unsourced_packet_types = { From d789c4573b9ea44cf01f8a6dbe51703a9ef17cbb Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 20 Jun 2018 17:44:58 -0700 Subject: [PATCH 05/13] Add couple of comments --- interface/src/avatar/AvatarManager.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 3ac0d01a8c..41cf615105 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -280,11 +280,13 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { simulateAvatarFades(deltaTime); + // Check on avatars with pending identities: const quint64 now = usecTimestampNow(); QWriteLocker writeLock(&_hashLock); for (auto avatarData = _pendingAvatars.begin(); avatarData != _pendingAvatars.end(); ++avatarData) { Avatar* pendingAvatar = dynamic_cast(avatarData->get()); if (now - pendingAvatar->getLastRenderUpdateTime() >= REQUEST_UNKNOWN_IDENTITY_DELAY) { + // Too long without an ID pendingAvatar->sendIdentityRequest(); avatarData = _pendingAvatars.erase(avatarData); if (avatarData == _pendingAvatars.end()) { From 6796af4b6a3b8c52110c3e2f9db3e5789f5f2ef9 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 21 Jun 2018 15:00:51 -0700 Subject: [PATCH 06/13] Move sendAvatarIdentityRequest() to a more logical location --- interface/src/avatar/AvatarManager.cpp | 17 +++++++++++++++-- interface/src/avatar/AvatarManager.h | 1 + libraries/avatars/src/AvatarData.cpp | 13 ------------- libraries/avatars/src/AvatarData.h | 1 - 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 41cf615105..45a472b794 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -55,7 +55,7 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / const QUuid MY_AVATAR_KEY; // NULL key // For an unknown avatar-data packet, wait this long before requesting the identity (in µs). -const quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 500 * 1000; +const quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1'000'000; AvatarManager::AvatarManager(QObject* parent) : _avatarsToFade(), @@ -287,7 +287,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { Avatar* pendingAvatar = dynamic_cast(avatarData->get()); if (now - pendingAvatar->getLastRenderUpdateTime() >= REQUEST_UNKNOWN_IDENTITY_DELAY) { // Too long without an ID - pendingAvatar->sendIdentityRequest(); + sendIdentityRequest(pendingAvatar->getID()); avatarData = _pendingAvatars.erase(avatarData); if (avatarData == _pendingAvatars.end()) { break; @@ -307,6 +307,19 @@ void AvatarManager::postUpdate(float deltaTime, const render::ScenePointer& scen } } +void AvatarManager::sendIdentityRequest(const QUuid& avatarID) const { + auto nodeList = DependencyManager::get(); + nodeList->eachMatchingNode( + [&](const SharedNodePointer& node)->bool { + return node->getType() == NodeType::AvatarMixer && node->getActiveSocket(); + }, + [&](const SharedNodePointer& node) { + auto packet = NLPacket::create(PacketType::AvatarIdentityRequest, NUM_BYTES_RFC4122_UUID, true); + packet->write(avatarID.toRfc4122()); + nodeList->sendPacket(std::move(packet), *node); + }); +} + void AvatarManager::simulateAvatarFades(float deltaTime) { if (_avatarsToFade.empty()) { return; diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 12446ef41a..0ab87fcab9 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -82,6 +82,7 @@ public: void updateMyAvatar(float deltaTime); void updateOtherAvatars(float deltaTime); + void sendIdentityRequest(const QUuid& avatarID) const; void postUpdate(float deltaTime, const render::ScenePointer& scene); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index f21c6c78c5..b5186ba8f4 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1919,19 +1919,6 @@ void AvatarData::sendIdentityPacket() { _identityDataChanged = false; } -void AvatarData::sendIdentityRequest() const { - auto nodeList = DependencyManager::get(); - auto packet = NLPacket::create(PacketType::AvatarIdentityRequest, NUM_BYTES_RFC4122_UUID, true); - packet->write(getID().toRfc4122()); - nodeList->eachMatchingNode( - [&](const SharedNodePointer& node)->bool { - return node->getType() == NodeType::AvatarMixer && node->getActiveSocket(); - }, - [&](const SharedNodePointer& node) { - nodeList->sendPacket(std::move(packet), *node); - }); -} - void AvatarData::updateJointMappings() { { QWriteLocker writeLock(&_jointDataLock); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index e1372076ac..b462e8a546 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -401,7 +401,6 @@ class AvatarData : public QObject, public SpatiallyNestable { Q_PROPERTY(float sensorToWorldScale READ getSensorToWorldScale) public: - void sendIdentityRequest() const; virtual QString getName() const override { return QString("Avatar:") + _displayName; } static const QString FRAME_NAME; From e5b35b1cd99a0b9a5d3d59ee097d5eaa7842aeaa Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 22 Jun 2018 12:01:00 -0700 Subject: [PATCH 07/13] Clang doesn't support digit separators yet --- interface/src/avatar/AvatarManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 45a472b794..e3f22cea7c 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -55,7 +55,7 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / const QUuid MY_AVATAR_KEY; // NULL key // For an unknown avatar-data packet, wait this long before requesting the identity (in µs). -const quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1'000'000; +constexpr quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1000000; AvatarManager::AvatarManager(QObject* parent) : _avatarsToFade(), From 269d803812ff59caf6440cfdcf089f78d94a7c49 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 22 Jun 2018 16:42:37 -0700 Subject: [PATCH 08/13] New approach to tracking no-identity duration --- interface/src/avatar/AvatarManager.cpp | 16 +++++++++------- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 7 ++++++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index e3f22cea7c..b876551fad 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -15,6 +15,8 @@ #include +#include "AvatarLogging.h" + #if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdouble-promotion" @@ -55,7 +57,7 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / const QUuid MY_AVATAR_KEY; // NULL key // For an unknown avatar-data packet, wait this long before requesting the identity (in µs). -constexpr quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1000000; +constexpr quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1000 * 1000; AvatarManager::AvatarManager(QObject* parent) : _avatarsToFade(), @@ -283,13 +285,13 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { // Check on avatars with pending identities: const quint64 now = usecTimestampNow(); QWriteLocker writeLock(&_hashLock); - for (auto avatarData = _pendingAvatars.begin(); avatarData != _pendingAvatars.end(); ++avatarData) { - Avatar* pendingAvatar = dynamic_cast(avatarData->get()); - if (now - pendingAvatar->getLastRenderUpdateTime() >= REQUEST_UNKNOWN_IDENTITY_DELAY) { + for (auto pendingAvatar = _pendingAvatars.begin(); pendingAvatar != _pendingAvatars.end(); ++pendingAvatar) { + if (now - pendingAvatar->creationTime >= REQUEST_UNKNOWN_IDENTITY_DELAY) { // Too long without an ID - sendIdentityRequest(pendingAvatar->getID()); - avatarData = _pendingAvatars.erase(avatarData); - if (avatarData == _pendingAvatars.end()) { + sendIdentityRequest(pendingAvatar->avatar->getID()); + qCDebug(avatars) << "Requesting identity for unknown avatar" << pendingAvatar->avatar->getID().toString(); + pendingAvatar = _pendingAvatars.erase(pendingAvatar); + if (pendingAvatar == _pendingAvatars.end()) { break; } } diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index fa70a64bbf..63028dd113 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -134,7 +134,7 @@ AvatarSharedPointer AvatarHashMap::parseAvatarData(QSharedPointer; + AvatarPendingHash _pendingAvatars; mutable QReadWriteLock _hashLock; private: From 0dca84210207a73932e97bbedba37457c5870d73 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 22 Jun 2018 17:06:33 -0700 Subject: [PATCH 09/13] Add avatar identity requests to analytics --- interface/src/Application.cpp | 1 + interface/src/avatar/AvatarManager.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 1910beecdd..2f8e2783c3 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -2114,6 +2114,7 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo || ((rightHandPose.valid || lastRightHandPose.valid) && (rightHandPose != lastRightHandPose)); lastLeftHandPose = leftHandPose; lastRightHandPose = rightHandPose; + properties["avatar_identity_requests_sent"] = DependencyManager::get()->getIdentityRequestsSent(); UserActivityLogger::getInstance().logAction("stats", properties); }); diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 0ab87fcab9..4d2f9c5002 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -158,6 +158,7 @@ public: Q_INVOKABLE void setAvatarSortCoefficient(const QString& name, const QScriptValue& value); float getMyAvatarSendRate() const { return _myAvatarSendRate.rate(); } + int getIdentityRequestsSent() const { return _identityRequestsSent; } public slots: @@ -195,6 +196,7 @@ private: int _numAvatarsNotUpdated { 0 }; float _avatarSimulationTime { 0.0f }; bool _shouldRender { true }; + int _identityRequestsSent { 0 }; static const quint64 REQUEST_UNKNOWN_IDENTITY_DELAY; }; From 3775bdc3352bacf920d8261cfd103a4fd8abfbc2 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 22 Jun 2018 17:20:43 -0700 Subject: [PATCH 10/13] Move PacketType::AvatarIdentityRequest so as to not break ICE --- libraries/networking/src/udt/PacketHeaders.h | 3 +- tools/dissectors/1-hfudt.lua | 51 ++++++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 0f30ba402c..38fe057d87 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -57,11 +57,10 @@ public: ICEServerQuery, OctreeStats, UNUSED_PACKET_TYPE_1, - UNUSED_PACKET_TYPE_2, + AvatarIdentityRequest, AssignmentClientStatus, NoisyMute, AvatarIdentity, - AvatarIdentityRequest, NodeIgnoreRequest, DomainConnectRequest, DomainServerRequireDTLS, diff --git a/tools/dissectors/1-hfudt.lua b/tools/dissectors/1-hfudt.lua index b4826997fc..137bee659b 100644 --- a/tools/dissectors/1-hfudt.lua +++ b/tools/dissectors/1-hfudt.lua @@ -87,36 +87,35 @@ local packet_types = { [23] = "ICEServerQuery", [24] = "OctreeStats", [25] = "Jurisdiction", - [26] = "JurisdictionRequest", + [26] = "AvatarIdentityRequest", [27] = "AssignmentClientStatus", [28] = "NoisyMute", [29] = "AvatarIdentity", - [30] = "AvatarIdentityRequest", - [31] = "AvatarBillboard", - [32] = "DomainConnectRequest", - [33] = "DomainServerRequireDTLS", - [34] = "NodeJsonStats", + [30] = "AvatarBillboard", + [31] = "DomainConnectRequest", + [32] = "DomainServerRequireDTLS", + [33] = "NodeJsonStats", [34] = "OctreeDataNack", - [36] = "StopNode", - [37] = "AudioEnvironment", - [38] = "EntityEditNack", - [39] = "ICEServerHeartbeat", - [40] = "ICEPing", - [41] = "ICEPingReply", - [42] = "EntityData", - [43] = "EntityQuery", - [44] = "EntityAdd", - [45] = "EntityErase", - [46] = "EntityEdit", - [47] = "DomainServerConnectionToken", - [48] = "DomainSettingsRequest", - [49] = "DomainSettings", - [50] = "AssetGet", - [51] = "AssetGetReply", - [52] = "AssetUpload", - [53] = "AssetUploadReply", - [54] = "AssetGetInfo", - [55] = "AssetGetInfoReply" + [35] = "StopNode", + [36] = "AudioEnvironment", + [37] = "EntityEditNack", + [38] = "ICEServerHeartbeat", + [39] = "ICEPing", + [40] = "ICEPingReply", + [41] = "EntityData", + [42] = "EntityQuery", + [43] = "EntityAdd", + [44] = "EntityErase", + [45] = "EntityEdit", + [46] = "DomainServerConnectionToken", + [47] = "DomainSettingsRequest", + [48] = "DomainSettings", + [49] = "AssetGet", + [50] = "AssetGetReply", + [51] = "AssetUpload", + [52] = "AssetUploadReply", + [53] = "AssetGetInfo", + [54] = "AssetGetInfoReply" } local unsourced_packet_types = { From ec67b8ad56f3cddd8f491ea44d45b19d9a74631e Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 3 Jul 2018 15:31:34 -0700 Subject: [PATCH 11/13] Address reviewer comments --- assignment-client/src/avatars/AvatarMixer.cpp | 43 ++++++------------- interface/src/avatar/AvatarManager.cpp | 6 ++- interface/src/avatar/AvatarManager.h | 4 +- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 3 +- .../networking/src/udt/PacketHeaders.cpp | 4 +- 6 files changed, 24 insertions(+), 38 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index b567119181..772c8643b3 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -612,38 +612,19 @@ void AvatarMixer::handleAvatarIdentityRequestPacket(QSharedPointergetMessage()) ); if (!avatarID.isNull()) { auto nodeList = DependencyManager::get(); - - nodeList->eachMatchingNode( - // Predicate: - [&](SharedNodePointer node) -> bool { - QMutexLocker lock(&node->getMutex()); - if (node->getType() == NodeType::Agent && node->getLinkedData()) { - AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); - if (avatarClientData) { - const AvatarData& avatarData = avatarClientData->getAvatar(); - if (avatarData.getID() == avatarID) { - return true; - } - } - } - return false; - }, - // Action: - [&](SharedNodePointer node) { - QMutexLocker lock(&node->getMutex()); - AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); - if (avatarClientData) { - const AvatarData& avatarData = avatarClientData->getAvatar(); - if (avatarData.getID() == avatarID) { - QByteArray serializedAvatar = avatarData.identityByteArray(); - auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); - identityPackets->write(serializedAvatar); - nodeList->sendPacketList(std::move(identityPackets), *senderNode); - ++_sumIdentityPackets; - } - } + auto node = nodeList->nodeWithUUID(avatarID); + if (node) { + QMutexLocker lock(&node->getMutex()); + AvatarMixerClientData* avatarClientData = dynamic_cast(node->getLinkedData()); + if (avatarClientData) { + const AvatarData& avatarData = avatarClientData->getAvatar(); + QByteArray serializedAvatar = avatarData.identityByteArray(); + auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); + identityPackets->write(serializedAvatar); + nodeList->sendPacketList(std::move(identityPackets), *senderNode); + ++_sumIdentityPackets; } - ); + } } } diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index b876551fad..b421a8ca42 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -57,7 +57,8 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / const QUuid MY_AVATAR_KEY; // NULL key // For an unknown avatar-data packet, wait this long before requesting the identity (in µs). -constexpr quint64 AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY = 5 * 1000 * 1000; +constexpr std::chrono::milliseconds AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY { 5 * 1000 }; +using std::chrono::steady_clock; AvatarManager::AvatarManager(QObject* parent) : _avatarsToFade(), @@ -283,7 +284,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { simulateAvatarFades(deltaTime); // Check on avatars with pending identities: - const quint64 now = usecTimestampNow(); + steady_clock::time_point now = steady_clock::now(); QWriteLocker writeLock(&_hashLock); for (auto pendingAvatar = _pendingAvatars.begin(); pendingAvatar != _pendingAvatars.end(); ++pendingAvatar) { if (now - pendingAvatar->creationTime >= REQUEST_UNKNOWN_IDENTITY_DELAY) { @@ -319,6 +320,7 @@ void AvatarManager::sendIdentityRequest(const QUuid& avatarID) const { auto packet = NLPacket::create(PacketType::AvatarIdentityRequest, NUM_BYTES_RFC4122_UUID, true); packet->write(avatarID.toRfc4122()); nodeList->sendPacket(std::move(packet), *node); + ++_identityRequestsSent; }); } diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 4d2f9c5002..9be748d59e 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -196,9 +196,9 @@ private: int _numAvatarsNotUpdated { 0 }; float _avatarSimulationTime { 0.0f }; bool _shouldRender { true }; - int _identityRequestsSent { 0 }; + mutable int _identityRequestsSent { 0 }; - static const quint64 REQUEST_UNKNOWN_IDENTITY_DELAY; + static const std::chrono::milliseconds REQUEST_UNKNOWN_IDENTITY_DELAY; }; #endif // hifi_AvatarManager_h diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 63028dd113..20308eccc6 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -134,7 +134,7 @@ AvatarSharedPointer AvatarHashMap::parseAvatarData(QSharedPointer #include +#include #include @@ -154,7 +155,7 @@ protected: AvatarHash _avatarHash; struct PendingAvatar { - quint64 creationTime; + std::chrono::steady_clock::time_point creationTime; AvatarSharedPointer avatar; }; using AvatarPendingHash = QHash; diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index 02d59674d6..51ef6de295 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -92,8 +92,10 @@ PacketVersion versionForPacketType(PacketType packetType) { return static_cast(PingVersion::IncludeConnectionID); case PacketType::AvatarQuery: return static_cast(AvatarQueryVersion::ConicalFrustums); - default: + case PacketType::AvatarIdentityRequest: return 22; + default: + return 21; } } From f77b2903a1cd9bf3c56929901666e196a6da26eb Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 5 Jul 2018 17:13:27 -0700 Subject: [PATCH 12/13] Minor tweaks --- interface/src/avatar/AvatarManager.cpp | 6 ++++-- interface/src/avatar/AvatarManager.h | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index b421a8ca42..bffa0538de 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -56,8 +56,10 @@ static const quint64 MIN_TIME_BETWEEN_MY_AVATAR_DATA_SENDS = USECS_PER_SECOND / // We add _myAvatar into the hash with all the other AvatarData, and we use the default NULL QUid as the key. const QUuid MY_AVATAR_KEY; // NULL key -// For an unknown avatar-data packet, wait this long before requesting the identity (in µs). -constexpr std::chrono::milliseconds AvatarManager::REQUEST_UNKNOWN_IDENTITY_DELAY { 5 * 1000 }; +namespace { + // For an unknown avatar-data packet, wait this long before requesting the identity. + constexpr std::chrono::milliseconds REQUEST_UNKNOWN_IDENTITY_DELAY { 5 * 1000 }; +} using std::chrono::steady_clock; AvatarManager::AvatarManager(QObject* parent) : diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 9be748d59e..ff29c1b381 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -197,8 +197,6 @@ private: float _avatarSimulationTime { 0.0f }; bool _shouldRender { true }; mutable int _identityRequestsSent { 0 }; - - static const std::chrono::milliseconds REQUEST_UNKNOWN_IDENTITY_DELAY; }; #endif // hifi_AvatarManager_h From 607a898c32b69e973db8bb4b689e329541ba3f9a Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 6 Jul 2018 15:16:13 -0700 Subject: [PATCH 13/13] Transmit a maximum number of identity requests before giving up --- interface/src/avatar/AvatarManager.cpp | 16 ++++++++++++---- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 1 + 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index bffa0538de..c791645e99 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -59,6 +59,7 @@ const QUuid MY_AVATAR_KEY; // NULL key namespace { // For an unknown avatar-data packet, wait this long before requesting the identity. constexpr std::chrono::milliseconds REQUEST_UNKNOWN_IDENTITY_DELAY { 5 * 1000 }; + constexpr int REQUEST_UNKNOWN_IDENTITY_TRANSMITS = 3; } using std::chrono::steady_clock; @@ -292,10 +293,17 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { if (now - pendingAvatar->creationTime >= REQUEST_UNKNOWN_IDENTITY_DELAY) { // Too long without an ID sendIdentityRequest(pendingAvatar->avatar->getID()); - qCDebug(avatars) << "Requesting identity for unknown avatar" << pendingAvatar->avatar->getID().toString(); - pendingAvatar = _pendingAvatars.erase(pendingAvatar); - if (pendingAvatar == _pendingAvatars.end()) { - break; + if (++pendingAvatar->transmits >= REQUEST_UNKNOWN_IDENTITY_TRANSMITS) { + qCDebug(avatars) << "Requesting identity for unknown avatar (final request)" << + pendingAvatar->avatar->getID().toString(); + + pendingAvatar = _pendingAvatars.erase(pendingAvatar); + if (pendingAvatar == _pendingAvatars.end()) { + break; + } + } else { + pendingAvatar->creationTime = now; + qCDebug(avatars) << "Requesting identity for unknown avatar" << pendingAvatar->avatar->getID().toString(); } } } diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 20308eccc6..174e81bb31 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -134,7 +134,7 @@ AvatarSharedPointer AvatarHashMap::parseAvatarData(QSharedPointer;