LYN-3708 | Optimize Prefab instance propagation to stabilize UX (#700)

* Add instanceToIgnore to calls leading to instances being added to the queue for propagation.

* Change PrefabUndoEntityUpdate to make it so that the instance triggering the prefab template change is not reloaded on propagation, since it will already be up to date due to the way we generated the patch to begin with.

* Add FindPrefabDomValue utility function for paths

* Expose the level root prefab template id in the Prefab EOS Interface

* Fix Instance Alias Path generation to work with the new FindValueInPrefabDom function

* Stop reloading ancestors on propagation, and fix instance reloading so that the level dom is used (and overrides are preserved)

* Remove commented out code, refactor FindPrefabDomValue for paths (was handling an edge case incorrectly, and it's not even triggered)

* Fix issue with PathView reference - with PathView already being a reference, this resulted in a copy and triggered a warning during automated review builds.

* Additional fix to the build warning, remove redundant error message

* Revert changes to Instance::GetAbsoluteInstanceAliasPath(), as they were impacting serialization.

* Remove the dependency to the level root prefab template in the propagation code, climb up the hierarchy instead. This allows tests to work despite not using the EOS properly.
Also use PrefabDomPaths to retrieve the instance dom from the root dom instead of iterating.

* Remove now unused PrefabDomUtils function, extend optimization to link updates.

* Trigger a full instance propagation to correctly refresh alias references.
This is an issue in the test because some operations are called from the backend API and will not trigger propagation properly. Tests will soon be rewritten to more properly represent frontend workflows.

* Fixes lingering issues with propagation:
- Restores code that fixes the selection if entityIds have changed;
- Fixes Do() function on link update. Prefab containers will propagate correctly while still being stable during editing.

* Remove GetRootPrefabInstanceTemplateId (no longer necessary after the code has been rewritten)

* Fix optimization code to account for instances being removed and propagation being run out of order in Create Prefab undo.

* Renamed variable, added comments for clarity.

* Restore asserts on instance not being found; Rename Do to Redo for clarity; Add comments.

* Fixed incomplete comment.
main
Danilo Aimini 5 years ago committed by GitHub
parent 47e5c72f2e
commit 80f62d0523
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -197,7 +197,7 @@ namespace AzToolsFramework
AZ::IO::PathView filePath, Prefab::InstanceOptionalReference instanceToParentUnder) override;
Prefab::InstanceOptionalReference GetRootPrefabInstance() override;
const AZStd::vector<AZ::Data::Asset<AZ::Data::AssetData>>& GetPlayInEditorAssetData() override;
//////////////////////////////////////////////////////////////////////////

@ -47,8 +47,13 @@ namespace AzToolsFramework
virtual void AppendEntityAliasToPatchPaths(PrefabDom& providedPatch, const AZ::EntityId& entityId) = 0;
//! Updates the template links (updating instances) for the given templateId using the providedPatch
virtual bool PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId) = 0;
//! Updates the template links (updating instances) for the given template and triggers propagation on its instances.
//! @param providedPatch The patch to apply to the template.
//! @param templateId The id of the template to update.
//! @param instanceToExclude An optional reference to an instance of the template being updated that should not be refreshes as part of propagation.
//! Defaults to nullopt, which means that all instances will be refreshed.
//! @return True if the template was patched correctly, false if the operation failed.
virtual bool PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) = 0;
virtual void ApplyPatchesToInstance(const AZ::EntityId& entityId, PrefabDom& patches, const Instance& instanceToAddPatches) = 0;

@ -172,7 +172,7 @@ namespace AzToolsFramework
}
}
bool InstanceToTemplatePropagator::PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId)
bool InstanceToTemplatePropagator::PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId, InstanceOptionalReference instanceToExclude)
{
PrefabDom& templateDomReference = m_prefabSystemComponentInterface->FindTemplateDom(templateId);
@ -184,7 +184,7 @@ namespace AzToolsFramework
if (result.GetOutcome() == AZ::JsonSerializationResult::Outcomes::Success)
{
m_prefabSystemComponentInterface->SetTemplateDirtyFlag(templateId, true);
m_prefabSystemComponentInterface->PropagateTemplateChanges(templateId);
m_prefabSystemComponentInterface->PropagateTemplateChanges(templateId, instanceToExclude);
return true;
}
else

@ -37,7 +37,7 @@ namespace AzToolsFramework
InstanceOptionalReference GetTopMostInstanceInHierarchy(AZ::EntityId entityId);
bool PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId) override;
bool PatchTemplate(PrefabDomValue& providedPatch, TemplateId templateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) override;
void ApplyPatchesToInstance(const AZ::EntityId& entityId, PrefabDom& patches, const Instance& instanceToAddPatches) override;

@ -56,7 +56,7 @@ namespace AzToolsFramework
AZ::Interface<InstanceUpdateExecutorInterface>::Unregister(this);
}
void InstanceUpdateExecutor::AddTemplateInstancesToQueue(TemplateId instanceTemplateId)
void InstanceUpdateExecutor::AddTemplateInstancesToQueue(TemplateId instanceTemplateId, InstanceOptionalReference instanceToExclude)
{
auto findInstancesResult =
m_templateInstanceMapperInterface->FindInstancesOwnedByTemplate(instanceTemplateId);
@ -70,9 +70,18 @@ namespace AzToolsFramework
return;
}
Instance* instanceToExcludePtr = nullptr;
if (instanceToExclude.has_value())
{
instanceToExcludePtr = &(instanceToExclude->get());
}
for (auto instance : findInstancesResult->get())
{
m_instancesUpdateQueue.emplace_back(instance);
if (instance != instanceToExcludePtr)
{
m_instancesUpdateQueue.emplace_back(instance);
}
}
}
@ -103,7 +112,7 @@ namespace AzToolsFramework
EntityIdList selectedEntityIds;
ToolsApplicationRequestBus::BroadcastResult(selectedEntityIds, &ToolsApplicationRequests::GetSelectedEntities);
ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::SetSelectedEntities, EntityIdList());
PrefabDom instanceDomFromRootDocument;
// Process all instances in the queue, capped to the batch size.
// Even though we potentially initialized the batch size to the queue, it's possible for the queue size to shrink
@ -148,13 +157,62 @@ namespace AzToolsFramework
continue;
}
Template& currentTemplate = currentTemplateReference->get();
Instance::EntityList newEntities;
if (PrefabDomUtils::LoadInstanceFromPrefabDom(*instanceToUpdate, newEntities, currentTemplate.GetPrefabDom()))
// Climb up to the root of the instance hierarchy from this instance
InstanceOptionalConstReference rootInstance = *instanceToUpdate;
AZStd::vector<InstanceOptionalConstReference> pathOfInstances;
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)
{
AZ_Assert(
false,
"InstanceUpdateExecutor::UpdateTemplateInstancesInQueue - "
"Could not load Instance DOM from the top level ancestor's DOM.");
isUpdateSuccessful = false;
continue;
}
PrefabDomValueReference instanceDomFromRoot = *instanceDomFromRootValue;
if (!instanceDomFromRoot.has_value())
{
AZ_Assert(
false,
"InstanceUpdateExecutor::UpdateTemplateInstancesInQueue - "
"Could not load Instance DOM from the top level ancestor's DOM.");
isUpdateSuccessful = false;
continue;
}
// If a link was created for a nested instance before the changes were propagated,
// then we associate it correctly here
instanceDomFromRootDocument.CopyFrom(instanceDomFromRoot->get(), instanceDomFromRootDocument.GetAllocator());
if (PrefabDomUtils::LoadInstanceFromPrefabDom(*instanceToUpdate, newEntities, instanceDomFromRootDocument))
{
// If a link was created for a nested instance before the changes were propagated,
// then we associate it correctly here
instanceToUpdate->GetNestedInstances([&](AZStd::unique_ptr<Instance>& nestedInstance) {
Template& currentTemplate = currentTemplateReference->get();
instanceToUpdate->GetNestedInstances([&](AZStd::unique_ptr<Instance>& nestedInstance)
{
if (nestedInstance->GetLinkId() != InvalidLinkId)
{
return;
@ -179,22 +237,11 @@ namespace AzToolsFramework
AzToolsFramework::EditorEntityContextRequestBus::Broadcast(
&AzToolsFramework::EditorEntityContextRequests::HandleEntitiesAdded, newEntities);
}
else
{
AZ_Error(
"Prefab", false,
"InstanceUpdateExecutor::UpdateTemplateInstancesInQueue - "
"Could not load Instance from Prefab DOM of Template with Id '%llu' on file path '%s'.",
currentTemplateId, currentTemplate.GetFilePath().c_str());
isUpdateSuccessful = false;
}
}
for (auto entityIdIterator = selectedEntityIds.begin(); entityIdIterator != selectedEntityIds.end(); entityIdIterator++)
{
// Since entities get recreated during propagation, we need to check whether the entities correspoding to the list
// of selected entity ids are present or not.
// Since entities get recreated during propagation, we need to check whether the entities
// corresponding to the list of selected entity ids are present or not.
AZ::Entity* entity = GetEntityById(*entityIdIterator);
if (entity == nullptr)
{

@ -35,7 +35,7 @@ namespace AzToolsFramework
explicit InstanceUpdateExecutor(int instanceCountToUpdateInBatch = 0);
void AddTemplateInstancesToQueue(TemplateId instanceTemplateId) override;
void AddTemplateInstancesToQueue(TemplateId instanceTemplateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) override;
bool UpdateTemplateInstancesInQueue() override;
virtual void RemoveTemplateInstanceFromQueue(const Instance* instance) override;

@ -27,7 +27,7 @@ namespace AzToolsFramework
virtual ~InstanceUpdateExecutorInterface() = default;
// Add all Instances of Template with given Id into a queue for updating them later.
virtual void AddTemplateInstancesToQueue(TemplateId instanceTemplateId) = 0;
virtual void AddTemplateInstancesToQueue(TemplateId instanceTemplateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) = 0;
// Update Instances in the waiting queue.
virtual bool UpdateTemplateInstancesInQueue() = 0;

@ -242,11 +242,13 @@ namespace AzToolsFramework
m_instanceToTemplateInterface->GenerateDomForEntity(containerAfterReset, *containerEntity);
// Update the state of the entity
PrefabUndoEntityUpdate* state = aznew PrefabUndoEntityUpdate(AZStd::to_string(static_cast<AZ::u64>(containerEntityId)));
state->SetParent(undoBatch.GetUndoBatch());
state->Capture(containerBeforeReset, containerAfterReset, containerEntityId);
auto templateId = instanceToCreate->get().GetTemplateId();
state->Redo();
PrefabDom transformPatch;
m_instanceToTemplateInterface->GeneratePatch(transformPatch, containerBeforeReset, containerAfterReset);
m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(transformPatch, containerEntityId);
m_instanceToTemplateInterface->PatchTemplate(transformPatch, templateId);
}
// This clears any entities marked as dirty due to reparenting of entities during the process of creating a prefab.
@ -661,12 +663,12 @@ namespace AzToolsFramework
else
{
Internal_HandleContainerOverride(
parentUndoBatch, entityId, patch, owningInstance->get().GetLinkId());
parentUndoBatch, entityId, patch, owningInstance->get().GetLinkId(), owningInstance->get().GetParentInstance());
}
}
else
{
Internal_HandleEntityChange(parentUndoBatch, entityId, beforeState, afterState);
Internal_HandleEntityChange(parentUndoBatch, entityId, beforeState, afterState, owningInstance);
if (isNewParentOwnedByDifferentInstance)
{
@ -679,25 +681,27 @@ namespace AzToolsFramework
}
void PrefabPublicHandler::Internal_HandleContainerOverride(
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch, const LinkId linkId)
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch,
const LinkId linkId, InstanceOptionalReference parentInstance)
{
// Save these changes as patches to the link
PrefabUndoLinkUpdate* linkUpdate = aznew PrefabUndoLinkUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
linkUpdate->SetParent(undoBatch);
linkUpdate->Capture(patch, linkId);
linkUpdate->Redo();
linkUpdate->Redo(parentInstance);
}
void PrefabPublicHandler::Internal_HandleEntityChange(
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState, PrefabDom& afterState)
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState,
PrefabDom& afterState, InstanceOptionalReference instance)
{
// Update the state of the entity
PrefabUndoEntityUpdate* state = aznew PrefabUndoEntityUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
state->SetParent(undoBatch);
state->Capture(beforeState, afterState, entityId);
state->Redo();
state->Redo(instance);
}
void PrefabPublicHandler::Internal_HandleInstanceChange(

@ -162,9 +162,11 @@ namespace AzToolsFramework
InstanceOptionalConstReference instance, const AZStd::unordered_set<AZ::IO::Path>& templateSourcePaths);
static void Internal_HandleContainerOverride(
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch, const LinkId linkId);
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch,
const LinkId linkId, InstanceOptionalReference parentInstance = AZStd::nullopt);
static void Internal_HandleEntityChange(
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState, PrefabDom& afterState);
UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState,
PrefabDom& afterState, InstanceOptionalReference instance = AZStd::nullopt);
void Internal_HandleInstanceChange(UndoSystem::URSequencePoint* undoBatch, AZ::Entity* entity, AZ::EntityId beforeParentId, AZ::EntityId afterParentId);
void UpdateLinkPatchesWithNewEntityAliases(

@ -141,8 +141,10 @@ namespace AzToolsFramework
return newInstance;
}
void PrefabSystemComponent::PropagateTemplateChanges(TemplateId templateId)
void PrefabSystemComponent::PropagateTemplateChanges(TemplateId templateId, InstanceOptionalReference instanceToExclude)
{
UpdatePrefabInstances(templateId, instanceToExclude);
auto templateIdToLinkIdsIterator = m_templateToLinkIdsMap.find(templateId);
if (templateIdToLinkIdsIterator != m_templateToLinkIdsMap.end())
{
@ -153,10 +155,6 @@ namespace AzToolsFramework
templateIdToLinkIdsIterator->second.end()));
UpdateLinkedInstances(linkIdsToUpdateQueue);
}
else
{
UpdatePrefabInstances(templateId);
}
}
void PrefabSystemComponent::UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom)
@ -174,9 +172,9 @@ namespace AzToolsFramework
}
}
void PrefabSystemComponent::UpdatePrefabInstances(const TemplateId& templateId)
void PrefabSystemComponent::UpdatePrefabInstances(const TemplateId& templateId, InstanceOptionalReference instanceToExclude)
{
m_instanceUpdateExecutor.AddTemplateInstancesToQueue(templateId);
m_instanceUpdateExecutor.AddTemplateInstancesToQueue(templateId, instanceToExclude);
}
void PrefabSystemComponent::UpdateLinkedInstances(AZStd::queue<LinkIds>& linkIdsQueue)
@ -250,8 +248,6 @@ namespace AzToolsFramework
if (targetTemplateIdToLinkIdMap[targetTemplateId].first.empty() &&
targetTemplateIdToLinkIdMap[targetTemplateId].second)
{
UpdatePrefabInstances(targetTemplateId);
auto templateToLinkIter = m_templateToLinkIdsMap.find(targetTemplateId);
if (templateToLinkIter != m_templateToLinkIdsMap.end())
{

@ -215,14 +215,14 @@ namespace AzToolsFramework
*/
void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) override;
void PropagateTemplateChanges(TemplateId templateId) override;
void PropagateTemplateChanges(TemplateId templateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) override;
/**
* Updates all Instances owned by a Template.
*
* @param templateId The id of the Template owning Instances to update.
*/
void UpdatePrefabInstances(const TemplateId& templateId);
void UpdatePrefabInstances(const TemplateId& templateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt);
private:
AZ_DISABLE_COPY_MOVE(PrefabSystemComponent);

@ -56,7 +56,7 @@ namespace AzToolsFramework
virtual PrefabDom& FindTemplateDom(TemplateId templateId) = 0;
virtual void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) = 0;
virtual void PropagateTemplateChanges(TemplateId templateId) = 0;
virtual void PropagateTemplateChanges(TemplateId templateId, InstanceOptionalReference instanceToExclude = AZStd::nullopt) = 0;
virtual AZStd::unique_ptr<Instance> InstantiatePrefab(AZ::IO::PathView filePath) = 0;
virtual AZStd::unique_ptr<Instance> InstantiatePrefab(const TemplateId& templateId) = 0;

@ -70,10 +70,10 @@ namespace AzToolsFramework
const AZ::EntityId& entityId)
{
//get the entity alias for future undo/redo
InstanceOptionalReference instanceOptionalReference = m_instanceEntityMapperInterface->FindOwningInstance(entityId);
AZ_Error("Prefab", instanceOptionalReference,
auto instanceReference = m_instanceEntityMapperInterface->FindOwningInstance(entityId);
AZ_Error("Prefab", instanceReference,
"Failed to find an owning instance for the entity with id %llu.", static_cast<AZ::u64>(entityId));
Instance& instance = instanceOptionalReference->get();
Instance& instance = instanceReference->get();
m_templateId = instance.GetTemplateId();
m_entityAlias = (instance.GetEntityAlias(entityId)).value();
@ -106,6 +106,17 @@ namespace AzToolsFramework
m_templateId);
}
void PrefabUndoEntityUpdate::Redo(InstanceOptionalReference instanceToExclude)
{
[[maybe_unused]] bool isPatchApplicationSuccessful =
m_instanceToTemplateInterface->PatchTemplate(m_redoPatch, m_templateId, instanceToExclude);
AZ_Error(
"Prefab", isPatchApplicationSuccessful,
"Applying the patch on the entity with alias '%s' in template with id '%llu' was unsuccessful", m_entityAlias.c_str(),
m_templateId);
}
//PrefabInstanceLinkUndo
PrefabUndoInstanceLink::PrefabUndoInstanceLink(const AZStd::string& undoOperationName)
: PrefabUndoBase(undoOperationName)
@ -290,7 +301,12 @@ namespace AzToolsFramework
UpdateLink(m_linkDomNext);
}
void PrefabUndoLinkUpdate::UpdateLink(PrefabDom& linkDom)
void PrefabUndoLinkUpdate::Redo(InstanceOptionalReference instanceToExclude)
{
UpdateLink(m_linkDomNext, instanceToExclude);
}
void PrefabUndoLinkUpdate::UpdateLink(PrefabDom& linkDom, InstanceOptionalReference instanceToExclude)
{
LinkReference link = m_prefabSystemComponentInterface->FindLink(m_linkId);
@ -304,7 +320,7 @@ namespace AzToolsFramework
//propagate the link changes
link->get().UpdateTarget();
m_prefabSystemComponentInterface->PropagateTemplateChanges(link->get().GetTargetTemplateId());
m_prefabSystemComponentInterface->PropagateTemplateChanges(link->get().GetTargetTemplateId(), instanceToExclude);
//mark as dirty
m_prefabSystemComponentInterface->SetTemplateDirtyFlag(link->get().GetTargetTemplateId(), true);

@ -71,11 +71,12 @@ namespace AzToolsFramework
void Capture(
PrefabDom& initialState,
PrefabDom& endState,
const AZ::EntityId& entity);
PrefabDom& endState, const AZ::EntityId& entity);
void Undo() override;
void Redo() override;
//! Overload to allow to apply the change, but prevent instanceToExclude from being refreshed.
void Redo(InstanceOptionalReference instanceToExclude);
private:
InstanceEntityMapperInterface* m_instanceEntityMapperInterface = nullptr;
@ -139,9 +140,11 @@ namespace AzToolsFramework
void Undo() override;
void Redo() override;
//! Overload to allow to apply the change, but prevent instanceToExclude from being refreshed.
void Redo(InstanceOptionalReference instanceToExclude);
private:
void UpdateLink(PrefabDom& linkDom);
void UpdateLink(PrefabDom& linkDom, InstanceOptionalReference instanceToExclude = AZStd::nullopt);
LinkId m_linkId;
PrefabDom m_linkDomNext; //data for delete/update

@ -242,6 +242,7 @@ namespace UnitTest
// Patch the nested prefab to reference an entity in its parent
ASSERT_TRUE(m_instanceToTemplateInterface->PatchEntityInTemplate(patch, newEntity->GetId()));
m_instanceUpdateExecutorInterface->AddTemplateInstancesToQueue(rootInstance->GetTemplateId());
m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue();
// Using the aliases we saved grab the updated entities so we can verify the entity reference is still preserved

Loading…
Cancel
Save