From a6430692d2a2e10f23e0a2ba5e483111ef4199e4 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 9 Jun 2023 18:22:40 +0200 Subject: [PATCH 1/8] Fix crash safe handling of internal errors and exceptions when drawing asset thumbnails #1138 --- .../Content/Thumbnails/ThumbnailsModule.cs | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/Source/Editor/Content/Thumbnails/ThumbnailsModule.cs b/Source/Editor/Content/Thumbnails/ThumbnailsModule.cs index 3e9274c83..a6996310e 100644 --- a/Source/Editor/Content/Thumbnails/ThumbnailsModule.cs +++ b/Source/Editor/Content/Thumbnails/ThumbnailsModule.cs @@ -242,21 +242,36 @@ namespace FlaxEditor.Content.Thumbnails if (!atlas.IsReady) return; - // Setup - _guiRoot.RemoveChildren(); - _guiRoot.AccentColor = request.Proxy.AccentColor; + try + { + // Setup + _guiRoot.RemoveChildren(); + _guiRoot.AccentColor = request.Proxy.AccentColor; - // Call proxy to prepare for thumbnail rendering - request.Proxy.OnThumbnailDrawBegin(request, _guiRoot, context); - _guiRoot.UnlockChildrenRecursive(); + // Call proxy to prepare for thumbnail rendering + request.Proxy.OnThumbnailDrawBegin(request, _guiRoot, context); + _guiRoot.UnlockChildrenRecursive(); - // Draw preview - context.Clear(_output.View(), Color.Black); - Render2D.CallDrawing(_guiRoot, context, _output); + // Draw preview + context.Clear(_output.View(), Color.Black); + Render2D.CallDrawing(_guiRoot, context, _output); - // Call proxy and cleanup UI (delete create controls, shared controls should be unlinked during OnThumbnailDrawEnd event) - request.Proxy.OnThumbnailDrawEnd(request, _guiRoot); - _guiRoot.DisposeChildren(); + // Call proxy and cleanup UI (delete create controls, shared controls should be unlinked during OnThumbnailDrawEnd event) + request.Proxy.OnThumbnailDrawEnd(request, _guiRoot); + } + catch (Exception ex) + { + // Handle internal errors gracefully (eg. when asset is corrupted and proxy fails) + Editor.LogError("Failed to render thumbnail icon for asset: " + request.Item); + Editor.LogWarning(ex); + request.FinishRender(ref SpriteHandle.Invalid); + RemoveRequest(request); + return; + } + finally + { + _guiRoot.DisposeChildren(); + } // Copy backbuffer with rendered preview into atlas SpriteHandle icon = atlas.OccupySlot(_output, request.Item.ID); From d798b10d4c7ba61dd3bc0eb527639e17b821c913 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 9 Jun 2023 22:49:19 +0200 Subject: [PATCH 2/8] Remove unlogical `PrefabManager::IsNotCreatingPrefab` (use `IsCreatingPrefab` flag) --- Source/Engine/Level/Actors/StaticModel.cpp | 2 +- Source/Engine/Level/Prefabs/PrefabManager.cpp | 4 +--- Source/Engine/Level/Prefabs/PrefabManager.h | 7 +------ Source/Engine/Terrain/TerrainChunk.cpp | 2 +- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/Source/Engine/Level/Actors/StaticModel.cpp b/Source/Engine/Level/Actors/StaticModel.cpp index 17c99a40a..3eb3b6012 100644 --- a/Source/Engine/Level/Actors/StaticModel.cpp +++ b/Source/Engine/Level/Actors/StaticModel.cpp @@ -419,7 +419,7 @@ void StaticModel::Serialize(SerializeStream& stream, const void* otherObj) if (HasLightmap() #if USE_EDITOR - && PrefabManager::IsNotCreatingPrefab + && !PrefabManager::IsCreatingPrefab #endif ) { diff --git a/Source/Engine/Level/Prefabs/PrefabManager.cpp b/Source/Engine/Level/Prefabs/PrefabManager.cpp index 9f35b9fdc..700c86408 100644 --- a/Source/Engine/Level/Prefabs/PrefabManager.cpp +++ b/Source/Engine/Level/Prefabs/PrefabManager.cpp @@ -21,7 +21,6 @@ #if USE_EDITOR bool PrefabManager::IsCreatingPrefab = false; -bool PrefabManager::IsNotCreatingPrefab = true; Dictionary> PrefabManager::PrefabsReferences; CriticalSection PrefabManager::PrefabsReferencesLocker; #endif @@ -316,8 +315,8 @@ bool PrefabManager::CreatePrefab(Actor* targetActor, const StringView& outputPat LOG(Info, "Creating prefab from actor {0} (total objects count: {2}) to {1}...", targetActor->ToString(), outputPath, sceneObjects->Count()); // Serialize to json data + ASSERT(!IsCreatingPrefab); IsCreatingPrefab = true; - IsNotCreatingPrefab = false; rapidjson_flax::StringBuffer actorsDataBuffer; { CompactJsonWriter writerObj(actorsDataBuffer); @@ -331,7 +330,6 @@ bool PrefabManager::CreatePrefab(Actor* targetActor, const StringView& outputPat writer.EndArray(); } IsCreatingPrefab = false; - IsNotCreatingPrefab = true; // Randomize the objects ids (prevent overlapping of the prefab instance objects ids and the prefab objects ids) Dictionary objectInstanceIdToPrefabObjectId; diff --git a/Source/Engine/Level/Prefabs/PrefabManager.h b/Source/Engine/Level/Prefabs/PrefabManager.h index b934a295f..0ff87536f 100644 --- a/Source/Engine/Level/Prefabs/PrefabManager.h +++ b/Source/Engine/Level/Prefabs/PrefabManager.h @@ -20,7 +20,7 @@ struct Transform; /// API_CLASS(Static) class FLAXENGINE_API PrefabManager { -DECLARE_SCRIPTING_TYPE_NO_SPAWN(PrefabManager); + DECLARE_SCRIPTING_TYPE_NO_SPAWN(PrefabManager); /// /// Spawns the instance of the prefab objects. Prefab will be spawned to the first loaded scene. @@ -92,11 +92,6 @@ DECLARE_SCRIPTING_TYPE_NO_SPAWN(PrefabManager); /// static bool IsCreatingPrefab; - /// - /// Helper global state flag set to false during prefab asset creation. Can be used to skip local objects data serialization to prefab data. - /// - static bool IsNotCreatingPrefab; - /// /// Creates the prefab asset from the given root actor. Saves it to the output file path. /// diff --git a/Source/Engine/Terrain/TerrainChunk.cpp b/Source/Engine/Terrain/TerrainChunk.cpp index be145ae43..7db786ec2 100644 --- a/Source/Engine/Terrain/TerrainChunk.cpp +++ b/Source/Engine/Terrain/TerrainChunk.cpp @@ -287,7 +287,7 @@ void TerrainChunk::Serialize(SerializeStream& stream, const void* otherObj) if (HasLightmap() #if USE_EDITOR - && PrefabManager::IsNotCreatingPrefab + && !PrefabManager::IsCreatingPrefab #endif ) { From 7c55d50507387f86c3a07caf1a9530fb23f9616e Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 9 Jun 2023 23:26:37 +0200 Subject: [PATCH 3/8] Various minor code cleanup tweaks --- Source/Engine/Level/Prefabs/Prefab.cpp | 2 -- Source/Engine/Level/Prefabs/PrefabManager.cpp | 32 ++++--------------- .../Localization/LocalizedStringTable.cpp | 4 +-- Source/Engine/Scripting/ScriptingObject.h | 2 +- 4 files changed, 9 insertions(+), 31 deletions(-) diff --git a/Source/Engine/Level/Prefabs/Prefab.cpp b/Source/Engine/Level/Prefabs/Prefab.cpp index 1c0d422a9..9c1060c0f 100644 --- a/Source/Engine/Level/Prefabs/Prefab.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.cpp @@ -105,7 +105,6 @@ Asset::LoadResult Prefab::loadAsset() // Allocate memory for objects ObjectsIds.EnsureCapacity(objectsCount * 2); - NestedPrefabs.EnsureCapacity(objectsCount); ObjectsDataCache.EnsureCapacity(objectsCount * 3); // Find serialized object ids (actors and scripts), they are used later for IDs mapping on prefab spawning via PrefabManager @@ -122,7 +121,6 @@ Asset::LoadResult Prefab::loadAsset() } ObjectsIds.Add(objectId); - ASSERT(!ObjectsDataCache.ContainsKey(objectId)); ObjectsDataCache.Add(objectId, &objData); ObjectsCount++; diff --git a/Source/Engine/Level/Prefabs/PrefabManager.cpp b/Source/Engine/Level/Prefabs/PrefabManager.cpp index 700c86408..4c91bae25 100644 --- a/Source/Engine/Level/Prefabs/PrefabManager.cpp +++ b/Source/Engine/Level/Prefabs/PrefabManager.cpp @@ -38,7 +38,7 @@ PrefabManagerService PrefabManagerServiceInstance; Actor* PrefabManager::SpawnPrefab(Prefab* prefab) { - Actor* parent = Level::Scenes.Count() != 0 ? Level::Scenes[0] : nullptr; + Actor* parent = Level::Scenes.Count() != 0 ? Level::Scenes.Get()[0] : nullptr; return SpawnPrefab(prefab, parent, nullptr); } @@ -46,9 +46,7 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, const Vector3& position) { auto instance = SpawnPrefab(prefab); if (instance) - { instance->SetPosition(position); - } return instance; } @@ -56,12 +54,7 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, const Vector3& position, const { auto instance = SpawnPrefab(prefab); if (instance) - { - auto transform = instance->GetTransform(); - transform.Translation = position; - transform.Orientation = rotation; - instance->SetTransform(transform); - } + instance->SetTransform(Transform(position, rotation, instance->GetScale())); return instance; } @@ -69,13 +62,7 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, const Vector3& position, const { auto instance = SpawnPrefab(prefab); if (instance) - { - Transform transform; - transform.Translation = position; - transform.Orientation = rotation; - transform.Scale = scale; - instance->SetTransform(transform); - } + instance->SetTransform(Transform(position, rotation, scale)); return instance; } @@ -83,17 +70,13 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, const Transform& transform) { auto instance = SpawnPrefab(prefab); if (instance) - { instance->SetTransform(transform); - } return instance; } Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, Dictionary* objectsCache, bool withSynchronization) { PROFILE_CPU_NAMED("Prefab.Spawn"); - - // Validate input if (prefab == nullptr) { Log::ArgumentNullException(); @@ -110,12 +93,11 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryToString()); return nullptr; } + const Guid prefabId = prefab->GetID(); // Note: we need to generate unique Ids for the deserialized objects (actors and scripts) to prevent Ids collisions // Prefab asset during loading caches the object Ids stored inside the file - const Guid prefabId = prefab->GetID(); - // Prepare CollectionPoolCache::ScopeCache sceneObjects = ActorsCache::SceneObjectsListCache.Get(); sceneObjects->Resize(objectsCount); @@ -255,11 +237,11 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryAdd(prefabObjectId, obj); - obj->LinkPrefab(prefabId, prefabObjectId); + obj->_prefabID = prefabId; + obj->_prefabObjectID = prefabObjectId; } // Update transformations diff --git a/Source/Engine/Localization/LocalizedStringTable.cpp b/Source/Engine/Localization/LocalizedStringTable.cpp index e14328f89..578077076 100644 --- a/Source/Engine/Localization/LocalizedStringTable.cpp +++ b/Source/Engine/Localization/LocalizedStringTable.cpp @@ -63,9 +63,7 @@ Asset::LoadResult LocalizedStringTable::loadAsset() return result; JsonTools::GetString(Locale, *Data, "Locale"); - Guid fallbackTable = Guid::Empty; - JsonTools::GetGuid(fallbackTable, *Data, "FallbackTable"); - FallbackTable = fallbackTable; + JsonTools::GetReference(FallbackTable, *Data, "FallbackTable"); const auto entriesMember = SERIALIZE_FIND_MEMBER((*Data), "Entries"); if (entriesMember != Data->MemberEnd() && entriesMember->value.IsObject()) { diff --git a/Source/Engine/Scripting/ScriptingObject.h b/Source/Engine/Scripting/ScriptingObject.h index 183e23297..860226986 100644 --- a/Source/Engine/Scripting/ScriptingObject.h +++ b/Source/Engine/Scripting/ScriptingObject.h @@ -160,7 +160,7 @@ public: template static T* Cast(ScriptingObject* obj) { - return obj && CanCast(obj->GetClass(), T::GetStaticClass()) ? (T*)obj : nullptr; + return obj && CanCast(obj->GetClass(), T::GetStaticClass()) ? static_cast(obj) : nullptr; } bool Is(const ScriptingTypeHandle& type) const; From b299ed32469f59467eb00d6fc3f7c329cbd7bffc Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 9 Jun 2023 23:34:55 +0200 Subject: [PATCH 4/8] Add more assertions to tests build --- Source/Engine/Content/Assets/Model.cpp | 2 +- Source/Engine/Content/Content.cpp | 4 ++-- Source/Engine/Level/Prefabs/PrefabManager.cpp | 21 ++++++++++++++----- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/Source/Engine/Content/Assets/Model.cpp b/Source/Engine/Content/Assets/Model.cpp index 9319f2667..5a008645d 100644 --- a/Source/Engine/Content/Assets/Model.cpp +++ b/Source/Engine/Content/Assets/Model.cpp @@ -1012,7 +1012,7 @@ Asset::LoadResult Model::load() } } -#if BUILD_DEBUG || BUILD_DEVELOPMENT +#if !BUILD_RELEASE // Validate LODs for (int32 lodIndex = 1; lodIndex < LODs.Count(); lodIndex++) { diff --git a/Source/Engine/Content/Content.cpp b/Source/Engine/Content/Content.cpp index e58aa458d..c28ed61ef 100644 --- a/Source/Engine/Content/Content.cpp +++ b/Source/Engine/Content/Content.cpp @@ -304,7 +304,7 @@ bool Content::GetAssetInfo(const StringView& path, AssetInfo& info) auto storage = ContentStorageManager::GetStorage(path); if (storage) { -#if BUILD_DEBUG +#if BUILD_DEBUG || FLAX_TESTS ASSERT(storage->GetPath() == path); #endif @@ -904,7 +904,7 @@ bool Content::IsAssetTypeIdInvalid(const ScriptingTypeHandle& type, const Script if (!type || !assetType) return false; -#if BUILD_DEBUG +#if BUILD_DEBUG || FLAX_TESTS // Peek types for debugging const auto& typeObj = type.GetType(); const auto& assetTypeObj = assetType.GetType(); diff --git a/Source/Engine/Level/Prefabs/PrefabManager.cpp b/Source/Engine/Level/Prefabs/PrefabManager.cpp index 4c91bae25..cc42967a4 100644 --- a/Source/Engine/Level/Prefabs/PrefabManager.cpp +++ b/Source/Engine/Level/Prefabs/PrefabManager.cpp @@ -189,26 +189,26 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryGetParent() == nullptr) { - sceneObjects->At(i) = nullptr; LOG(Warning, "Scene object {0} {1} has missing parent object after load. Removing it.", obj->GetID(), obj->ToString()); + sceneObjects->At(i) = nullptr; obj->DeleteObject(); continue; } -#if USE_EDITOR && !BUILD_RELEASE +#if (USE_EDITOR && !BUILD_RELEASE) || FLAX_TESTS // Check for not being added to the parent (eg. invalid setup events fault on registration) auto actor = dynamic_cast(obj); auto script = dynamic_cast(obj); if (obj->GetParent() == obj || (actor && !actor->GetParent()->Children.Contains(actor)) || (script && !script->GetParent()->Scripts.Contains(script))) { - sceneObjects->At(i) = nullptr; LOG(Warning, "Scene object {0} {1} has invalid parent object linkage after load. Removing it.", obj->GetID(), obj->ToString()); + sceneObjects->At(i) = nullptr; obj->DeleteObject(); continue; } #endif -#if USE_EDITOR && BUILD_DEBUG +#if (USE_EDITOR && BUILD_DEBUG) || FLAX_TESTS // Check for being added to parent not from spawned prefab (eg. invalid parentId linkage fault) bool hasParentInInstance = false; for (int32 j = 0; j < sceneObjects->Count(); j++) @@ -221,11 +221,22 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryAt(i) = nullptr; LOG(Warning, "Scene object {0} {1} has invalid parent object after load. Removing it.", obj->GetID(), obj->ToString()); + sceneObjects->At(i) = nullptr; obj->DeleteObject(); continue; } + +#if FLAX_TESTS + // Perform extensive validation of the prefab instance structure + if (actor && actor->HasActorInHierarchy(actor)) + { + LOG(Warning, "Scene object {0} {1} has invalid hierarchy after load. Removing it.", obj->GetID(), obj->ToString()); + sceneObjects->At(i) = nullptr; + obj->DeleteObject(); + continue; + } +#endif #endif } From 60ddf0ea89794f0a2be324463642644bff411c09 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 9 Jun 2023 23:36:31 +0200 Subject: [PATCH 5/8] Add automated test for loading nested prefab with different root actor #1138 --- Source/Engine/Tests/TestPrefabs.cpp | 84 ++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/Source/Engine/Tests/TestPrefabs.cpp b/Source/Engine/Tests/TestPrefabs.cpp index 5656b6119..211fea930 100644 --- a/Source/Engine/Tests/TestPrefabs.cpp +++ b/Source/Engine/Tests/TestPrefabs.cpp @@ -8,7 +8,6 @@ #include "Engine/Level/Prefabs/Prefab.h" #include "Engine/Level/Prefabs/PrefabManager.h" #include "Engine/Scripting/ScriptingObjectReference.h" - #include TEST_CASE("Prefabs") @@ -330,4 +329,87 @@ TEST_CASE("Prefabs") Content::DeleteAsset(nestedActorPrefab); Content::DeleteAsset(testActorPrefab); } + SECTION("Test Loading Nested Prefab After Changing Root") + { + // https://github.com/FlaxEngine/FlaxEngine/issues/1138 + + // Create base prefab with 3 objects + AssetReference prefabBase = Content::CreateVirtualAsset(); + REQUIRE(prefabBase); + Guid id; + Guid::Parse("2b3334524c696dcfa93cabacd2a4f404", id); + prefabBase->ChangeID(id); + auto prefabBaseInit = prefabBase->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"82ce814f4d913e58eb35ab8b0b7e2eef\"," + "\"TypeName\": \"FlaxEngine.DirectionalLight\"," + "\"Name\": \"1\"" + "}," + "{" + "\"ID\": \"589bcfaa4bd1a53435129480e5bbdb3b\"," + "\"TypeName\": \"FlaxEngine.Camera\"," + "\"ParentID\": \"82ce814f4d913e58eb35ab8b0b7e2eef\"," + "\"Name\": \"2\"" + "}," + "{" + "\"ID\": \"9e81c24342e61af456411ea34593841d\"," + "\"TypeName\": \"FlaxEngine.UICanvas\"," + "\"ParentID\": \"589bcfaa4bd1a53435129480e5bbdb3b\"," + "\"Name\": \"3\"" + "}" + "]"); + REQUIRE(!prefabBaseInit); + + // Create nested prefab but with 'old' state where root object is different + AssetReference prefabNested = Content::CreateVirtualAsset(); + REQUIRE(prefabNested); + Guid::Parse("a71447e947cbd2deea018a8377636ce6", id); + prefabNested->ChangeID(id); + auto prefabNestedInit = prefabNested->Init(Prefab::TypeName, + "[" + "{" + "\"ID\": \"597ab8ea43a5c58b8d06f58f9364d261\"," + "\"PrefabID\": \"2b3334524c696dcfa93cabacd2a4f404\"," + "\"PrefabObjectID\": \"589bcfaa4bd1a53435129480e5bbdb3b\"" + "}," + "{" + "\"ID\": \"1a6228d84897ff3b2f444ea263c3657e\"," + "\"PrefabID\": \"2b3334524c696dcfa93cabacd2a4f404\"," + "\"PrefabObjectID\": \"82ce814f4d913e58eb35ab8b0b7e2eef\"," + "\"ParentID\": \"597ab8ea43a5c58b8d06f58f9364d261\"" + "}," + "{" + "\"ID\": \"f8fbee1349f749396ab6c2ad34f3afec\"," + "\"PrefabID\": \"2b3334524c696dcfa93cabacd2a4f404\"," + "\"PrefabObjectID\": \"9e81c24342e61af456411ea34593841d\"," + "\"ParentID\": \"597ab8ea43a5c58b8d06f58f9364d261\"" + "}" + "]"); + REQUIRE(!prefabNestedInit); + + // Spawn test instances of both prefabs + ScriptingObjectReference instanceBase = PrefabManager::SpawnPrefab(prefabBase); + ScriptingObjectReference instanceNested = PrefabManager::SpawnPrefab(prefabNested); + + // Verify scenario + REQUIRE(instanceBase); + REQUIRE(instanceBase->GetName() == TEXT("1")); + REQUIRE(instanceBase->GetChildrenCount() == 1); + REQUIRE(instanceBase->Children[0]->GetName() == TEXT("2")); + REQUIRE(instanceBase->Children[0]->GetChildrenCount() == 1); + REQUIRE(instanceBase->Children[0]->Children[0]->GetName() == TEXT("3")); + REQUIRE(instanceNested); + REQUIRE(instanceNested->GetName() == TEXT("1")); + REQUIRE(instanceNested->GetChildrenCount() == 1); + REQUIRE(instanceNested->Children[0]->GetName() == TEXT("2")); + REQUIRE(instanceNested->Children[0]->GetChildrenCount() == 1); + REQUIRE(instanceNested->Children[0]->Children[0]->GetName() == TEXT("3")); + + // Cleanup + instanceNested->DeleteObject(); + instanceBase->DeleteObject(); + Content::DeleteAsset(prefabNested); + Content::DeleteAsset(prefabBase); + } } From 958c7b2181ea808480f3f3609680eae51840c70e Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Sun, 11 Jun 2023 20:43:31 +0200 Subject: [PATCH 6/8] Fix spawning nested prefab with different root actor #1138 --- Source/Engine/Level/Prefabs/Prefab.Apply.cpp | 2 +- Source/Engine/Level/Prefabs/Prefab.cpp | 31 ++++++++++++++++ Source/Engine/Level/Prefabs/Prefab.h | 6 +--- Source/Engine/Level/Prefabs/PrefabManager.cpp | 36 ++++++++++++------- Source/Engine/Level/SceneObject.h | 1 + Source/Engine/Level/SceneObjectsFactory.cpp | 6 ++-- Source/Engine/Level/SceneObjectsFactory.h | 2 +- 7 files changed, 61 insertions(+), 23 deletions(-) diff --git a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp index a5abf2221..53453afe9 100644 --- a/Source/Engine/Level/Prefabs/Prefab.Apply.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.Apply.cpp @@ -336,7 +336,7 @@ bool PrefabInstanceData::SynchronizePrefabInstances(PrefabInstancesData& prefabI continue; } - SceneObject* obj = SceneObjectsFactory::Spawn(context, *(ISerializable::DeserializeStream*)data); + SceneObject* obj = SceneObjectsFactory::Spawn(context, *data); if (!obj) continue; obj->RegisterObject(); diff --git a/Source/Engine/Level/Prefabs/Prefab.cpp b/Source/Engine/Level/Prefabs/Prefab.cpp index 9c1060c0f..c2c2463b2 100644 --- a/Source/Engine/Level/Prefabs/Prefab.cpp +++ b/Source/Engine/Level/Prefabs/Prefab.cpp @@ -2,6 +2,7 @@ #include "Prefab.h" #include "Engine/Serialization/JsonTools.h" +#include "Engine/Content/Content.h" #include "Engine/Content/Factories/JsonAssetFactory.h" #include "Engine/Core/Log.h" #include "Engine/Level/Prefabs/PrefabManager.h" @@ -21,6 +22,36 @@ Prefab::Prefab(const SpawnParams& params, const AssetInfo* info) { } +Guid Prefab::GetRootObjectId() const +{ + ASSERT(IsLoaded()); + ScopeLock lock(Locker); + + // Root is always the first but handle case when prefab root was reordered in the base prefab while the nested prefab has still the old state + // TODO: resave and force sync prefabs during game cooking so this step could be skipped in game + int32 objectIndex = 0; + if (NestedPrefabs.HasItems()) + { + const auto& data = *Data; + const Guid basePrefabId = JsonTools::GetGuid(data[objectIndex], "PrefabID"); + if (const auto basePrefab = Content::Load(basePrefabId)) + { + const Guid basePrefabRootId = basePrefab->GetRootObjectId(); + for (int32 i = 0; i < ObjectsCount; i++) + { + const Guid prefabObjectId = JsonTools::GetGuid(data[i], "PrefabObjectID"); + if (prefabObjectId == basePrefabRootId) + { + objectIndex = i; + break; + } + } + } + } + + return ObjectsIds[objectIndex]; +} + Actor* Prefab::GetDefaultInstance() { ScopeLock lock(Locker); diff --git a/Source/Engine/Level/Prefabs/Prefab.h b/Source/Engine/Level/Prefabs/Prefab.h index ed82473c9..5e0075edf 100644 --- a/Source/Engine/Level/Prefabs/Prefab.h +++ b/Source/Engine/Level/Prefabs/Prefab.h @@ -50,11 +50,7 @@ public: /// /// Gets the root object identifier (prefab object ID). Asset must be loaded. /// - Guid GetRootObjectId() const - { - ASSERT(IsLoaded()); - return ObjectsIds[0]; - } + Guid GetRootObjectId() const; /// /// Requests the default prefab object instance. Deserializes the prefab objects from the asset. Skips if already done. diff --git a/Source/Engine/Level/Prefabs/PrefabManager.cpp b/Source/Engine/Level/Prefabs/PrefabManager.cpp index cc42967a4..88d80a41a 100644 --- a/Source/Engine/Level/Prefabs/PrefabManager.cpp +++ b/Source/Engine/Level/Prefabs/PrefabManager.cpp @@ -121,7 +121,7 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryAt(i) = obj; if (obj) obj->RegisterObject(); @@ -146,16 +146,25 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryIsEmpty()) + // Pick prefab root object + if (sceneObjects->IsEmpty()) { LOG(Warning, "No valid objects in prefab."); return nullptr; } - auto root = (Actor*)sceneObjects.Value->At(0); + Actor* root = nullptr; + const Guid prefabRootObjectId = prefab->GetRootObjectId(); + for (int32 i = 0; i < objectsCount; i++) + { + if (JsonTools::GetGuid(data[i], "ID") == prefabRootObjectId) + { + root = dynamic_cast(sceneObjects->At(i)); + break; + } + } if (!root) { - LOG(Warning, "Failed to load prefab root object."); + LOG(Warning, "Missing prefab root object."); return nullptr; } @@ -167,6 +176,8 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, Dictionary_parent) + root->_parent->Children.Remove(root); root->_parent = parent; if (parent) parent->Children.Add(root); @@ -174,16 +185,16 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryCount(); i++) { - auto obj = sceneObjects->At(i); + SceneObject* obj = sceneObjects->At(i); if (obj) obj->Initialize(); } // Delete objects without parent or with invalid linkage to the prefab - for (int32 i = 1; i < sceneObjects->Count(); i++) + for (int32 i = 0; i < sceneObjects->Count(); i++) { SceneObject* obj = sceneObjects->At(i); - if (!obj) + if (!obj || obj == root) continue; // Check for missing parent (eg. parent object has been deleted) @@ -251,8 +262,7 @@ Actor* PrefabManager::SpawnPrefab(Prefab* prefab, Actor* parent, DictionaryAdd(prefabObjectId, obj); - obj->_prefabID = prefabId; - obj->_prefabObjectID = prefabObjectId; + obj->LinkPrefab(prefabId, prefabObjectId); } // Update transformations @@ -317,7 +327,7 @@ bool PrefabManager::CreatePrefab(Actor* targetActor, const StringView& outputPat writer.StartArray(); for (int32 i = 0; i < sceneObjects->Count(); i++) { - SceneObject* obj = sceneObjects.Value->At(i); + SceneObject* obj = sceneObjects->At(i); writer.SceneObject(obj); } writer.EndArray(); @@ -335,7 +345,7 @@ bool PrefabManager::CreatePrefab(Actor* targetActor, const StringView& outputPat for (int32 i = 0; i < sceneObjects->Count(); i++) { // Generate new IDs for the prefab objects (other than reference instance used to create prefab) - const SceneObject* obj = sceneObjects.Value->At(i); + const SceneObject* obj = sceneObjects->At(i); objectInstanceIdToPrefabObjectId.Add(obj->GetSceneObjectId(), Guid::New()); } { @@ -382,7 +392,7 @@ bool PrefabManager::CreatePrefab(Actor* targetActor, const StringView& outputPat for (int32 i = 0; i < sceneObjects->Count(); i++) { - SceneObject* obj = sceneObjects.Value->At(i); + SceneObject* obj = sceneObjects->At(i); Guid prefabObjectId; if (objectInstanceIdToPrefabObjectId.TryGet(obj->GetSceneObjectId(), prefabObjectId)) diff --git a/Source/Engine/Level/SceneObject.h b/Source/Engine/Level/SceneObject.h index 0425a3a76..64bda4a59 100644 --- a/Source/Engine/Level/SceneObject.h +++ b/Source/Engine/Level/SceneObject.h @@ -58,6 +58,7 @@ API_CLASS(Abstract, NoSpawn) class FLAXENGINE_API SceneObject : public Scripting { DECLARE_SCRIPTING_TYPE_NO_SPAWN(SceneObject); friend PrefabInstanceData; + friend PrefabManager; friend Actor; friend Level; friend ScriptsFactory; diff --git a/Source/Engine/Level/SceneObjectsFactory.cpp b/Source/Engine/Level/SceneObjectsFactory.cpp index 35629dc6a..00fd29be0 100644 --- a/Source/Engine/Level/SceneObjectsFactory.cpp +++ b/Source/Engine/Level/SceneObjectsFactory.cpp @@ -31,7 +31,7 @@ void SceneObjectsFactory::Context::SetupIdsMapping(const SceneObject* obj) } } -SceneObject* SceneObjectsFactory::Spawn(Context& context, ISerializable::DeserializeStream& stream) +SceneObject* SceneObjectsFactory::Spawn(Context& context, const ISerializable::DeserializeStream& stream) { // Get object id Guid id = JsonTools::GetGuid(stream, "ID"); @@ -81,7 +81,7 @@ SceneObject* SceneObjectsFactory::Spawn(Context& context, ISerializable::Deseria context.Modifier->IdsMapping[prefabObjectId] = id; // Create prefab instance (recursive prefab loading to support nested prefabs) - obj = Spawn(context, *(ISerializable::DeserializeStream*)prefabData); + obj = Spawn(context, *prefabData); } else { @@ -584,7 +584,7 @@ void SceneObjectsFactory::SynchronizeNewPrefabInstance(Context& context, PrefabS data.Modifier->IdsMapping[prefabObjectId] = id; // Create prefab instance (recursive prefab loading to support nested prefabs) - auto child = Spawn(context, *(ISerializable::DeserializeStream*)prefabData); + auto child = Spawn(context, *prefabData); if (!child) { LOG(Warning, "Failed to create object {1} from prefab {0}.", prefab->ToString(), prefabObjectId); diff --git a/Source/Engine/Level/SceneObjectsFactory.h b/Source/Engine/Level/SceneObjectsFactory.h index 1534b7359..d3f5b3dd7 100644 --- a/Source/Engine/Level/SceneObjectsFactory.h +++ b/Source/Engine/Level/SceneObjectsFactory.h @@ -35,7 +35,7 @@ public: /// /// The serialization context. /// The serialized data stream. - static SceneObject* Spawn(Context& context, ISerializable::DeserializeStream& stream); + static SceneObject* Spawn(Context& context, const ISerializable::DeserializeStream& stream); /// /// Deserializes the scene object from the specified data value. From 31c9b85a3fe570e9175905d36925da07f2cf4ec9 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Sun, 11 Jun 2023 20:44:04 +0200 Subject: [PATCH 7/8] Fix prefab diff context menu in Editor to properly diff against arrays --- Source/Editor/CustomEditors/Dedicated/ActorEditor.cs | 11 ++++------- .../Editor/CustomEditors/Values/ListValueContainer.cs | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs b/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs index a8773a8d7..efa13d48a 100644 --- a/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs +++ b/Source/Editor/CustomEditors/Dedicated/ActorEditor.cs @@ -218,7 +218,7 @@ namespace FlaxEditor.CustomEditors.Dedicated node.Text = Utilities.Utils.GetPropertyNameUI(sceneObject.GetType().Name); } // Array Item - else if (editor.ParentEditor?.Values?.Type.IsArray ?? false) + else if (editor.ParentEditor is CollectionEditor) { node.Text = "Element " + editor.ParentEditor.ChildrenEditors.IndexOf(editor); } @@ -261,16 +261,14 @@ namespace FlaxEditor.CustomEditors.Dedicated } // Skip if no change detected - if (!editor.Values.IsReferenceValueModified && skipIfNotModified) + var isRefEdited = editor.Values.IsReferenceValueModified; + if (!isRefEdited && skipIfNotModified) return null; TreeNode result = null; - - if (editor.ChildrenEditors.Count == 0) + if (editor.ChildrenEditors.Count == 0 || (isRefEdited && editor is CollectionEditor)) result = CreateDiffNode(editor); - bool isScriptEditorWithRefValue = editor is ScriptsEditor && editor.Values.HasReferenceValue; - for (int i = 0; i < editor.ChildrenEditors.Count; i++) { var child = ProcessDiff(editor.ChildrenEditors[i], !isScriptEditorWithRefValue); @@ -278,7 +276,6 @@ namespace FlaxEditor.CustomEditors.Dedicated { if (result == null) result = CreateDiffNode(editor); - result.AddChild(child); } } diff --git a/Source/Editor/CustomEditors/Values/ListValueContainer.cs b/Source/Editor/CustomEditors/Values/ListValueContainer.cs index 13098b503..e19c5ad53 100644 --- a/Source/Editor/CustomEditors/Values/ListValueContainer.cs +++ b/Source/Editor/CustomEditors/Values/ListValueContainer.cs @@ -51,7 +51,7 @@ namespace FlaxEditor.CustomEditors if (values.HasReferenceValue) { - if (values.ReferenceValue is IList v && values.Count == v.Count && v.Count > index) + if (values.ReferenceValue is IList v && v.Count > index) { _referenceValue = v[index]; _hasReferenceValue = true; From 5f8e5d44dc7b87b83e1ba18a0be33d3d4be2fdeb Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Sun, 11 Jun 2023 21:35:50 +0200 Subject: [PATCH 8/8] Fix RPC invoking on object with different ID but matching parent and type --- Source/Engine/Networking/NetworkManager.cpp | 2 +- Source/Engine/Networking/NetworkReplicator.cpp | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Source/Engine/Networking/NetworkManager.cpp b/Source/Engine/Networking/NetworkManager.cpp index 88c59ad9a..b18136e23 100644 --- a/Source/Engine/Networking/NetworkManager.cpp +++ b/Source/Engine/Networking/NetworkManager.cpp @@ -15,7 +15,7 @@ #include "Engine/Profiler/ProfilerCPU.h" #include "Engine/Scripting/Scripting.h" -#define NETWORK_PROTOCOL_VERSION 1 +#define NETWORK_PROTOCOL_VERSION 2 float NetworkManager::NetworkFPS = 60.0f; NetworkPeer* NetworkManager::Peer = nullptr; diff --git a/Source/Engine/Networking/NetworkReplicator.cpp b/Source/Engine/Networking/NetworkReplicator.cpp index 6f6211b74..3c08a01aa 100644 --- a/Source/Engine/Networking/NetworkReplicator.cpp +++ b/Source/Engine/Networking/NetworkReplicator.cpp @@ -95,6 +95,8 @@ PACK_STRUCT(struct NetworkMessageObjectRpc { NetworkMessageIDs ID = NetworkMessageIDs::ObjectRpc; Guid ObjectId; + Guid ParentId; + char ObjectTypeName[128]; // TODO: introduce networked-name to synchronize unique names as ushort (less data over network) char RpcTypeName[128]; // TODO: introduce networked-name to synchronize unique names as ushort (less data over network) char RpcName[128]; // TODO: introduce networked-name to synchronize unique names as ushort (less data over network) uint16 ArgsSize; @@ -1546,11 +1548,14 @@ void NetworkInternal::NetworkReplicatorUpdate() //NETWORK_REPLICATOR_LOG(Info, "[NetworkReplicator] Rpc {}::{} object ID={}", e.Name.First.ToString(), String(e.Name.Second), item.ToString()); NetworkMessageObjectRpc msgData; msgData.ObjectId = item.ObjectId; + msgData.ParentId = item.ParentId; if (isClient) { // Remap local client object ids into server ids IdsRemappingTable.KeyOf(msgData.ObjectId, &msgData.ObjectId); + IdsRemappingTable.KeyOf(msgData.ParentId, &msgData.ParentId); } + GetNetworkName(msgData.ObjectTypeName, obj->GetType().Fullname); GetNetworkName(msgData.RpcTypeName, e.Name.First.GetType().Fullname); GetNetworkName(msgData.RpcName, e.Name.Second); msgData.ArgsSize = (uint16)e.ArgsData.Length(); @@ -1949,7 +1954,7 @@ void NetworkInternal::OnNetworkMessageObjectRpc(NetworkEvent& event, NetworkClie NetworkMessageObjectRpc msgData; event.Message.ReadStructure(msgData); ScopeLock lock(ObjectsLock); - NetworkReplicatedObject* e = ResolveObject(msgData.ObjectId); + NetworkReplicatedObject* e = ResolveObject(msgData.ObjectId, msgData.ParentId, msgData.ObjectTypeName); if (e) { auto& item = *e;