From 364816421985c052521cf7b444e124760c0a1025 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 19 Mar 2013 21:44:18 +0000 Subject: Prevent multiple instances of the same item ID being appended to an AvatarAppearance It looks like this was happening when AttachmentsModule.RezAttachments was doing a secondary set of each attachment to update with the asset ID (initially they only have the inventory ID). However, with multi-attach this was appending a second copy of the same attachment rather than updating the data that was already there. This commit requires both simulator and service to be updated. --- OpenSim/Framework/AvatarAppearance.cs | 78 +++++++++++++--------- .../Avatar/Attachments/AttachmentsModule.cs | 4 +- .../Avatar/AvatarFactory/AvatarFactoryModule.cs | 12 +++- 3 files changed, 60 insertions(+), 34 deletions(-) diff --git a/OpenSim/Framework/AvatarAppearance.cs b/OpenSim/Framework/AvatarAppearance.cs index 95e9667..494ae5e 100644 --- a/OpenSim/Framework/AvatarAppearance.cs +++ b/OpenSim/Framework/AvatarAppearance.cs @@ -459,45 +459,59 @@ namespace OpenSim.Framework if (attachpoint == 0) return false; - if (item == UUID.Zero) + lock (m_attachments) { - lock (m_attachments) + if (item == UUID.Zero) { if (m_attachments.ContainsKey(attachpoint)) { m_attachments.Remove(attachpoint); return true; } + + return false; } - - return false; - } - // 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 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; - } - - // check if this is an append or a replace, 0x80 marks it as an append - if ((attachpoint & 0x80) > 0) - { - // strip the append bit - int point = attachpoint & 0x7F; - AppendAttachment(new AvatarAttachment(point, item, asset)); - } - else - { - ReplaceAttachment(new AvatarAttachment(attachpoint,item, asset)); + // 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 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) + { +// m_log.DebugFormat( +// "[AVATAR APPEARANCE]: Found existing attachment for {0}, asset {1} at point {2}", +// existingAttachment.ItemID, existingAttachment.AssetID, existingAttachment.AttachPoint); + + if (existingAttachment.AssetID != UUID.Zero && existingAttachment.AttachPoint == (attachpoint & 0x7F)) + { + m_log.DebugFormat( + "[AVATAR APPEARANCE]: Ignoring attempt to attach an already attached item {0} at point {1}", + item, attachpoint); + + return false; + } + else + { + // Remove it here so that the later append does not add a second attachment but we still update + // the assetID + DetachAttachment(existingAttachment.ItemID); + } + } + + // check if this is an append or a replace, 0x80 marks it as an append + if ((attachpoint & 0x80) > 0) + { + // strip the append bit + int point = attachpoint & 0x7F; + AppendAttachment(new AvatarAttachment(point, item, asset)); + } + else + { + ReplaceAttachment(new AvatarAttachment(attachpoint,item, asset)); + } } return true; @@ -547,6 +561,10 @@ namespace OpenSim.Framework int index = kvp.Value.FindIndex(delegate(AvatarAttachment a) { return a.ItemID == itemID; }); if (index >= 0) { +// m_log.DebugFormat( +// "[AVATAR APPEARANCE]: Detaching attachment {0}, index {1}, point {2}", +// m_attachments[kvp.Key][index].ItemID, index, m_attachments[kvp.Key][index].AttachPoint); + // Remove it from the list of attachments at that attach point m_attachments[kvp.Key].RemoveAt(index); diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index 72ba3cf..ad17aa9 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -547,8 +547,8 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments } // m_log.DebugFormat( -// "[ATTACHMENTS MODULE]: Detaching object {0} {1} for {2} in {3}", -// so.Name, so.LocalId, sp.Name, m_scene.Name); +// "[ATTACHMENTS MODULE]: Detaching object {0} {1} (FromItemID {2}) for {3} in {4}", +// so.Name, so.LocalId, so.FromItemID, sp.Name, m_scene.Name); // Scripts MUST be snapshotted before the object is // removed from the scene because doing otherwise will diff --git a/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs b/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs index 00d1fd8..ff5bf9f 100644 --- a/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/AvatarFactory/AvatarFactoryModule.cs @@ -326,7 +326,7 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory public void QueueAppearanceSave(UUID agentid) { - // m_log.WarnFormat("[AVFACTORY]: Queue appearance save for {0}", agentid); +// m_log.DebugFormat("[AVFACTORY]: Queueing appearance save for {0}", agentid); // 10000 ticks per millisecond, 1000 milliseconds per second long timestamp = DateTime.Now.Ticks + Convert.ToInt64(m_savetime * 1000 * 10000); @@ -529,7 +529,7 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory return; } - // m_log.WarnFormat("[AVFACTORY] avatar {0} save appearance",agentid); +// m_log.DebugFormat("[AVFACTORY]: Saving appearance for avatar {0}", agentid); // This could take awhile since it needs to pull inventory // We need to do it at the point of save so that there is a sufficient delay for any upload of new body part/shape @@ -538,6 +538,14 @@ namespace OpenSim.Region.CoreModules.Avatar.AvatarFactory // multiple save requests. SetAppearanceAssets(sp.UUID, sp.Appearance); +// List attachments = sp.Appearance.GetAttachments(); +// foreach (AvatarAttachment att in attachments) +// { +// m_log.DebugFormat( +// "[AVFACTORY]: For {0} saving attachment {1} at point {2}", +// sp.Name, att.ItemID, att.AttachPoint); +// } + m_scene.AvatarService.SetAppearance(agentid, sp.Appearance); // Trigger this here because it's the final step in the set/queue/save process for appearance setting. -- cgit v1.1