feat: add ObjectRef for scene loading to not rely on runtime id's

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"

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<std::unique_ptr<Component>>& GetComponents() {

destrum/include/destrum/ObjectModel/ObjectRef.h

@@ -0,0 +1,37 @@
+#ifndef DESTRUM_OBJECTREF_H
+#define DESTRUM_OBJECTREF_H
+
+#include <unordered_map>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include <destrum/ObjectModel/ObjectId.h>
+
+class GameObject;
+class Scene;
+
+class ObjectRef final {
+public:
+    using ObjectMap = std::unordered_map<ObjectId, GameObject*>;
+
+    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

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<ObjectId>::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<ObjectId>::max() && id >= s_NextId) {
-        s_NextId = id + 1;
-    }
-}
-
 ObjectId GameObject::AllocateId() {
     if (s_NextId == InvalidObjectId ||
         s_NextId >= std::numeric_limits<ObjectId>::max() - 1) {

destrum/src/ObjectModel/ObjectRef.cpp

@@ -0,0 +1,38 @@
+#include <destrum/ObjectModel/ObjectRef.h>
+
+#include <nlohmann/json.hpp>
+
+#include <destrum/ObjectModel/GameObject.h>
+#include <destrum/Scene/Scene.h>
+
+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<ObjectId>()) : ObjectRef{};
+}

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<std::string>();
     }
     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()) {

lightkeeper/include/components/InteractableButton.h

@@ -6,7 +6,7 @@
 #include <cstddef>
 #include <vector>
 
-#include <destrum/ObjectModel/ObjectId.h>
+#include <destrum/ObjectModel/ObjectRef.h>
 
 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<ObjectId>& GetTargets() const { return m_Targets; }
+    [[nodiscard]] const std::vector<ObjectRef>& 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<ObjectId> m_Targets;
+    std::vector<ObjectRef> m_Targets;
 };
 
 #endif // LIGHTKEEPER_INTERACTABLEBUTTON_H

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<ObjectId>{});
+    m_Targets = data.value("targets", std::vector<ObjectRef>{});
+}
+
+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<ObjectId> 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<int>(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<Activatable>() == nullptr) {
                 continue;

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)

tests/destrum_tests.cpp

@@ -7,6 +7,7 @@
 #include <iostream>
 #include <memory>
 #include <string>
+#include <unordered_set>
 
 #include <glm/gtc/constants.hpp>
 #include <glm/gtc/packing.hpp>
@@ -44,6 +45,9 @@
 #include <destrum/Graphics/imageLoader.h>
 #include <destrum/Graphics/Managers/LineRenderingManager.h>
 
+#include <components/Activatable.h>
+#include <components/InteractableButton.h>
+
 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<RemovingListener>* 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<ObjectId> 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>("InteractableButton");
+        ComponentFactory::Register<TestActivatable>("TestActivatable");
+
+        Scene& scene = SceneManager::GetInstance().CreateScene("button targets");
+        GameObject* door = scene.CreateGameObject("door");
+        door->AddComponent<TestActivatable>();
+        GameObject* lamp = scene.CreateGameObject("lamp");
+        lamp->AddComponent<TestActivatable>();
+        GameObject* buttonObject = scene.CreateGameObject("button");
+        auto* button = buttonObject->AddComponent<InteractableButton>();
+        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, &copy}) {
+            loaded->CommitPendingAdditions();
+            GameObject* loadedButton = findByName(*loaded, "button");
+            auto* loadedComponent = loadedButton != nullptr ? loadedButton->GetComponent<InteractableButton>() : 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<TestActivatable>()->activations == 1 &&
+                  loadedLamp->GetComponent<TestActivatable>()->activations == 1,
+                  "pressing a loaded button must activate its targets");
+        }
+
+        SceneManager::GetInstance().Destroy();
+    }
+
+    void testButtonDropsMissingTargets()
+    {
+        ComponentFactory::Register<InteractableButton>("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<InteractableButton>();
+        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;
 }