From 32444d98cb13423fdf8c874e4fbb7ea17670d7c5 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 31 Aug 2011 16:29:51 +0100 Subject: Make SP.Attachments available as sp.GetAttachments() instead. The approach here, as in other parts of OpenSim, is to return a copy of the list rather than the attachments list itself This prevents callers from forgetting to lock the list when they read it, as was happening in various parts of the codebase. It also improves liveness. This might improve attachment anomolies when performing region crossings. --- .../Avatar/Attachments/AttachmentsModule.cs | 19 +++---- .../Attachments/Tests/AttachmentsModuleTests.cs | 10 ++-- .../EntityTransfer/EntityTransferModule.cs | 61 ++++++++++++---------- .../Scripting/WorldComm/WorldCommModule.cs | 12 +++-- .../Region/Framework/Interfaces/IScenePresence.cs | 7 ++- OpenSim/Region/Framework/Scenes/ScenePresence.cs | 51 +++++++++++++----- .../Region/OptionalModules/World/NPC/NPCModule.cs | 10 ++-- 7 files changed, 98 insertions(+), 72 deletions(-) (limited to 'OpenSim') diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index f4bc495..9e5ce8f 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -136,10 +136,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments public void SaveChangedAttachments(IScenePresence sp) { - // Need to copy this list because DetachToInventoryPrep mods it - List attachments = new List(sp.Attachments); - - foreach (SceneObjectGroup grp in attachments) + foreach (SceneObjectGroup grp in sp.GetAttachments()) { if (grp.HasGroupChanged) // Resizer scripts? { @@ -273,14 +270,12 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments // Remove any previous attachments UUID itemID = UUID.Zero; - foreach (SceneObjectGroup grp in sp.Attachments) - { - if (grp.AttachmentPoint == attachmentPt) - { - itemID = grp.GetFromItemID(); - break; - } - } + + List attachments = sp.GetAttachments(attachmentPt); + + // At the moment we can only deal with a single attachment + if (attachments.Count != 0) + itemID = attachments[0].GetFromItemID(); if (itemID != UUID.Zero) DetachSingleAttachmentToInv(itemID, sp); diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs index b0146a1..363e258 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs @@ -106,7 +106,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests // Check status on scene presence Assert.That(m_presence.HasAttachments(), Is.True); - List attachments = m_presence.Attachments; + List attachments = m_presence.GetAttachments(); Assert.That(attachments.Count, Is.EqualTo(1)); SceneObjectGroup attSo = attachments[0]; Assert.That(attSo.Name, Is.EqualTo(attName)); @@ -140,7 +140,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests // Check scene presence status Assert.That(m_presence.HasAttachments(), Is.True); - List attachments = m_presence.Attachments; + List attachments = m_presence.GetAttachments(); Assert.That(attachments.Count, Is.EqualTo(1)); SceneObjectGroup attSo = attachments[0]; Assert.That(attSo.Name, Is.EqualTo(attName)); @@ -174,7 +174,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests // Check scene presence status Assert.That(m_presence.HasAttachments(), Is.False); - List attachments = m_presence.Attachments; + List attachments = m_presence.GetAttachments(); Assert.That(attachments.Count, Is.EqualTo(0)); // Check appearance status @@ -208,7 +208,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests // Check status on scene presence Assert.That(m_presence.HasAttachments(), Is.False); - List attachments = m_presence.Attachments; + List attachments = m_presence.GetAttachments(); Assert.That(attachments.Count, Is.EqualTo(0)); // Check item status @@ -237,7 +237,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests ScenePresence presence = SceneHelpers.AddScenePresence(scene, acd); Assert.That(presence.HasAttachments(), Is.True); - List attachments = presence.Attachments; + List attachments = presence.GetAttachments(); Assert.That(attachments.Count, Is.EqualTo(1)); SceneObjectGroup attSo = attachments[0]; diff --git a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs index 7963e53..c24cc17 100644 --- a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs +++ b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs @@ -208,7 +208,7 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer sp.ControllingClient.SendLocalTeleport(position, lookAt, teleportFlags); sp.Teleport(position); - foreach (SceneObjectGroup grp in sp.Attachments) + foreach (SceneObjectGroup grp in sp.GetAttachments()) sp.Scene.EventManager.TriggerOnScriptChangedEvent(grp.LocalId, (uint)Changed.TELEPORT); } else // Another region possibly in another simulator @@ -559,11 +559,11 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer protected virtual void AgentHasMovedAway(ScenePresence sp, bool logout) { - foreach (SceneObjectGroup sop in sp.Attachments) + foreach (SceneObjectGroup sop in sp.GetAttachments()) { sop.Scene.DeleteSceneObject(sop, true); } - sp.Attachments.Clear(); + sp.ClearAttachments(); } protected void KillEntity(Scene scene, uint localID) @@ -1764,34 +1764,33 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer protected bool CrossAttachmentsIntoNewRegion(GridRegion destination, ScenePresence sp, bool silent) { - List m_attachments = sp.Attachments; - lock (m_attachments) + List m_attachments = sp.GetAttachments(); + + // Validate + foreach (SceneObjectGroup gobj in m_attachments) { - // Validate - foreach (SceneObjectGroup gobj in m_attachments) - { - if (gobj == null || gobj.IsDeleted) - return false; - } + if (gobj == null || gobj.IsDeleted) + return false; + } - foreach (SceneObjectGroup gobj in m_attachments) + foreach (SceneObjectGroup gobj in m_attachments) + { + // If the prim group is null then something must have happened to it! + if (gobj != null && gobj.RootPart != null) { - // If the prim group is null then something must have happened to it! - if (gobj != null && gobj.RootPart != null) - { - // Set the parent localID to 0 so it transfers over properly. - gobj.RootPart.SetParentLocalId(0); - gobj.AbsolutePosition = gobj.RootPart.AttachedPos; - gobj.IsAttachment = false; - //gobj.RootPart.LastOwnerID = gobj.GetFromAssetID(); - m_log.DebugFormat("[ENTITY TRANSFER MODULE]: Sending attachment {0} to region {1}", gobj.UUID, destination.RegionName); - CrossPrimGroupIntoNewRegion(destination, gobj, silent); - } + // Set the parent localID to 0 so it transfers over properly. + gobj.RootPart.SetParentLocalId(0); + gobj.AbsolutePosition = gobj.RootPart.AttachedPos; + gobj.IsAttachment = false; + //gobj.RootPart.LastOwnerID = gobj.GetFromAssetID(); + m_log.DebugFormat("[ENTITY TRANSFER MODULE]: Sending attachment {0} to region {1}", gobj.UUID, destination.RegionName); + CrossPrimGroupIntoNewRegion(destination, gobj, silent); } - m_attachments.Clear(); - - return true; } + + sp.ClearAttachments(); + + return true; } #endregion @@ -1840,7 +1839,9 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer int i = 0; if (sp.InTransitScriptStates.Count > 0) { - sp.Attachments.ForEach(delegate(SceneObjectGroup sog) + List attachments = sp.GetAttachments(); + + foreach (SceneObjectGroup sog in attachments) { if (i < sp.InTransitScriptStates.Count) { @@ -1849,8 +1850,10 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer sog.ResumeScripts(); } else - m_log.ErrorFormat("[ENTITY TRANSFER MODULE]: InTransitScriptStates.Count={0} smaller than Attachments.Count={1}", sp.InTransitScriptStates.Count, sp.Attachments.Count); - }); + m_log.ErrorFormat( + "[ENTITY TRANSFER MODULE]: InTransitScriptStates.Count={0} smaller than Attachments.Count={1}", + sp.InTransitScriptStates.Count, attachments.Count); + } sp.InTransitScriptStates.Clear(); } diff --git a/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs b/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs index 22352f5..057500c 100644 --- a/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs +++ b/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs @@ -283,7 +283,7 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm } /// - /// Delivers the message to. + /// Delivers the message to a scene entity. /// /// /// Target. @@ -314,17 +314,19 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm m_scene.SimChatBroadcast(Utils.StringToBytes(msg), ChatTypeEnum.Owner, 0, pos, name, id, false); } - List attachments = sp.Attachments; + List attachments = sp.GetAttachments(); + // Nothing left to do if (attachments == null) return true; // Get uuid of attachments List targets = new List(); - foreach ( SceneObjectGroup sog in attachments ) + foreach (SceneObjectGroup sog in attachments) { targets.Add(sog.UUID); } + // Need to check each attachment foreach (ListenerInfo li in m_listenerManager.GetListeners(UUID.Zero, channel, name, id, msg)) { @@ -334,9 +336,10 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm if (m_scene.GetSceneObjectPart(li.GetHostID()) == null) continue; - if ( targets.Contains(li.GetHostID())) + if (targets.Contains(li.GetHostID())) QueueMessage(new ListenerInfo(li, name, id, msg)); } + return true; } @@ -363,6 +366,7 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm break; } } + return true; } diff --git a/OpenSim/Region/Framework/Interfaces/IScenePresence.cs b/OpenSim/Region/Framework/Interfaces/IScenePresence.cs index b07c821..788b36f 100644 --- a/OpenSim/Region/Framework/Interfaces/IScenePresence.cs +++ b/OpenSim/Region/Framework/Interfaces/IScenePresence.cs @@ -58,10 +58,13 @@ namespace OpenSim.Region.Framework.Interfaces /// /// The scene objects attached to this avatar. /// + /// + /// A copy of the list. + /// /// /// Do not change this list directly - use methods such as - /// AddAttachment() and RemoveAttachment(). Lock this list when performing any read operations upon it. + /// AddAttachment() and RemoveAttachment(). /// - List Attachments { get; } + List GetAttachments(); } } \ No newline at end of file diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 0a91989..f5c72e1 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -114,10 +114,13 @@ namespace OpenSim.Region.Framework.Scenes } protected ScenePresenceAnimator m_animator; - public List Attachments - { - get { return m_attachments; } - } + /// + /// Attachments recorded on this avatar. + /// + /// + /// TODO: For some reason, we effectively have a list both here and in Appearance. Need to work out if this is + /// necessary. + /// protected List m_attachments = new List(); private Dictionary scriptedcontrols = new Dictionary(); @@ -940,15 +943,18 @@ namespace OpenSim.Region.Framework.Scenes // and it has already rezzed the attachments and started their scripts. // We do the following only for non-login agents, because their scripts // haven't started yet. - if (wasChild && Attachments != null && Attachments.Count > 0) + lock (m_attachments) { - m_log.DebugFormat("[SCENE PRESENCE]: Restarting scripts in attachments..."); - // Resume scripts - Attachments.ForEach(delegate(SceneObjectGroup sog) + if (wasChild && m_attachments != null && m_attachments.Count > 0) { - sog.RootPart.ParentGroup.CreateScriptInstances(0, false, m_scene.DefaultScriptEngine, GetStateSource()); - sog.ResumeScripts(); - }); + m_log.DebugFormat("[SCENE PRESENCE]: Restarting scripts in attachments..."); + // Resume scripts + foreach (SceneObjectGroup sog in m_attachments) + { + sog.RootPart.ParentGroup.CreateScriptInstances(0, false, m_scene.DefaultScriptEngine, GetStateSource()); + sog.ResumeScripts(); + } + } } // send the animations of the other presences to me @@ -3472,9 +3478,19 @@ namespace OpenSim.Region.Framework.Scenes m_attachments.Add(gobj); } } - + /// - /// Get the scene object attached to the given point. + /// Get all the presence's attachments. + /// + /// A copy of the list which contains the attachments. + public List GetAttachments() + { + lock (m_attachments) + return new List(m_attachments); + } + + /// + /// Get the scene objects attached to the given point. /// /// /// Returns an empty list if there were no attachments at the point. @@ -3521,6 +3537,15 @@ namespace OpenSim.Region.Framework.Scenes m_attachments.Remove(gobj); } + /// + /// Clear all attachments + /// + public void ClearAttachments() + { + lock (m_attachments) + m_attachments.Clear(); + } + public bool ValidateAttachments() { lock (m_attachments) diff --git a/OpenSim/Region/OptionalModules/World/NPC/NPCModule.cs b/OpenSim/Region/OptionalModules/World/NPC/NPCModule.cs index 79c79e4..e58dca2 100644 --- a/OpenSim/Region/OptionalModules/World/NPC/NPCModule.cs +++ b/OpenSim/Region/OptionalModules/World/NPC/NPCModule.cs @@ -144,14 +144,10 @@ namespace OpenSim.Region.OptionalModules.World.NPC return false; // FIXME: An extremely bad bit of code that reaches directly into the attachments list and manipulates it - List attachments = sp.Attachments; - lock (attachments) - { - foreach (SceneObjectGroup att in attachments) - scene.DeleteSceneObject(att, false); + foreach (SceneObjectGroup att in sp.GetAttachments()) + scene.DeleteSceneObject(att, false); - attachments.Clear(); - } + sp.ClearAttachments(); AvatarAppearance npcAppearance = new AvatarAppearance(appearance, true); sp.Appearance = npcAppearance; -- cgit v1.1