Assign lowest available suffix when display-names collide

assignment-client/src/avatars/AvatarMixer.cpp

@@ -38,6 +38,19 @@ const QString AVATAR_MIXER_LOGGING_NAME = "avatar-mixer";
 // FIXME - what we'd actually like to do is send to users at ~50% of their present rate down to 30hz. Assume 90 for now.
 const int AVATAR_MIXER_BROADCAST_FRAMES_PER_SECOND = 45;
 
+const QRegularExpression AvatarMixer::suffixedNamePattern { R"(^\s*(.+)\s*_(\d)+\s*$)" };
+
+// Lexicographic comparison:
+bool AvatarMixer::SessionDisplayName::operator<(const SessionDisplayName& rhs) const {
+    if (_baseName < rhs._baseName) {
+        return true;
+    } else if (rhs._baseName < _baseName) {
+        return false;
+    } else {
+        return _suffix < rhs._suffix;
+    }
+}
+
 AvatarMixer::AvatarMixer(ReceivedMessage& message) :
     ThreadedAssignment(message),
     _slavePool(&_slaveSharedData)
@@ -313,27 +326,40 @@ void AvatarMixer::manageIdentityData(const SharedNodePointer& node) {
     bool sendIdentity = false;
     if (nodeData && nodeData->getAvatarSessionDisplayNameMustChange()) {
         AvatarData& avatar = nodeData->getAvatar();
-        const QString& existingBaseDisplayName = nodeData->getBaseDisplayName();
-        if (--_sessionDisplayNames[existingBaseDisplayName].second <= 0) {
-            _sessionDisplayNames.remove(existingBaseDisplayName);
+        const QString& existingBaseDisplayName = nodeData->getAvatar().getSessionDisplayName();
+        if (!existingBaseDisplayName.isEmpty()) {
+            SessionDisplayName existingDisplayName { existingBaseDisplayName };
+
+            auto suffixMatch = suffixedNamePattern.match(existingBaseDisplayName);
+            if (suffixMatch.hasMatch()) {
+                existingDisplayName._baseName = suffixMatch.captured(1);
+                existingDisplayName._suffix = suffixMatch.captured(2).toInt();
+            }
+            _sessionDisplayNames.erase(existingDisplayName);
         }
 
         QString baseName = avatar.getDisplayName().trimmed();
         const QRegularExpression curses { "fuck|shit|damn|cock|cunt" }; // POC. We may eventually want something much more elaborate (subscription?).
         baseName = baseName.replace(curses, "*"); // Replace rather than remove, so that people have a clue that the person's a jerk.
-        const QRegularExpression trailingDigits { "\\s*(_\\d+\\s*)?(\\s*\\n[^$]*)?$" }; // trailing whitespace "_123" and any subsequent lines
+        static const QRegularExpression trailingDigits { R"(\s*(_\d+\s*)?(\s*\n[^$]*)?$)" }; // trailing whitespace "_123" and any subsequent lines
         baseName = baseName.remove(trailingDigits);
         if (baseName.isEmpty()) {
             baseName = "anonymous";
         }
 
-        QPair<int, int>& soFar = _sessionDisplayNames[baseName]; // Inserts and answers 0, 0 if not already present, which is what we want.
-        int& highWater = soFar.first;
-        nodeData->setBaseDisplayName(baseName);
-        QString sessionDisplayName = (highWater > 0) ? baseName + "_" + QString::number(highWater) : baseName;
+        SessionDisplayName newDisplayName { baseName };
+        auto nameIter = _sessionDisplayNames.lower_bound(newDisplayName);
+        if (nameIter != _sessionDisplayNames.end() && nameIter->_baseName == baseName) {
+            // Existing instance(s) of name; find first free suffix
+            while (*nameIter == newDisplayName && ++newDisplayName._suffix && ++nameIter != _sessionDisplayNames.end())
+                ;
+        }
+
+        _sessionDisplayNames.insert(newDisplayName);
+        QString sessionDisplayName = (newDisplayName._suffix > 0) ? baseName + "_" + QString::number(newDisplayName._suffix) : baseName;
         avatar.setSessionDisplayName(sessionDisplayName);
-        highWater++;
-        soFar.second++; // refcount
+        nodeData->setBaseDisplayName(baseName);
+
         nodeData->flagIdentityChange();
         nodeData->setAvatarSessionDisplayNameMustChange(false);
         sendIdentity = true;
@@ -410,9 +436,19 @@ void AvatarMixer::handleAvatarKilled(SharedNodePointer avatarNode) {
            QMutexLocker nodeDataLocker(&avatarNode->getLinkedData()->getMutex());
            AvatarMixerClientData* nodeData = dynamic_cast<AvatarMixerClientData*>(avatarNode->getLinkedData());
            const QString& baseDisplayName = nodeData->getBaseDisplayName();
-           // No sense guarding against very rare case of a node with no entry, as this will work without the guard and do one less lookup in the common case.
-           if (--_sessionDisplayNames[baseDisplayName].second <= 0) {
-               _sessionDisplayNames.remove(baseDisplayName);
+           const QString& displayName = nodeData->getAvatar().getSessionDisplayName();
+           SessionDisplayName exitingDisplayName { displayName };
+
+           auto suffixMatch = suffixedNamePattern.match(displayName);
+           if (suffixMatch.hasMatch()) {
+               exitingDisplayName._baseName = suffixMatch.captured(1);
+               exitingDisplayName._suffix = suffixMatch.captured(2).toInt();
+           }
+           auto displayNameIter = _sessionDisplayNames.find(exitingDisplayName);
+           if (displayNameIter == _sessionDisplayNames.end()) {
+               qCDebug(avatars, "Exiting avatar displayname", displayName, "not found");
+           } else {
+               _sessionDisplayNames.erase(displayNameIter);
            }
         }
 

assignment-client/src/avatars/AvatarMixer.h

@@ -15,6 +15,7 @@
 #ifndef hifi_AvatarMixer_h
 #define hifi_AvatarMixer_h
 
+#include <set>
 #include <shared/RateCounter.h>
 #include <PortableHighResolutionClock.h>
 
@@ -88,7 +89,24 @@ private:
 
     RateCounter<> _broadcastRate;
     p_high_resolution_clock::time_point _lastDebugMessage;
-    QHash<QString, QPair<int, int>> _sessionDisplayNames;
+
+    // Pair of basename + uniquifying integer suffix.
+    struct SessionDisplayName {
+        explicit SessionDisplayName(QString baseName = QString(), int suffix = 0) :
+            _baseName(baseName),
+            _suffix(suffix) { }
+        // Does lexicographic ordering:
+        bool operator<(const SessionDisplayName& rhs) const;
+        bool operator==(const SessionDisplayName& rhs) const {
+            return _baseName == rhs._baseName && _suffix == rhs._suffix;
+        }
+
+        QString _baseName;
+        int _suffix;
+    };
+    static const QRegularExpression suffixedNamePattern;
+
+    std::set<SessionDisplayName> _sessionDisplayNames;
 
     quint64 _displayNameManagementElapsedTime { 0 }; // total time spent in broadcastAvatarData/display name management... since last stats window
     quint64 _ignoreCalculationElapsedTime { 0 };

libraries/avatars/src/AvatarData.cpp

@@ -632,9 +632,11 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent
 
     // include jointData if there is room for the most minimal section. i.e. no translations or rotations.
     IF_AVATAR_SPACE(PACKET_HAS_JOINT_DATA, AvatarDataPacket::minJointDataSize(numJoints)) {
-        // Allow for faux joints + translation bit-vector:
-        const ptrdiff_t minSizeForJoint = sizeof(AvatarDataPacket::SixByteQuat)
-            + jointBitVectorSize + AvatarDataPacket::FAUX_JOINTS_SIZE;
+        // Minimum space required for another rotation joint -
+        // size of joint + following translation bit-vector + translation scale + faux joints:
+        const ptrdiff_t minSizeForJoint = sizeof(AvatarDataPacket::SixByteQuat) + jointBitVectorSize +
+            sizeof(float) + AvatarDataPacket::FAUX_JOINTS_SIZE;
+
         auto startSection = destinationBuffer;
 
         // compute maxTranslationDimension before we send any joint data.
@@ -724,6 +726,7 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent
             const JointData& data = joints[i];
             const JointData& last = lastSentJointData[i];
 
+            // Note minSizeForJoint is conservative since there isn't a following bit-vector + scale.
             if (packetEnd - destinationBuffer >= minSizeForJoint) {
                 if (!data.translationIsDefaultPose) {
                     if (sendAll || last.translationIsDefaultPose || (!cullSmallChanges && last.translation != data.translation)