In SetAttachment, if the existing attachment has no asset id then carry on rather than abort.

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 then the asset ids (which are now present) are never set, and NPC attachments
later fail unless the attachment is detached and reattached.

Hopefully resolves part of http://opensimulator.org/mantis/view.php?id=5653

OpenSim/Framework/AvatarAppearance.cs

@@ -414,12 +414,16 @@ namespace OpenSim.Framework
 
         internal void ReplaceAttachment(AvatarAttachment attach)
         {
+//            m_log.DebugFormat(
+//                "[AVATAR APPEARANCE]: Replacing itemID={0}, assetID={1} at {2}",
+//                attach.ItemID, attach.AssetID, attach.AttachPoint);
+
             m_attachments[attach.AttachPoint] = new List<AvatarAttachment>();
             m_attachments[attach.AttachPoint].Add(attach);
         }
 
         /// <summary>
-        /// Add an attachment
+        /// Set an attachment
         /// </summary>
         /// <remarks>
         /// If the attachpoint has the
@@ -449,11 +453,20 @@ namespace OpenSim.Framework
                     m_attachments.Remove(attachpoint);
                     return true;
                 }
+                
                 return false;
             }
 
-            // check if the item is already attached at this point
+            // When a user logs in, the attachment item ids are pulled from persistence in the Avatars table.  However,
-            if (GetAttachpoint(item) == (attachpoint & 0x7F))
+            // 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;
@@ -474,6 +487,23 @@ namespace OpenSim.Framework
             return true;
         }
 
+        /// <summary>
+        /// If the item is already attached, return it.
+        /// </summary>
+        /// <param name="itemID"></param>
+        /// <returns>Returns null if this item is not attached.</returns>
+        public AvatarAttachment GetAttachmentForItem(UUID itemID)
+        {
+            foreach (KeyValuePair<int, List<AvatarAttachment>> kvp in m_attachments)
+            {
+                int index = kvp.Value.FindIndex(delegate(AvatarAttachment a) { return a.ItemID == itemID; });
+                if (index >= 0)
+                    return kvp.Value[index];
+            }
+
+            return null;
+        }
+
         public int GetAttachpoint(UUID itemID)
         {
             foreach (KeyValuePair<int, List<AvatarAttachment>> kvp in m_attachments)

OpenSim/Framework/ChildAgentDataUpdate.cs

@@ -628,6 +628,7 @@ namespace OpenSim.Framework
                         // We know all of these must end up as attachments so we
                         // append rather than replace to ensure multiple attachments
                         // per point continues to work
+//                        m_log.DebugFormat("[CHILDAGENTDATAUPDATE]: Appending attachments for {0}", AgentID);
                         Appearance.AppendAttachment(new AvatarAttachment((OSDMap)o));
                     }
                 }

OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs

@@ -247,6 +247,14 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
             Assert.That(attSo.IsAttachment);
             Assert.That(attSo.UsesPhysics, Is.False);
             Assert.That(attSo.IsTemporary, Is.False);
+
+            // Check appearance status
+            List<AvatarAttachment> retreivedAttachments = presence.Appearance.GetAttachments();
+            Assert.That(retreivedAttachments.Count, Is.EqualTo(1));
+            Assert.That(retreivedAttachments[0].AttachPoint, Is.EqualTo((int)AttachmentPoint.Chest));
+            Assert.That(retreivedAttachments[0].ItemID, Is.EqualTo(attItemId));
+            Assert.That(retreivedAttachments[0].AssetID, Is.EqualTo(attAssetId));
+            Assert.That(presence.Appearance.GetAttachpoint(attItemId), Is.EqualTo((int)AttachmentPoint.Chest));
         }
 
         // I'm commenting this test because scene setup NEEDS InventoryService to 

OpenSim/Services/Interfaces/IAvatarService.cs

@@ -262,7 +262,6 @@ namespace OpenSim.Services.Interfaces
                             UUID.Parse(Data["SkirtItem"]),
                             UUID.Parse(Data["SkirtAsset"]));
 
-
                 if (Data.ContainsKey("VisualParams"))
                 {
                     string[] vps = Data["VisualParams"].Split(new char[] {','});
@@ -291,7 +290,6 @@ namespace OpenSim.Services.Interfaces
                     }
                 }
 
-
                 // Attachments
                 Dictionary<string, string> attchs = new Dictionary<string, string>();
                 foreach (KeyValuePair<string, string> _kvp in Data)
@@ -308,7 +306,7 @@ namespace OpenSim.Services.Interfaces
                     UUID uuid = UUID.Zero;
                     UUID.TryParse(_kvp.Value, out uuid);
 
-                    appearance.SetAttachment(point,uuid,UUID.Zero);
+                    appearance.SetAttachment(point, uuid, UUID.Zero);
                 }
 
                 if (appearance.Wearables[AvatarWearable.BODY].Count == 0)