Fix recent regression where an item worn to an attachment point that was already occupied did not remove the previous attachment (current behaviour)

Regression was commit ccd6f4 (Tue Mar 5 23:47:36 2013)
Added regression test for this case.

OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs

@@ -289,16 +289,21 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
             if (!Enabled)
                 return false;
 
-            if (AttachObjectInternal(sp, group, attachmentPt, silent, temp))
+            return AttachObjectInternal(sp, group, attachmentPt, silent, temp, false);
-            {
-                m_scene.EventManager.TriggerOnAttach(group.LocalId, group.FromItemID, sp.UUID);
-                return true;
-            }
-
-            return false;
         }
 
-        private bool AttachObjectInternal(IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool silent, bool temp)
+        /// <summary>
+        /// Internal method which actually does all the work for attaching an object.
+        /// </summary>
+        /// <returns>The object attached.</returns>
+        /// <param name='sp'></param>
+        /// <param name='group'>The object to attach.</param>
+        /// <param name='attachmentPt'></param>
+        /// <param name='silent'></param>
+        /// <param name='temp'></param>
+        /// <param name='resumeScripts'>If true then scripts are resumed on the attached object.</param>
+        private bool AttachObjectInternal(
+            IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool silent, bool temp, bool resumeScripts)
         {
 //                m_log.DebugFormat(
 //                    "[ATTACHMENTS MODULE]: Attaching object {0} {1} to {2} point {3} from ground (silent = {4})",
@@ -322,44 +327,44 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
                 return false;
             }
 
+            Vector3 attachPos = group.AbsolutePosition;
+
+            // TODO: this short circuits multiple attachments functionality  in  LL viewer 2.1+ and should
+            // be removed when that functionality is implemented in opensim
+            attachmentPt &= 0x7f;
+            
+            // If the attachment point isn't the same as the one previously used
+            // set it's offset position = 0 so that it appears on the attachment point
+            // and not in a weird location somewhere unknown.
+            if (attachmentPt != 0 && attachmentPt != group.AttachmentPoint)
+            {
+                attachPos = Vector3.Zero;
+            }
+
+            // AttachmentPt 0 means the client chose to 'wear' the attachment.
+            if (attachmentPt == 0)
+            {
+                // Check object for stored attachment point
+                attachmentPt = group.AttachmentPoint;
+            }
+
+            // if we still didn't find a suitable attachment point.......
+            if (attachmentPt == 0)
+            {
+                // Stick it on left hand with Zero Offset from the attachment point.
+                attachmentPt = (uint)AttachmentPoint.LeftHand;
+                attachPos = Vector3.Zero;
+            }
+
             // Remove any previous attachments
             List<SceneObjectGroup> existingAttachments = sp.GetAttachments(attachmentPt);
 
             // At the moment we can only deal with a single attachment
             if (existingAttachments.Count != 0 && existingAttachments[0].FromItemID != UUID.Zero)
-                DetachSingleAttachmentToInv(sp, group);
+                DetachSingleAttachmentToInv(sp, existingAttachments[0]);
 
             lock (sp.AttachmentsSyncLock)
             {
-                Vector3 attachPos = group.AbsolutePosition;
-
-                // TODO: this short circuits multiple attachments functionality  in  LL viewer 2.1+ and should
-                // be removed when that functionality is implemented in opensim
-                attachmentPt &= 0x7f;
-                
-                // If the attachment point isn't the same as the one previously used
-                // set it's offset position = 0 so that it appears on the attachment point
-                // and not in a weird location somewhere unknown.
-                if (attachmentPt != 0 && attachmentPt != group.AttachmentPoint)
-                {
-                    attachPos = Vector3.Zero;
-                }
-
-                // AttachmentPt 0 means the client chose to 'wear' the attachment.
-                if (attachmentPt == 0)
-                {
-                    // Check object for stored attachment point
-                    attachmentPt = group.AttachmentPoint;
-                }
-
-                // if we still didn't find a suitable attachment point.......
-                if (attachmentPt == 0)
-                {
-                    // Stick it on left hand with Zero Offset from the attachment point.
-                    attachmentPt = (uint)AttachmentPoint.LeftHand;
-                    attachPos = Vector3.Zero;
-                }
-
                 group.AttachmentPoint = attachmentPt;
                 group.AbsolutePosition = attachPos;
 
@@ -367,6 +372,17 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
                     UpdateUserInventoryWithAttachment(sp, group, attachmentPt, temp);
 
                 AttachToAgent(sp, group, attachmentPt, attachPos, silent);
+
+                if (resumeScripts)
+                {
+                    // Fire after attach, so we don't get messy perms dialogs
+                    // 4 == AttachedRez
+                    group.CreateScriptInstances(0, true, m_scene.DefaultScriptEngine, 4);
+                    group.ResumeScripts();
+                }
+
+                // Do this last so that event listeners have access to all the effects of the attachment
+                m_scene.EventManager.TriggerOnAttach(group.LocalId, group.FromItemID, sp.UUID);
             }
 
             return true;
@@ -391,8 +407,8 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
                 return null;
 
 //            m_log.DebugFormat(
-//                "[ATTACHMENTS MODULE]: RezSingleAttachmentFromInventory to point {0} from item {1} for {2}",
+//                "[ATTACHMENTS MODULE]: RezSingleAttachmentFromInventory to point {0} from item {1} for {2} in {3}",
-//                (AttachmentPoint)AttachmentPt, itemID, sp.Name);
+//                (AttachmentPoint)AttachmentPt, itemID, sp.Name, m_scene.Name);
 
             // TODO: this short circuits multiple attachments functionality  in  LL viewer 2.1+ and should
             // be removed when that functionality is implemented in opensim
@@ -525,6 +541,10 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
                 return;
             }
 
+//            m_log.DebugFormat(
+//                "[ATTACHMENTS MODULE]: Detaching object {0} {1} for {2} in {3}", 
+//                so.Name, so.LocalId, sp.Name, m_scene.Name);
+
             // Scripts MUST be snapshotted before the object is
             // removed from the scene because doing otherwise will
             // clobber the run flag
@@ -846,60 +866,42 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
                 return null;
             }
 
-            // Remove any previous attachments
-            List<SceneObjectGroup> attachments = sp.GetAttachments(attachmentPt);
-
-            // At the moment we can only deal with a single attachment
-            if (attachments.Count != 0)
-                DetachSingleAttachmentToInv(sp, attachments[0]);
-
-            lock (sp.AttachmentsSyncLock)
-            {
 //                    m_log.DebugFormat(
 //                        "[ATTACHMENTS MODULE]: Rezzed single object {0} for attachment to {1} on point {2} in {3}",
 //                        objatt.Name, sp.Name, attachmentPt, m_scene.Name);
 
-                // HasGroupChanged is being set from within RezObject.  Ideally it would be set by the caller.
+            // HasGroupChanged is being set from within RezObject.  Ideally it would be set by the caller.
-                objatt.HasGroupChanged = false;
+            objatt.HasGroupChanged = false;
-                bool tainted = false;
+            bool tainted = false;
-                if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
+            if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
-                    tainted = true;
+                tainted = true;
 
-                // FIXME: Detect whether it's really likely for AttachObject to throw an exception in the normal
+            // FIXME: Detect whether it's really likely for AttachObject to throw an exception in the normal
-                // course of events.  If not, then it's probably not worth trying to recover the situation
+            // course of events.  If not, then it's probably not worth trying to recover the situation
-                // since this is more likely to trigger further exceptions and confuse later debugging.  If
+            // since this is more likely to trigger further exceptions and confuse later debugging.  If
-                // exceptions can be thrown in expected error conditions (not NREs) then make this consistent
+            // exceptions can be thrown in expected error conditions (not NREs) then make this consistent
-                // since other normal error conditions will simply return false instead.
+            // since other normal error conditions will simply return false instead.
-                // This will throw if the attachment fails
+            // This will throw if the attachment fails
-                try
+            try
-                {
+            {
-                    AttachObjectInternal(sp, objatt, attachmentPt, false, false);
+                AttachObjectInternal(sp, objatt, attachmentPt, false, false, true);
-                }
-                catch (Exception e)
-                {
-                    m_log.ErrorFormat(
-                        "[ATTACHMENTS MODULE]: Failed to attach {0} {1} for {2}, exception {3}{4}",
-                        objatt.Name, objatt.UUID, sp.Name, e.Message, e.StackTrace);
-
-                    // Make sure the object doesn't stick around and bail
-                    sp.RemoveAttachment(objatt);
-                    m_scene.DeleteSceneObject(objatt, false);
-                    return null;
-                }
-
-                if (tainted)
-                    objatt.HasGroupChanged = true;
-
-                // Fire after attach, so we don't get messy perms dialogs
-                // 4 == AttachedRez
-                objatt.CreateScriptInstances(0, true, m_scene.DefaultScriptEngine, 4);
-                objatt.ResumeScripts();
-
-                // Do this last so that event listeners have access to all the effects of the attachment
-                m_scene.EventManager.TriggerOnAttach(objatt.LocalId, itemID, sp.UUID);
-
-                return objatt;
             }
+            catch (Exception e)
+            {
+                m_log.ErrorFormat(
+                    "[ATTACHMENTS MODULE]: Failed to attach {0} {1} for {2}, exception {3}{4}",
+                    objatt.Name, objatt.UUID, sp.Name, e.Message, e.StackTrace);
+
+                // Make sure the object doesn't stick around and bail
+                sp.RemoveAttachment(objatt);
+                m_scene.DeleteSceneObject(objatt, false);
+                return null;
+            }
+
+            if (tainted)
+                objatt.HasGroupChanged = true;
+
+            return objatt;
         }
 
         /// <summary>

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

@@ -293,13 +293,75 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
             // Check appearance status
             Assert.That(sp.Appearance.GetAttachments().Count, Is.EqualTo(1));
             Assert.That(sp.Appearance.GetAttachpoint(attItem.ID), Is.EqualTo((int)AttachmentPoint.Chest));
-
             Assert.That(scene.GetSceneObjectGroups().Count, Is.EqualTo(1));
 
             // Check events
             Assert.That(m_numberOfAttachEventsFired, Is.EqualTo(1));
         }
 
+        /// <summary>
+        /// Test wearing an attachment from inventory, as opposed to explicit choosing the rez point
+        /// </summary>
+        [Test]
+        public void TestWearAttachmentFromInventory()
+        {
+            TestHelpers.InMethod();
+//            TestHelpers.EnableLogging();
+
+            Scene scene = CreateTestScene();
+            UserAccount ua1 = UserAccountHelpers.CreateUserWithInventory(scene, 0x1);
+            ScenePresence sp = SceneHelpers.AddScenePresence(scene, ua1.PrincipalID);
+
+            InventoryItemBase attItem1 = CreateAttachmentItem(scene, ua1.PrincipalID, "att1", 0x10, 0x20);
+            InventoryItemBase attItem2 = CreateAttachmentItem(scene, ua1.PrincipalID, "att2", 0x11, 0x21);
+
+            {
+                scene.AttachmentsModule.RezSingleAttachmentFromInventory(sp, attItem1.ID, (uint)AttachmentPoint.Default);
+
+                // default attachment point is currently the left hand.
+                Assert.That(sp.HasAttachments(), Is.True);
+                List<SceneObjectGroup> attachments = sp.GetAttachments();
+                Assert.That(attachments.Count, Is.EqualTo(1));
+                SceneObjectGroup attSo = attachments[0];
+                Assert.That(attSo.Name, Is.EqualTo(attItem1.Name));
+                Assert.That(attSo.AttachmentPoint, Is.EqualTo((byte)AttachmentPoint.LeftHand));
+                Assert.That(attSo.IsAttachment);
+
+                // Check appearance status
+                Assert.That(sp.Appearance.GetAttachments().Count, Is.EqualTo(1));
+                Assert.That(sp.Appearance.GetAttachpoint(attItem1.ID), Is.EqualTo((int)AttachmentPoint.LeftHand));
+                Assert.That(scene.GetSceneObjectGroups().Count, Is.EqualTo(1));
+
+                // Check events
+                Assert.That(m_numberOfAttachEventsFired, Is.EqualTo(1));
+            }
+
+            // Test wearing a second attachment at the same position
+            // Until multiple attachments at one point is implemented, this will remove the first attachment
+            // This test relies on both attachments having the same default attachment point (in this case LeftHand 
+            // since none other has been set).
+            {
+                scene.AttachmentsModule.RezSingleAttachmentFromInventory(sp, attItem2.ID, (uint)AttachmentPoint.Default);
+
+                // default attachment point is currently the left hand.
+                Assert.That(sp.HasAttachments(), Is.True);
+                List<SceneObjectGroup> attachments = sp.GetAttachments();
+                Assert.That(attachments.Count, Is.EqualTo(1));
+                SceneObjectGroup attSo = attachments[0];
+                Assert.That(attSo.Name, Is.EqualTo(attItem2.Name));
+                Assert.That(attSo.AttachmentPoint, Is.EqualTo((byte)AttachmentPoint.LeftHand));
+                Assert.That(attSo.IsAttachment);
+
+                // Check appearance status
+                Assert.That(sp.Appearance.GetAttachments().Count, Is.EqualTo(1));
+                Assert.That(sp.Appearance.GetAttachpoint(attItem2.ID), Is.EqualTo((int)AttachmentPoint.LeftHand));
+                Assert.That(scene.GetSceneObjectGroups().Count, Is.EqualTo(1));
+
+                // Check events
+                Assert.That(m_numberOfAttachEventsFired, Is.EqualTo(3));
+            }
+        }
+
         /// <summary>
         /// Test specific conditions associated with rezzing a scripted attachment from inventory.
         /// </summary>