From 6706e189d5b1c04e4931c96b1a9c233f3606b71a Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 6 Mar 2013 23:11:32 +0000 Subject: minor: remove some completely unused string local vars added recently in commit 984faf2 --- OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs | 2 -- 1 file changed, 2 deletions(-) (limited to 'OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs') diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index 3ccf9f4..b6a7481 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -324,7 +324,6 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments // Remove any previous attachments List existingAttachments = sp.GetAttachments(attachmentPt); - string existingAttachmentScriptState = null; // At the moment we can only deal with a single attachment if (existingAttachments.Count != 0 && existingAttachments[0].FromItemID != UUID.Zero) @@ -849,7 +848,6 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments // Remove any previous attachments List attachments = sp.GetAttachments(attachmentPt); - string previousAttachmentScriptedState = null; // At the moment we can only deal with a single attachment if (attachments.Count != 0) -- cgit v1.1 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 +++++++++++---------- 1 file changed, 90 insertions(+), 88 deletions(-) (limited to 'OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs') 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; } /// -- cgit v1.1 From 3611d33b00650ccc71994b331e4c6595f95d3131 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Mon, 18 Mar 2013 22:04:27 +0000 Subject: Improve rejection of any attempt to reattach an object that is already attached. This also adds/extends regression tests for wearing attachments directly for the scene and attempting to reattach/rewear already attached objects. --- .../Avatar/Attachments/AttachmentsModule.cs | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs') diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index 2092d6f..1c28f49 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -305,6 +305,15 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments private bool AttachObjectInternal( IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool silent, bool temp, bool resumeScripts) { + if (sp.GetAttachments().Contains(group)) + { +// m_log.WarnFormat( +// "[ATTACHMENTS MODULE]: Ignoring request to attach {0} {1} to {2} on {3} since it's already attached", +// group.Name, group.LocalId, sp.Name, AttachmentPt); + + return false; + } + // m_log.DebugFormat( // "[ATTACHMENTS MODULE]: Attaching object {0} {1} to {2} point {3} from ground (silent = {4})", // group.Name, group.LocalId, sp.Name, attachmentPt, silent); @@ -318,15 +327,6 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments return false; } - if (sp.GetAttachments(attachmentPt).Contains(group)) - { -// m_log.WarnFormat( -// "[ATTACHMENTS MODULE]: Ignoring request to attach {0} {1} to {2} on {3} since it's already attached", -// group.Name, group.LocalId, sp.Name, AttachmentPt); - - return false; - } - Vector3 attachPos = group.AbsolutePosition; // TODO: this short circuits multiple attachments functionality in LL viewer 2.1+ and should @@ -336,13 +336,13 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments // 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) + if (attachmentPt != (uint)AttachmentPoint.Default && attachmentPt != group.AttachmentPoint) { attachPos = Vector3.Zero; } - // AttachmentPt 0 means the client chose to 'wear' the attachment. - if (attachmentPt == 0) + // AttachmentPt 0 (default) means the client chose to 'wear' the attachment. + if (attachmentPt == (uint)AttachmentPoint.Default) { // Check object for stored attachment point attachmentPt = group.AttachmentPoint; -- cgit v1.1