From c9c07c4269bdd2a742b0ece1e1647bec1ce30d33 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 09:38:23 -0800 Subject: [PATCH 01/13] Basically rebase and squash --- .../src/avatars/AvatarMixerSlave.cpp | 24 +++++---- interface/src/avatar/AvatarManager.cpp | 2 +- libraries/avatars/src/AvatarData.cpp | 48 ++++++++++++----- libraries/avatars/src/AvatarData.h | 3 +- scripts/system/pal.js | 52 +++++++------------ 5 files changed, 71 insertions(+), 58 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 49b4b1ced4..1c386caab7 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -137,14 +137,18 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // keep track of the number of other avatar frames skipped int numAvatarsWithSkippedFrames = 0; - // When this is true, the AvatarMixer will send Avatar data to a client about avatars that are not in the view frustrum - bool getsOutOfView = nodeData->getRequestsDomainListData(); - - // When this is true, the AvatarMixer will send Avatar data to a client about avatars that they've ignored - bool getsIgnoredByMe = getsOutOfView; + // When this is true, the AvatarMixer will send Avatar data to a client + // about avatars they've ignored or that are out of view + bool PALIsOpen = nodeData->getRequestsDomainListData(); // When this is true, the AvatarMixer will send Avatar data to a client about avatars that have ignored them - bool getsAnyIgnored = getsIgnoredByMe && node->getCanKick(); + bool getsAnyIgnored = PALIsOpen && node->getCanKick(); + + // Increase minimumBytesPerAvatar if the PAL is open or we're gettingAnyIgnored + if (PALIsOpen || getsAnyIgnored) { + minimumBytesPerAvatar += sizeof(AvatarDataPacket::AvatarGlobalPosition) + + sizeof(AvatarDataPacket::AudioLoudness); + } // setup a PacketList for the avatarPackets auto avatarPacketList = NLPacketList::create(PacketType::BulkAvatarData); @@ -222,7 +226,7 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // or that has ignored the viewing node if (!avatarNode->getLinkedData() || avatarNode->getUUID() == node->getUUID() - || (node->isIgnoringNodeWithID(avatarNode->getUUID()) && !getsIgnoredByMe) + || (node->isIgnoringNodeWithID(avatarNode->getUUID()) && !PALIsOpen) || (avatarNode->isIgnoringNodeWithID(node->getUUID()) && !getsAnyIgnored)) { shouldIgnore = true; } else { @@ -335,9 +339,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { if (overBudget) { overBudgetAvatars++; _stats.overBudgetAvatars++; - detail = AvatarData::NoData; - } else if (!isInView && !getsOutOfView) { - detail = AvatarData::NoData; + detail = (PALIsOpen || getsAnyIgnored) ? AvatarData::PALMinimum : AvatarData::NoData; + } else if (!isInView) { + detail = (PALIsOpen || getsAnyIgnored) ? AvatarData::PALMinimum : AvatarData::NoData; nodeData->incrementAvatarOutOfView(); } else { detail = distribution(generator) < AVATAR_SEND_FULL_UPDATE_RATIO diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 7417f73102..94ce444416 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -329,7 +329,7 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID, KillAvatarReason remo } void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { - AvatarHashMap::handleRemovedAvatar(removedAvatar); + AvatarHashMap::handleRemovedAvatar(removedAvatar, removalReason); // removedAvatar is a shared pointer to an AvatarData but we need to get to the derived Avatar // class in this context so we can call methods that don't exist at the base class. diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 8025c680ca..7b33ada89c 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -186,6 +186,7 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent bool cullSmallChanges = (dataDetail == CullSmallData); bool sendAll = (dataDetail == SendAllData); bool sendMinimum = (dataDetail == MinimumData); + bool sendPALMinimum = (dataDetail == PALMinimum); lazyInitHeadData(); @@ -222,24 +223,45 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent auto parentID = getParentID(); bool hasAvatarGlobalPosition = true; // always include global position - bool hasAvatarOrientation = sendAll || rotationChangedSince(lastSentTime); - bool hasAvatarBoundingBox = sendAll || avatarBoundingBoxChangedSince(lastSentTime); - bool hasAvatarScale = sendAll || avatarScaleChangedSince(lastSentTime); - bool hasLookAtPosition = sendAll || lookAtPositionChangedSince(lastSentTime); - bool hasAudioLoudness = sendAll || audioLoudnessChangedSince(lastSentTime); - bool hasSensorToWorldMatrix = sendAll || sensorToWorldMatrixChangedSince(lastSentTime); - bool hasAdditionalFlags = sendAll || additionalFlagsChangedSince(lastSentTime); + bool hasAvatarOrientation = false; + bool hasAvatarBoundingBox = false; + bool hasAvatarScale = false; + bool hasLookAtPosition = false; + bool hasAudioLoudness = false; + bool hasSensorToWorldMatrix = false; + bool hasAdditionalFlags = false; // local position, and parent info only apply to avatars that are parented. The local position // and the parent info can change independently though, so we track their "changed since" // separately - bool hasParentInfo = sendAll || parentInfoChangedSince(lastSentTime); - bool hasAvatarLocalPosition = hasParent() && (sendAll || - tranlationChangedSince(lastSentTime) || - parentInfoChangedSince(lastSentTime)); + bool hasParentInfo = false; + bool hasAvatarLocalPosition = false; - bool hasFaceTrackerInfo = !dropFaceTracking && hasFaceTracker() && (sendAll || faceTrackerInfoChangedSince(lastSentTime)); - bool hasJointData = sendAll || !sendMinimum; + bool hasFaceTrackerInfo = false; + bool hasJointData = false; + + if (sendPALMinimum) { + hasAudioLoudness = true; + } else { + hasAvatarOrientation = sendAll || rotationChangedSince(lastSentTime); + hasAvatarBoundingBox = sendAll || avatarBoundingBoxChangedSince(lastSentTime); + hasAvatarScale = sendAll || avatarScaleChangedSince(lastSentTime); + hasLookAtPosition = sendAll || lookAtPositionChangedSince(lastSentTime); + hasAudioLoudness = sendAll || audioLoudnessChangedSince(lastSentTime); + hasSensorToWorldMatrix = sendAll || sensorToWorldMatrixChangedSince(lastSentTime); + hasAdditionalFlags = sendAll || additionalFlagsChangedSince(lastSentTime); + + // local position, and parent info only apply to avatars that are parented. The local position + // and the parent info can change independently though, so we track their "changed since" + // separately + hasParentInfo = sendAll || parentInfoChangedSince(lastSentTime); + hasAvatarLocalPosition = hasParent() && (sendAll || + tranlationChangedSince(lastSentTime) || + parentInfoChangedSince(lastSentTime)); + + hasFaceTrackerInfo = !dropFaceTracking && hasFaceTracker() && (sendAll || faceTrackerInfoChangedSince(lastSentTime)); + hasJointData = sendAll || !sendMinimum; + } // Leading flags, to indicate how much data is actually included in the packet... AvatarDataPacket::HasFlags packetStateFlags = diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index c2240f400f..ac6c2fcbe0 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -379,7 +379,8 @@ public: MinimumData, CullSmallData, IncludeSmallData, - SendAllData + SendAllData, + PALMinimum } AvatarDataDetail; virtual QByteArray toByteArrayStateful(AvatarDataDetail dataDetail); diff --git a/scripts/system/pal.js b/scripts/system/pal.js index 4914cbe34c..f9d0a60f5a 100644 --- a/scripts/system/pal.js +++ b/scripts/system/pal.js @@ -526,17 +526,23 @@ var button; var buttonName = "PEOPLE"; var tablet = null; +function onTabletScreenChanged(type, url) { + if (type !== "QML" || url !== "../Pal.qml") { + off(); + } +} + function startup() { tablet = Tablet.getTablet("com.highfidelity.interface.tablet.system"); button = tablet.addButton({ text: buttonName, icon: "icons/tablet-icons/people-i.svg", - activeIcon: "icons/tablet-icons/people-a.svg", sortOrder: 7 }); tablet.fromQml.connect(fromQml); button.clicked.connect(onTabletButtonClicked); tablet.screenChanged.connect(onTabletScreenChanged); + Users.usernameFromIDReply.connect(usernameFromIDReply); Window.domainChanged.connect(clearLocalQMLDataAndClosePAL); Window.domainConnectionRefused.connect(clearLocalQMLDataAndClosePAL); @@ -567,39 +573,17 @@ function off() { Users.requestsDomainListData = false; } -var onPalScreen = false; -var shouldActivateButton = false; - function onTabletButtonClicked() { - if (onPalScreen) { - // for toolbar-mode: go back to home screen, this will close the window. - tablet.gotoHomeScreen(); - } else { - shouldActivateButton = true; - tablet.loadQMLSource("../Pal.qml"); - onPalScreen = true; - Users.requestsDomainListData = true; - populateUserList(); - isWired = true; - Script.update.connect(updateOverlays); - Controller.mousePressEvent.connect(handleMouseEvent); - Controller.mouseMoveEvent.connect(handleMouseMoveEvent); - triggerMapping.enable(); - triggerPressMapping.enable(); - audioTimer = createAudioInterval(conserveResources ? AUDIO_LEVEL_CONSERVED_UPDATE_INTERVAL_MS : AUDIO_LEVEL_UPDATE_INTERVAL_MS); - } -} - -function onTabletScreenChanged(type, url) { - // for toolbar mode: change button to active when window is first openend, false otherwise. - button.editProperties({isActive: shouldActivateButton}); - shouldActivateButton = false; - onPalScreen = false; - - // disable sphere overlays when not on pal screen. - if (type !== "QML" || url !== "../Pal.qml") { - off(); - } + tablet.loadQMLSource("../Pal.qml"); + Users.requestsDomainListData = true; + populateUserList(); + isWired = true; + Script.update.connect(updateOverlays); + Controller.mousePressEvent.connect(handleMouseEvent); + Controller.mouseMoveEvent.connect(handleMouseMoveEvent); + triggerMapping.enable(); + triggerPressMapping.enable(); + audioTimer = createAudioInterval(conserveResources ? AUDIO_LEVEL_CONSERVED_UPDATE_INTERVAL_MS : AUDIO_LEVEL_UPDATE_INTERVAL_MS); } // @@ -699,12 +683,14 @@ function shutdown() { button.clicked.disconnect(onTabletButtonClicked); tablet.removeButton(button); tablet.screenChanged.disconnect(onTabletScreenChanged); + Users.usernameFromIDReply.disconnect(usernameFromIDReply); Window.domainChanged.disconnect(clearLocalQMLDataAndClosePAL); Window.domainConnectionRefused.disconnect(clearLocalQMLDataAndClosePAL); Messages.subscribe(CHANNEL); Messages.messageReceived.disconnect(receiveMessage); Users.avatarDisconnected.disconnect(avatarDisconnected); + off(); } From b927472e4e272846c59e8ab0cbc1904a48fa9540 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 11:25:04 -0800 Subject: [PATCH 02/13] Fixup after rebase --- scripts/system/pal.js | 52 +++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/scripts/system/pal.js b/scripts/system/pal.js index f9d0a60f5a..4914cbe34c 100644 --- a/scripts/system/pal.js +++ b/scripts/system/pal.js @@ -526,23 +526,17 @@ var button; var buttonName = "PEOPLE"; var tablet = null; -function onTabletScreenChanged(type, url) { - if (type !== "QML" || url !== "../Pal.qml") { - off(); - } -} - function startup() { tablet = Tablet.getTablet("com.highfidelity.interface.tablet.system"); button = tablet.addButton({ text: buttonName, icon: "icons/tablet-icons/people-i.svg", + activeIcon: "icons/tablet-icons/people-a.svg", sortOrder: 7 }); tablet.fromQml.connect(fromQml); button.clicked.connect(onTabletButtonClicked); tablet.screenChanged.connect(onTabletScreenChanged); - Users.usernameFromIDReply.connect(usernameFromIDReply); Window.domainChanged.connect(clearLocalQMLDataAndClosePAL); Window.domainConnectionRefused.connect(clearLocalQMLDataAndClosePAL); @@ -573,17 +567,39 @@ function off() { Users.requestsDomainListData = false; } +var onPalScreen = false; +var shouldActivateButton = false; + function onTabletButtonClicked() { - tablet.loadQMLSource("../Pal.qml"); - Users.requestsDomainListData = true; - populateUserList(); - isWired = true; - Script.update.connect(updateOverlays); - Controller.mousePressEvent.connect(handleMouseEvent); - Controller.mouseMoveEvent.connect(handleMouseMoveEvent); - triggerMapping.enable(); - triggerPressMapping.enable(); - audioTimer = createAudioInterval(conserveResources ? AUDIO_LEVEL_CONSERVED_UPDATE_INTERVAL_MS : AUDIO_LEVEL_UPDATE_INTERVAL_MS); + if (onPalScreen) { + // for toolbar-mode: go back to home screen, this will close the window. + tablet.gotoHomeScreen(); + } else { + shouldActivateButton = true; + tablet.loadQMLSource("../Pal.qml"); + onPalScreen = true; + Users.requestsDomainListData = true; + populateUserList(); + isWired = true; + Script.update.connect(updateOverlays); + Controller.mousePressEvent.connect(handleMouseEvent); + Controller.mouseMoveEvent.connect(handleMouseMoveEvent); + triggerMapping.enable(); + triggerPressMapping.enable(); + audioTimer = createAudioInterval(conserveResources ? AUDIO_LEVEL_CONSERVED_UPDATE_INTERVAL_MS : AUDIO_LEVEL_UPDATE_INTERVAL_MS); + } +} + +function onTabletScreenChanged(type, url) { + // for toolbar mode: change button to active when window is first openend, false otherwise. + button.editProperties({isActive: shouldActivateButton}); + shouldActivateButton = false; + onPalScreen = false; + + // disable sphere overlays when not on pal screen. + if (type !== "QML" || url !== "../Pal.qml") { + off(); + } } // @@ -683,14 +699,12 @@ function shutdown() { button.clicked.disconnect(onTabletButtonClicked); tablet.removeButton(button); tablet.screenChanged.disconnect(onTabletScreenChanged); - Users.usernameFromIDReply.disconnect(usernameFromIDReply); Window.domainChanged.disconnect(clearLocalQMLDataAndClosePAL); Window.domainConnectionRefused.disconnect(clearLocalQMLDataAndClosePAL); Messages.subscribe(CHANNEL); Messages.messageReceived.disconnect(receiveMessage); Users.avatarDisconnected.disconnect(avatarDisconnected); - off(); } From 81ce5cffcd6eb53dff514b8a41acca148acb62a0 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 11:47:42 -0800 Subject: [PATCH 03/13] Force-send ID packets when PAL is open --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 1c386caab7..e6ca5e49df 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -311,7 +311,8 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); // make sure we send out identity packets to and from new arrivals. - bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()); + // Also make sure we send identity packets if the PAL is open. + bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()) || (PALIsOpen || getsAnyIgnored); // FIXME - this clause seems suspicious "... || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp ..." if (!overBudget From 4025601c2bfc8e9510585d7fcdb8ede57a80545e Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 14:28:29 -0800 Subject: [PATCH 04/13] Force send ID even when 'overBudget' --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index e6ca5e49df..37406675b9 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -311,15 +311,16 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); // make sure we send out identity packets to and from new arrivals. - // Also make sure we send identity packets if the PAL is open. bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()) || (PALIsOpen || getsAnyIgnored); // FIXME - this clause seems suspicious "... || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp ..." - if (!overBudget + if ((!overBudget && otherNodeData->getIdentityChangeTimestamp().time_since_epoch().count() > 0 && (forceSend || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp - || distribution(generator) < IDENTITY_SEND_PROBABILITY)) { + || distribution(generator) < IDENTITY_SEND_PROBABILITY)) || + // Also make sure we send identity packets if the PAL is open. + (PALIsOpen || getsAnyIgnored)) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } From 9969d422d6d07eeeea29b2c3a925934edfa7dcf4 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 15:38:30 -0800 Subject: [PATCH 05/13] Cleanup --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 37406675b9..88b363b73a 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -311,7 +311,7 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); // make sure we send out identity packets to and from new arrivals. - bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()) || (PALIsOpen || getsAnyIgnored); + bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()); // FIXME - this clause seems suspicious "... || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp ..." if ((!overBudget From cfb8534d71dd3b1da32a11e02a3ba1374d0add9c Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 24 Feb 2017 17:00:57 -0800 Subject: [PATCH 06/13] Comment --- libraries/avatars/src/AvatarData.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 7b33ada89c..06df75d451 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -250,10 +250,6 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent hasAudioLoudness = sendAll || audioLoudnessChangedSince(lastSentTime); hasSensorToWorldMatrix = sendAll || sensorToWorldMatrixChangedSince(lastSentTime); hasAdditionalFlags = sendAll || additionalFlagsChangedSince(lastSentTime); - - // local position, and parent info only apply to avatars that are parented. The local position - // and the parent info can change independently though, so we track their "changed since" - // separately hasParentInfo = sendAll || parentInfoChangedSince(lastSentTime); hasAvatarLocalPosition = hasParent() && (sendAll || tranlationChangedSince(lastSentTime) || From 718ecea404459d9eb779ada40c5b99e1103fc7b2 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Mon, 27 Feb 2017 16:10:53 -0800 Subject: [PATCH 07/13] Potential non-spammy solution using pseudo-state --- .../src/avatars/AvatarMixerSlave.cpp | 31 ++++++++++++------- .../src/avatars/AvatarMixerSlave.h | 12 +++++++ libraries/avatars/src/AvatarData.h | 4 +-- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 88b363b73a..15cf89754e 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -80,16 +80,6 @@ int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, static const int AVATAR_MIXER_BROADCAST_FRAMES_PER_SECOND = 45; -// FIXME - There is some old logic (unchanged as of 2/17/17) that randomly decides to send an identity -// packet. That logic had the following comment about the constants it uses... -// -// An 80% chance of sending a identity packet within a 5 second interval. -// assuming 60 htz update rate. -// -// Assuming the calculation of the constant is in fact correct for 80% and 60hz and 5 seconds (an assumption -// that I have not verified) then the constant is definitely wrong now, since we send at 45hz. -const float IDENTITY_SEND_PROBABILITY = 1.0f / 187.0f; - void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { quint64 start = usecTimestampNow(); @@ -150,6 +140,23 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { sizeof(AvatarDataPacket::AudioLoudness); } + if (PALIsOpen) { + if (_identitySendProbability == DEFAULT_IDENTITY_SEND_PROBABILITY) + { + // The client has just opened the PAL. Force all identity packets to be sent to + // this client. + _identitySendProbability = 1.0f; + } else { + // The user recently opened the PAL, but we've already gone through the above conditional. + // We want to receive identity updates more often than default when the PAL is open + // to be more confident that the user will see the most up-to-date information in the PAL. + _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY * 2; + } + } else { + // If the PAL is closed, reset the identitySendProbability to the default. + _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY; + } + // setup a PacketList for the avatarPackets auto avatarPacketList = NLPacketList::create(PacketType::BulkAvatarData); @@ -318,9 +325,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { && otherNodeData->getIdentityChangeTimestamp().time_since_epoch().count() > 0 && (forceSend || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp - || distribution(generator) < IDENTITY_SEND_PROBABILITY)) || + || distribution(generator) < _identitySendProbability)) || // Also make sure we send identity packets if the PAL is open. - (PALIsOpen || getsAnyIgnored)) { + ((PALIsOpen || getsAnyIgnored) && distribution(generator) < _identitySendProbability)) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } diff --git a/assignment-client/src/avatars/AvatarMixerSlave.h b/assignment-client/src/avatars/AvatarMixerSlave.h index 04141d9d72..2fa5fa4484 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.h +++ b/assignment-client/src/avatars/AvatarMixerSlave.h @@ -101,6 +101,18 @@ private: float _maxKbpsPerNode { 0.0f }; float _throttlingRatio { 0.0f }; + + // FIXME - There is some old logic (unchanged as of 2/17/17) that randomly decides to send an identity + // packet. That logic had the following comment about the constants it uses... + // + // An 80% chance of sending a identity packet within a 5 second interval. + // assuming 60 htz update rate. + // + // Assuming the calculation of the constant is in fact correct for 80% and 60hz and 5 seconds (an assumption + // that I have not verified) then the constant is definitely wrong now, since we send at 45hz. + const float DEFAULT_IDENTITY_SEND_PROBABILITY = 1.0f / 187.0f; + float _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY; + AvatarMixerSlaveStats _stats; }; diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index ac6c2fcbe0..f0759aedbd 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -376,11 +376,11 @@ public: typedef enum { NoData, + PALMinimum, MinimumData, CullSmallData, IncludeSmallData, - SendAllData, - PALMinimum + SendAllData } AvatarDataDetail; virtual QByteArray toByteArrayStateful(AvatarDataDetail dataDetail); From 8bdbbd4b25aa2cb8d943cacf3724a4bb13a95224 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 1 Mar 2017 09:07:27 -0800 Subject: [PATCH 08/13] CR feedback 1 --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 15cf89754e..49353fd88f 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -134,15 +134,14 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // When this is true, the AvatarMixer will send Avatar data to a client about avatars that have ignored them bool getsAnyIgnored = PALIsOpen && node->getCanKick(); - // Increase minimumBytesPerAvatar if the PAL is open or we're gettingAnyIgnored - if (PALIsOpen || getsAnyIgnored) { + // Increase minimumBytesPerAvatar if the PAL is open + if (PALIsOpen) { minimumBytesPerAvatar += sizeof(AvatarDataPacket::AvatarGlobalPosition) + sizeof(AvatarDataPacket::AudioLoudness); } if (PALIsOpen) { - if (_identitySendProbability == DEFAULT_IDENTITY_SEND_PROBABILITY) - { + if (_identitySendProbability == DEFAULT_IDENTITY_SEND_PROBABILITY) { // The client has just opened the PAL. Force all identity packets to be sent to // this client. _identitySendProbability = 1.0f; @@ -327,7 +326,7 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp || distribution(generator) < _identitySendProbability)) || // Also make sure we send identity packets if the PAL is open. - ((PALIsOpen || getsAnyIgnored) && distribution(generator) < _identitySendProbability)) { + (PALIsOpen && distribution(generator) < _identitySendProbability)) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } @@ -348,9 +347,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { if (overBudget) { overBudgetAvatars++; _stats.overBudgetAvatars++; - detail = (PALIsOpen || getsAnyIgnored) ? AvatarData::PALMinimum : AvatarData::NoData; + detail = PALIsOpen ? AvatarData::PALMinimum : AvatarData::NoData; } else if (!isInView) { - detail = (PALIsOpen || getsAnyIgnored) ? AvatarData::PALMinimum : AvatarData::NoData; + detail = PALIsOpen ? AvatarData::PALMinimum : AvatarData::NoData; nodeData->incrementAvatarOutOfView(); } else { detail = distribution(generator) < AVATAR_SEND_FULL_UPDATE_RATIO From fda87b3a531c95856a949d5df1d9e4032883f20a Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 1 Mar 2017 09:09:41 -0800 Subject: [PATCH 09/13] Quick cleanup --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 49353fd88f..47efb39c3c 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -134,13 +134,10 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // When this is true, the AvatarMixer will send Avatar data to a client about avatars that have ignored them bool getsAnyIgnored = PALIsOpen && node->getCanKick(); - // Increase minimumBytesPerAvatar if the PAL is open if (PALIsOpen) { + // Increase minimumBytesPerAvatar if the PAL is open minimumBytesPerAvatar += sizeof(AvatarDataPacket::AvatarGlobalPosition) + sizeof(AvatarDataPacket::AudioLoudness); - } - - if (PALIsOpen) { if (_identitySendProbability == DEFAULT_IDENTITY_SEND_PROBABILITY) { // The client has just opened the PAL. Force all identity packets to be sent to // this client. From 5fa5e6e0a74eabcfe1ab99ecd216bde4734044fc Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 1 Mar 2017 14:27:50 -0800 Subject: [PATCH 10/13] First pass at implementing Brad's simplification ideas --- .../src/avatars/AvatarMixerClientData.cpp | 10 ------- .../src/avatars/AvatarMixerClientData.h | 9 ++---- .../src/avatars/AvatarMixerSlave.cpp | 28 ++----------------- .../src/avatars/AvatarMixerSlave.h | 12 -------- 4 files changed, 6 insertions(+), 53 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 717e14535f..43b8816111 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -65,15 +65,6 @@ int AvatarMixerClientData::parseData(ReceivedMessage& message) { // compute the offset to the data payload return _avatar->parseDataFromBuffer(message.readWithoutCopy(message.getBytesLeftToRead())); } - -bool AvatarMixerClientData::checkAndSetHasReceivedFirstPacketsFrom(const QUuid& uuid) { - if (_hasReceivedFirstPacketsFrom.find(uuid) == _hasReceivedFirstPacketsFrom.end()) { - _hasReceivedFirstPacketsFrom.insert(uuid); - return false; - } - return true; -} - uint64_t AvatarMixerClientData::getLastBroadcastTime(const QUuid& nodeUUID) const { // return the matching PacketSequenceNumber, or the default if we don't have it auto nodeMatch = _lastBroadcastTimes.find(nodeUUID); @@ -103,7 +94,6 @@ void AvatarMixerClientData::ignoreOther(SharedNodePointer self, SharedNodePointe killPacket->writePrimitive(KillAvatarReason::YourAvatarEnteredTheirBubble); } DependencyManager::get()->sendUnreliablePacket(*killPacket, *self); - _hasReceivedFirstPacketsFrom.erase(other->getUUID()); } } diff --git a/assignment-client/src/avatars/AvatarMixerClientData.h b/assignment-client/src/avatars/AvatarMixerClientData.h index 51b8d692e2..1449005246 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.h +++ b/assignment-client/src/avatars/AvatarMixerClientData.h @@ -45,8 +45,6 @@ public: const AvatarData* getConstAvatarData() const { return _avatar.get(); } AvatarSharedPointer getAvatarSharedPointer() const { return _avatar; } - bool checkAndSetHasReceivedFirstPacketsFrom(const QUuid& uuid); - uint16_t getLastBroadcastSequenceNumber(const QUuid& nodeUUID) const; void setLastBroadcastSequenceNumber(const QUuid& nodeUUID, uint16_t sequenceNumber) { _lastBroadcastSequenceNumbers[nodeUUID] = sequenceNumber; } @@ -63,8 +61,8 @@ public: uint16_t getLastReceivedSequenceNumber() const { return _lastReceivedSequenceNumber; } - HRCTime getIdentityChangeTimestamp() const { return _identityChangeTimestamp; } - void flagIdentityChange() { _identityChangeTimestamp = p_high_resolution_clock::now(); } + uint64_t getIdentityChangeTimestamp() const { return _identityChangeTimestamp; } + void flagIdentityChange() { _identityChangeTimestamp = usecTimestampNow(); } bool getAvatarSessionDisplayNameMustChange() const { return _avatarSessionDisplayNameMustChange; } void setAvatarSessionDisplayNameMustChange(bool set = true) { _avatarSessionDisplayNameMustChange = set; } @@ -139,7 +137,6 @@ private: uint16_t _lastReceivedSequenceNumber { 0 }; std::unordered_map _lastBroadcastSequenceNumbers; - std::unordered_set _hasReceivedFirstPacketsFrom; std::unordered_map _lastBroadcastTimes; // this is a map of the last time we encoded an "other" avatar for @@ -147,7 +144,7 @@ private: std::unordered_map _lastOtherAvatarEncodeTime; std::unordered_map> _lastOtherAvatarSentJoints; - HRCTime _identityChangeTimestamp; + uint64_t _identityChangeTimestamp; bool _avatarSessionDisplayNameMustChange{ false }; int _numAvatarsSentLastFrame = 0; diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 47efb39c3c..60dfad7687 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -138,19 +138,6 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // Increase minimumBytesPerAvatar if the PAL is open minimumBytesPerAvatar += sizeof(AvatarDataPacket::AvatarGlobalPosition) + sizeof(AvatarDataPacket::AudioLoudness); - if (_identitySendProbability == DEFAULT_IDENTITY_SEND_PROBABILITY) { - // The client has just opened the PAL. Force all identity packets to be sent to - // this client. - _identitySendProbability = 1.0f; - } else { - // The user recently opened the PAL, but we've already gone through the above conditional. - // We want to receive identity updates more often than default when the PAL is open - // to be more confident that the user will see the most up-to-date information in the PAL. - _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY * 2; - } - } else { - // If the PAL is closed, reset the identitySendProbability to the default. - _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY; } // setup a PacketList for the avatarPackets @@ -313,18 +300,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); - // make sure we send out identity packets to and from new arrivals. - bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID()); - - // FIXME - this clause seems suspicious "... || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp ..." - if ((!overBudget - && otherNodeData->getIdentityChangeTimestamp().time_since_epoch().count() > 0 - && (forceSend - || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp - || distribution(generator) < _identitySendProbability)) || - // Also make sure we send identity packets if the PAL is open. - (PALIsOpen && distribution(generator) < _identitySendProbability)) { - + // If the time that the mixer sent AVATAR DATA about Avatar B to Avatar A is BEFORE OR EQUAL TO + // the time that Avatar B flagged an IDENTITY DATA change, send IDENTITY DATA about Avatar B to Avatar A. + if (nodeData->getLastBroadcastTime(otherNode->getUUID()) <= otherNodeData->getIdentityChangeTimestamp()) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } diff --git a/assignment-client/src/avatars/AvatarMixerSlave.h b/assignment-client/src/avatars/AvatarMixerSlave.h index 2fa5fa4484..04141d9d72 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.h +++ b/assignment-client/src/avatars/AvatarMixerSlave.h @@ -101,18 +101,6 @@ private: float _maxKbpsPerNode { 0.0f }; float _throttlingRatio { 0.0f }; - - // FIXME - There is some old logic (unchanged as of 2/17/17) that randomly decides to send an identity - // packet. That logic had the following comment about the constants it uses... - // - // An 80% chance of sending a identity packet within a 5 second interval. - // assuming 60 htz update rate. - // - // Assuming the calculation of the constant is in fact correct for 80% and 60hz and 5 seconds (an assumption - // that I have not verified) then the constant is definitely wrong now, since we send at 45hz. - const float DEFAULT_IDENTITY_SEND_PROBABILITY = 1.0f / 187.0f; - float _identitySendProbability = DEFAULT_IDENTITY_SEND_PROBABILITY; - AvatarMixerSlaveStats _stats; }; From 5418a7c2304c2773fb09584070b28e8707db13f3 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 1 Mar 2017 16:54:53 -0800 Subject: [PATCH 11/13] Important addition before rebase --- assignment-client/src/avatars/AvatarMixerClientData.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 43b8816111..0df3edbd11 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -94,6 +94,7 @@ void AvatarMixerClientData::ignoreOther(SharedNodePointer self, SharedNodePointe killPacket->writePrimitive(KillAvatarReason::YourAvatarEnteredTheirBubble); } DependencyManager::get()->sendUnreliablePacket(*killPacket, *self); + setLastBroadcastTime(other->getUUID(), 0); } } From 685bd95105f96b828ce62292b8b83b3291c6e83a Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 3 Mar 2017 11:21:55 -0800 Subject: [PATCH 12/13] I think I'm done. Not straightforward! --- assignment-client/src/avatars/AvatarMixer.cpp | 26 ++++++++++++++++++- .../src/avatars/AvatarMixerClientData.cpp | 2 +- .../src/avatars/AvatarMixerSlave.cpp | 3 ++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 0f6863f9ae..1d1a1a57a8 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -409,7 +409,31 @@ void AvatarMixer::handleKillAvatarPacket(QSharedPointer message void AvatarMixer::handleNodeIgnoreRequestPacket(QSharedPointer message, SharedNodePointer senderNode) { auto start = usecTimestampNow(); - senderNode->parseIgnoreRequestMessage(message); + auto nodeList = DependencyManager::get(); + AvatarMixerClientData* nodeData = reinterpret_cast(senderNode->getLinkedData()); + bool addToIgnore; + message->readPrimitive(&addToIgnore); + while (message->getBytesLeftToRead()) { + // parse out the UUID being ignored from the packet + QUuid ignoredUUID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); + // Reset the lastBroadcastTime for the ignored avatar to 0 + // so the AvatarMixer knows it'll have to send identity data about the ignored avatar + // to the ignorer if the ignorer unignores. + nodeData->setLastBroadcastTime(ignoredUUID, 0); + + // Reset the lastBroadcastTime for the ignorer (FROM THE PERSPECTIVE OF THE IGNORED) to 0 + // so the AvatarMixer knows it'll have to send identity data about the ignorer + // to the ignored if the ignorer unignores. + auto ignoredNode = nodeList->nodeWithUUID(ignoredUUID); + AvatarMixerClientData* ignoredNodeData = reinterpret_cast(ignoredNode->getLinkedData()); + ignoredNodeData->setLastBroadcastTime(senderNode->getUUID(), 0); + + if (addToIgnore) { + senderNode->addIgnoredNode(ignoredUUID); + } else { + senderNode->removeIgnoredNode(ignoredUUID); + } + } auto end = usecTimestampNow(); _handleNodeIgnoreRequestPacketElapsedTime += (end - start); } diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 0df3edbd11..15a7f50fa3 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -93,8 +93,8 @@ void AvatarMixerClientData::ignoreOther(SharedNodePointer self, SharedNodePointe } else { killPacket->writePrimitive(KillAvatarReason::YourAvatarEnteredTheirBubble); } - DependencyManager::get()->sendUnreliablePacket(*killPacket, *self); setLastBroadcastTime(other->getUUID(), 0); + DependencyManager::get()->sendUnreliablePacket(*killPacket, *self); } } diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 60dfad7687..05de209e81 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -222,7 +222,8 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { } else { // Check to see if the space bubble is enabled - if (node->isIgnoreRadiusEnabled() || avatarNode->isIgnoreRadiusEnabled()) { + // Don't bother with these checks if the other avatar has their bubble enabled and we're gettingAnyIgnored + if (node->isIgnoreRadiusEnabled() || (avatarNode->isIgnoreRadiusEnabled() && !getsAnyIgnored)) { // Define the scale of the box for the current other node glm::vec3 otherNodeBoxScale = (avatarNodeData->getPosition() - avatarNodeData->getGlobalBoundingBoxCorner()) * 2.0f; From 0000b8fb2159a9b489fa61cd56f6b21ed9fa021e Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 3 Mar 2017 17:01:57 -0800 Subject: [PATCH 13/13] OMG I think it's actually fully working --- assignment-client/src/avatars/AvatarMixer.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 1d1a1a57a8..c10a616818 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -365,6 +365,28 @@ void AvatarMixer::handleRequestsDomainListDataPacket(QSharedPointerreadPrimitive(&isRequesting); nodeData->setRequestsDomainListData(isRequesting); qCDebug(avatars) << "node" << nodeData->getNodeID() << "requestsDomainListData" << isRequesting; + + // If we just opened the PAL... + if (isRequesting) { + // For each node in the NodeList... + auto nodeList = DependencyManager::get(); + nodeList->eachMatchingNode( + // Discover the valid nodes we're ignoring... + [&](const SharedNodePointer& node)->bool { + if (node->getUUID() != senderNode->getUUID() && + (nodeData->isRadiusIgnoring(node->getUUID()) || + senderNode->isIgnoringNodeWithID(node->getUUID()))) { + return true; + } + return false; + }, + // ...For those nodes, reset the lastBroadcastTime to 0 + // so that the AvatarMixer will send Identity data to us + [&](const SharedNodePointer& node) { + nodeData->setLastBroadcastTime(node->getUUID(), 0); + } + ); + } } } auto end = usecTimestampNow();