From ff526ecafb496b7185d9774b246fd146f33f0160 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Tue, 2 Jun 2026 13:23:21 +0200 Subject: [PATCH] Fix nested prefab stack overflow when adding new object to nested prefabs hierarchy https://github.com/LOOPDISK/FlaxEngine/pull/44 --- Source/Engine/Level/Actors/AnimatedModel.cpp | 3 +- Source/Engine/Level/Prefabs/Prefab.Apply.cpp | 60 +++++---- Source/Engine/Level/Prefabs/Prefab.h | 2 +- Source/Engine/Level/SceneObjectsFactory.cpp | 2 +- Source/Engine/Tests/TestPrefabs.cpp | 121 +++++++++++++++++++ 5 files changed, 159 insertions(+), 29 deletions(-) diff --git a/Source/Engine/Level/Actors/AnimatedModel.cpp b/Source/Engine/Level/Actors/AnimatedModel.cpp index ce106dc83..bc3c5b8fb 100644 --- a/Source/Engine/Level/Actors/AnimatedModel.cpp +++ b/Source/Engine/Level/Actors/AnimatedModel.cpp @@ -820,8 +820,7 @@ void AnimatedModel::RunBlendShapeDeformer(const MeshBase* mesh, MeshDeformationD void AnimatedModel::BeginPlay(SceneBeginData* data) { - if (SkinnedModel && SkinnedModel->IsLoaded()) - PreInitSkinningData(); + PreInitSkinningData(); // Base ModelInstanceActor::BeginPlay(data); diff --git a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp index cb6c82b5d..d972ee520 100644 --- a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp @@ -24,6 +24,7 @@ #include "Engine/Profiler/ProfilerCPU.h" #include "Engine/Threading/MainThreadTask.h" #include "Editor/Editor.h" +#include "FlaxEngine.Gen.h" // Apply flow: // - collect all prefabs using this prefab (load and create default instances) @@ -772,7 +773,13 @@ bool Prefab::ApplyAll(Actor* targetActor) if (ApplyAllInternal(targetActor, true, thisPrefabInstancesData)) return true; - SyncNestedPrefabs(allPrefabs, allPrefabsInstancesData); + // Sync nested prefabs + if (allPrefabs.HasItems()) + { + LOG(Info, "Updating referencing prefabs"); + HashSet synced; + SyncNestedPrefabs(allPrefabs, allPrefabsInstancesData, synced); + } const auto endTime = DateTime::NowUTC(); LOG(Info, "Prefab updated! {0} ms", (int32)(endTime - startTime).GetTotalMilliseconds()); @@ -1027,8 +1034,14 @@ bool Prefab::ApplyAllInternal(Actor* targetActor, bool linkTargetActorObjectToPr rapidjson_flax::Document targetDataDocument; if (NestedPrefabs.HasItems()) { + // Use initial data buffer (unstripped) but reorder objects to match the sequence (eg. when new object was added to the nested prefab) targetDataDocument.Parse(dataBuffer.GetString(), dataBuffer.GetSize()); - SceneObjectsFactory::PrefabSyncData prefabSyncData(*sceneObjects.Value, targetDataDocument, modifier.Value); + Array reorderedObjects = *sceneObjects.Value; + newPrefabInstanceIdToDataIndexCounter = 0; + for (auto i = newPrefabInstanceIdToDataIndex.Begin(); i.IsNotEnd(); ++i) + reorderedObjects.Insert(i->Value, sceneObjects->At(newPrefabInstanceIdToDataIndexStart + newPrefabInstanceIdToDataIndexCounter++)); + reorderedObjects.Resize(sceneObjects.Value->Count()); // reorderedObjects matches order in targetDataDocument + SceneObjectsFactory::PrefabSyncData prefabSyncData(reorderedObjects, targetDataDocument, modifier.Value); SceneObjectsFactory::SetupPrefabInstances(context, prefabSyncData); if (context.Instances.HasItems()) @@ -1236,7 +1249,7 @@ bool Prefab::UpdateInternal(const Array& defaultInstanceObjects, r { return Init(TypeName, StringAnsiView(tmpBuffer.GetString(), (int32)tmpBuffer.GetSize())); } -#if 1 // Set to 0 to use memory-only reload that does not modifies the source file - useful for testing and debugging prefabs apply +#if 1 // Set to 0 to use memory-only reload that does not modify the source file - useful for testing and debugging prefabs apply #if COMPILE_WITH_ASSETS_IMPORTER Locker.Unlock(); @@ -1295,7 +1308,7 @@ bool Prefab::UpdateInternal(const Array& defaultInstanceObjects, r _defaultInstance->DeleteObject(); _defaultInstance = nullptr; } - _isLoaded = false; + _loadState = 0; // Update prefab data manually (to prevent updating source asset file - just for testing) Document.Parse(buffer.GetString(), buffer.GetSize()); @@ -1348,7 +1361,7 @@ bool Prefab::UpdateInternal(const Array& defaultInstanceObjects, r NestedPrefabs.Add(prefabId); } } - _isLoaded = true; + _loadState = 1; } #endif @@ -1395,34 +1408,31 @@ bool Prefab::SyncChangesInternal(PrefabInstancesData& prefabInstancesData) return ApplyAllInternal(targetActor, false, prefabInstancesData); } -void Prefab::SyncNestedPrefabs(const NestedPrefabsList& allPrefabs, Array& allPrefabsInstancesData) const +void Prefab::SyncNestedPrefabs(const NestedPrefabsList& allPrefabs, Array& allPrefabsInstancesData, HashSet& synced) const { PROFILE_CPU(); - LOG(Info, "Updating referencing prefabs"); - - // TODO: this may not work well for very complex prefab nesting -> loop order matters, maybe build a graph of dependencies? // Call recursive for all referencing prefab assets to refresh nested prefabs for (int32 i = 0; i < allPrefabs.Count(); i++) { - auto nestedPrefab = allPrefabs[i].Get(); - if (nestedPrefab) + Prefab* nestedPrefab = allPrefabs[i].Get(); + if (!nestedPrefab || synced.Contains(nestedPrefab->GetID())) + continue; + if (nestedPrefab->WaitForLoaded()) { - if (nestedPrefab->WaitForLoaded()) - { - LOG(Warning, "Waiting for prefab asset load failed."); - continue; - } + LOG(Warning, "Waiting for '{}' load failed.", nestedPrefab->ToString()); + continue; + } - // Sync only if prefab is used by this prefab (directly) and it has been captured before - const int32 nestedPrefabIndex = nestedPrefab->NestedPrefabs.Find(GetID()); - if (nestedPrefabIndex != -1) - { - if (nestedPrefab->SyncChangesInternal(allPrefabsInstancesData[i])) - continue; - nestedPrefab->SyncNestedPrefabs(allPrefabs, allPrefabsInstancesData); - ObjectsRemovalService::Flush(); - } + // Sync only if prefab is used by this prefab (directly) and it has been captured before + const int32 nestedPrefabIndex = nestedPrefab->NestedPrefabs.Find(GetID()); + if (nestedPrefabIndex != -1) + { + synced.Add(nestedPrefab->GetID()); + if (nestedPrefab->SyncChangesInternal(allPrefabsInstancesData[i])) + continue; + nestedPrefab->SyncNestedPrefabs(allPrefabs, allPrefabsInstancesData, synced); + ObjectsRemovalService::Flush(); } } } diff --git a/Source/Engine/Level/Prefabs/Prefab.h b/Source/Engine/Level/Prefabs/Prefab.h index cde99a2cb..b1200ca29 100644 --- a/Source/Engine/Level/Prefabs/Prefab.h +++ b/Source/Engine/Level/Prefabs/Prefab.h @@ -104,7 +104,7 @@ private: bool ApplyAllInternal(Actor* targetActor, bool linkTargetActorObjectToPrefab, PrefabInstancesData& prefabInstancesData); bool UpdateInternal(const Array& defaultInstanceObjects, rapidjson_flax::StringBuffer& tmpBuffer); bool SyncChangesInternal(PrefabInstancesData& prefabInstancesData); - void SyncNestedPrefabs(const NestedPrefabsList& allPrefabs, Array& allPrefabsInstancesData) const; + void SyncNestedPrefabs(const NestedPrefabsList& allPrefabs, Array& allPrefabsInstancesData, HashSet& synced) const; #endif void DeleteDefaultInstance(); diff --git a/Source/Engine/Level/SceneObjectsFactory.cpp b/Source/Engine/Level/SceneObjectsFactory.cpp index 59d23a994..e9931eb20 100644 --- a/Source/Engine/Level/SceneObjectsFactory.cpp +++ b/Source/Engine/Level/SceneObjectsFactory.cpp @@ -752,7 +752,7 @@ void SceneObjectsFactory::SynchronizePrefabInstances(Context& context, PrefabSyn obj->SetOrderInParent(order); } - // Setup hierarchy for the prefab instances (ensure any new objects are connected) + // Setup hierarchy for the prefab instances (after adding new objects to ensure they are connected, eg. when reparenting existing prefab into a new root) for (const auto& instance : context.Instances) { const auto& prefabStartData = data.Data[instance.StatIndex]; diff --git a/Source/Engine/Tests/TestPrefabs.cpp b/Source/Engine/Tests/TestPrefabs.cpp index 91bce81cc..fb34a7284 100644 --- a/Source/Engine/Tests/TestPrefabs.cpp +++ b/Source/Engine/Tests/TestPrefabs.cpp @@ -8,6 +8,7 @@ #include "Engine/Level/Actors/EmptyActor.h" #include "Engine/Level/Actors/DirectionalLight.h" #include "Engine/Level/Actors/ExponentialHeightFog.h" +#include "Engine/Level/Actors/AnimatedModel.h" #include "Engine/Level/Prefabs/Prefab.h" #include "Engine/Level/Prefabs/PrefabManager.h" #include "Engine/Scripting/ScriptingObjectReference.h" @@ -905,4 +906,124 @@ TEST_CASE("Prefabs") instance1->DeleteObject(); instance2->DeleteObject(); } + SECTION("Test Adding Object To Base Prefab") + { + // https://github.com/LOOPDISK/FlaxEngine/pull/44 + + // Create inner prefab with 3 objects in hierarchy + AssetReference prefabInner = Content::CreateVirtualAsset(); + REQUIRE(prefabInner); + Guid id; + Guid::Parse("15dbe4b0416be0777a6ce59e8788b10f", id); + prefabInner->ChangeID(id); + auto prefabInnerInit = prefabInner->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"3de462104f56f681c14650a0171f88fb\"," + "\"TypeName\" : \"FlaxEngine.SpotLight\"," + "\"Name\" : \"Inner.Root\"" + "}," + "{" + "\"ID\": \"19b181f846b6911635ffacb902c93c6a\"," + "\"TypeName\" : \"FlaxEngine.StaticModel\"," + "\"ParentID\" : \"3de462104f56f681c14650a0171f88fb\"," + "\"Name\" : \"Inner.Cube\"" + "}," + "{" + "\"ID\": \"8950889f4a2e752d55165fbf10eaf184\"," + "\"TypeName\" : \"FlaxEngine.AnimatedModel\"," + "\"ParentID\" : \"19b181f846b6911635ffacb902c93c6a\"," + "\"Name\" : \"Inner.Model\"" + "}" + "]"); + REQUIRE(!prefabInnerInit); + + // Create outer prefab with 2 instances of inner prefab + AssetReference prefabOuter = Content::CreateVirtualAsset(); + REQUIRE(prefabOuter); + SCOPE_EXIT{ Content::DeleteAsset(prefabOuter); }; + Guid::Parse("2ab744714f746e31855f41815612d14b", id); + prefabOuter->ChangeID(id); + auto prefabOuterInit = prefabOuter->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"dba7f4bb4acfd62608b9a8bf550f31a5\"," + "\"TypeName\": \"FlaxEngine.EmptyActor\"," + "\"Name\": \"Outer.Root\"" + "}," + "{" + "\"ID\": \"a3b705284432bed9f043829c04a2bc8f\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"3de462104f56f681c14650a0171f88fb\"," + "\"ParentID\": \"dba7f4bb4acfd62608b9a8bf550f31a5\"," + "\"Name\": \"Instance 1\"" + "}," + "{" + "\"ID\": \"06a8c15a41b822dd27f3ac9d79b142d3\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"19b181f846b6911635ffacb902c93c6a\"," + "\"ParentID\": \"a3b705284432bed9f043829c04a2bc8f\"" + "}," + "{" + "\"ID\": \"4759fb9e4c4dda3b61ab5ab43949e42f\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"8950889f4a2e752d55165fbf10eaf184\"," + "\"ParentID\": \"06a8c15a41b822dd27f3ac9d79b142d3\"" + "}," + "{" + "\"ID\": \"1225be664c0c081e714bbf93e09b99e4\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"3de462104f56f681c14650a0171f88fb\"," + "\"ParentID\": \"dba7f4bb4acfd62608b9a8bf550f31a5\"," + "\"Name\": \"Instance 2\"" + "}," + "{" + "\"ID\": \"b397243540322182b806ad8339b7b617\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"19b181f846b6911635ffacb902c93c6a\"," + "\"ParentID\": \"1225be664c0c081e714bbf93e09b99e4\"" + "}," + "{" + "\"ID\": \"2c3b8e824daf038a58df528a238ca2de\"," + "\"PrefabID\": \"15dbe4b0416be0777a6ce59e8788b10f\"," + "\"PrefabObjectID\": \"8950889f4a2e752d55165fbf10eaf184\"," + "\"ParentID\": \"b397243540322182b806ad8339b7b617\"" + "}" + "]"); + REQUIRE(!prefabOuterInit); + + // Spawn test instances of both prefabs + ScriptingObjectReference instanceInner = PrefabManager::SpawnPrefab(prefabInner); + ScriptingObjectReference instanceOuter = PrefabManager::SpawnPrefab(prefabOuter); + + // Add new object to the inner prefab + instanceInner->Children[0]->GetOrAddChild(); + + // Apply changes + bool applyResult = PrefabManager::ApplyAll(instanceInner); + REQUIRE(!applyResult); + + // Check state of outer instance to properly reflect hierarchy + REQUIRE(instanceOuter); + REQUIRE(instanceOuter->Children.Count() == 2); + REQUIRE(instanceOuter->Children[0] != nullptr); + REQUIRE(instanceOuter->Children[0]->Children.Count() == 1); + REQUIRE(instanceOuter->Children[0]->Children[0]); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children.Count() == 2); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children[0]->Is()); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children[1]->Is()); + REQUIRE(instanceOuter->Children[1] != nullptr); + REQUIRE(instanceOuter->Children[1]->Children.Count() == 1); + REQUIRE(instanceOuter->Children[1]->Children[0]); + REQUIRE(instanceOuter->Children[1]->Children[0]->Children.Count() == 2); + REQUIRE(instanceOuter->Children[1]->Children[0]->Children[0]->Is()); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children[1]->Is()); + REQUIRE(instanceOuter->Children[0]->Children[0] != instanceOuter->Children[1]->Children[0]); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children[0] != instanceOuter->Children[1]->Children[0]->Children[0]); + REQUIRE(instanceOuter->Children[0]->Children[0]->Children[1] != instanceOuter->Children[1]->Children[0]->Children[1]); + + // Cleanup + instanceInner->DeleteObject(); + instanceOuter->DeleteObject(); + } }