From d21c2ccfd7eaf810910edfad00ba65325b1ac1b8 Mon Sep 17 00:00:00 2001 From: Bram Verhulst Date: Sun, 18 Oct 2026 17:15:51 +0200 Subject: [PATCH] feat: add ObjectRef for scene loading to not rely on runtime id's --- destrum/CMakeLists.txt | 1 + .../include/destrum/ObjectModel/GameObject.h | 3 - .../include/destrum/ObjectModel/ObjectRef.h | 37 +++++ destrum/src/ObjectModel/GameObject.cpp | 13 -- destrum/src/ObjectModel/ObjectRef.cpp | 38 +++++ destrum/src/Serialization/SceneSerializer.cpp | 15 +- .../include/components/InteractableButton.h | 7 +- .../src/components/InteractableButton.cpp | 34 ++-- tests/CMakeLists.txt | 4 + tests/destrum_tests.cpp | 145 ++++++++++++++++++ 10 files changed, 262 insertions(+), 35 deletions(-) create mode 100644 destrum/include/destrum/ObjectModel/ObjectRef.h create mode 100644 destrum/src/ObjectModel/ObjectRef.cpp diff --git a/destrum/CMakeLists.txt b/destrum/CMakeLists.txt index 06e8d66..25ae391 100644 --- a/destrum/CMakeLists.txt +++ b/destrum/CMakeLists.txt @@ -96,6 +96,7 @@ set(SRC_FILES "src/ObjectModel/Component.cpp" "src/ObjectModel/GameObject.cpp" "src/ObjectModel/Object.cpp" + "src/ObjectModel/ObjectRef.cpp" "src/ObjectModel/Transform.cpp" "src/Scene/Scene.cpp" diff --git a/destrum/include/destrum/ObjectModel/GameObject.h b/destrum/include/destrum/ObjectModel/GameObject.h index ef8cd79..008588b 100644 --- a/destrum/include/destrum/ObjectModel/GameObject.h +++ b/destrum/include/destrum/ObjectModel/GameObject.h @@ -44,9 +44,6 @@ public: [[nodiscard]] ObjectId GetId() const { return m_Id; } - // Use this only while loading a scene. - void SetIdForDeserialization(ObjectId id); - [[nodiscard]] Scene* GetScene() const { return m_Scene; } [[nodiscard]] std::vector>& GetComponents() { diff --git a/destrum/include/destrum/ObjectModel/ObjectRef.h b/destrum/include/destrum/ObjectModel/ObjectRef.h new file mode 100644 index 0000000..eb9d22b --- /dev/null +++ b/destrum/include/destrum/ObjectModel/ObjectRef.h @@ -0,0 +1,37 @@ +#ifndef DESTRUM_OBJECTREF_H +#define DESTRUM_OBJECTREF_H + +#include + +#include + +#include + +class GameObject; +class Scene; + +class ObjectRef final { +public: + using ObjectMap = std::unordered_map; + + ObjectRef() = default; + explicit ObjectRef(ObjectId id) : m_Id(id) {} + explicit ObjectRef(const GameObject& object); + + [[nodiscard]] ObjectId GetId() const { return m_Id; } + [[nodiscard]] bool IsSet() const { return m_Id != InvalidObjectId; } + + [[nodiscard]] GameObject* Get(const Scene& scene) const; + + bool Resolve(const ObjectMap& objects); + + bool operator==(const ObjectRef&) const = default; + +private: + ObjectId m_Id{InvalidObjectId}; +}; + +void to_json(nlohmann::json& json, const ObjectRef& ref); +void from_json(const nlohmann::json& json, ObjectRef& ref); + +#endif //DESTRUM_OBJECTREF_H diff --git a/destrum/src/ObjectModel/GameObject.cpp b/destrum/src/ObjectModel/GameObject.cpp index fa1144f..5f2f039 100644 --- a/destrum/src/ObjectModel/GameObject.cpp +++ b/destrum/src/ObjectModel/GameObject.cpp @@ -39,19 +39,6 @@ GameObject::GameObject(const std::string& name) m_Id(AllocateId()) { } -void GameObject::SetIdForDeserialization(ObjectId id) { - if (id == InvalidObjectId || id >= std::numeric_limits::max() - 1) { - throw std::out_of_range("Object ID is reserved or cannot be incremented"); - } - - m_Id = id; - - // Prevent newly created objects from reusing loaded IDs. - if (id != std::numeric_limits::max() && id >= s_NextId) { - s_NextId = id + 1; - } -} - ObjectId GameObject::AllocateId() { if (s_NextId == InvalidObjectId || s_NextId >= std::numeric_limits::max() - 1) { diff --git a/destrum/src/ObjectModel/ObjectRef.cpp b/destrum/src/ObjectModel/ObjectRef.cpp new file mode 100644 index 0000000..56994a2 --- /dev/null +++ b/destrum/src/ObjectModel/ObjectRef.cpp @@ -0,0 +1,38 @@ +#include + +#include + +#include +#include + +ObjectRef::ObjectRef(const GameObject& object) + : m_Id(object.GetId()) { +} + +GameObject* ObjectRef::Get(const Scene& scene) const { + return scene.FindGameObject(m_Id); +} + +bool ObjectRef::Resolve(const ObjectMap& objects) { + if (m_Id == InvalidObjectId) { + return true; + } + + const auto it = objects.find(m_Id); + if (it == objects.end() || it->second == nullptr) { + // Keeping an unmatched file id would alias an unrelated runtime object. + m_Id = InvalidObjectId; + return false; + } + + m_Id = it->second->GetId(); + return true; +} + +void to_json(nlohmann::json& json, const ObjectRef& ref) { + json = ref.GetId(); +} + +void from_json(const nlohmann::json& json, ObjectRef& ref) { + ref = json.is_number_unsigned() ? ObjectRef(json.get()) : ObjectRef{}; +} diff --git a/destrum/src/Serialization/SceneSerializer.cpp b/destrum/src/Serialization/SceneSerializer.cpp index b9aac05..e6fdd56 100644 --- a/destrum/src/Serialization/SceneSerializer.cpp +++ b/destrum/src/Serialization/SceneSerializer.cpp @@ -563,8 +563,8 @@ bool SceneSerializer::ConstructScene(Scene& scene, LoadResult& result, const std::string name = objectJson.value("name", "GameObject"); const bool active = objectJson.value("active", true); + // File ids are local to the file, every loaded object gets a fresh runtime id GameObject* object = stagingScene.CreateGameObject(name); - object->SetIdForDeserialization(id); object->SetActive(active); idMap.emplace(id, object); @@ -687,11 +687,14 @@ bool SceneSerializer::ConstructScene(Scene& scene, LoadResult& result, scene.m_name = root.at("name").get(); } scene.m_SelectedObjectId = InvalidObjectId; - scene.m_CurrentCameraId = root.value("currentCamera", InvalidObjectId); - if (scene.m_CurrentCameraId != InvalidObjectId && !idMap.contains(scene.m_CurrentCameraId)) { - spdlog::warn("Scene \"{}\" current camera id {} does not exist, ignoring it", - scene.m_name, scene.m_CurrentCameraId); - scene.m_CurrentCameraId = InvalidObjectId; + scene.m_CurrentCameraId = InvalidObjectId; + if (const ObjectId cameraFileId = root.value("currentCamera", InvalidObjectId); cameraFileId != InvalidObjectId) { + if (const auto it = idMap.find(cameraFileId); it != idMap.end()) { + scene.m_CurrentCameraId = it->second->GetId(); + } else { + spdlog::warn("Scene \"{}\" current camera id {} does not exist, ignoring it", + scene.m_name, cameraFileId); + } } scene.m_Environment = SceneEnvironment{}; if (root.contains("environment") && root.at("environment").is_object()) { diff --git a/lightkeeper/include/components/InteractableButton.h b/lightkeeper/include/components/InteractableButton.h index 4a19d9f..ff2d9ee 100644 --- a/lightkeeper/include/components/InteractableButton.h +++ b/lightkeeper/include/components/InteractableButton.h @@ -6,7 +6,7 @@ #include #include -#include +#include class InteractableButton final : public Interactable { public: @@ -19,6 +19,7 @@ public: nlohmann::json Serialize() const override; void Deserialize(const nlohmann::json& data) override; + void ResolveReferences(const ObjectMap& objects) override; void ImGuiInspector() override; [[nodiscard]] std::size_t GetColorIndex() const { return m_ColorIndex; } @@ -29,7 +30,7 @@ public: // Every Activatable on a target object is activated when the button is pressed void AddTarget(const GameObject& target); void RemoveTarget(ObjectId target); - [[nodiscard]] const std::vector& GetTargets() const { return m_Targets; } + [[nodiscard]] const std::vector& GetTargets() const { return m_Targets; } private: void ApplyColor(); @@ -39,7 +40,7 @@ private: std::size_t m_ColorIndex{0}; int m_PressCount{0}; bool m_CycleColors{true}; - std::vector m_Targets; + std::vector m_Targets; }; #endif // LIGHTKEEPER_INTERACTABLEBUTTON_H diff --git a/lightkeeper/src/components/InteractableButton.cpp b/lightkeeper/src/components/InteractableButton.cpp index 995f4da..a306aa4 100644 --- a/lightkeeper/src/components/InteractableButton.cpp +++ b/lightkeeper/src/components/InteractableButton.cpp @@ -52,15 +52,16 @@ void InteractableButton::Interact(GameObject& interactor) void InteractableButton::AddTarget(const GameObject& target) { - if (target.GetId() == InvalidObjectId || std::ranges::find(m_Targets, target.GetId()) != m_Targets.end()) { + const ObjectRef ref(target); + if (!ref.IsSet() || std::ranges::find(m_Targets, ref) != m_Targets.end()) { return; } - m_Targets.push_back(target.GetId()); + m_Targets.push_back(ref); } void InteractableButton::RemoveTarget(ObjectId target) { - std::erase(m_Targets, target); + std::erase(m_Targets, ObjectRef(target)); } void InteractableButton::ActivateTargets(GameObject& interactor) @@ -70,10 +71,10 @@ void InteractableButton::ActivateTargets(GameObject& interactor) return; } - for (const ObjectId targetId : m_Targets) { - GameObject* target = scene->FindGameObject(targetId); + for (const ObjectRef& targetRef : m_Targets) { + GameObject* target = targetRef.Get(*scene); if (target == nullptr) { - spdlog::warn("{} targets object {} which no longer exists", GetGameObject()->GetName(), targetId); + spdlog::warn("{} targets object {} which no longer exists", GetGameObject()->GetName(), targetRef.GetId()); continue; } @@ -122,7 +123,19 @@ void InteractableButton::Deserialize(const nlohmann::json& data) m_ColorIndex = data.value("colorIndex", m_ColorIndex) % ButtonColors.size(); m_CycleColors = data.value("cycleColors", m_CycleColors); m_PressCount = data.value("pressCount", m_PressCount); - m_Targets = data.value("targets", std::vector{}); + m_Targets = data.value("targets", std::vector{}); +} + +void InteractableButton::ResolveReferences(const ObjectMap& objects) +{ + std::erase_if(m_Targets, [&](ObjectRef& target) { + const ObjectId fileId = target.GetId(); + if (!target.Resolve(objects)) { + spdlog::warn("{} targets object {} which is not in the scene file, dropping it", GetGameObject()->GetName(), fileId); + return true; + } + return !target.IsSet(); + }); } void InteractableButton::ImGuiInspector() @@ -149,8 +162,9 @@ void InteractableButton::DrawTargetsInspector() } std::optional targetToRemove; - for (const ObjectId targetId : m_Targets) { - const GameObject* target = scene->FindGameObject(targetId); + for (const ObjectRef& targetRef : m_Targets) { + const ObjectId targetId = targetRef.GetId(); + const GameObject* target = targetRef.Get(*scene); ImGui::PushID(static_cast(targetId)); if (ImGui::SmallButton("x")) { targetToRemove = targetId; @@ -170,7 +184,7 @@ void InteractableButton::DrawTargetsInspector() if (ImGui::BeginCombo("##AddTarget", "Add target...")) { for (const auto& object : scene->GetObjects()) { const bool alreadyTargeted = object != nullptr && - std::ranges::find(m_Targets, object->GetId()) != m_Targets.end(); + std::ranges::find(m_Targets, ObjectRef(*object)) != m_Targets.end(); if (object == nullptr || object.get() == GetGameObject() || alreadyTargeted || object->GetComponent() == nullptr) { continue; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ef64f60..e387345 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,5 +1,8 @@ add_executable(destrum_tests destrum_tests.cpp + ${CMAKE_SOURCE_DIR}/lightkeeper/src/components/Activatable.cpp + ${CMAKE_SOURCE_DIR}/lightkeeper/src/components/Interactable.cpp + ${CMAKE_SOURCE_DIR}/lightkeeper/src/components/InteractableButton.cpp ) set_target_properties(destrum_tests PROPERTIES @@ -12,6 +15,7 @@ target_compile_definitions(destrum_tests PRIVATE SDL_MAIN_HANDLED DESTRUM_ENGINE_ASSETS_SRC_DIR="${CMAKE_SOURCE_DIR}/destrum/assets_src" ) +target_include_directories(destrum_tests PRIVATE "${CMAKE_SOURCE_DIR}/lightkeeper/include") target_link_libraries(destrum_tests PRIVATE destrum::destrum) destrum_enable_warnings(destrum_tests) diff --git a/tests/destrum_tests.cpp b/tests/destrum_tests.cpp index 66b0c0c..5ac1a1f 100644 --- a/tests/destrum_tests.cpp +++ b/tests/destrum_tests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -44,6 +45,9 @@ #include #include +#include +#include + namespace { class TestComponent final : public Component { public: @@ -53,6 +57,18 @@ namespace { std::string GetTypeName() const override { return "TestComponent"; } }; + class TestActivatable final : public Activatable { + public: + explicit TestActivatable(GameObject& owner) : Activatable(owner, "TestActivatable") {} + + std::string GetTypeName() const override { return "TestActivatable"; } + + int activations{0}; + + protected: + void OnActivate(GameObject&) override { ++activations; } + }; + class RemovingListener final : public EventListener { public: explicit RemovingListener(std::unique_ptr* victim) @@ -1473,6 +1489,132 @@ namespace { !PrimitiveMeshes::MakeFromKey("Box").has_value(), "invalid primitive keys must not build a mesh"); } + + void testSceneFileIdsAreLocal() + { + const auto path = std::filesystem::temp_directory_path() / "destrum_local_ids_scene.json"; + std::ofstream{path} + << R"({"version":2,"currentCamera":2,"objects":[)" + R"({"id":1,"name":"root"},)" + R"({"id":2,"name":"child","parent":1,"components":[{"type":"CameraComponent"}]}]})"; + + Scene& first = SceneManager::GetInstance().CreateScene("local ids a"); + Scene& second = SceneManager::GetInstance().CreateScene("local ids b"); + check(SceneSerializer::Load(first, path), "the first copy of the scene must load"); + check(SceneSerializer::Load(second, path), "the second copy of the scene must load"); + std::filesystem::remove(path); + first.CommitPendingAdditions(); + second.CommitPendingAdditions(); + + std::unordered_set ids; + std::size_t count = 0; + for (const Scene* scene : {&first, &second}) { + for (const auto& object : scene->GetObjects()) { + ids.insert(object->GetId()); + ++count; + } + } + check(count == 4 && ids.size() == 4, "loading one file into two scenes must not duplicate ids"); + + for (const Scene* scene : {&first, &second}) { + const CameraComponent* camera = scene->GetCurrentCamera(); + check(camera != nullptr && camera->GetGameObject()->GetName() == "child" && + camera->GetGameObject()->GetScene() == scene, + "the current camera must be remapped to the object in its own scene"); + const Transform* parent = camera->GetGameObject()->GetTransform().GetParent(); + check(parent != nullptr && parent->GetOwner()->GetName() == "root" && + parent->GetOwner()->GetScene() == scene, + "parents must be remapped within each loaded scene"); + } + + const GameObject* fresh = first.CreateGameObject("fresh"); + check(!ids.contains(fresh->GetId()), "objects created after a load must get unused ids"); + + SceneManager::GetInstance().Destroy(); + } + + GameObject* findByName(const Scene& scene, const std::string& name) + { + for (const auto& object : scene.GetObjects()) { + if (object->GetName() == name) { + return object.get(); + } + } + return nullptr; + } + + void testButtonTargetsRoundTrip() + { + ComponentFactory::Register("InteractableButton"); + ComponentFactory::Register("TestActivatable"); + + Scene& scene = SceneManager::GetInstance().CreateScene("button targets"); + GameObject* door = scene.CreateGameObject("door"); + door->AddComponent(); + GameObject* lamp = scene.CreateGameObject("lamp"); + lamp->AddComponent(); + GameObject* buttonObject = scene.CreateGameObject("button"); + auto* button = buttonObject->AddComponent(); + button->SetCycleColors(false); + button->AddTarget(*door); + button->AddTarget(*lamp); + button->AddTarget(*door); + scene.CommitPendingAdditions(); + check(button->GetTargets().size() == 2, "a button must not target the same object twice"); + + const auto path = std::filesystem::temp_directory_path() / "destrum_button_targets_scene.json"; + check(SceneSerializer::Save(scene, path), "the button scene must save"); + check(SceneSerializer::Load(scene, path), "the button scene must load"); + + Scene& copy = SceneManager::GetInstance().CreateScene("button targets copy"); + check(SceneSerializer::Load(copy, path), "the button scene must load into a second scene"); + std::filesystem::remove(path); + + for (Scene* loaded : {&scene, ©}) { + loaded->CommitPendingAdditions(); + GameObject* loadedButton = findByName(*loaded, "button"); + auto* loadedComponent = loadedButton != nullptr ? loadedButton->GetComponent() : nullptr; + check(loadedComponent != nullptr, "the button must survive a round-trip"); + + const auto& targets = loadedComponent->GetTargets(); + check(targets.size() == 2, "button targets must survive a round-trip"); + GameObject* loadedDoor = targets[0].Get(*loaded); + GameObject* loadedLamp = targets[1].Get(*loaded); + check(loadedDoor == findByName(*loaded, "door") && loadedDoor != nullptr && + loadedLamp == findByName(*loaded, "lamp") && loadedLamp != nullptr, + "button targets must point at the loaded objects in their own scene"); + + loadedComponent->Interact(*loadedButton); + check(loadedDoor->GetComponent()->activations == 1 && + loadedLamp->GetComponent()->activations == 1, + "pressing a loaded button must activate its targets"); + } + + SceneManager::GetInstance().Destroy(); + } + + void testButtonDropsMissingTargets() + { + ComponentFactory::Register("InteractableButton"); + + Scene& scene = SceneManager::GetInstance().CreateScene("missing targets"); + const auto path = std::filesystem::temp_directory_path() / "destrum_missing_targets_scene.json"; + std::ofstream{path} + << R"({"version":2,"objects":[{"id":5,"name":"button","components":)" + R"([{"type":"InteractableButton","data":{"cycleColors":false,"targets":[5,99]}}]}]})"; + + check(SceneSerializer::Load(scene, path), "a button with a dangling target must still load"); + std::filesystem::remove(path); + scene.CommitPendingAdditions(); + + const GameObject* object = scene.GetObjects().front().get(); + const auto* button = object->GetComponent(); + check(button != nullptr && button->GetTargets().size() == 1 && + button->GetTargets().front().GetId() == object->GetId(), + "dangling targets must be dropped and valid ones remapped"); + + SceneManager::GetInstance().Destroy(); + } } int main() @@ -1518,6 +1660,9 @@ int main() testSceneLoadDropsMissingCamera(); testGlmJson(); testPrimitiveMeshKeys(); + testSceneFileIdsAreLocal(); + testButtonTargetsRoundTrip(); + testButtonDropsMissingTargets(); std::cout << "destrum tests passed\n"; return EXIT_SUCCESS; }