diff --git a/OloEngine/src/OloEngine/Scene/SceneSerializer.cpp b/OloEngine/src/OloEngine/Scene/SceneSerializer.cpp index b79675c0e..bb01b04d6 100644 --- a/OloEngine/src/OloEngine/Scene/SceneSerializer.cpp +++ b/OloEngine/src/OloEngine/Scene/SceneSerializer.cpp @@ -1601,7 +1601,8 @@ namespace OloEngine return Static; } - static void DeserializeEntityComponents(Entity& deserializedEntity, const YAML::Node& entity) + static void DeserializeEntityComponents(Entity& deserializedEntity, const YAML::Node& entity, + std::unordered_map>& animatedModelCache) { if (auto transformComponent = entity["TransformComponent"]; transformComponent) { @@ -2883,7 +2884,24 @@ namespace OloEngine auto absolutePath = assetDirectory / relativePath; anim.m_SourceFilePath = absolutePath.string(); - auto animatedModel = Ref::Create(anim.m_SourceFilePath); + // Dedup: many entities in a crowd scene share the same source + // path (issue #525 cheap-wins slice). Reuse the already-loaded + // AnimatedModel instead of re-running Assimp import + allocating + // a fresh MeshSource/GPU-buffer graph per entity. The model/mesh + // data is shared; PopulateAnimatedEntity only reads from it and + // writes into this entity's own components, so per-entity + // playback state (below) stays independent. + Ref animatedModel; + if (auto cacheIt = animatedModelCache.find(anim.m_SourceFilePath); cacheIt != animatedModelCache.end()) + { + animatedModel = cacheIt->second; + } + else + { + animatedModel = Ref::Create(anim.m_SourceFilePath); + animatedModelCache.emplace(anim.m_SourceFilePath, animatedModel); + } + if (animatedModel) { // Reload MeshComponent / SkeletonComponent / AnimationStateComponent / @@ -5917,7 +5935,7 @@ namespace OloEngine Entity deserializedEntity = m_Scene->CreateEntityWithUUID(uuid, name); try { - DeserializeEntityComponents(deserializedEntity, entityNode); + DeserializeEntityComponents(deserializedEntity, entityNode, m_AnimatedModelCache); } catch (...) { @@ -6424,6 +6442,17 @@ namespace OloEngine auto entities = data["Entities"]; if (entities && entities.IsSequence()) { + // Pre-size the entity pool and the 3 components every entity gets + // (Scene::CreateEntityWithUUID) so none of them reallocate mid-load + // for large scenes (issue #525 cheap-wins slice). Reserve is safe to + // call regardless of EnTT owning-group membership — it resizes the + // existing storage without touching group ordering. + const sizet entityCount = entities.size(); + m_Scene->m_Registry.storage().reserve(entityCount); + m_Scene->m_Registry.storage().reserve(entityCount); + m_Scene->m_Registry.storage().reserve(entityCount); + m_Scene->m_Registry.storage().reserve(entityCount); + for (auto entity : entities) { try @@ -6495,6 +6524,19 @@ namespace OloEngine createdUUIDs.reserve(entitiesNode.size()); + // Same reserve rationale as the primary Deserialize path above (issue #525): + // pre-size the entity pool plus the 3 always-added components so additive + // loads of large scenes don't reallocate mid-load. This grows existing + // storage (entities may already be present), so it's a strict upper bound, + // not exact — still avoids the repeated-reallocation cost. + { + const sizet additionalCount = entitiesNode.size(); + m_Scene->m_Registry.storage().reserve(m_Scene->m_Registry.storage().size() + additionalCount); + m_Scene->m_Registry.storage().reserve(m_Scene->m_Registry.storage().size() + additionalCount); + m_Scene->m_Registry.storage().reserve(m_Scene->m_Registry.storage().size() + additionalCount); + m_Scene->m_Registry.storage().reserve(m_Scene->m_Registry.storage().size() + additionalCount); + } + for (auto entity : entitiesNode) { try diff --git a/OloEngine/src/OloEngine/Scene/SceneSerializer.h b/OloEngine/src/OloEngine/Scene/SceneSerializer.h index 8ad7a87f9..a939183a5 100644 --- a/OloEngine/src/OloEngine/Scene/SceneSerializer.h +++ b/OloEngine/src/OloEngine/Scene/SceneSerializer.h @@ -2,9 +2,12 @@ #include "OloEngine/Scene/Scene.h" #include "OloEngine/Core/UUID.h" +#include "OloEngine/Renderer/AnimatedModel.h" #include #include +#include +#include #include #include @@ -52,5 +55,15 @@ namespace OloEngine Entity DeserializeEntity(u64 uuid, const std::string& name, const YAML::Node& entityNode); Ref m_Scene; + + // Dedup cache for AnimatedModel loads within a single Deserialize() / + // DeserializeFromYAML() / DeserializeAdditive() call, keyed by resolved + // absolute source path (issue #525 cheap-wins slice). Many entities in a + // crowd scene share the same rigged-character source file; without this, + // each one re-runs Assimp import + allocates a fresh MeshSource/GPU-buffer + // graph. The model/mesh data is shared via Ref; per-entity + // playback state (AnimationStateComponent) is still populated freshly for + // every entity, so animations stay independent per entity. + std::unordered_map> m_AnimatedModelCache; }; } // namespace OloEngine diff --git a/OloEngine/tests/CMakeLists.txt b/OloEngine/tests/CMakeLists.txt index e8859d72e..6f26240de 100644 --- a/OloEngine/tests/CMakeLists.txt +++ b/OloEngine/tests/CMakeLists.txt @@ -142,6 +142,7 @@ add_executable(OloEngine-Tests Rendering/RenderingRegressionTest.cpp Rendering/SelectionOutlineTest.cpp Rendering/DeccerCubesLoadingTest.cpp + Rendering/AnimatedModelDedupTest.cpp # Rendering property tests (require GL 4.5+ context; skip gracefully if unavailable) Rendering/PropertyTests/RenderPropertyTest.cpp Rendering/PropertyTests/PlanarReflectionVisualEvidenceTest.cpp diff --git a/OloEngine/tests/Rendering/AnimatedModelDedupTest.cpp b/OloEngine/tests/Rendering/AnimatedModelDedupTest.cpp new file mode 100644 index 000000000..5e2484f4c --- /dev/null +++ b/OloEngine/tests/Rendering/AnimatedModelDedupTest.cpp @@ -0,0 +1,186 @@ +// OLO_TEST_LAYER: integration +// ============================================================================= +// AnimatedModelDedupTest — issue #525 cheap-wins slice. +// +// Pins the SceneSerializer::DeserializeEntityComponents dedup cache: when a +// scene has multiple entities whose AnimationStateComponent shares the same +// resolved SourceFilePath (e.g. a crowd of the same rigged character), the +// deserializer must reuse the already-loaded Ref instead of +// re-running Assimp import + allocating a fresh MeshSource/GPU-buffer graph +// per entity. Model/mesh data (MeshComponent::m_MeshSource, +// SkeletonComponent::m_Skeleton) must end up SHARED (same Ref target), while +// per-entity playback state (AnimationStateComponent scalars) must stay +// independent — that's the correctness subtlety the HANDOVER.md research +// flagged as needing verification, not just a speed measurement. +// +// Needs a live GL context: loading the real glTF exercises the full +// AnimatedModel -> MeshSource::Build() path (glCreateVertexArrays / GPU +// buffers), same as AssetSceneLoadTest / DeccerCubesLoadingTest. Skips +// cleanly on a headless box with no GPU. +// ============================================================================= + +#include "OloEnginePCH.h" + +#include + +#include "OloEngine/Animation/AnimatedMeshComponents.h" +#include "OloEngine/Project/Project.h" +#include "OloEngine/Renderer/Renderer.h" +#include "OloEngine/Renderer/Renderer3D.h" +#include "OloEngine/Renderer/RendererTypes.h" +#include "OloEngine/Renderer/Debug/GLStateGuard.h" +#include "OloEngine/Scene/Components.h" +#include "OloEngine/Scene/Entity.h" +#include "OloEngine/Scene/Scene.h" +#include "OloEngine/Scene/SceneSerializer.h" + +#include "Rendering/PropertyTests/RenderPropertyTest.h" + +#include +#include +#include + +#ifndef OLO_TEST_EDITOR_ROOT +#error "OLO_TEST_EDITOR_ROOT must be defined by the test target's CMake — see OloEngine/tests/CMakeLists.txt" +#endif + +namespace OloEngine::Tests +{ + namespace + { + namespace fs = std::filesystem; + + Entity FindByTag(Scene& scene, const std::string& tag) + { + auto view = scene.GetAllEntitiesWith(); + for (auto e : view) + { + if (view.get(e).Tag == tag) + return Entity{ e, &scene }; + } + return {}; + } + + // Points Project::GetAssetDirectory() at the real OloEditor/assets tree + // (read-only — AnimatedModel::Create loads directly from disk, no asset + // manager involved) by writing a throwaway .oloproj into a temp dir whose + // AssetDirectory field is an ABSOLUTE path. ProjectSerializer keeps an + // absolute AssetDirectory as-is (does not resolve it against the project + // file's own directory), so Project::GetAssetDirectory() resolves to + // exactly that path regardless of where the .oloproj itself lives. + fs::path WriteThrowawayProjectPointingAt(const fs::path& assetDir, const fs::path& tempDir) + { + std::error_code ec; + fs::create_directories(tempDir, ec); + + const fs::path projectFile = tempDir / "DedupTest.oloproj"; + std::ofstream out(projectFile); + out << "Project:\n" + << " Name: DedupTest\n" + << " StartScene: \"dummy.olo\"\n" + << " AssetDirectory: \"" << assetDir.generic_string() << "\"\n" + << " ScriptModulePath: \"dummy.dll\"\n"; + out.close(); + return projectFile; + } + } // namespace + + TEST(AnimatedModelDedup, SharedSourcePathReusesModelButKeepsPlaybackStateIndependent) + { + // AnimatedModel -> MeshSource::Build() needs a live GL 4.6 context. + OLO_ENSURE_GPU_OR_SKIP(); + + if (!Renderer3D::IsInitialized()) + { + Renderer::Init(RendererType::Renderer3D, /*loadingWindow=*/nullptr); + } + GLStateGuard glGuard("AnimatedModelDedup.Deserialize", GLStateGuard::Policy::Restore); + + const fs::path assetDir = fs::path{ OLO_TEST_EDITOR_ROOT } / "assets"; + const fs::path modelPath = assetDir / "models" / "RiggedSimple" / "RiggedSimple.gltf"; + ASSERT_TRUE(fs::exists(modelPath)) + << "Fixture asset missing: " << modelPath.string(); + + const fs::path tempDir = fs::temp_directory_path() / "OloEngineAnimatedModelDedupTest"; + std::error_code ec; + fs::remove_all(tempDir, ec); + const fs::path projectFile = WriteThrowawayProjectPointingAt(assetDir, tempDir); + struct Cleanup + { + fs::path Dir; + ~Cleanup() + { + std::error_code cleanupEc; + fs::remove_all(Dir, cleanupEc); + } + } cleanup{ tempDir }; + + ASSERT_TRUE(Project::Load(projectFile)) << "Project::Load failed for throwaway test project."; + ASSERT_EQ(Project::GetAssetDirectory(), assetDir) + << "Absolute AssetDirectory must resolve verbatim, not relative to the project file."; + + // Two entities pointing at the SAME source model, with distinct + // playback scalars so a state-sharing bug (rather than a model-sharing + // win) would be visible after reload. + constexpr const char* kTagA = "DedupCrowdMemberA"; + constexpr const char* kTagB = "DedupCrowdMemberB"; + std::string yaml; + { + auto scene = Scene::Create(); + + Entity entityA = scene->CreateEntity(kTagA); + auto& animA = entityA.AddComponent(); + animA.m_SourceFilePath = modelPath.string(); + animA.m_CurrentTime = 0.25f; + animA.m_IsPlaying = true; + + Entity entityB = scene->CreateEntity(kTagB); + auto& animB = entityB.AddComponent(); + animB.m_SourceFilePath = modelPath.string(); + animB.m_CurrentTime = 0.75f; + animB.m_IsPlaying = false; + + yaml = SceneSerializer(scene).SerializeToYAML(); + } + + auto reloaded = Scene::Create(); + ASSERT_TRUE(SceneSerializer(reloaded).DeserializeFromYAML(yaml)) + << "Deserialize failed for the shared-source-path crowd scene."; + + Entity restoredA = FindByTag(*reloaded, kTagA); + Entity restoredB = FindByTag(*reloaded, kTagB); + ASSERT_TRUE(static_cast(restoredA)) << "Entity '" << kTagA << "' missing after reload."; + ASSERT_TRUE(static_cast(restoredB)) << "Entity '" << kTagB << "' missing after reload."; + + // --- Model data is SHARED (the dedup win) --- + ASSERT_TRUE(restoredA.HasComponent()); + ASSERT_TRUE(restoredB.HasComponent()); + EXPECT_EQ(restoredA.GetComponent().m_MeshSource, + restoredB.GetComponent().m_MeshSource) + << "Two entities sharing a SourceFilePath must reuse the same MeshSource " + "instead of each re-running Assimp import (issue #525 dedup cache)."; + + ASSERT_TRUE(restoredA.HasComponent()); + ASSERT_TRUE(restoredB.HasComponent()); + EXPECT_EQ(restoredA.GetComponent().m_Skeleton, + restoredB.GetComponent().m_Skeleton) + << "Two entities sharing a SourceFilePath must reuse the same Skeleton."; + + // --- Playback state stays INDEPENDENT per entity --- + ASSERT_TRUE(restoredA.HasComponent()); + ASSERT_TRUE(restoredB.HasComponent()); + const auto& restoredAnimA = restoredA.GetComponent(); + const auto& restoredAnimB = restoredB.GetComponent(); + EXPECT_NEAR(restoredAnimA.m_CurrentTime, 0.25f, 1e-5f); + EXPECT_NEAR(restoredAnimB.m_CurrentTime, 0.75f, 1e-5f); + EXPECT_TRUE(restoredAnimA.m_IsPlaying); + EXPECT_FALSE(restoredAnimB.m_IsPlaying); + + // Mutating one entity's playback state post-load must not leak into the + // other, even though they share the underlying model/skeleton Refs. + restoredA.GetComponent().m_CurrentTime = 0.9f; + EXPECT_NEAR(restoredB.GetComponent().m_CurrentTime, 0.75f, 1e-5f) + << "Entity B's playback state was corrupted by mutating entity A — " + "the dedup cache must not alias per-entity AnimationStateComponents."; + } +} // namespace OloEngine::Tests