From a7a9a8a614549c7492e4954189e9f4df2473ca1e Mon Sep 17 00:00:00 2001
From: Justin Clark-Casey (justincc)
Date: Mon, 18 Mar 2013 20:42:08 +0000
Subject: 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.
---
.../Avatar/Attachments/AttachmentsModule.cs | 178 +++++++++++----------
.../Attachments/Tests/AttachmentsModuleTests.cs | 64 +++++++-
2 files changed, 153 insertions(+), 89 deletions(-)
(limited to 'OpenSim/Region/CoreModules/Avatar')
diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
index b6a7481..2092d6f 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
+++ b/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))
- {
- m_scene.EventManager.TriggerOnAttach(group.LocalId, group.FromItemID, sp.UUID);
- return true;
- }
-
- return false;
+ return AttachObjectInternal(sp, group, attachmentPt, silent, temp, false);
}
-
- private bool AttachObjectInternal(IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool silent, bool temp)
+
+ ///
+ /// Internal method which actually does all the work for attaching an object.
+ ///
+ /// The object attached.
+ ///
+ /// The object to attach.
+ ///
+ ///
+ ///
+ /// If true then scripts are resumed on the attached object.
+ 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 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}",
-// (AttachmentPoint)AttachmentPt, itemID, sp.Name);
+// "[ATTACHMENTS MODULE]: RezSingleAttachmentFromInventory to point {0} from item {1} for {2} in {3}",
+// (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 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.
- objatt.HasGroupChanged = false;
- bool tainted = false;
- if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
- tainted = true;
-
- // 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
- // 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
- // since other normal error conditions will simply return false instead.
- // This will throw if the attachment fails
- try
- {
- AttachObjectInternal(sp, objatt, attachmentPt, false, false);
- }
- 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;
+ // HasGroupChanged is being set from within RezObject. Ideally it would be set by the caller.
+ objatt.HasGroupChanged = false;
+ bool tainted = false;
+ if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
+ tainted = true;
+
+ // 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
+ // 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
+ // since other normal error conditions will simply return false instead.
+ // This will throw if the attachment fails
+ try
+ {
+ 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);
- // Fire after attach, so we don't get messy perms dialogs
- // 4 == AttachedRez
- objatt.CreateScriptInstances(0, true, m_scene.DefaultScriptEngine, 4);
- objatt.ResumeScripts();
+ // Make sure the object doesn't stick around and bail
+ sp.RemoveAttachment(objatt);
+ m_scene.DeleteSceneObject(objatt, false);
+ return null;
+ }
- // 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);
+ if (tainted)
+ objatt.HasGroupChanged = true;
- return objatt;
- }
+ return objatt;
}
///
diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
index 0ee01c7..624adcf 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
@@ -293,7 +293,6 @@ 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
@@ -301,6 +300,69 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
}
///
+ /// Test wearing an attachment from inventory, as opposed to explicit choosing the rez point
+ ///
+ [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 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 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));
+ }
+ }
+
+ ///
/// Test specific conditions associated with rezzing a scripted attachment from inventory.
///
[Test]
--
cgit v1.1