From 4ed5f642ed9810190e72c5ba505d4528daa17917 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Fri, 11 Jan 2019 14:51:44 -0800 Subject: [PATCH 1/5] Case20615 - Changing Avatars doesn't update to everyone intermittently There were two problems: 1) If an update to a trait (simple or instance) was not sent because there was an outstanding (unacked) message, it would not be resent until a new trait was updated. Expected - it should be sent when the ack for the outstanding packet is received. 2) Trait versions were improperly being set to zero (the default) when an ack is received. --- .../src/avatars/AvatarMixerClientData.cpp | 4 ++- .../src/avatars/AvatarMixerSlave.cpp | 32 +++++++++++-------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 01cc9ff7a0..0caa7a0d9e 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -208,7 +208,9 @@ void AvatarMixerClientData::processBulkAvatarTraitsAckMessage(ReceivedMessage& m auto simpleReceivedIt = traitVersions.simpleCBegin(); while (simpleReceivedIt != traitVersions.simpleCEnd()) { auto traitType = static_cast(std::distance(traitVersions.simpleCBegin(), simpleReceivedIt)); - _perNodeAckedTraitVersions[nodeId][traitType] = *simpleReceivedIt; + if (*simpleReceivedIt != AvatarTraits::DEFAULT_TRAIT_VERSION) { + _perNodeAckedTraitVersions[nodeId][traitType] = *simpleReceivedIt; + } simpleReceivedIt++; } diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 5e84df0d7f..c23d2cc533 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -112,6 +112,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis // to see if there is any new traits data for this avatar that we need to send auto timeOfLastTraitsSent = listeningNodeData->getLastOtherAvatarTraitsSendPoint(otherNodeLocalID); auto timeOfLastTraitsChange = sendingNodeData->getLastReceivedTraitsChange(); + auto nextTimeOfLastTraitsSent = timeOfLastTraitsChange; qint64 bytesWritten = 0; @@ -134,16 +135,20 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis auto& lastAckedVersionRef = lastAckedVersions[traitType]; // hold sending more traits until we've been acked that the last one we sent was received - if (lastSentVersionRef == lastAckedVersionRef && lastReceivedVersions[traitType] > lastSentVersionRef) { - bytesWritten += addTraitsNodeHeader(listeningNodeData, sendingNodeData, traitsPacketList, bytesWritten); - // there is an update to this trait, add it to the traits packet - bytesWritten += sendingAvatar->packTrait(traitType, traitsPacketList, lastReceivedVersion); - // update the last sent version - lastSentVersionRef = lastReceivedVersion; - // Remember which versions we sent in this particular packet - // so we can verify when it's acked. - auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), otherNodeLocalID); - pendingTraitVersions[traitType] = lastReceivedVersion; + if (lastSentVersionRef == lastAckedVersionRef) { + if (lastReceivedVersion > lastSentVersionRef) { + bytesWritten += addTraitsNodeHeader(listeningNodeData, sendingNodeData, traitsPacketList, bytesWritten); + // there is an update to this trait, add it to the traits packet + bytesWritten += sendingAvatar->packTrait(traitType, traitsPacketList, lastReceivedVersion); + // update the last sent version + lastSentVersionRef = lastReceivedVersion; + // Remember which versions we sent in this particular packet + // so we can verify when it's acked. + auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), otherNodeLocalID); + pendingTraitVersions[traitType] = lastReceivedVersion; + } + } else { + nextTimeOfLastTraitsSent = timeOfLastTraitsSent; } ++simpleReceivedIt; @@ -182,6 +187,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis // version to go on, otherwise we drop the received trait if (sentInstanceIt != sentIDValuePairs.end() && (ackedInstanceIt == ackIDValuePairs.end() || sentInstanceIt->value != ackedInstanceIt->value)) { + nextTimeOfLastTraitsSent = timeOfLastTraitsSent; continue; } if (!isDeleted && (sentInstanceIt == sentIDValuePairs.end() || receivedVersion > sentInstanceIt->value)) { @@ -223,10 +229,10 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis if (bytesWritten) { // write a null trait type to mark the end of trait data for this avatar bytesWritten += traitsPacketList.writePrimitive(AvatarTraits::NullTrait); + // since we send all traits for this other avatar, update the time of last traits sent + // to match the time of last traits change + listeningNodeData->setLastOtherAvatarTraitsSendPoint(otherNodeLocalID, nextTimeOfLastTraitsSent); } - // since we send all traits for this other avatar, update the time of last traits sent - // to match the time of last traits change - listeningNodeData->setLastOtherAvatarTraitsSendPoint(otherNodeLocalID, timeOfLastTraitsChange); } From d3528ba2899fdeec53b166644f32995b60680b00 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Fri, 11 Jan 2019 16:33:23 -0800 Subject: [PATCH 2/5] Code review changes --- assignment-client/src/avatars/AvatarMixerClientData.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 987cb7aa2f..b7d2f5cdf8 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -207,8 +207,8 @@ void AvatarMixerClientData::processBulkAvatarTraitsAckMessage(ReceivedMessage& m // process simple traits auto simpleReceivedIt = traitVersions.simpleCBegin(); while (simpleReceivedIt != traitVersions.simpleCEnd()) { - auto traitType = static_cast(std::distance(traitVersions.simpleCBegin(), simpleReceivedIt)); if (*simpleReceivedIt != AvatarTraits::DEFAULT_TRAIT_VERSION) { + auto traitType = static_cast(std::distance(traitVersions.simpleCBegin(), simpleReceivedIt)); _perNodeAckedTraitVersions[nodeId][traitType] = *simpleReceivedIt; } simpleReceivedIt++; From cd6495286dd33dc02f8523cae7428fbbe39961aa Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Fri, 11 Jan 2019 16:49:04 -0800 Subject: [PATCH 3/5] More code review changes --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index c23d2cc533..02e66afb29 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -106,11 +106,11 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis // in that packet_ are ignored. Updates to traits not in that packet will // be sent. - auto otherNodeLocalID = sendingNodeData->getNodeLocalID(); + auto sendingNodeLocalID = sendingNodeData->getNodeLocalID(); // Perform a simple check with two server clock time points // to see if there is any new traits data for this avatar that we need to send - auto timeOfLastTraitsSent = listeningNodeData->getLastOtherAvatarTraitsSendPoint(otherNodeLocalID); + auto timeOfLastTraitsSent = listeningNodeData->getLastOtherAvatarTraitsSendPoint(sendingNodeLocalID); auto timeOfLastTraitsChange = sendingNodeData->getLastReceivedTraitsChange(); auto nextTimeOfLastTraitsSent = timeOfLastTraitsChange; @@ -122,8 +122,8 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis auto sendingAvatar = sendingNodeData->getAvatarSharedPointer(); // compare trait versions so we can see what exactly needs to go out - auto& lastSentVersions = listeningNodeData->getLastSentTraitVersions(otherNodeLocalID); - auto& lastAckedVersions = listeningNodeData->getLastAckedTraitVersions(otherNodeLocalID); + auto& lastSentVersions = listeningNodeData->getLastSentTraitVersions(sendingNodeLocalID); + auto& lastAckedVersions = listeningNodeData->getLastAckedTraitVersions(sendingNodeLocalID); const auto& lastReceivedVersions = sendingNodeData->getLastReceivedTraitVersions(); auto simpleReceivedIt = lastReceivedVersions.simpleCBegin(); @@ -144,7 +144,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis lastSentVersionRef = lastReceivedVersion; // Remember which versions we sent in this particular packet // so we can verify when it's acked. - auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), otherNodeLocalID); + auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), sendingNodeLocalID); pendingTraitVersions[traitType] = lastReceivedVersion; } } else { @@ -204,7 +204,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), - otherNodeLocalID); + sendingNodeLocalID); pendingTraitVersions.instanceInsert(traitType, instanceID, receivedVersion); } else if (isDeleted && sentInstanceIt != sentIDValuePairs.end() && absoluteReceivedVersion > sentInstanceIt->value) { @@ -218,7 +218,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis auto& pendingTraitVersions = listeningNodeData->getPendingTraitVersions(listeningNodeData->getTraitsMessageSequence(), - otherNodeLocalID); + sendingNodeLocalID); pendingTraitVersions.instanceInsert(traitType, instanceID, absoluteReceivedVersion); } From fca15c1362f52d4ef0c5902aeda99e0d41a91417 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Fri, 11 Jan 2019 17:13:32 -0800 Subject: [PATCH 4/5] CR Fix --- 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 02e66afb29..afde3be07e 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -231,7 +231,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis bytesWritten += traitsPacketList.writePrimitive(AvatarTraits::NullTrait); // since we send all traits for this other avatar, update the time of last traits sent // to match the time of last traits change - listeningNodeData->setLastOtherAvatarTraitsSendPoint(otherNodeLocalID, nextTimeOfLastTraitsSent); + listeningNodeData->setLastOtherAvatarTraitsSendPoint(sendingNodeLocalID, nextTimeOfLastTraitsSent); } } From 69af0ea0ee7a4204302063876c2b69a5aa22a0a7 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Fri, 11 Jan 2019 17:52:47 -0800 Subject: [PATCH 5/5] CR Fix --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index afde3be07e..6b039e2c03 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -112,7 +112,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis // to see if there is any new traits data for this avatar that we need to send auto timeOfLastTraitsSent = listeningNodeData->getLastOtherAvatarTraitsSendPoint(sendingNodeLocalID); auto timeOfLastTraitsChange = sendingNodeData->getLastReceivedTraitsChange(); - auto nextTimeOfLastTraitsSent = timeOfLastTraitsChange; + bool allTraitsUpdated = true; qint64 bytesWritten = 0; @@ -148,7 +148,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis pendingTraitVersions[traitType] = lastReceivedVersion; } } else { - nextTimeOfLastTraitsSent = timeOfLastTraitsSent; + allTraitsUpdated = false; } ++simpleReceivedIt; @@ -187,7 +187,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis // version to go on, otherwise we drop the received trait if (sentInstanceIt != sentIDValuePairs.end() && (ackedInstanceIt == ackIDValuePairs.end() || sentInstanceIt->value != ackedInstanceIt->value)) { - nextTimeOfLastTraitsSent = timeOfLastTraitsSent; + allTraitsUpdated = false; continue; } if (!isDeleted && (sentInstanceIt == sentIDValuePairs.end() || receivedVersion > sentInstanceIt->value)) { @@ -231,7 +231,9 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis bytesWritten += traitsPacketList.writePrimitive(AvatarTraits::NullTrait); // since we send all traits for this other avatar, update the time of last traits sent // to match the time of last traits change - listeningNodeData->setLastOtherAvatarTraitsSendPoint(sendingNodeLocalID, nextTimeOfLastTraitsSent); + if (allTraitsUpdated) { + listeningNodeData->setLastOtherAvatarTraitsSendPoint(sendingNodeLocalID, timeOfLastTraitsChange); + } } }