From 4ee2f341dc0dc709aedb446b57d5cca61b86160d Mon Sep 17 00:00:00 2001 From: Nicholas Van Sickle Date: Fri, 19 Nov 2021 10:05:19 -0800 Subject: [PATCH] Fix several Prefab outliner ordering issues (#5747) * Fix several Prefab outliner ordering issues This change does a few things to address instability in entity order when prefabs are enabled: - Changes ordering behavior on entity add to always be "insert after the last selected sibling of the new entity, or at the end if there is no selected sibling" - Adds some logic to ensure selection stays frozen during prefab propagation to allow this behavior to be used - Alters delta generation in `PrefabPublicHandler::CreateEntity` to use the template instead of reserializing the DOM - this avoids a whole bunch of patching issues caused by EditorEntitySortComponent doing post-hoc order fix-up and should generally be safer/faster as we're producing patches for the actual target for those patches - Because the duplicate action is DOM-driven, and there's some thorniness around making changes that will affect the template during propagation, this adds `PrefabPublicHandler::AddNewEntityToSortOrder` to directly patch the DOM for the duplicate case Two bits of this come from patches from the incredibly helpful @AMZN-daimini - Alters `PrefabPublicHandler::GenerateUndoNodesForEntityChangeAndUpdateCache` behavior for determining what's an override: we now check to see if we're part of the current focused instance but *not* owned by the focus instance directly. This lets entity order for nested prefabs get saved to the owning prefab instead of as an override. I'm putting this up for discussion without a feature flag gating it, but we may wish to make this a toggle or disable it outright for stabilization (in which case entity order won't be saved to the owning prefab) - Adds a custom serializer for EditorEntitySortComponent. This isn't strictly necessary now, but it was very useful for debugging and ended up receiving much more manual testing its migration path and save/load path more than I've tested without using the serializer. Signed-off-by: Nicholas Van Sickle * Add missing serializers Signed-off-by: Nicholas Van Sickle * Address some review feedback Signed-off-by: Nicholas Van Sickle * Generate patch in `PrefabPublicHandler::CreateEntity` using the instance's fully evaluated template in-memory Signed-off-by: Nicholas Van Sickle * Fix up comment Signed-off-by: Nicholas Van Sickle * Fix Linux build Signed-off-by: Nicholas Van Sickle * Try to make test less timing dependent (haven't been able to repro failure locally) Signed-off-by: Nicholas Van Sickle * Fix another build issue Signed-off-by: Nicholas Van Sickle * Fix unit test failures Signed-off-by: Nicholas Van Sickle * Fix a duplicate issue with sanitization that was causing sporadic test failure (thanks, test!) Signed-off-by: Nicholas Van Sickle * One more Linux fix... Signed-off-by: Nicholas Van Sickle --- .../EntityOutliner_EntityOrdering.py | 21 +- .../Application/ToolsApplication.cpp | 39 +++ .../Application/ToolsApplication.h | 11 + .../Entity/EditorEntityModel.cpp | 25 +- .../Entity/EditorEntitySortComponent.cpp | 110 ++++++-- .../Entity/EditorEntitySortComponent.h | 5 + .../EditorEntitySortComponentSerializer.cpp | 137 ++++++++++ .../EditorEntitySortComponentSerializer.h | 31 +++ .../Instance/InstanceUpdateExecutor.cpp | 5 +- .../AzToolsFramework/Prefab/PrefabDomUtils.h | 3 + .../Prefab/PrefabPublicHandler.cpp | 247 +++++++++++++++++- .../Prefab/PrefabPublicHandler.h | 2 + .../PropertyEditor/EntityPropertyEditor.cpp | 1 - .../aztoolsframework_files.cmake | 2 + 14 files changed, 598 insertions(+), 41 deletions(-) create mode 100644 Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.cpp create mode 100644 Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.h diff --git a/AutomatedTesting/Gem/PythonTests/editor/EditorScripts/EntityOutliner_EntityOrdering.py b/AutomatedTesting/Gem/PythonTests/editor/EditorScripts/EntityOutliner_EntityOrdering.py index e4575d4d17..5fa8130302 100644 --- a/AutomatedTesting/Gem/PythonTests/editor/EditorScripts/EntityOutliner_EntityOrdering.py +++ b/AutomatedTesting/Gem/PythonTests/editor/EditorScripts/EntityOutliner_EntityOrdering.py @@ -95,7 +95,8 @@ def EntityOutliner_EntityOrdering(): entity_outliner_model.dropMimeData( mime_data, QtCore.Qt.MoveAction, target_row, 0, target_index.parent() ) - QtWidgets.QApplication.processEvents() + # Wait after move to let events (i.e. prefab propagation) process + general.idle_wait(1.0) # Move an entity before another entity in the order by dragging the source above the target move_entity_before = lambda source_name, target_name: _move_entity( @@ -119,24 +120,24 @@ def EntityOutliner_EntityOrdering(): # Our new entity should be given a name with a number automatically new_entity = f"Entity{i+1}" - # The new entity should be added to the top of its parent entity - expected_order = [new_entity] + expected_order + # The new entity should be added to the bottom of its parent entity + expected_order = expected_order + [new_entity] verify_entities_sorted(expected_order) - # 3) Move "Entity1" to the top of the order - move_entity_before("Entity1", "Entity5") - expected_order = ["Entity1", "Entity5", "Entity4", "Entity3", "Entity2"] + # 3) Move "Entity5" to the top of the order + move_entity_before("Entity5", "Entity1") + expected_order = ["Entity5", "Entity1", "Entity2", "Entity3", "Entity4"] verify_entities_sorted(expected_order) - # 4) Move "Entity4" to the bottom of the order - move_entity_after("Entity4", "Entity2") - expected_order = ["Entity1", "Entity5", "Entity3", "Entity2", "Entity4"] + # 4) Move "Entity2" to the bottom of the order + move_entity_after("Entity2", "Entity4") + expected_order = ["Entity5", "Entity1", "Entity3", "Entity4", "Entity2"] verify_entities_sorted(expected_order) # 5) Add another new entity, ensure the rest of the order is unchanged create_entity() - expected_order = ["Entity6", "Entity1", "Entity5", "Entity3", "Entity2", "Entity4"] + expected_order = ["Entity5", "Entity1", "Entity3", "Entity4", "Entity2", "Entity6"] verify_entities_sorted(expected_order) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp index d3b3b09583..0412e454a6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp @@ -238,12 +238,14 @@ namespace AzToolsFramework , m_isInIsolationMode(false) { ToolsApplicationRequests::Bus::Handler::BusConnect(); + AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler::BusConnect(); m_undoCache.RegisterToUndoCacheInterface(); } ToolsApplication::~ToolsApplication() { + AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler::BusDisconnect(); ToolsApplicationRequests::Bus::Handler::BusDisconnect(); Stop(); } @@ -564,6 +566,12 @@ namespace AzToolsFramework void ToolsApplication::MarkEntitySelected(AZ::EntityId entityId) { AZ_PROFILE_FUNCTION(AzToolsFramework); + + if (m_freezeSelectionUpdates) + { + return; + } + AZ_Assert(entityId.IsValid(), "Invalid entity Id being marked as selected."); EntityIdList::iterator foundIter = AZStd::find(m_selectedEntities.begin(), m_selectedEntities.end(), entityId); @@ -583,6 +591,11 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); + if (m_freezeSelectionUpdates) + { + return; + } + EntityIdList entitiesSelected; entitiesSelected.reserve(entitiesToSelect.size()); @@ -606,6 +619,12 @@ namespace AzToolsFramework void ToolsApplication::MarkEntityDeselected(AZ::EntityId entityId) { AZ_PROFILE_FUNCTION(AzToolsFramework); + + if (m_freezeSelectionUpdates) + { + return; + } + auto foundIter = AZStd::find(m_selectedEntities.begin(), m_selectedEntities.end(), entityId); if (foundIter != m_selectedEntities.end()) { @@ -623,6 +642,11 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); + if (m_freezeSelectionUpdates) + { + return; + } + ToolsApplicationEvents::Bus::Broadcast(&ToolsApplicationEvents::BeforeEntitySelectionChanged); EntityIdSet entitySetToDeselect(entitiesToDeselect.begin(), entitiesToDeselect.end()); @@ -677,6 +701,11 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); + if (m_freezeSelectionUpdates) + { + return; + } + // We're setting the selection set as a batch from an external caller. // * Filter out any unselectable entities // * Calculate selection/deselection delta so we can notify specific entities only on change. @@ -1567,6 +1596,16 @@ namespace AzToolsFramework } } + void ToolsApplication::OnPrefabInstancePropagationBegin() + { + m_freezeSelectionUpdates = true; + } + + void ToolsApplication::OnPrefabInstancePropagationEnd() + { + m_freezeSelectionUpdates = false; + } + void ToolsApplication::CreateUndosForDirtyEntities() { AZ_PROFILE_FUNCTION(AzToolsFramework); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.h index 5e9208b475..3dcb43f17d 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.h @@ -16,6 +16,7 @@ #include #include #include +#include #pragma once @@ -29,6 +30,7 @@ namespace AzToolsFramework class ToolsApplication : public AzFramework::Application , public ToolsApplicationRequests::Bus::Handler + , public AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler { public: AZ_RTTI(ToolsApplication, "{2895561E-BE90-4CC3-8370-DD46FCF74C01}", AzFramework::Application); @@ -169,6 +171,14 @@ namespace AzToolsFramework }; ////////////////////////////////////////////////////////////////////////// + ////////////////////////////////////////////////////////////////////////// + // PrefabPublicNotificationBus::Handler + + void OnPrefabInstancePropagationBegin() override; + void OnPrefabInstancePropagationEnd() override; + + ////////////////////////////////////////////////////////////////////////// + void CreateUndosForDirtyEntities(); void ConsistencyCheckUndoCache(); AZ::Aabb m_selectionBounds; @@ -181,6 +191,7 @@ namespace AzToolsFramework bool m_isDuringUndoRedo; bool m_isInIsolationMode; EntityIdSet m_isolatedEntityIdSet; + bool m_freezeSelectionUpdates = false; EditorEntityAPI* m_editorEntityAPI = nullptr; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp index eaff64ba2e..e4d4f40bce 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp @@ -449,16 +449,23 @@ namespace AzToolsFramework AZStd::unordered_map>::const_iterator orderItr = m_savedOrderInfo.find(childId); if (orderItr != m_savedOrderInfo.end() && orderItr->second.first == parentId) { - bool sortOrderUpdated = AzToolsFramework::RecoverEntitySortInfo(parentId, childId, orderItr->second.second); - m_savedOrderInfo.erase(childId); - - // force notify the child sort order changed on the parent entity info, but only if the restore didn't actually modify - // the order internally (and sent ChildEntityOrderArrayUpdated). that may seem heavy handed, and it is, but necessary - // to combat scenarios when the initial override detection returns a false positive (see comment about IDH comparisons - // in OnChildSortOrderChanged) and the slice instance source-to-live mapping hasn't been fully reconstructed yet. - if (!sortOrderUpdated) + bool isPrefabEnabled = false; + AzFramework::ApplicationRequests::Bus::BroadcastResult( + isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled); + // If prefabs are enabled, rely on the component to do a sanity check instead of restoring the order from the model + if (!isPrefabEnabled) { - parentInfo.OnChildSortOrderChanged(); + bool sortOrderUpdated = AzToolsFramework::RecoverEntitySortInfo(parentId, childId, orderItr->second.second); + m_savedOrderInfo.erase(childId); + + // force notify the child sort order changed on the parent entity info, but only if the restore didn't actually modify + // the order internally (and sent ChildEntityOrderArrayUpdated). that may seem heavy handed, and it is, but necessary + // to combat scenarios when the initial override detection returns a false positive (see comment about IDH comparisons + // in OnChildSortOrderChanged) and the slice instance source-to-live mapping hasn't been fully reconstructed yet. + if (!sortOrderUpdated) + { + parentInfo.OnChildSortOrderChanged(); + } } } else diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.cpp index 6936397187..8ade8c470f 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.cpp @@ -8,11 +8,17 @@ #include "EditorEntitySortComponent.h" #include "EditorEntityInfoBus.h" #include "EditorEntityHelpers.h" +#include #include #include +#include #include #include #include +#include +#include +#include +#include static_assert(sizeof(AZ::u64) == sizeof(AZ::EntityId), "We use AZ::EntityId for Persistent ID, which is a u64 under the hood. These must be the same size otherwise the persistent id will have to be rewritten"); @@ -51,6 +57,12 @@ namespace AzToolsFramework ; } } + + AZ::JsonRegistrationContext* jsonRegistration = azrtti_cast(context); + if (jsonRegistration) + { + jsonRegistration->Serializer()->HandlesType(); + } } void EditorEntitySortComponent::GetProvidedServices(AZ::ComponentDescriptor::DependencyArrayType& services) @@ -167,9 +179,6 @@ namespace AzToolsFramework } MarkDirtyAndSendChangedEvent(); - - // Use the ToolsApplication to mark the entity dirty, this will only do something if we already have an undo batch - ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::AddDirtyEntity, GetEntityId()); return true; } @@ -187,6 +196,10 @@ namespace AzToolsFramework else { EntityOrderArray::iterator insertPosition = GetFirstSelectedEntityPosition(); + if (insertPosition != m_childEntityOrderArray.end()) + { + ++insertPosition; + } retval = AddChildEntityInternal(entityId, false, insertPosition); } @@ -220,9 +233,6 @@ namespace AzToolsFramework MarkDirtyAndSendChangedEvent(); - // Use the ToolsApplication to mark the entity dirty, this will only do something if we already have an undo batch - ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::AddDirtyEntity, GetEntityId()); - return true; } return false; @@ -272,6 +282,12 @@ namespace AzToolsFramework void EditorEntitySortComponent::OnPrefabInstancePropagationEnd() { m_ignoreIncomingOrderChanges = false; + + if (m_shouldSanityCheckStateAfterPropagation) + { + SanitizeOrderEntryArray(); + m_shouldSanityCheckStateAfterPropagation = false; + } } void EditorEntitySortComponent::MarkDirtyAndSendChangedEvent() @@ -280,14 +296,8 @@ namespace AzToolsFramework // one of the event listeners needs to build the InstanceDataHierarchy m_entityOrderIsDirty = true; - // Force an immediate update for prefabs, which won't receive PrepareSave - bool isPrefabEnabled = false; - AzFramework::ApplicationRequests::Bus::BroadcastResult( - isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled); - if (isPrefabEnabled) - { - PrepareSave(); - } + // Use the ToolsApplication to mark the entity dirty, this will only do something if we already have an undo batch + ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::AddDirtyEntity, GetEntityId()); EditorEntitySortNotificationBus::Event(GetEntityId(), &EditorEntitySortNotificationBus::Events::ChildEntityOrderArrayUpdated); } @@ -308,9 +318,8 @@ namespace AzToolsFramework isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled); if (isPrefabEnabled) { - PostLoad(); + m_shouldSanityCheckStateAfterPropagation = true; } - // Send out that the order for our entity is now updated EditorEntitySortNotificationBus::Event(GetEntityId(), &EditorEntitySortNotificationBus::Events::ChildEntityOrderArrayUpdated); } @@ -336,6 +345,73 @@ namespace AzToolsFramework m_entityOrderIsDirty = false; } + void EditorEntitySortComponent::SanitizeOrderEntryArray() + { + bool shouldEmitDirtyState = false; + + // Remove invalid and duplicate entries that point at non-existent entities + AZStd::unordered_set duplicateIds; + for (auto it = m_childEntityOrderArray.begin(); it != m_childEntityOrderArray.end();) + { + if (!it->IsValid() || GetEntityById(*it) == nullptr || duplicateIds.contains(*it)) + { + it = m_childEntityOrderArray.erase(it); + shouldEmitDirtyState = true; + } + else + { + duplicateIds.insert(*it); + ++it; + } + } + + // Append any missing children + EntityIdList children; + AZ::TransformBus::EventResult(children, GetEntityId(), &AZ::TransformBus::Events::GetChildren); + for (auto it = m_childEntityOrderArray.begin(); it != m_childEntityOrderArray.end(); ++it) + { + if (auto removedChildrenIt = AZStd::remove(children.begin(), children.end(), *it); removedChildrenIt != children.end()) + { + children.erase(removedChildrenIt); + } + } + + AZStd::sort(children.begin(), children.end(), [](AZ::EntityId lhs, AZ::EntityId rhs) + { + return GetEntityById(lhs)->GetName() < GetEntityById(rhs)->GetName(); + }); + + if (!children.empty()) + { + shouldEmitDirtyState = true; + EntityOrderArray::iterator insertPosition = GetFirstSelectedEntityPosition(); + if (insertPosition != m_childEntityOrderArray.end()) + { + ++insertPosition; + } + m_childEntityOrderArray.insert(insertPosition, children.begin(), children.end()); + } + + // Clear out the vector to be rebuilt from persistent id + m_childEntityOrderEntryArray.resize(m_childEntityOrderArray.size()); + for (size_t i = 0; i < m_childEntityOrderArray.size(); ++i) + { + m_childEntityOrderEntryArray[i] = { + m_childEntityOrderArray[i], + static_cast(i) + }; + } + + RebuildEntityOrderCache(); + + if (shouldEmitDirtyState) + { + ToolsApplicationRequests::Bus::Broadcast(&ToolsApplicationRequests::Bus::Events::AddDirtyEntity, GetEntityId()); + } + + m_entityOrderIsDirty = false; + } + void EditorEntitySortComponent::PostLoad() { // Clear out the vector to be rebuilt from persistent id @@ -383,7 +459,7 @@ namespace AzToolsFramework firstSelectedEntityPos = selectedEntityPos < firstSelectedEntityPos ? selectedEntityPos : firstSelectedEntityPos; } - return firstSelectedEntityPos == m_childEntityOrderArray.end() ? m_childEntityOrderArray.begin() : firstSelectedEntityPos; + return firstSelectedEntityPos; } } } // namespace AzToolsFramework diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.h index 806e903c96..a4715fb041 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.h @@ -23,6 +23,8 @@ namespace AzToolsFramework , public EditorEntityContextNotificationBus::Handler , public AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler { + friend class JsonEditorEntitySortComponentSerializer; + public: AZ_COMPONENT(EditorEntitySortComponent, "{6EA1E03D-68B2-466D-97F7-83998C8C27F0}", EditorComponentBase); @@ -64,6 +66,8 @@ namespace AzToolsFramework void PrepareSave(); void PostLoad(); + void SanitizeOrderEntryArray(); + class EntitySortSerializationEvents : public AZ::SerializeContext::IEventHandler { @@ -112,6 +116,7 @@ namespace AzToolsFramework bool m_entityOrderIsDirty = true; ///< This flag indicates our stored serialization order data is out of date and must be rebuilt before serialization occurs bool m_ignoreIncomingOrderChanges = false; ///< This is set when prefab propagation occurs so that non-authored order changes can be ignored + bool m_shouldSanityCheckStateAfterPropagation = false; //< This is set after activation, to queue a cleanup of any invalid state after the next prefab propagation. }; } } // namespace AzToolsFramework diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.cpp new file mode 100644 index 0000000000..0ec13cedda --- /dev/null +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.cpp @@ -0,0 +1,137 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include +#include +#include +#include + +namespace AzToolsFramework::Components +{ + AZ_CLASS_ALLOCATOR_IMPL(JsonEditorEntitySortComponentSerializer, AZ::SystemAllocator, 0); + + AZ::JsonSerializationResult::Result JsonEditorEntitySortComponentSerializer::Load( + void* outputValue, + [[maybe_unused]] const AZ::Uuid& outputValueTypeId, + const rapidjson::Value& inputValue, + AZ::JsonDeserializerContext& context) + { + namespace JSR = AZ::JsonSerializationResult; + + AZ_Assert( + azrtti_typeid() == outputValueTypeId, + "Unable to deserialize EditorEntitySortComponent from json because the provided type is %s.", + outputValueTypeId.ToString().c_str()); + + EditorEntitySortComponent* sortComponentInstance = reinterpret_cast(outputValue); + AZ_Assert(sortComponentInstance, "Output value for JsonEditorEntitySortComponentSerializer can't be null."); + + JSR::ResultCode result(JSR::Tasks::ReadField); + { + JSR::ResultCode componentIdLoadResult = ContinueLoadingFromJsonObjectField( + &sortComponentInstance->m_id, azrtti_typeidm_id)>(), inputValue, + "Id", context); + + result.Combine(componentIdLoadResult); + } + + { + sortComponentInstance->m_childEntityOrderArray.clear(); + JSR::ResultCode enryLoadResult = ContinueLoadingFromJsonObjectField( + &sortComponentInstance->m_childEntityOrderArray, + azrtti_typeidm_childEntityOrderArray)>(), inputValue, "Child Entity Order", + context); + + // Migrate ChildEntityOrderEntryArray -> ChildEntityOrderArray + if (sortComponentInstance->m_childEntityOrderArray.empty()) + { + enryLoadResult = ContinueLoadingFromJsonObjectField( + &sortComponentInstance->m_childEntityOrderEntryArray, + azrtti_typeidm_childEntityOrderEntryArray)>(), inputValue, + "ChildEntityOrderEntryArray", context); + + AZStd::sort( + sortComponentInstance->m_childEntityOrderEntryArray.begin(), + sortComponentInstance->m_childEntityOrderEntryArray.end(), + [](const EditorEntitySortComponent::EntityOrderEntry& lhs, + const EditorEntitySortComponent::EntityOrderEntry& rhs) -> bool + { + return lhs.m_sortIndex < rhs.m_sortIndex; + }); + + // Sort by index and copy to the order array, any duplicates or invalid entries will be cleaned up by the sanitization pass + sortComponentInstance->m_childEntityOrderArray.resize(sortComponentInstance->m_childEntityOrderEntryArray.size()); + for (size_t i = 0; i < sortComponentInstance->m_childEntityOrderEntryArray.size(); ++i) + { + sortComponentInstance->m_childEntityOrderArray[i] = sortComponentInstance->m_childEntityOrderEntryArray[i].m_entityId; + } + } + + sortComponentInstance->RebuildEntityOrderCache(); + + result.Combine(enryLoadResult); + } + + return context.Report( + result, + result.GetProcessing() != JSR::Processing::Halted ? "Successfully loaded EditorEntitySortComponent information." + : "Failed to load EditorEntitySortComponent information."); + } + + AZ::JsonSerializationResult::Result JsonEditorEntitySortComponentSerializer::Store( + rapidjson::Value& outputValue, + const void* inputValue, + const void* defaultValue, + [[maybe_unused]] const AZ::Uuid& valueTypeId, + AZ::JsonSerializerContext& context) + { + namespace JSR = AZ::JsonSerializationResult; + + AZ_Assert( + azrtti_typeid() == valueTypeId, + "Unable to Serialize EditorEntitySortComponent because the provided type is %s.", + valueTypeId.ToString().c_str()); + + const EditorEntitySortComponent* sortComponentInstance = reinterpret_cast(inputValue); + AZ_Assert(sortComponentInstance, "Input value for JsonEditorEntitySortComponentSerializer can't be null."); + const EditorEntitySortComponent* defaultsortComponentInstance = + reinterpret_cast(defaultValue); + + JSR::ResultCode result(JSR::Tasks::WriteValue); + { + AZ::ScopedContextPath subPathName(context, "m_id"); + const AZ::ComponentId* componentId = &sortComponentInstance->m_id; + const AZ::ComponentId* defaultComponentId = + defaultsortComponentInstance ? &defaultsortComponentInstance->m_id : nullptr; + + JSR::ResultCode resultComponentId = ContinueStoringToJsonObjectField( + outputValue, "Id", componentId, defaultComponentId, azrtti_typeidm_id)>(), + context); + + result.Combine(resultComponentId); + } + + { + AZ::ScopedContextPath subPathName(context, "m_childEntityOrderArray"); + const EntityOrderArray* childEntityOrderArray = &sortComponentInstance->m_childEntityOrderArray; + const EntityOrderArray* defaultChildEntityOrderArray = + defaultsortComponentInstance ? &defaultsortComponentInstance->m_childEntityOrderArray : nullptr; + + JSR::ResultCode resultParentEntityId = ContinueStoringToJsonObjectField( + outputValue, "Child Entity Order", childEntityOrderArray, defaultChildEntityOrderArray, + azrtti_typeidm_childEntityOrderArray)>(), context); + + result.Combine(resultParentEntityId); + } + + return context.Report( + result, + result.GetProcessing() != JSR::Processing::Halted ? "Successfully stored EditorEntitySortComponent information." + : "Failed to store EditorEntitySortComponent information."); + } +} // namespace AzToolsFramework::Components diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.h new file mode 100644 index 0000000000..29d1f9c14a --- /dev/null +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponentSerializer.h @@ -0,0 +1,31 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include +#include + +namespace AzToolsFramework::Components +{ + class JsonEditorEntitySortComponentSerializer + : public AZ::BaseJsonSerializer + { + public: + AZ_RTTI(JsonEditorEntitySortComponentSerializer, "{5104782E-B34F-4D87-B1DF-BDFB1AF20D58}", BaseJsonSerializer); + AZ_CLASS_ALLOCATOR_DECL; + + AZ::JsonSerializationResult::Result Load( + void* outputValue, const AZ::Uuid& outputValueTypeId, const rapidjson::Value& inputValue, + AZ::JsonDeserializerContext& context) override; + + AZ::JsonSerializationResult::Result Store( + rapidjson::Value& outputValue, const void* inputValue, const void* defaultValue, const AZ::Uuid& valueTypeId, + AZ::JsonSerializerContext& context) override; + }; +} // namespace AzToolsFramework::Components diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp index 9ef74167a6..6fe3e9e92b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp @@ -20,6 +20,7 @@ #include #include #include +#include namespace AzToolsFramework { @@ -244,10 +245,10 @@ namespace AzToolsFramework selectedEntityIds.erase(entityIdIterator--); } } - ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::SetSelectedEntities, selectedEntityIds); - // Notify Propagation has ended + // Notify Propagation has ended, then update selection (which is frozen during propagation, so this order matters) PrefabPublicNotificationBus::Broadcast(&PrefabPublicNotifications::OnPrefabInstancePropagationEnd); + ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::SetSelectedEntities, selectedEntityIds); } m_updatingTemplateInstancesInQueue = false; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h index 7cab24ad9f..89d1a046e5 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h @@ -28,6 +28,9 @@ namespace AzToolsFramework inline static const char* EntityIdName = "Id"; inline static const char* EntitiesName = "Entities"; inline static const char* ContainerEntityName = "ContainerEntity"; + inline static const char* ComponentsName = "Components"; + inline static const char* EntityOrderName = "Child Entity Order"; + inline static const char* TypeName = "$type"; /** * Find Prefab value from given parent value and target value's name. diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 32600853fd..26cc16f34b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -29,6 +30,7 @@ #include #include #include +#include #include @@ -595,8 +597,41 @@ namespace AzToolsFramework Instance& entityOwningInstance = owningInstanceOfParentEntity->get(); + // Get the template for our owning instance from the root prefab DOM and use that to generate our patch + AZStd::vector pathOfInstances; + + InstanceOptionalReference rootInstance = owningInstanceOfParentEntity; + while (rootInstance->get().GetParentInstance() != AZStd::nullopt) + { + pathOfInstances.emplace_back(rootInstance); + rootInstance = rootInstance->get().GetParentInstance(); + } + + AZStd::string aliasPathResult = ""; + for (auto instanceIter = pathOfInstances.rbegin(); instanceIter != pathOfInstances.rend(); ++instanceIter) + { + aliasPathResult.append("/Instances/"); + aliasPathResult.append((*instanceIter)->get().GetInstanceAlias()); + } + + PrefabDomPath rootPrefabDomPath(aliasPathResult.c_str()); + + PrefabDom& rootPrefabTemplateDom = m_prefabSystemComponentInterface->FindTemplateDom(rootInstance->get().GetTemplateId()); + + auto instanceDomFromRootValue = rootPrefabDomPath.Get(rootPrefabTemplateDom); + if (!instanceDomFromRootValue) + { + return AZ::Failure("Could not load Instance DOM from the top level ancestor's DOM."); + } + + PrefabDomValueReference instanceDomFromRoot = *instanceDomFromRootValue; + if (!instanceDomFromRoot.has_value()) + { + return AZ::Failure("Could not load Instance DOM from the top level ancestor's DOM."); + } + PrefabDom instanceDomBeforeUpdate; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBeforeUpdate, entityOwningInstance); + instanceDomBeforeUpdate.CopyFrom(instanceDomFromRoot.value().get(), instanceDomBeforeUpdate.GetAllocator()); ScopedUndoBatch undoBatch("Add Entity"); @@ -674,6 +709,9 @@ namespace AzToolsFramework bool isInstanceContainerEntity = IsInstanceContainerEntity(entityId) && !IsLevelInstanceContainerEntity(entityId); bool isNewParentOwnedByDifferentInstance = false; + bool isInFocusTree = m_prefabFocusPublicInterface->IsOwningPrefabInFocusHierarchy(entityId); + bool isOwnedByFocusedPrefabInstance = m_prefabFocusPublicInterface->IsOwningPrefabBeingFocused(entityId); + if (beforeParentId != afterParentId) { // If the entity parent changed, verify if the owning instance changed too @@ -727,7 +765,7 @@ namespace AzToolsFramework } } - if (isInstanceContainerEntity) + if (isInFocusTree && !isOwnedByFocusedPrefabInstance) { if (isNewParentOwnedByDifferentInstance) { @@ -1653,6 +1691,144 @@ namespace AzToolsFramework return true; } + void PrefabPublicHandler::AddNewEntityToSortOrder( + Instance& owningInstance, + PrefabDom& domToAddEntityUnder, + const EntityAlias& parentEntityAlias, + const EntityAlias& entityToAddAlias) + { + // Find the parent entity to get its sort order component + auto findParentEntity = [&]() -> rapidjson::Value* + { + if (auto containerEntityIter = domToAddEntityUnder.FindMember(PrefabDomUtils::ContainerEntityName); + containerEntityIter != domToAddEntityUnder.MemberEnd()) + { + if (parentEntityAlias == containerEntityIter->value[PrefabDomUtils::EntityIdName].GetString()) + { + return &containerEntityIter->value; + } + } + + if (auto entitiesIter = domToAddEntityUnder.FindMember(PrefabDomUtils::EntitiesName); + entitiesIter != domToAddEntityUnder.MemberEnd()) + { + for (auto entityIter = entitiesIter->value.MemberBegin(); entityIter != entitiesIter->value.MemberEnd(); ++entityIter) + { + if (parentEntityAlias == entityIter->value[PrefabDomUtils::EntityIdName].GetString()) + { + return &entityIter->value; + } + } + } + + return nullptr; + }; + + rapidjson::Value* parentEntityValue = findParentEntity(); + if (parentEntityValue == nullptr) + { + return; + } + + // Get the list of selected entities, we'll insert our duplicated entities after the last selected + // sibling in their parent's list, e.g. for: + // - Entity1 + // - Entity2 (selected) + // - Entity3 + // - Entity4 (selected) + // - Entity5 + // Our duplicate selection command would create duplicate Entity2 and Entity4 and insert them after Entity4: + // - Entity1 + // - Entity2 + // - Entity3 + // - Entity4 + // - Entity2 (new, selected after duplicate) + // - Entity4 (new, selected after duplicate) + // - Entity5 + AzToolsFramework::EntityIdList selectedEntities; + AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult( + selectedEntities, &AzToolsFramework::ToolsApplicationRequests::GetSelectedEntities); + + // Find the EditorEntitySortComponent DOM + auto componentsIter = parentEntityValue->FindMember(PrefabDomUtils::ComponentsName); + if (componentsIter == parentEntityValue->MemberEnd()) + { + return; + } + + for (auto componentIter = componentsIter->value.MemberBegin(); componentIter != componentIter->value.MemberEnd(); + ++componentIter) + { + // Check the component type + auto typeFieldIter = componentIter->value.FindMember(PrefabDomUtils::TypeName); + if (typeFieldIter == componentIter->value.MemberEnd()) + { + continue; + } + + AZ::JsonDeserializerSettings jsonDeserializerSettings; + AZ::Uuid typeId = AZ::Uuid::CreateNull(); + AZ::JsonSerialization::LoadTypeId(typeId, typeFieldIter->value); + + if (typeId != azrtti_typeid()) + { + continue; + } + + // Check for the entity order field + auto orderMembersIter = componentIter->value.FindMember(PrefabDomUtils::EntityOrderName); + if (orderMembersIter == componentIter->value.MemberEnd() || !orderMembersIter->value.IsArray()) + { + continue; + } + + // Scan for the last selected entity in the list (if any) to determine where to add our entries + rapidjson::Value newOrder(rapidjson::kArrayType); + auto insertValuesAfter = orderMembersIter->value.End(); + for (auto orderMemberIter = orderMembersIter->value.Begin(); orderMemberIter != orderMembersIter->value.End(); + ++orderMemberIter) + { + if (!orderMemberIter->IsString()) + { + continue; + } + const char* value = orderMemberIter->GetString(); + for (AZ::EntityId selectedEntity : selectedEntities) + { + auto alias = owningInstance.GetEntityAlias(selectedEntity); + if (alias.has_value() && alias.value().get() == value) + { + insertValuesAfter = orderMemberIter; + break; + } + } + } + + // Construct our new array with the new order - insertion may happen at end, so check for that in the loop itself + for (auto orderMemberIter = orderMembersIter->value.Begin();; ++orderMemberIter) + { + if (orderMemberIter != orderMembersIter->value.End()) + { + newOrder.PushBack(orderMemberIter->Move(), domToAddEntityUnder.GetAllocator()); + } + if (orderMemberIter == insertValuesAfter) + { + newOrder.PushBack( + rapidjson::Value(entityToAddAlias.c_str(), domToAddEntityUnder.GetAllocator()), + domToAddEntityUnder.GetAllocator()); + } + if (orderMemberIter == orderMembersIter->value.End()) + { + break; + } + } + + // Replace the order with our newly constructed one + orderMembersIter->value.Swap(newOrder); + break; + } + } + void PrefabPublicHandler::DuplicateNestedEntitiesInInstance(Instance& commonOwningInstance, const AZStd::vector& entities, PrefabDom& domToAddDuplicatedEntitiesUnder, EntityIdList& duplicatedEntityIds, AZStd::unordered_map& oldAliasToNewAliasMap) @@ -1710,6 +1886,73 @@ namespace AzToolsFramework PrefabDom entityDomAfter(&domToAddDuplicatedEntitiesUnder.GetAllocator()); entityDomAfter.Parse(newEntityDomString.toUtf8().constData()); + EntityAlias parentEntityAlias; + if (auto componentsIter = entityDomAfter.FindMember(PrefabDomUtils::ComponentsName); + componentsIter != entityDomAfter.MemberEnd()) + { + auto checkComponent = [&](const rapidjson::Value& value) -> bool + { + if (!value.IsObject()) + { + return false; + } + + // Check the component type + auto typeFieldIter = value.FindMember(PrefabDomUtils::TypeName); + if (typeFieldIter == value.MemberEnd()) + { + return false; + } + + AZ::JsonDeserializerSettings jsonDeserializerSettings; + AZ::Uuid typeId = AZ::Uuid::CreateNull(); + AZ::JsonSerialization::LoadTypeId(typeId, typeFieldIter->value); + + // Prefabs get serialized with the Editor transform component type, check for that + if (typeId != azrtti_typeid()) + { + return false; + } + + if (auto parentEntityIter = value.FindMember("Parent Entity"); + parentEntityIter != value.MemberEnd()) + { + parentEntityAlias = parentEntityIter->value.GetString(); + return true; + } + return false; + }; + + if (componentsIter->value.IsObject()) + { + for (auto componentIter = componentsIter->value.MemberBegin(); componentIter != componentsIter->value.MemberEnd(); + ++componentIter) + { + if (checkComponent(componentIter->value)) + { + break; + } + } + } + else if (componentsIter->value.IsArray()) + { + for (auto componentIter = componentsIter->value.Begin(); componentIter != componentsIter->value.End(); + ++componentIter) + { + if (checkComponent(*componentIter)) + { + break; + } + } + } + } + + // Insert our entity into its parent's sort order + if (!parentEntityAlias.empty()) + { + AddNewEntityToSortOrder(commonOwningInstance, domToAddDuplicatedEntitiesUnder, parentEntityAlias, newEntityAlias); + } + // Add the new Entity DOM to the Entities member of the instance rapidjson::Value aliasName(newEntityAlias.c_str(), static_cast(newEntityAlias.length()), domToAddDuplicatedEntitiesUnder.GetAllocator()); entitiesIter->value.AddMember(AZStd::move(aliasName), entityDomAfter, domToAddDuplicatedEntitiesUnder.GetAllocator()); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 4961be9d77..dd071ac09f 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -78,6 +78,8 @@ namespace AzToolsFramework InstanceOptionalReference GetOwnerInstanceByEntityId(AZ::EntityId entityId) const; bool EntitiesBelongToSameInstance(const EntityIdList& entityIds) const; + void AddNewEntityToSortOrder(Instance& owningInstance, PrefabDom& domToAddEntityUnder, + const EntityAlias& parentEntityAlias, const EntityAlias& entityToAddAlias); /** * Duplicate a list of entities owned by a common owning instance by directly diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/EntityPropertyEditor.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/EntityPropertyEditor.cpp index 9ecbdf5ffc..fa419d5f14 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/EntityPropertyEditor.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/EntityPropertyEditor.cpp @@ -1599,7 +1599,6 @@ namespace AzToolsFramework for (size_t entityIndex = 1; entityIndex < m_selectedEntityIds.size(); ++entityIndex) { entity = GetSelectedEntityById(m_selectedEntityIds[entityIndex]); - AZ_Assert(entity, "Entity id selected for display but no such entity exists"); if (!entity) { continue; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/aztoolsframework_files.cmake b/Code/Framework/AzToolsFramework/AzToolsFramework/aztoolsframework_files.cmake index d61d6486a9..9a891498de 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/aztoolsframework_files.cmake +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/aztoolsframework_files.cmake @@ -148,6 +148,8 @@ set(FILES Entity/EditorEntitySortBus.h Entity/EditorEntitySortComponent.cpp Entity/EditorEntitySortComponent.h + Entity/EditorEntitySortComponentSerializer.cpp + Entity/EditorEntitySortComponentSerializer.h Entity/EditorEntityTransformBus.h Entity/PrefabEditorEntityOwnershipInterface.h Entity/PrefabEditorEntityOwnershipService.h