Prevent multiple instances of the same item ID being appended to an AvatarAppearance

It looks like this was happening when AttachmentsModule.RezAttachments was doing a secondary set of each attachment to update with the asset ID (initially they only have the inventory ID).
However, with multi-attach this was appending a second copy of the same attachment rather than updating the data that was already there.
This commit requires both simulator and service to be updated.

3 files changed, 60 insertions, 34 deletions

diff --git a/OpenSim/Framework/AvatarAppearance.cs b/OpenSim/Framework/AvatarAppearance.cs
index 95e9667..494ae5e 100644
--- a/OpenSim/Framework/AvatarAppearance.cs
+++ b/OpenSim/Framework/AvatarAppearance.cs
@@ -459,45 +459,59 @@ namespace OpenSim.Framework
             if (attachpoint == 0)
                 return false;
 
-            if (item == UUID.Zero)
+            lock (m_attachments)
             {
-                lock (m_attachments)
+                if (item == UUID.Zero)
                 {
                     if (m_attachments.ContainsKey(attachpoint))
                     {
                         m_attachments.Remove(attachpoint);
                         return true;
                     }
+                    
+                    return false;
                 }
-                
-                return false;
-            }
 
-            // When a user logs in, the attachment item ids are pulled from persistence in the Avatars table.  However,
-            // the asset ids are not saved.  When the avatar enters a simulator the attachments are set again.  If
-            // we simply perform an item check here then the asset ids (which are now present) are never set, and NPC attachments
-            // later fail unless the attachment is detached and reattached.
-            //
-            // Therefore, we will carry on with the set if the existing attachment has no asset id.
-            AvatarAttachment existingAttachment = GetAttachmentForItem(item);
-            if (existingAttachment != null
-                && existingAttachment.AssetID != UUID.Zero
-                && existingAttachment.AttachPoint == (attachpoint & 0x7F))
-            {
-                // m_log.DebugFormat("[AVATAR APPEARANCE] attempt to attach an already attached item {0}",item);
-                return false;
-            }
-            
-            // check if this is an append or a replace, 0x80 marks it as an append
-            if ((attachpoint & 0x80) > 0)
-            {
-                // strip the append bit
-                int point = attachpoint & 0x7F;
-                AppendAttachment(new AvatarAttachment(point, item, asset));
-            }
-            else
-            {
-                ReplaceAttachment(new AvatarAttachment(attachpoint,item, asset));
+                // When a user logs in, the attachment item ids are pulled from persistence in the Avatars table.  However,
+                // the asset ids are not saved.  When the avatar enters a simulator the attachments are set again.  If
+                // we simply perform an item check here then the asset ids (which are now present) are never set, and NPC attachments
+                // later fail unless the attachment is detached and reattached.
+                //
+                // Therefore, we will carry on with the set if the existing attachment has no asset id.
+                AvatarAttachment existingAttachment = GetAttachmentForItem(item);
+                if (existingAttachment != null)
+                {
+//                    m_log.DebugFormat(
+//                        "[AVATAR APPEARANCE]: Found existing attachment for {0}, asset {1} at point {2}", 
+//                        existingAttachment.ItemID, existingAttachment.AssetID, existingAttachment.AttachPoint);
+
+                    if (existingAttachment.AssetID != UUID.Zero && existingAttachment.AttachPoint == (attachpoint & 0x7F))
+                    {
+                        m_log.DebugFormat(
+                            "[AVATAR APPEARANCE]: Ignoring attempt to attach an already attached item {0} at point {1}", 
+                            item, attachpoint);
+
+                        return false;
+                    }
+                    else
+                    {
+                        // Remove it here so that the later append does not add a second attachment but we still update
+                        // the assetID
+                        DetachAttachment(existingAttachment.ItemID);
+                    }
+                }
+                
+                // check if this is an append or a replace, 0x80 marks it as an append
+                if ((attachpoint & 0x80) > 0)
+                {
+                    // strip the append bit
+                    int point = attachpoint & 0x7F;
+                    AppendAttachment(new AvatarAttachment(point, item, asset));
+                }
+                else
+                {
+                    ReplaceAttachment(new AvatarAttachment(attachpoint,item, asset));
+                }
             }
 
             return true;
@@ -547,6 +561,10 @@ namespace OpenSim.Framework
                     int index = kvp.Value.FindIndex(delegate(AvatarAttachment a) { return a.ItemID == itemID; });
                     if (index >= 0)
                     {
+//                        m_log.DebugFormat(
+//                            "[AVATAR APPEARANCE]: Detaching attachment {0}, index {1}, point {2}", 
+//                            m_attachments[kvp.Key][index].ItemID, index, m_attachments[kvp.Key][index].AttachPoint);
+
                         // Remove it from the list of attachments at that attach point
                         m_attachments[kvp.Key].RemoveAt(index);
     
diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
index 72ba3cf..ad17aa9 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
@@ -547,8 +547,8 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
             }
 
 //            m_log.DebugFormat(
-//                "[ATTACHMENTS MODULE]: Detaching object {0} {1} for {2} in {3}", 
-//                so.Name, so.LocalId, sp.Name, m_scene.Name);
+//                "[ATTACHMENTS MODULE]: Detaching object {0} {1} (FromItemID {2}) for {3} in {4}", 
+//                so.Name, so.LocalId, so.FromItemID, sp.Name, m_scene.Name);
 
             // Scripts MUST be snapshotted before the object is
             // removed from the scene because doing otherwise will
diff --git a/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs b/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs
index 00d1fd8..ff5bf9f 100644
--- a/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs
+++ b/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs
@@ -326,7 +326,7 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory
 
         public void QueueAppearanceSave(UUID agentid)
         {
-            // m_log.WarnFormat("[AVFACTORY]: Queue appearance save for {0}", agentid);
+//            m_log.DebugFormat("[AVFACTORY]: Queueing appearance save for {0}", agentid);
 
             // 10000 ticks per millisecond, 1000 milliseconds per second
             long timestamp = DateTime.Now.Ticks + Convert.ToInt64(m_savetime * 1000 * 10000);
@@ -529,7 +529,7 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory
                 return;
             }
 
-            // m_log.WarnFormat("[AVFACTORY] avatar {0} save appearance",agentid);
+//            m_log.DebugFormat("[AVFACTORY]: Saving appearance for avatar {0}", agentid);
 
             // This could take awhile since it needs to pull inventory
             // We need to do it at the point of save so that there is a sufficient delay for any upload of new body part/shape
@@ -538,6 +538,14 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory
             // multiple save requests.
             SetAppearanceAssets(sp.UUID, sp.Appearance);
 
+//            List<AvatarAttachment> attachments = sp.Appearance.GetAttachments();
+//            foreach (AvatarAttachment att in attachments)
+//            {
+//                m_log.DebugFormat(
+//                    "[AVFACTORY]: For {0} saving attachment {1} at point {2}",
+//                    sp.Name, att.ItemID, att.AttachPoint);
+//            }
+
             m_scene.AvatarService.SetAppearance(agentid, sp.Appearance);
 
             // Trigger this here because it's the final step in the set/queue/save process for appearance setting.