Remove prefab undo cache dependency on CreatePrefab use case

Code/Framework/AzToolsFramework/AzToolsFramework/API/ToolsApplicationAPI.h

@@ -239,6 +239,11 @@ namespace AzToolsFramework
         */
         virtual int RemoveDirtyEntity(AZ::EntityId target) = 0;
 
+        /*!
+         * Clears the dirty entity set.
+         */
+        virtual void ClearDirtyEntities() = 0;
+
         /*!
          * \return true if an undo/redo operation is in progress.
          */

Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp

@@ -1354,6 +1354,11 @@ namespace AzToolsFramework
         return static_cast<int>(m_dirtyEntities.erase(entityId));
     }
 
+    void ToolsApplication::ClearDirtyEntities()
+    {
+        m_dirtyEntities.clear();
+    }
+
     void ToolsApplication::UndoPressed()
     {
         if (m_undoStack)

Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.h

@@ -85,6 +85,7 @@ namespace AzToolsFramework
 
         void AddDirtyEntity(AZ::EntityId entityId) override;
         int RemoveDirtyEntity(AZ::EntityId entityId) override;
+        void ClearDirtyEntities() override;
         bool IsDuringUndoRedo() override { return m_isDuringUndoRedo; }
         void UndoPressed() override;
         void RedoPressed() override;

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp

@@ -276,18 +276,14 @@ namespace AzToolsFramework
             PrefabDomValueReference linkPatchesReference =
                 PrefabDomUtils::FindPrefabDomValue(linkDom, PrefabDomUtils::PatchesName);
 
-            // This logic only covers addition of patches. If patches already exists, the given list of patches must be appended to them.
+            /*
-            if (!linkPatchesReference.has_value())
+            If the original allocator the patches were created with gets destroyed, then the patches would become garbage in the
-            {
+            linkDom. Since we cannot guarantee the lifecycle of the patch allocators, we are doing a copy of the patches here to
-                /*
+            associate them with the linkDom's allocator.
-                If the original allocator the patches were created with gets destroyed, then the patches would become garbage in the
+            */
-                linkDom. Since we cannot guarantee the lifecycle of the patch allocators, we are doing a copy of the patches here to
+            PrefabDom patchesCopy;
-                associate them with the linkDom's allocator.
+            patchesCopy.CopyFrom(patches, linkDom.GetAllocator());
-                */
+            linkDom.AddMember(rapidjson::StringRef(PrefabDomUtils::PatchesName), patchesCopy, linkDom.GetAllocator());
-                PrefabDom patchesCopy;
-                patchesCopy.CopyFrom(patches, linkDom.GetAllocator());
-                linkDom.AddMember(rapidjson::StringRef(PrefabDomUtils::PatchesName), patchesCopy, linkDom.GetAllocator());
-            }
         }
     }
 }

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.cpp

@@ -234,5 +234,10 @@ namespace AzToolsFramework
             }
         }
 
+        PrefabDomValueConstReference Link::GetLinkPatches()
+        {
+            return PrefabDomUtils::FindPrefabDomValue(m_linkDom, PrefabDomUtils::PatchesName);
+        }
+
     } // namespace Prefab
 } // namespace AzToolsFramework

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.h

@@ -79,6 +79,8 @@ namespace AzToolsFramework
              */
             void AddLinkIdToInstanceDom(PrefabDomValue& instanceDomValue);
 
+            PrefabDomValueConstReference GetLinkPatches();
+
         private:
 
             /**

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp

@@ -33,8 +33,6 @@
 #include <AzToolsFramework/Prefab/PrefabUndoHelpers.h>
 #include <AzToolsFramework/ToolsComponents/TransformComponent.h>
 
-#include <QString>
-
 namespace AzToolsFramework
 {
     namespace Prefab
@@ -98,9 +96,13 @@ namespace AzToolsFramework
                         AZStd::string("Could not create a new prefab out of the entities provided - invalid selection."));
                 }
 
+                AZStd::unordered_map<AZ::EntityId, AZStd::string> oldEntityAliases;
+
                 // Detach the retrieved entities
                 for (AZ::Entity* entity : entities)
                 {
+                    AZ::EntityId entityId = entity->GetId();
+                    oldEntityAliases.emplace(entityId, commonRootEntityOwningInstance->get().GetEntityAlias(entityId)->get());
                     commonRootEntityOwningInstance->get().DetachEntity(entity->GetId()).release();
                 }
 
@@ -110,15 +112,18 @@ namespace AzToolsFramework
                 {
                     AZStd::unique_ptr<Instance> outInstance = commonRootEntityOwningInstance->get().DetachNestedInstance(nestedInstance->GetInstanceAlias());
 
-                    auto linkRef = m_prefabSystemComponentInterface->FindLink(nestedInstance->GetLinkId());
+                    LinkId detachingInstanceLinkId = nestedInstance->GetLinkId();
+                    auto linkRef = m_prefabSystemComponentInterface->FindLink(detachingInstanceLinkId);
+                    AZ_Assert(linkRef.has_value(), "Unable to find link with id '%llu' during prefab creation.", detachingInstanceLinkId);
 
-                    if (linkRef.has_value())
+                    PrefabDomValueConstReference linkPatches = linkRef->get().GetLinkPatches();
-                    {
+                    AZ_Assert(
-                        PrefabDom oldLinkPatches;
+                        linkPatches.has_value(), "Unable to get patches on link with id '%llu' during prefab creation.",
-                        oldLinkPatches.CopyFrom(linkRef->get().GetLinkDom(), oldLinkPatches.GetAllocator());
+                        detachingInstanceLinkId);
 
-                        nestedInstanceLinkPatchesMap.emplace(nestedInstance, AZStd::move(oldLinkPatches));
+                    PrefabDom linkPatchesCopy;
-                    }
+                    linkPatchesCopy.CopyFrom(linkPatches->get(), linkPatchesCopy.GetAllocator());
+                    nestedInstanceLinkPatchesMap.emplace(nestedInstance, AZStd::move(linkPatchesCopy));
                     
                     RemoveLink(outInstance, commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch());
 
@@ -182,6 +187,24 @@ namespace AzToolsFramework
                     if (nestedInstanceLinkPatchesMap.contains(nestedInstance.get()))
                     {
                         previousPatch = AZStd::move(nestedInstanceLinkPatchesMap[nestedInstance.get()]);
+                        rapidjson::StringBuffer buffer;
+                        rapidjson::Writer<rapidjson::StringBuffer> writer(buffer);
+                        previousPatch.Accept(writer);
+                        QString previousPatchString(buffer.GetString());
+
+                        for (AZ::Entity* entity : entities)
+                        {
+                            AZ::EntityId entityId = entity->GetId();
+                            AZStd::string oldEntityAlias = oldEntityAliases[entityId];
+                            EntityAliasOptionalReference newEntityAlias = instanceToCreate->get().GetEntityAlias(entityId);
+                            AZ_Assert(
+                                newEntityAlias.has_value(),
+                                "Could not fetch entity alias for entity with id '%llu' during prefab creation.",
+                                static_cast<AZ::u64>(entityId));
+                            ReplaceOldAliases(previousPatchString, oldEntityAlias, newEntityAlias->get());
+                        }
+
+                        previousPatch.Parse(previousPatchString.toUtf8().constData());
                     }
 
                     // These link creations shouldn't be undone because that would put the template in a non-usable state if a user
@@ -203,36 +226,23 @@ namespace AzToolsFramework
                         m_instanceToTemplateInterface->GeneratePatch(reparentPatch, containerEntityDomBefore, containerEntityDomAfter);
                         m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(reparentPatch, nestedInstanceContainerEntityId);
 
-                        // Update the cache - this prevents these changes from being stored in the regular undo/redo nodes as a separate step
+                        // We ar not parenting this undo node to the undo batch because we don't want the user to undo these changes
-                        m_prefabUndoCache.Store(nestedInstanceContainerEntityId, AZStd::move(containerEntityDomAfter));
+                        // so that the newly created template and link remain unaffected for supporting instantiating the template later.
-
+                        PrefabUndoLinkUpdate linkUpdate = PrefabUndoLinkUpdate(AZStd::to_string(static_cast<AZ::u64>(nestedInstanceContainerEntityId)));
-                        // Save these changes as patches to the link
+                        linkUpdate.Capture(reparentPatch, nestedInstance->GetLinkId());
-                        PrefabUndoLinkUpdate* linkUpdate = aznew PrefabUndoLinkUpdate(AZStd::to_string(static_cast<AZ::u64>(nestedInstanceContainerEntityId)));
+                        linkUpdate.Redo();
-                        linkUpdate->SetParent(undoBatch.GetUndoBatch());
-                        linkUpdate->Capture(reparentPatch, nestedInstance->GetLinkId());
-
-                        linkUpdate->Redo();
                     }
                 });
-
+                
                 // Create a link between the templates of the newly created instance and the instance it's being parented under.
                 CreateLink(
                     instanceToCreate->get(), commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch(),
                     AZStd::move(patch));
 
-                for (AZ::Entity* topLevelEntity : topLevelEntities)
+                // This clears any entities marked as dirty due to reparenting of entities during the process of creating a prefab.
-                {
+                // We are doing this so that the changes in those enities are not queued up twice for propagation.
-                    AZ::EntityId topLevelEntityId = topLevelEntity->GetId();
+                AzToolsFramework::ToolsApplicationRequestBus::Broadcast(
-                    if (topLevelEntityId.IsValid())
+                    &AzToolsFramework::ToolsApplicationRequestBus::Events::ClearDirtyEntities);
-                    {
-                        m_prefabUndoCache.UpdateCache(topLevelEntity->GetId());
-
-                        // Parenting entities would mark entities as dirty. But we want to unmark the top level entities as dirty because
-                        // if we don't, the template created would be updated and cause issues with undo operation followed by instantiation.
-                        ToolsApplicationRequests::Bus::Broadcast(
-                            &ToolsApplicationRequests::Bus::Events::RemoveDirtyEntity, topLevelEntity->GetId());
-                    }
-                }
                 
                 // Select Container Entity
                 {
@@ -824,15 +834,7 @@ namespace AzToolsFramework
                     // This will cover both cases where an alias could be used in a normal entity vs. an instance
                     for (auto aliasMapIter : oldAliasToNewAliasMap)
                     {
-                        QString oldAliasQuotes = QString("\"%1\"").arg(aliasMapIter.first.c_str());
+                        ReplaceOldAliases(newEntityDomString, aliasMapIter.first, aliasMapIter.second);
-                        QString newAliasQuotes = QString("\"%1\"").arg(aliasMapIter.second.c_str());
-
-                        newEntityDomString.replace(oldAliasQuotes, newAliasQuotes);
-
-                        QString oldAliasPathRef = QString("/%1").arg(aliasMapIter.first.c_str());
-                        QString newAliasPathRef = QString("/%1").arg(aliasMapIter.second.c_str());
-
-                        newEntityDomString.replace(oldAliasPathRef, newAliasPathRef);
                     }
 
                     // Create the new Entity DOM from parsing the JSON string
@@ -1233,5 +1235,18 @@ namespace AzToolsFramework
 
             return true;
         }
+
+        void PrefabPublicHandler::ReplaceOldAliases(QString& stringToReplace, AZStd::string_view oldAlias, AZStd::string_view newAlias)
+        {
+            QString oldAliasQuotes = QString("\"%1\"").arg(oldAlias.data());
+            QString newAliasQuotes = QString("\"%1\"").arg(newAlias.data());
+
+            stringToReplace.replace(oldAliasQuotes, newAliasQuotes);
+
+            QString oldAliasPathRef = QString("/%1").arg(oldAlias.data());
+            QString newAliasPathRef = QString("/%1").arg(newAlias.data());
+
+            stringToReplace.replace(oldAliasPathRef, newAliasPathRef);
+        }
     } // namespace Prefab
 } // namespace AzToolsFramework

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h

@@ -14,12 +14,15 @@
 
 #include <AzCore/Math/Vector3.h>
 #include <AzCore/Memory/SystemAllocator.h>
+#include <AzCore/std/string/string_view.h>
 
 #include <AzToolsFramework/Prefab/Instance/Instance.h>
 #include <AzToolsFramework/Prefab/PrefabPublicInterface.h>
 #include <AzToolsFramework/Prefab/PrefabSystemComponentInterface.h>
 #include <AzToolsFramework/Prefab/PrefabUndoCache.h>
 
+#include <QString>
+
 namespace AzToolsFramework
 {
     using EntityList = AZStd::vector<AZ::Entity*>;
@@ -130,6 +133,8 @@ namespace AzToolsFramework
             bool IsCyclicalDependencyFound(
                 InstanceOptionalConstReference instance, const AZStd::unordered_set<AZ::IO::Path>& templateSourcePaths);
 
+            void ReplaceOldAliases(QString& stringToReplace, AZStd::string_view oldAlias, AZStd::string_view newAlias);
+
             static Instance* GetParentInstance(Instance* instance);
             static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant);
             static void GenerateContainerEntityTransform(const EntityList& topLevelEntities, AZ::Vector3& translation, AZ::Quaternion& rotation);