From 2d956ebb36cd0b2274ba1b31f64e2488c4961fa2 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Wed, 26 Feb 2025 17:46:22 +0100 Subject: [PATCH] Fix error when applying prefab changes with missing (deleted) nested prefabs #3244 --- .../CustomEditors/Dedicated/ActorEditor.cs | 3 +- Source/Editor/Managed/ManagedEditor.cpp | 15 ---- Source/Editor/Managed/ManagedEditor.h | 1 - .../Undo/Actions/BreakPrefabLinkAction.cs | 9 ++- Source/Engine/Level/Prefabs/Prefab.Apply.cpp | 29 ++++++- Source/Engine/Level/Prefabs/Prefab.cpp | 18 +++++ Source/Engine/Level/Prefabs/Prefab.h | 9 +++ Source/Engine/Tests/TestPrefabs.cpp | 76 ++++++++++++++++++- 8 files changed, 136 insertions(+), 24 deletions(-) diff --git a/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs b/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs index d1cdd8780..a93969e36 100644 --- a/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs +++ b/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs @@ -69,8 +69,7 @@ namespace FlaxEditor.CustomEditors.Dedicated Values.SetReferenceValue(prefabInstance); // Display prefab UI (when displaying object inside Prefab Window then display only nested prefabs) - var prefabId = prefab.ID; - Editor.GetPrefabNestedObject(ref prefabId, ref prefabObjectId, out var nestedPrefabId, out var nestedPrefabObjectId); + prefab.GetNestedObject(ref prefabObjectId, out var nestedPrefabId, out var nestedPrefabObjectId); var nestedPrefab = FlaxEngine.Content.Load(nestedPrefabId); var panel = layout.CustomContainer(); panel.CustomControl.Height = 20.0f; diff --git a/Source/Editor/Managed/ManagedEditor.cpp b/Source/Editor/Managed/ManagedEditor.cpp index c4a052f9e..652878d4d 100644 --- a/Source/Editor/Managed/ManagedEditor.cpp +++ b/Source/Editor/Managed/ManagedEditor.cpp @@ -617,21 +617,6 @@ void ManagedEditor::WipeOutLeftoverSceneObjects() ObjectsRemovalService::Flush(); } -void ManagedEditor::GetPrefabNestedObject(const Guid& prefabId, const Guid& prefabObjectId, Guid& outPrefabId, Guid& outPrefabObjectId) -{ - outPrefabId = Guid::Empty; - outPrefabObjectId = Guid::Empty; - const auto prefab = Content::Load(prefabId); - if (!prefab) - return; - const ISerializable::DeserializeStream** prefabObjectDataPtr = prefab->ObjectsDataCache.TryGet(prefabObjectId); - if (!prefabObjectDataPtr) - return; - const ISerializable::DeserializeStream& prefabObjectData = **prefabObjectDataPtr; - JsonTools::GetGuidIfValid(outPrefabId, prefabObjectData, "PrefabID"); - JsonTools::GetGuidIfValid(outPrefabObjectId, prefabObjectData, "PrefabObjectID"); -} - void ManagedEditor::OnEditorAssemblyLoaded(MAssembly* assembly) { ASSERT(!HasManagedInstance()); diff --git a/Source/Editor/Managed/ManagedEditor.h b/Source/Editor/Managed/ManagedEditor.h index 7bd0cdefa..4722db997 100644 --- a/Source/Editor/Managed/ManagedEditor.h +++ b/Source/Editor/Managed/ManagedEditor.h @@ -259,7 +259,6 @@ public: API_FUNCTION(Internal) static Array GetVisualScriptLocals(); API_FUNCTION(Internal) static bool EvaluateVisualScriptLocal(VisualScript* script, API_PARAM(Ref) VisualScriptLocal& local); API_FUNCTION(Internal) static void WipeOutLeftoverSceneObjects(); - API_FUNCTION(Internal) static void GetPrefabNestedObject(API_PARAM(Ref) const Guid& prefabId, API_PARAM(Ref) const Guid& prefabObjectId, API_PARAM(Out) Guid& outPrefabId, API_PARAM(Out) Guid& outPrefabObjectId); private: void OnEditorAssemblyLoaded(MAssembly* assembly); diff --git a/Source/Editor/Undo/Actions/BreakPrefabLinkAction.cs b/Source/Editor/Undo/Actions/BreakPrefabLinkAction.cs index a6f338633..a4fb8e1f5 100644 --- a/Source/Editor/Undo/Actions/BreakPrefabLinkAction.cs +++ b/Source/Editor/Undo/Actions/BreakPrefabLinkAction.cs @@ -33,9 +33,14 @@ namespace FlaxEditor.Actions // Check if this object comes from another nested prefab (to break link only from the top-level prefab) Item nested; nested.ID = ID; - Editor.GetPrefabNestedObject(ref PrefabID, ref PrefabObjectID, out nested.PrefabID, out nested.PrefabObjectID); - if (nested.PrefabID != Guid.Empty && nested.PrefabObjectID != Guid.Empty) + var prefab = FlaxEngine.Content.Load(PrefabID); + if (prefab != null && + prefab.GetNestedObject(ref PrefabObjectID, out nested.PrefabID, out nested.PrefabObjectID) && + nested.PrefabID != Guid.Empty && + nested.PrefabObjectID != Guid.Empty) + { nestedPrefabLinks.Add(nested); + } } } } diff --git a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp index deb3ed9e9..418ecd477 100644 --- a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp @@ -707,7 +707,7 @@ bool Prefab::ApplyAll(Actor* targetActor) for (int32 i = 0; i < nestedPrefabIds.Count(); i++) { const auto nestedPrefab = Content::LoadAsync(nestedPrefabIds[i]); - if (nestedPrefab && nestedPrefab != this && (nestedPrefab->Flags & ObjectFlags::WasMarkedToDelete) == ObjectFlags::None) + if (nestedPrefab && nestedPrefab != this && EnumHasNoneFlags(nestedPrefab->Flags, ObjectFlags::WasMarkedToDelete)) { allPrefabs.Add(nestedPrefab); } @@ -778,6 +778,29 @@ bool Prefab::ApplyAllInternal(Actor* targetActor, bool linkTargetActorObjectToPr for (int32 i = 0; i < targetObjects->Count(); i++) { SceneObject* obj = targetObjects.Value->At(i); + + // Check the whole chain of prefab references to be valid for this object + bool brokenPrefab = false; + Guid nestedPrefabId = obj->GetPrefabID(), nestedPrefabObjectId = obj->GetPrefabObjectID(); + while (!brokenPrefab && nestedPrefabId.IsValid() && nestedPrefabObjectId.IsValid()) + { + auto prefab = Content::Load(nestedPrefabId); + if (prefab) + { + prefab->GetNestedObject(nestedPrefabObjectId, nestedPrefabId, nestedPrefabObjectId); + } + else + { + LOG(Warning, "Missing prefab {0}.", nestedPrefabId); + brokenPrefab = true; + } + } + if (brokenPrefab) + { + LOG(Warning, "Broken prefab reference on object {0}. Breaking linkage to inline object inside prefab.", GetObjectName(obj)); + obj->BreakPrefabLink(); + } + writer.SceneObject(obj); } writer.EndArray(); @@ -809,7 +832,7 @@ bool Prefab::ApplyAllInternal(Actor* targetActor, bool linkTargetActorObjectToPr SceneObject* obj = targetObjects.Value->At(i); auto data = it->GetObject(); - // Check if object is from that prefab + // Check if object is from this prefab if (obj->GetPrefabID() == prefabId) { if (!obj->GetPrefabObjectID().IsValid()) @@ -883,7 +906,7 @@ bool Prefab::ApplyAllInternal(Actor* targetActor, bool linkTargetActorObjectToPr { const SceneObject* obj = targetObjects->At(i); - // Check if object is from that prefab + // Check if object is from this prefab if (obj->GetPrefabID() == prefabId) { // Map prefab instance to existing prefab object diff --git a/Source/Engine/Level/Prefabs/Prefab.cpp b/Source/Engine/Level/Prefabs/Prefab.cpp index b85be7287..a50ec3900 100644 --- a/Source/Engine/Level/Prefabs/Prefab.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.cpp @@ -94,6 +94,24 @@ SceneObject* Prefab::GetDefaultInstance(const Guid& objectId) return result; } +bool Prefab::GetNestedObject(const Guid& objectId, Guid& outPrefabId, Guid& outObjectId) const +{ + if (WaitForLoaded()) + return false; + bool result = false; + Guid result1 = Guid::Empty, result2 = Guid::Empty; + const ISerializable::DeserializeStream** prefabObjectDataPtr = ObjectsDataCache.TryGet(objectId); + if (prefabObjectDataPtr) + { + const ISerializable::DeserializeStream& prefabObjectData = **prefabObjectDataPtr; + result = JsonTools::GetGuidIfValid(result1, prefabObjectData, "PrefabID") && + JsonTools::GetGuidIfValid(result2, prefabObjectData, "PrefabObjectID"); + } + outPrefabId = result1; + outObjectId = result2; + return result; +} + void Prefab::DeleteDefaultInstance() { ScopeLock lock(Locker); diff --git a/Source/Engine/Level/Prefabs/Prefab.h b/Source/Engine/Level/Prefabs/Prefab.h index 5b7de8637..2e8931d32 100644 --- a/Source/Engine/Level/Prefabs/Prefab.h +++ b/Source/Engine/Level/Prefabs/Prefab.h @@ -70,6 +70,15 @@ public: /// The object of the prefab loaded from the prefab. Contains the default values. It's not added to gameplay but deserialized with postLoad and init event fired. API_FUNCTION() SceneObject* GetDefaultInstance(API_PARAM(Ref) const Guid& objectId); + /// + /// Gets the reference to the other nested prefab for a specific prefab object. + /// + /// The ID of the object in this prefab. + /// Result ID of the prefab asset referenced by the given object. + /// Result ID of the prefab object referenced by the given object. + /// True if got valid reference, otherwise false. + API_FUNCTION() bool GetNestedObject(API_PARAM(Ref) const Guid& objectId, API_PARAM(Out) Guid& outPrefabId, API_PARAM(Out) Guid& outObjectId) const; + #if USE_EDITOR /// /// Applies the difference from the prefab object instance, saves the changes and synchronizes them with the active instances of the prefab asset. diff --git a/Source/Engine/Tests/TestPrefabs.cpp b/Source/Engine/Tests/TestPrefabs.cpp index fd8ffa1e2..b90a84436 100644 --- a/Source/Engine/Tests/TestPrefabs.cpp +++ b/Source/Engine/Tests/TestPrefabs.cpp @@ -559,7 +559,7 @@ TEST_CASE("Prefabs") Content::DeleteAsset(prefabNested1); Content::DeleteAsset(prefabBase); } - SECTION("Test Applying Prefab ChangeTo Object References") + SECTION("Test Applying Prefab Change To Object References") { // https://github.com/FlaxEngine/FlaxEngine/issues/3136 @@ -614,4 +614,78 @@ TEST_CASE("Prefabs") instanceB->DeleteObject(); Content::DeleteAsset(prefab); } + SECTION("Test Applying Prefab With Missing Nested Prefab") + { + // https://github.com/FlaxEngine/FlaxEngine/issues/3244 + + // Create Prefab B with just root object + AssetReference prefabB = Content::CreateVirtualAsset(); + REQUIRE(prefabB); + Guid id; + Guid::Parse("25dbe4b0416be0777a6ce59e8788b10f", id); + prefabB->ChangeID(id); + auto prefabBInit = prefabB->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"aac6b9644492fbca1a6ab0a7904a557e\"," + "\"TypeName\": \"FlaxEngine.ExponentialHeightFog\"," + "\"Name\": \"Prefab B.Root\"" + "}" + "]"); + REQUIRE(!prefabBInit); + + // Create Prefab A with nested Prefab B attached to the root + AssetReference prefabA = Content::CreateVirtualAsset(); + REQUIRE(prefabA); + Guid::Parse("4cb744714f746e31855f41815612d14b", id); + prefabA->ChangeID(id); + auto prefabAInit = prefabA->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"244274a04cc60d56a2f024bfeef5772d\"," + "\"TypeName\": \"FlaxEngine.SpotLight\"," + "\"Name\": \"Prefab A.Root\"" + "}," + "{" + "\"ID\": \"1e51f1094f430733333f8280e78dfcc3\"," + "\"PrefabID\": \"25dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"aac6b9644492fbca1a6ab0a7904a557e\"," + "\"ParentID\": \"244274a04cc60d56a2f024bfeef5772d\"" + "}" + "]"); + REQUIRE(!prefabAInit); + + // Spawn test instances of both prefabs + ScriptingObjectReference instanceA = PrefabManager::SpawnPrefab(prefabA); + ScriptingObjectReference instanceB = PrefabManager::SpawnPrefab(prefabB); + + // Delete nested prefab + Content::DeleteAsset(prefabB); + + // Apply instance A and verify it's fine even tho prefab B doesn't exist anymore + bool applyResult = PrefabManager::ApplyAll(instanceA); + REQUIRE(!applyResult); + + // Check state of objects + REQUIRE(instanceA); + REQUIRE(instanceA->Children.Count() == 1); + REQUIRE(instanceA->Children[0] != nullptr); + REQUIRE(instanceA->Children[0]->Is()); + REQUIRE(instanceB); + REQUIRE(instanceB->Is()); + + // Verify if prefab has new data to properly spawn another prefab + ScriptingObjectReference instanceC = PrefabManager::SpawnPrefab(prefabA); + REQUIRE(instanceC); + REQUIRE(instanceC->Children.Count() == 1); + REQUIRE(instanceC->Children[0] != nullptr); + REQUIRE(instanceC->Children[0]->Is()); + + // Cleanup + instanceA->DeleteObject(); + instanceB->DeleteObject(); + instanceC->DeleteObject(); + Content::DeleteAsset(prefabA); + + } }