Resolve issues tracking worn attachments that aren't in COF yet (see PR for more details)

Author: KittyBarnett

indra/newview/llappearancemgr.cpp

@@ -2541,6 +2541,23 @@ void LLAppearanceMgr::updateAppearanceFromCOF(bool enforce_item_restrictions,
     // Update attachments to match those requested.
     if (isAgentAvatarValid())
     {
+        // Include attachments which should be in COF but don't have their link created yet
+        std::set<LLUUID> pendingAttachments;
+        LLAttachmentsMgr::instance().getPendingAttachments(pendingAttachments);
+        for (const LLUUID& idAttachItem : pendingAttachments)
+        {
+            if ( !gAgentAvatarp->isWearingAttachment(idAttachItem) || isLinkedInCOF(idAttachItem) )
+            {
+                LLAttachmentsMgr::instance().clearPendingAttachmentLink(idAttachItem);
+                continue;
+            }
+
+            if (LLViewerInventoryItem* pAttachItem = gInventory.getItem(idAttachItem))
+            {
+                obj_items.push_back(pAttachItem);
+            }
+        }
+
         LL_DEBUGS("Avatar") << self_av_string() << "Updating " << obj_items.size() << " attachments" << LL_ENDL;
         LLAgentWearables::llvo_vec_t objects_to_remove;
         LLAgentWearables::llvo_vec_t objects_to_retain;
@@ -2567,7 +2584,11 @@ void LLAppearanceMgr::updateAppearanceFromCOF(bool enforce_item_restrictions,
         }
 
         // Take off the attachments that will no longer be in the outfit.
-        LLAgentWearables::userRemoveMultipleAttachments(objects_to_remove);
+        // (don't remove attachments until avatar is fully loaded - reduces random attaching/detaching/reattaching at log-on)
+        if (gAgentAvatarp->isFullyLoaded())
+        {
+            LLAgentWearables::userRemoveMultipleAttachments(objects_to_remove);
+        }
 
         // Restore attachment pos overrides for the attachments that
         // are remaining in the outfit.
@@ -4140,6 +4161,7 @@ void LLAppearanceMgr::removeItemsFromAvatar(const uuid_vec_t& ids_to_remove, nul
             continue;
         }
         removeCOFItemLinks(linked_item_id, cb);
+        LLAttachmentsMgr::instance().clearPendingAttachmentLink(linked_item_id);
         addDoomedTempAttachment(linked_item_id);
     }
 }

indra/newview/llappearancemgr.h

@@ -150,6 +150,7 @@ class LLAppearanceMgr: public LLSingleton<LLAppearanceMgr>
     // Attachment link management
     void unregisterAttachment(const LLUUID& item_id);
     void registerAttachment(const LLUUID& item_id);
+    bool getAttachmentInvLinkEnable() const { return mAttachmentInvLinkEnabled; }
     void setAttachmentInvLinkEnable(bool val);
 
     // Add COF link to individual item.

indra/newview/llattachmentsmgr.cpp

@@ -42,10 +42,19 @@ const F32 MAX_ATTACHMENT_REQUEST_LIFETIME = 30.0F;
 const F32 MIN_RETRY_REQUEST_TIME = 5.0F;
 const F32 MAX_BAD_COF_TIME = 30.0F;
 
+class LLRegisterAttachmentCallback : public LLRequestServerAppearanceUpdateOnDestroy
+{
+public:
+    void fire(const LLUUID& item_id) override
+    {
+        LLAttachmentsMgr::instance().onRegisterAttachmentComplete(item_id);
+        LLRequestServerAppearanceUpdateOnDestroy::fire(item_id);
+    }
+};
+
 LLAttachmentsMgr::LLAttachmentsMgr():
     mAttachmentRequests("attach",MIN_RETRY_REQUEST_TIME),
-    mDetachRequests("detach",MIN_RETRY_REQUEST_TIME),
-    mQuestionableCOFLinks("badcof",MAX_BAD_COF_TIME)
+    mDetachRequests("detach",MIN_RETRY_REQUEST_TIME)
 {
 }
 
@@ -113,8 +122,6 @@ void LLAttachmentsMgr::onIdle()
 
     expireOldDetachRequests();
 
-    checkInvalidCOFLinks();
-
     spamStatusInfo();
 }
 
@@ -222,6 +229,11 @@ void LLAttachmentsMgr::linkRecentlyArrivedAttachments()
 {
     if (mRecentlyArrivedAttachments.size())
     {
+        if (!LLAppearanceMgr::instance().getAttachmentInvLinkEnable())
+        {
+            return;
+        }
+
         // One or more attachments have arrived but have not yet been
         // processed for COF links
         if (mAttachmentRequests.empty())
@@ -268,17 +280,49 @@ void LLAttachmentsMgr::linkRecentlyArrivedAttachments()
         }
         if (ids_to_link.size())
         {
-            LLPointer<LLInventoryCallback> cb = new LLRequestServerAppearanceUpdateOnDestroy();
-            for (uuid_vec_t::const_iterator uuid_it = ids_to_link.begin();
-                 uuid_it != ids_to_link.end(); ++uuid_it)
+            LLPointer<LLInventoryCallback> cb = new LLRegisterAttachmentCallback();
+            for (const LLUUID& id_item: ids_to_link)
             {
-                LLAppearanceMgr::instance().addCOFItemLink(*uuid_it, cb);
+                if (std::find(mPendingAttachLinks.begin(), mPendingAttachLinks.end(), id_item) == mPendingAttachLinks.end())
+                {
+                    LLAppearanceMgr::instance().addCOFItemLink(id_item, cb);
+                    mPendingAttachLinks.insert(id_item);
+                }
             }
         }
         mRecentlyArrivedAttachments.clear();
     }
 }
 
+bool LLAttachmentsMgr::getPendingAttachments(std::set<LLUUID>& ids) const
+{
+    ids.clear();
+
+    // Returns the combined set of attachments that are pending link creation and those that currently have an ongoing link creation process.
+    set_union(mRecentlyArrivedAttachments.begin(), mRecentlyArrivedAttachments.end(), mPendingAttachLinks.begin(), mPendingAttachLinks.end(), std::inserter(ids, ids.begin()));
+
+    return !ids.empty();
+}
+
+void LLAttachmentsMgr::clearPendingAttachmentLink(const LLUUID& idItem)
+{
+    mPendingAttachLinks.erase(idItem);
+}
+
+void LLAttachmentsMgr::onRegisterAttachmentComplete(const LLUUID& id_item_link)
+{
+    if (const LLUUID& id_item = gInventory.getLinkedItemID(id_item_link); id_item != id_item_link)
+    {
+        clearPendingAttachmentLink(id_item);
+
+        // It may have been detached already in which case we should remove the COF link
+        if ( isAgentAvatarValid() && !gAgentAvatarp->isWearingAttachment(id_item) )
+        {
+            LLAppearanceMgr::instance().removeCOFItemLinks(id_item);
+        }
+    }
+}
+
 LLAttachmentsMgr::LLItemRequestTimes::LLItemRequestTimes(const std::string& op_name, F32 timeout):
     mOpName(op_name),
     mTimeout(timeout)
@@ -407,6 +451,8 @@ void LLAttachmentsMgr::onDetachRequested(const LLUUID& inv_item_id)
 
 void LLAttachmentsMgr::onDetachCompleted(const LLUUID& inv_item_id)
 {
+    clearPendingAttachmentLink(inv_item_id);
+
     LLTimer timer;
     LLInventoryItem *item = gInventory.getItem(inv_item_id);
     if (mDetachRequests.getTime(inv_item_id, timer))
@@ -428,10 +474,6 @@ void LLAttachmentsMgr::onDetachCompleted(const LLUUID& inv_item_id)
     {
         LL_DEBUGS("Avatar") << "ATT detach on shutdown for " << (item ? item->getName() : "UNKNOWN") << " " << inv_item_id << LL_ENDL;
     }
-
-    LL_DEBUGS("Avatar") << "ATT detached item flagging as questionable for COF link checking "
-                        << (item ? item->getName() : "UNKNOWN") << " id " << inv_item_id << LL_ENDL;
-    mQuestionableCOFLinks.addTime(inv_item_id);
 }
 
 bool LLAttachmentsMgr::isAttachmentStateComplete() const
@@ -440,81 +482,8 @@ bool LLAttachmentsMgr::isAttachmentStateComplete() const
         && mAttachmentRequests.empty()
         && mDetachRequests.empty()
         && mRecentlyArrivedAttachments.empty()
-        && mQuestionableCOFLinks.empty();
-}
-
-// Check for attachments that are (a) linked in COF and (b) not
-// attached to the avatar.  This is a rotten function to have to
-// include, because it runs the risk of either repeatedly spamming out
-// COF link removals if they're failing for some reason, or getting
-// into a tug of war with some other sequence of events that's in the
-// process of adding the attachment in question. However, it's needed
-// because we have no definitive source of authority for what things
-// are actually supposed to be attached. Scripts, run on the server
-// side, can remove an attachment without our expecting it. If this
-// happens to an attachment that's just been added, then the COF link
-// creation may still be in flight, and we will have to delete the
-// link after it shows up.
-//
-// Note that we only flag items for possible link removal if they have
-// been previously detached. This means that an attachment failure
-// will leave the link in the COF, where it will hopefully resolve
-// correctly on relog.
-//
-// See related: MAINT-5070, MAINT-4409
-//
-void LLAttachmentsMgr::checkInvalidCOFLinks()
-{
-    if (!gInventory.isInventoryUsable() || mQuestionableCOFLinks.empty())
-    {
-        return;
-    }
-    LLInventoryModel::cat_array_t cat_array;
-    LLInventoryModel::item_array_t item_array;
-    gInventory.collectDescendents(LLAppearanceMgr::instance().getCOF(),
-                                  cat_array,item_array,LLInventoryModel::EXCLUDE_TRASH);
-    for (S32 i=0; i<item_array.size(); i++)
-    {
-        const LLViewerInventoryItem* inv_item = item_array.at(i).get();
-        const LLUUID& item_id = inv_item->getLinkedUUID();
-        if (inv_item->getType() == LLAssetType::AT_OBJECT)
-        {
-            LLTimer timer;
-            bool is_flagged_questionable = mQuestionableCOFLinks.getTime(item_id,timer);
-            bool is_wearing_attachment = isAgentAvatarValid() && gAgentAvatarp->isWearingAttachment(item_id);
-            if (is_wearing_attachment && is_flagged_questionable)
-            {
-                LL_DEBUGS("Avatar") << "ATT was flagged questionable but is now "
-                                    << (is_wearing_attachment ? "attached " : "")
-                                    <<"removing flag after "
-                                    << timer.getElapsedTimeF32() << " item "
-                                    << inv_item->getName() << " id " << item_id << LL_ENDL;
-                mQuestionableCOFLinks.removeTime(item_id);
-            }
-        }
-    }
-
-    for(LLItemRequestTimes::iterator it = mQuestionableCOFLinks.begin();
-        it != mQuestionableCOFLinks.end(); )
-    {
-        LLItemRequestTimes::iterator curr_it = it;
-        ++it;
-        const LLUUID& item_id = curr_it->first;
-        LLViewerInventoryItem *inv_item = gInventory.getItem(item_id);
-        if (curr_it->second.getElapsedTimeF32() > MAX_BAD_COF_TIME)
-        {
-            if (LLAppearanceMgr::instance().isLinkedInCOF(item_id))
-            {
-                LL_DEBUGS("Avatar") << "ATT Linked in COF but not attached or requested, deleting link after "
-                                    << curr_it->second.getElapsedTimeF32() << " seconds for "
-                                    << (inv_item ? inv_item->getName() : "UNKNOWN") << " id " << item_id << LL_ENDL;
-                LLAppearanceMgr::instance().removeCOFItemLinks(item_id);
-            }
-            mQuestionableCOFLinks.erase(curr_it);
-            continue;
-        }
-    }
-}
+        && mPendingAttachLinks.empty();
+ }
 
 void LLAttachmentsMgr::spamStatusInfo()
 {

indra/newview/llattachmentsmgr.h

@@ -85,8 +85,14 @@ class LLAttachmentsMgr: public LLSingleton<LLAttachmentsMgr>
     void onDetachRequested(const LLUUID& inv_item_id);
     void onDetachCompleted(const LLUUID& inv_item_id);
 
+    void clearPendingAttachmentLink(const LLUUID& idItem);
+    bool getPendingAttachments(std::set<LLUUID>& ids) const;
     bool isAttachmentStateComplete() const;
 
+protected:
+    void onRegisterAttachmentComplete(const LLUUID& id_item_link);
+    friend class LLRegisterAttachmentCallback;
+
 private:
 
     class LLItemRequestTimes: public std::map<LLUUID,LLTimer>
@@ -109,7 +115,6 @@ class LLAttachmentsMgr: public LLSingleton<LLAttachmentsMgr>
     void linkRecentlyArrivedAttachments();
     void expireOldAttachmentRequests();
     void expireOldDetachRequests();
-    void checkInvalidCOFLinks();
     void spamStatusInfo();
 
     // Attachments that we are planning to rez but haven't requested from the server yet.
@@ -124,9 +129,8 @@ class LLAttachmentsMgr: public LLSingleton<LLAttachmentsMgr>
     // Attachments that have arrived but have not been linked in the COF yet.
     std::set<LLUUID> mRecentlyArrivedAttachments;
     LLTimer mCOFLinkBatchTimer;
-
-    // Attachments that are linked in the COF but may be invalid.
-    LLItemRequestTimes mQuestionableCOFLinks;
+    // Attachments that have pending COF link creation
+    std::set<LLUUID> mPendingAttachLinks;
 };
 
 #endif

indra/newview/llvoavatar.cpp

@@ -8535,7 +8535,7 @@ bool LLVOAvatar::processFullyLoadedChange(bool loading)
 
 bool LLVOAvatar::isFullyLoaded() const
 {
-    return (mRenderUnloadedAvatar || mFullyLoaded);
+    return (mRenderUnloadedAvatar && !isSelf()) || mFullyLoaded;
 }
 
 bool LLVOAvatar::hasFirstFullAttachmentData() const