diff --git a/.gitignore b/.gitignore index 8b11db9..5329d96 100644 --- a/.gitignore +++ b/.gitignore @@ -33,6 +33,7 @@ # Build directories build/ +build-*/ out/ cmake-build-*/ @@ -63,6 +64,10 @@ docs/doxygen/ docs/build/ docs/sphinx/api/ +# Dear ImGui writes its window layout next to the working directory when the +# curve_visualization example is run. +imgui.ini + # OS-specific files .DS_Store Thumbs.db diff --git a/CHANGELOG.md b/CHANGELOG.md index dbcc8d1..b14f9ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,51 @@ While the major version is `0`, breaking changes may land in a minor release. ## [Unreleased] +### Changed + +- **Breaking:** the `Id` lookups on `Animation` now return references rather + than pointers, matching the index and name overloads — + `channel(Id)` and `operator[](Id)`, in both the const and non-const forms. + They never returned null: the underlying `unordered_map::at` throws + `std::out_of_range` on a miss, so the pointer return only invited dead null + checks. Behavior on a miss is unchanged; callers replace `->` with `.` ([#52]). + +### Added + +- `Animation::sort_channels()`, sorting channels by name, and an overload + taking a comparator for any other ordering. Both are stable. Only the index + order changes: the channels themselves are not moved, so ids keep resolving + and references taken beforehand stay valid. + +### Removed + +- **Breaking:** `Id`'s constructor is now private, so ids can only originate + from the library — obtain them from `Channel::id()`, or use `Id::invalid()` + for a sentinel. A fabricated id was never able to do anything a real one + could not, but because ids are handed out from one counter shared by every + `Animation`, a hand-made id could silently resolve to an unrelated channel. +- The `glad` dependency. The examples now rely on the loader that Dear ImGui + already bundles, and the handful of direct GL calls in + `curve_visualization` are OpenGL 1.1 core, resolved by linking `OpenGL::GL`. + This also removes the `CMAKE_POLICY_VERSION_MINIMUM` workaround that glad + 0.1.36 required under CMake 4 ([#53]). + +### Fixed + +- The `curve_visualization` example showed no plot on a first run. Its plot + window was opened without a size, so it auto-fitted to its content — but that + content is a plot sized `ImVec2(-1, -1)`, meaning "fill the available space". + On the first frame the two resolved to nothing, the window collapsed to a few + pixels behind the curve editor, and ImGui persisted that size to `imgui.ini` + from then on. Both windows now get a first-run position and size derived from + the viewport, using `ImGuiCond_FirstUseEver` so an arranged layout is kept. +- `imgui.ini`, which the `curve_visualization` example writes to the working + directory, is now ignored rather than showing up as untracked noise in the + repository root. Alternate build directories (`build-*/`) are ignored too. + +[#52]: https://github.com/Actualize-Interactive/anim/issues/52 +[#53]: https://github.com/Actualize-Interactive/anim/issues/53 + ## [0.2.0] - 2026-07-25 First release prepared for the public repository. It contains two small diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index 7fb298e..dd610e9 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -7,13 +7,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) include(FetchContent) set(FETCHCONTENT_BASE_DIR ${CMAKE_BINARY_DIR}/_deps CACHE PATH "Base directory for FetchContent downloads") -# glad 0.1.36 declares cmake_minimum_required(VERSION 3.0), and CMake 4.0 -# removed compatibility with anything below 3.5, so configuring the examples -# fails outright without this. Scoped to the examples: the library and the test -# suite do not depend on it, and it is not inherited by anyone consuming anim. -# Remove once glad is bumped to a release that requires 3.5 or newer. -set(CMAKE_POLICY_VERSION_MINIMUM 3.5) - # Fetch GLFW first (required by ImGui) FetchContent_Declare( glfw @@ -35,15 +28,6 @@ FetchContent_Declare( ) FetchContent_MakeAvailable(imgui) -# Fetch glad (OpenGL loader) -FetchContent_Declare( - glad - GIT_REPOSITORY https://github.com/Dav1dde/glad.git - GIT_TAG v0.1.36 - GIT_SHALLOW TRUE -) -FetchContent_MakeAvailable(glad) - # Fetch ImPlot FetchContent_Declare( implot @@ -69,14 +53,14 @@ target_include_directories(imgui_lib PUBLIC ${imgui_SOURCE_DIR}/backends ) -target_link_libraries(imgui_lib PUBLIC glfw glad) +target_link_libraries(imgui_lib PUBLIC glfw) # Find OpenGL find_package(OpenGL REQUIRED) target_link_libraries(imgui_lib PUBLIC OpenGL::GL) -# Use glad as OpenGL loader for all platforms -target_compile_definitions(imgui_lib PUBLIC IMGUI_IMPL_OPENGL_LOADER_GLAD) +# No loader definition: with none set, the ImGui OpenGL3 backend falls back to +# its own bundled loader (backends/imgui_impl_opengl3_loader.h). # Create ImPlot library target add_library(implot_lib STATIC diff --git a/examples/curve_visualization.cpp b/examples/curve_visualization.cpp index ce441f6..4e86105 100644 --- a/examples/curve_visualization.cpp +++ b/examples/curve_visualization.cpp @@ -1,5 +1,4 @@ #include -#include #include #include "imgui.h" #include "imgui_impl_glfw.h" @@ -159,12 +158,6 @@ int main() { glfwMakeContextCurrent(window); glfwSwapInterval(1); // Enable vsync - // Initialize OpenGL loader - if (!gladLoadGLLoader((GLADloadproc)glfwGetProcAddress)) { - std::cerr << "Failed to initialize OpenGL loader" << std::endl; - return 1; - } - // Setup Dear ImGui context IMGUI_CHECKVERSION(); ImGui::CreateContext(); @@ -235,6 +228,21 @@ int main() { } + // Give both windows a sensible first-run size and position. Without + // this the plot window auto-fits to its content, but its content is a + // plot sized ImVec2(-1,-1) ("fill the available space"), so on the + // first frame the two resolve to nothing and the window collapses to a + // few pixels — which ImGui then persists to imgui.ini. FirstUseEver + // means a layout the user has arranged is still respected. + const ImGuiViewport* viewport = ImGui::GetMainViewport(); + const ImVec2 work_pos = viewport->WorkPos; + const ImVec2 work_size = viewport->WorkSize; + const float editor_width = std::max(320.0f, work_size.x * 0.22f); + const float pad = 12.0f; + + ImGui::SetNextWindowPos(ImVec2(work_pos.x + pad, work_pos.y + pad), ImGuiCond_FirstUseEver); + ImGui::SetNextWindowSize(ImVec2(work_size.x - editor_width - pad * 3.0f, + work_size.y - pad * 2.0f), ImGuiCond_FirstUseEver); ImGui::Begin("Curves Plot — Ctrl-click to move keyframes & handles (panning disabled)###CurvesPlot"); // Disable ImPlot's click-drag panning so it doesn't fight with dragging @@ -515,6 +523,10 @@ int main() { } ImGui::End(); + ImGui::SetNextWindowPos(ImVec2(work_pos.x + work_size.x - editor_width - pad, + work_pos.y + pad), ImGuiCond_FirstUseEver); + ImGui::SetNextWindowSize(ImVec2(editor_width, work_size.y - pad * 2.0f), + ImGuiCond_FirstUseEver); ImGui::Begin("Curve Editor"); // No CollapsingHeader for "Edit Curves", content is directly in the curve's TreeNode for (size_t i = 0; i < animation.size(); ++i) { diff --git a/include/anim/animation.hpp b/include/anim/animation.hpp index 66d09f5..495375e 100644 --- a/include/anim/animation.hpp +++ b/include/anim/animation.hpp @@ -67,18 +67,18 @@ class Animation { /// @brief Name lookup. @throws std::out_of_range if none matches. Channel& operator[](const std::string& channel_name); /// @brief Returns the channel with @p channel_id. @throws std::out_of_range if none matches. - Channel* channel(Id channel_id); + Channel& channel(Id channel_id); /// @brief Id lookup. @throws std::out_of_range if none matches. - Channel* operator[](Id channel_id); + Channel& operator[](Id channel_id); /// @brief Returns the first channel named @p channel_name. @throws std::out_of_range if none matches. const Channel& channel(const std::string& channel_name) const; /// @brief Name lookup. @throws std::out_of_range if none matches. const Channel& operator[](const std::string& channel_name) const; /// @brief Returns the channel with @p channel_id. @throws std::out_of_range if none matches. - const Channel* channel(Id channel_id) const; + const Channel& channel(Id channel_id) const; /// @brief Id lookup. @throws std::out_of_range if none matches. - const Channel* operator[](Id channel_id) const; + const Channel& operator[](Id channel_id) const; /// @brief Number of channels. inline size_t size() const { return m_channels.size(); } @@ -105,6 +105,24 @@ class Animation { */ void reorder_channel(Id channel_id, size_t to_index); + /** + * @brief Sorts the channels by name, ascending. + * + * The sort is stable, so channels sharing a name keep their relative order. + * Only the index order changes: ids, names and keyframes are untouched, and + * because the channels themselves are not moved, references and pointers + * obtained before the sort — including those from channel(Id) — stay valid. + */ + void sort_channels(); + + /** + * @brief Sorts the channels using a custom ordering. + * @param comparator A strict weak ordering; returns true when the first + * channel should be placed before the second. + * @see sort_channels() + */ + void sort_channels(const std::function& comparator); + /// @brief Removes all channels. void clear(); /// @brief Removes the channel at @p index. @throws std::out_of_range if out of range. diff --git a/include/anim/id.hpp b/include/anim/id.hpp index 124b371..d15ed7e 100644 --- a/include/anim/id.hpp +++ b/include/anim/id.hpp @@ -19,9 +19,6 @@ namespace anim { struct Id { const uint64_t id; ///< The underlying identifier value (immutable). - /// @brief Constructs an Id wrapping @p value. - explicit Id(uint64_t value) : id(value) {} - /// @brief Explicit conversion back to the underlying integer value. explicit operator uint64_t() const { return id; } @@ -50,6 +47,20 @@ struct Id { bool is_valid() const { return id != static_cast(-1); } + +private: + /** + * @brief Wraps a raw identifier value. + * + * Private so that ids can only originate from the library. An Id fabricated + * by a caller would either fail to resolve or, because ids are handed out + * from one counter shared by every Animation, resolve to some unrelated + * channel. Obtain ids from Channel::id(); use invalid() for a sentinel. + */ + explicit Id(uint64_t value) : id(value) {} + + friend class Animation; ///< Mints ids for the channels it creates. + friend class Channel; ///< Stores the id it was created with. }; } // namespace anim diff --git a/src/animation.cpp b/src/animation.cpp index 7d625b5..81796e7 100644 --- a/src/animation.cpp +++ b/src/animation.cpp @@ -98,11 +98,11 @@ Channel& Animation::operator[](const std::string& channel_name) { return channel(channel_name); } -Channel* Animation::channel(Id channel_id) { - return m_channel_map.at(channel_id); +Channel& Animation::channel(Id channel_id) { + return *m_channel_map.at(channel_id); } -Channel* Animation::operator[](Id channel_id) { +Channel& Animation::operator[](Id channel_id) { return channel(channel_id); } @@ -120,11 +120,11 @@ const Channel& Animation::operator[](const std::string& channel_name) const { return channel(channel_name); } -const Channel* Animation::channel(Id channel_id) const { - return m_channel_map.at(channel_id); +const Channel& Animation::channel(Id channel_id) const { + return *m_channel_map.at(channel_id); } -const Channel* Animation::operator[](Id channel_id) const { +const Channel& Animation::operator[](Id channel_id) const { return channel(channel_id); } @@ -175,9 +175,26 @@ void Animation::reorder_channel(Id channel_id, size_t to_index) { reorder_channel(from_index, to_index); } -void Animation::clear() { +void Animation::sort_channels() { + sort_channels([](const Channel& lhs, const Channel& rhs) { + return lhs.name() < rhs.name(); + }); +} + +void Animation::sort_channels(const std::function& comparator) { + // Reordering the owning pointers leaves the Channel objects themselves in + // place, so m_channel_map stays valid and so does anything the caller is + // already holding a reference to. + std::stable_sort(m_channels.begin(), m_channels.end(), + [&comparator](const std::unique_ptr& lhs, + const std::unique_ptr& rhs) { + return comparator(*lhs, *rhs); + }); +} + +void Animation::clear() { m_channel_map.clear(); - m_channels.clear(); + m_channels.clear(); } void Animation::remove_channel(size_t index) { diff --git a/tests/test_animation.cpp b/tests/test_animation.cpp index 9624d63..8401906 100644 --- a/tests/test_animation.cpp +++ b/tests/test_animation.cpp @@ -185,9 +185,9 @@ TEST_CASE("Animation Channel Management", "[Animation]") { REQUIRE(animation.channel(2).name() == "channel1"); // IDs should still work - REQUIRE(animation.channel(id0) == &ch0); - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id2) == &ch2); + REQUIRE(&animation.channel(id0) == &ch0); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id2) == &ch2); } SECTION("Reorder by name") { Channel& ch0 = animation.create_channel("channel0"); @@ -205,7 +205,7 @@ TEST_CASE("Animation Channel Management", "[Animation]") { REQUIRE(animation.channel(2).name() == "channel0"); // ID access should still work - REQUIRE(animation.channel(id0) == &ch0); + REQUIRE(&animation.channel(id0) == &ch0); } SECTION("Reorder by ID") { @@ -224,7 +224,7 @@ TEST_CASE("Animation Channel Management", "[Animation]") { REQUIRE(animation.channel(2).name() == "channel1"); // ID access should still work - REQUIRE(animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id1) == &ch1); } SECTION("Reorder to current position") { @@ -246,9 +246,9 @@ TEST_CASE("Animation Channel Management", "[Animation]") { REQUIRE(animation.channel(2).name() == "channel2"); // ID access should still work - REQUIRE(animation.channel(id0) == &ch0); - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id2) == &ch2); + REQUIRE(&animation.channel(id0) == &ch0); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id2) == &ch2); } } @@ -1150,35 +1150,35 @@ TEST_CASE("Animation Id-based access error paths", "[Animation][Id]") { Id id_b = b.id(); SECTION("channel(Id) returns the matching channel") { - REQUIRE(anim.channel(id_a) == &a); - REQUIRE(anim[id_b] == &b); + REQUIRE(&anim.channel(id_a) == &a); + REQUIRE(&anim[id_b] == &b); const Animation& canim = anim; - REQUIRE(canim.channel(id_a) == &a); - REQUIRE(canim[id_b] == &b); + REQUIRE(&canim.channel(id_a) == &a); + REQUIRE(&canim[id_b] == &b); } SECTION("channel(Id) throws for a non-existent id") { - REQUIRE_THROWS_AS(anim.channel(Id(999999)), std::out_of_range); + REQUIRE_THROWS_AS(anim.channel(Id::invalid()), std::out_of_range); const Animation& canim = anim; - REQUIRE_THROWS_AS(canim.channel(Id(999999)), std::out_of_range); + REQUIRE_THROWS_AS(canim.channel(Id::invalid()), std::out_of_range); } SECTION("remove_channel(Id) removes the right channel") { anim.remove_channel(id_a); REQUIRE(anim.num_channels() == 1); REQUIRE(anim.channel(0).name() == "b"); - REQUIRE(anim.channel(id_b) == &b); + REQUIRE(&anim.channel(id_b) == &b); // The removed id is gone from the map REQUIRE_THROWS_AS(anim.channel(id_a), std::out_of_range); } SECTION("remove_channel(Id) throws for a non-existent id") { - REQUIRE_THROWS_AS(anim.remove_channel(Id(999999)), std::out_of_range); + REQUIRE_THROWS_AS(anim.remove_channel(Id::invalid()), std::out_of_range); REQUIRE(anim.num_channels() == 2); // unchanged } SECTION("reorder_channel(Id) throws for a non-existent id") { - REQUIRE_THROWS_AS(anim.reorder_channel(Id(999999), 0), std::out_of_range); + REQUIRE_THROWS_AS(anim.reorder_channel(Id::invalid(), 0), std::out_of_range); } SECTION("reorder_channel(Id) throws for an out-of-range target index") { @@ -1186,3 +1186,114 @@ TEST_CASE("Animation Id-based access error paths", "[Animation][Id]") { } } + +TEST_CASE("Animation::sort_channels", "[Animation]") { + Animation animation("sorting"); + + SECTION("sorts by name ascending") { + animation.create_channel("delta"); + animation.create_channel("alpha"); + animation.create_channel("charlie"); + animation.create_channel("bravo"); + + animation.sort_channels(); + + REQUIRE(animation.channel_names() == + std::vector{"alpha", "bravo", "charlie", "delta"}); + } + + SECTION("is stable for channels sharing a name") { + // Distinguish the duplicates by keyframe count, which sorting must not + // reorder relative to each other. + Channel& first_dup = animation.create_channel("same"); + first_dup.create_keyframe(0.0, 0.0); + animation.create_channel("zulu"); + Channel& second_dup = animation.create_channel("same"); + second_dup.create_keyframe(0.0, 0.0); + second_dup.create_keyframe(1.0, 1.0); + + animation.sort_channels(); + + REQUIRE(animation.channel(0).name() == "same"); + REQUIRE(animation.channel(1).name() == "same"); + REQUIRE(animation.channel(2).name() == "zulu"); + // Original relative order of the two "same" channels is preserved. + REQUIRE(animation.channel(0).num_keyframes() == 1); + REQUIRE(animation.channel(1).num_keyframes() == 2); + } + + SECTION("accepts a custom comparator") { + animation.create_channel("alpha"); + animation.create_channel("bravo"); + animation.create_channel("charlie"); + + animation.sort_channels([](const Channel& lhs, const Channel& rhs) { + return lhs.name() > rhs.name(); // descending + }); + + REQUIRE(animation.channel_names() == + std::vector{"charlie", "bravo", "alpha"}); + } + + SECTION("a comparator may use any channel state, not just the name") { + Channel& few = animation.create_channel("few"); + Channel& many = animation.create_channel("many"); + few.create_keyframe(0.0, 0.0); + many.create_keyframe(0.0, 0.0); + many.create_keyframe(1.0, 1.0); + many.create_keyframe(2.0, 2.0); + + animation.sort_channels([](const Channel& lhs, const Channel& rhs) { + return lhs.num_keyframes() > rhs.num_keyframes(); + }); + + REQUIRE(animation.channel(0).name() == "many"); + REQUIRE(animation.channel(1).name() == "few"); + } + + SECTION("ids, lookups and existing references survive the sort") { + Channel& zulu = animation.create_channel("zulu"); + Channel& alpha = animation.create_channel("alpha"); + const Id zulu_id = zulu.id(); + const Id alpha_id = alpha.id(); + zulu.create_keyframe(3.0, 7.0); + + animation.sort_channels(); + + // Index order changed... + REQUIRE(animation.channel(0).name() == "alpha"); + REQUIRE(animation.channel(1).name() == "zulu"); + + // ...but the channels themselves did not move, so ids still resolve to + // the same objects and references taken beforehand remain valid. + REQUIRE(&animation.channel(zulu_id) == &zulu); + REQUIRE(&animation.channel(alpha_id) == &alpha); + REQUIRE(zulu.id() == zulu_id); + REQUIRE(alpha.id() == alpha_id); + REQUIRE(zulu.num_keyframes() == 1); + REQUIRE(zulu.evaluate(3.0) == Catch::Approx(7.0)); + } + + SECTION("is a no-op on an empty animation") { + REQUIRE(animation.empty()); + animation.sort_channels(); + REQUIRE(animation.empty()); + REQUIRE(animation.num_channels() == 0); + } + + SECTION("is a no-op on a single channel") { + animation.create_channel("only"); + animation.sort_channels(); + REQUIRE(animation.num_channels() == 1); + REQUIRE(animation.channel(0).name() == "only"); + } + + SECTION("sorting an already sorted animation changes nothing") { + animation.create_channel("alpha"); + animation.create_channel("bravo"); + animation.sort_channels(); + animation.sort_channels(); + REQUIRE(animation.channel_names() == + std::vector{"alpha", "bravo"}); + } +} diff --git a/tests/test_id_functionality.cpp b/tests/test_id_functionality.cpp index dcc40ca..220ec1f 100644 --- a/tests/test_id_functionality.cpp +++ b/tests/test_id_functionality.cpp @@ -67,14 +67,14 @@ TEST_CASE("Animation ID-based channel access", "[Animation][Id]") { Id id2 = ch2.id(); // Access channels by ID - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id2) == &ch2); - REQUIRE(animation[id1] == &ch1); - REQUIRE(animation[id2] == &ch2); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id2) == &ch2); + REQUIRE(&animation[id1] == &ch1); + REQUIRE(&animation[id2] == &ch2); // Verify we can access the channel's properties through ID - REQUIRE(animation.channel(id1)->name() == "channel1"); - REQUIRE(animation.channel(id2)->name() == "channel2"); + REQUIRE(animation.channel(id1).name() == "channel1"); + REQUIRE(animation.channel(id2).name() == "channel2"); } SECTION("Invalid ID throws exception") { @@ -88,8 +88,10 @@ TEST_CASE("Animation ID-based channel access", "[Animation][Id]") { REQUIRE_THROWS_AS(animation.channel(invalid_id), std::out_of_range); REQUIRE_THROWS_AS(animation[invalid_id], std::out_of_range); - // Try to access with a non-existent but valid ID - Id non_existent_id(999999); + // A genuine id from a different animation: valid, correctly formed, and + // simply not owned by this one. + Animation other_animation("other_animation"); + Id non_existent_id = other_animation.create_channel("elsewhere").id(); REQUIRE_THROWS_AS(animation.channel(non_existent_id), std::out_of_range); REQUIRE_THROWS_AS(animation[non_existent_id], std::out_of_range); } @@ -156,14 +158,14 @@ TEST_CASE("Animation ID-based channel access with const support", "[Animation][I Id id2 = ch2.id(); // Access channels by ID - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id2) == &ch2); - REQUIRE(animation[id1] == &ch1); - REQUIRE(animation[id2] == &ch2); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id2) == &ch2); + REQUIRE(&animation[id1] == &ch1); + REQUIRE(&animation[id2] == &ch2); // Verify we can access the channel's properties through ID - REQUIRE(animation.channel(id1)->name() == "channel1"); - REQUIRE(animation.channel(id2)->name() == "channel2"); + REQUIRE(animation.channel(id1).name() == "channel1"); + REQUIRE(animation.channel(id2).name() == "channel2"); } SECTION("Access const channel by ID") { @@ -178,14 +180,14 @@ TEST_CASE("Animation ID-based channel access with const support", "[Animation][I const Animation& const_animation = animation; // Access const channels by ID - REQUIRE(const_animation.channel(id1) == &ch1); - REQUIRE(const_animation.channel(id2) == &ch2); - REQUIRE(const_animation[id1] == &ch1); - REQUIRE(const_animation[id2] == &ch2); + REQUIRE(&const_animation.channel(id1) == &ch1); + REQUIRE(&const_animation.channel(id2) == &ch2); + REQUIRE(&const_animation[id1] == &ch1); + REQUIRE(&const_animation[id2] == &ch2); // Verify we can access the channel's properties through ID - REQUIRE(const_animation.channel(id1)->name() == "channel1"); - REQUIRE(const_animation.channel(id2)->name() == "channel2"); + REQUIRE(const_animation.channel(id1).name() == "channel1"); + REQUIRE(const_animation.channel(id2).name() == "channel2"); } SECTION("Invalid ID throws exception") { @@ -202,8 +204,10 @@ TEST_CASE("Animation ID-based channel access with const support", "[Animation][I REQUIRE_THROWS_AS(animation.channel(invalid_id), std::out_of_range); REQUIRE_THROWS_AS(animation[invalid_id], std::out_of_range); - // Try to access with a non-existent but valid ID - Id non_existent_id(999999); + // A genuine id from a different animation: valid, correctly formed, and + // simply not owned by this one. + Animation other_animation("other_animation"); + Id non_existent_id = other_animation.create_channel("elsewhere").id(); REQUIRE_THROWS_AS(animation.channel(non_existent_id), std::out_of_range); REQUIRE_THROWS_AS(animation[non_existent_id], std::out_of_range); } @@ -218,9 +222,9 @@ TEST_CASE("Channel map maintenance", "[Animation][Id]") { Channel& ch3 = animation.create_channel("channel3"); // Normal creation // Verify all channels are accessible by ID - REQUIRE(animation.channel(ch1.id()) == &ch1); - REQUIRE(animation.channel(ch2.id()) == &ch2); - REQUIRE(animation.channel(ch3.id()) == &ch3); + REQUIRE(&animation.channel(ch1.id()) == &ch1); + REQUIRE(&animation.channel(ch2.id()) == &ch2); + REQUIRE(&animation.channel(ch3.id()) == &ch3); // Verify channel order in vector is correct REQUIRE(animation.channel(0).name() == "channel2"); // ch2 was inserted at index 0 @@ -237,9 +241,9 @@ TEST_CASE("Channel map maintenance", "[Animation][Id]") { Channel& ch3 = animation.create_channel("channel3"); // Add at end // Verify all channels are accessible by ID - REQUIRE(animation.channel(ch1.id()) == &ch1); - REQUIRE(animation.channel(ch2.id()) == &ch2); - REQUIRE(animation.channel(ch3.id()) == &ch3); + REQUIRE(&animation.channel(ch1.id()) == &ch1); + REQUIRE(&animation.channel(ch2.id()) == &ch2); + REQUIRE(&animation.channel(ch3.id()) == &ch3); // Verify insertion order REQUIRE(animation.channel(0).name() == "channel2"); REQUIRE(animation.channel(1).name() == "channel1"); @@ -264,8 +268,8 @@ TEST_CASE("Channel map maintenance", "[Animation][Id]") { REQUIRE_THROWS_AS(animation.channel(id2), std::out_of_range); // ch1 and ch3 should still be accessible - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id3) == &ch3); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id3) == &ch3); // Check vector state REQUIRE(animation.num_channels() == 2); @@ -291,8 +295,8 @@ TEST_CASE("Channel map maintenance", "[Animation][Id]") { REQUIRE_THROWS_AS(animation.channel(id2), std::out_of_range); // ch1 and ch3 should still be accessible - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id3) == &ch3); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id3) == &ch3); REQUIRE(animation.num_channels() == 2); } @@ -315,8 +319,8 @@ TEST_CASE("Channel map maintenance", "[Animation][Id]") { REQUIRE_THROWS_AS(animation.channel(id2), std::out_of_range); // ch1 and ch3 should still be accessible - REQUIRE(animation.channel(id1) == &ch1); - REQUIRE(animation.channel(id3) == &ch3); + REQUIRE(&animation.channel(id1) == &ch1); + REQUIRE(&animation.channel(id3) == &ch3); REQUIRE(animation.num_channels() == 2); } @@ -357,8 +361,8 @@ TEST_CASE("Channel ID persistence through operations", "[Animation][Id]") { // Verify all channels are still accessible by their IDs for (size_t i = 0; i < num_channels; ++i) { - REQUIRE(animation.channel(channel_ids[i]) == &animation.channel(i)); - REQUIRE(animation.channel(channel_ids[i])->name() == "channel_" + std::to_string(i)); + REQUIRE(&animation.channel(channel_ids[i]) == &animation.channel(i)); + REQUIRE(animation.channel(channel_ids[i]).name() == "channel_" + std::to_string(i)); } } @@ -378,8 +382,8 @@ TEST_CASE("Channel ID persistence through operations", "[Animation][Id]") { // ID should still be the same and channel should be accessible REQUIRE(ch.id() == original_id); - REQUIRE(animation.channel(original_id) == &ch); - REQUIRE(animation.channel(original_id)->name() == "modified_channel"); + REQUIRE(&animation.channel(original_id) == &ch); + REQUIRE(animation.channel(original_id).name() == "modified_channel"); } } @@ -453,12 +457,13 @@ TEST_CASE("Edge cases and error conditions", "[Animation][Id]") { Channel& ch = animation.create_channel("test_channel"); Id valid_id = ch.id(); - // Try to remove with non-existent ID - Id non_existent_id(999999); + // A genuine id from a different animation: valid but not owned here. + Animation other_animation("other_animation"); + Id non_existent_id = other_animation.create_channel("elsewhere").id(); REQUIRE_THROWS_AS(animation.remove_channel(non_existent_id), std::out_of_range); // Original channel should still be accessible - REQUIRE(animation.channel(valid_id) == &ch); + REQUIRE(&animation.channel(valid_id) == &ch); } SECTION("Multiple operations maintain consistency") { @@ -484,9 +489,9 @@ TEST_CASE("Edge cases and error conditions", "[Animation][Id]") { // Verify final state REQUIRE_THROWS_AS(animation.channel(id1), std::out_of_range); // ch1 removed REQUIRE_THROWS_AS(animation.channel(id2), std::out_of_range); // ch2 removed - REQUIRE(animation.channel(id3) == &ch3); // ch3 still exists - REQUIRE(animation.channel(id4) == &ch4); // ch4 exists - REQUIRE(animation.channel(id5) == &ch5); // ch5 exists + REQUIRE(&animation.channel(id3) == &ch3); // ch3 still exists + REQUIRE(&animation.channel(id4) == &ch4); // ch4 exists + REQUIRE(&animation.channel(id5) == &ch5); // ch5 exists REQUIRE(animation.num_channels() == 3); REQUIRE(animation.channel(0).name() == "ch5"); @@ -495,49 +500,72 @@ TEST_CASE("Edge cases and error conditions", "[Animation][Id]") { } } +TEST_CASE("Id cannot be minted outside the library", "[Id]") { + // Ids are handed out by Animation and stored by Channel; the constructor is + // private so a caller cannot fabricate one. Enforced at compile time rather + // than described in a comment. + static_assert(!std::is_constructible_v, + "Id must not be constructible from a raw value by callers."); + static_assert(!std::is_default_constructible_v, + "Id must not be default-constructible."); + + // Copying an id you were given is still fine -- that is how callers pass + // them around and key containers with them. + static_assert(std::is_copy_constructible_v, + "Id must remain copy-constructible."); + SUCCEED("Id construction is restricted to the library"); +} + TEST_CASE("Id ordering and hashing", "[Id]") { + // Ids can only come from the library, so take real ones. They are handed + // out from a single increasing counter, so these are in ascending order. + Animation animation("id_semantics"); + const Id first = animation.create_channel("first").id(); + const Id second = animation.create_channel("second").id(); + const Id third = animation.create_channel("third").id(); + SECTION("operator< gives a strict weak ordering") { - REQUIRE(Id(1) < Id(2)); - REQUIRE_FALSE(Id(2) < Id(1)); - REQUIRE_FALSE(Id(5) < Id(5)); // irreflexive + REQUIRE(first < second); + REQUIRE_FALSE(second < first); + REQUIRE_FALSE(first < first); // irreflexive } SECTION("Id is usable as a std::set key (relies on operator<)") { std::set ids; - ids.insert(Id(3)); - ids.insert(Id(1)); - ids.insert(Id(2)); - ids.insert(Id(1)); // duplicate, must not grow the set + ids.insert(third); + ids.insert(first); + ids.insert(second); + ids.insert(first); // duplicate, must not grow the set REQUIRE(ids.size() == 3); // std::set iterates in ascending order auto it = ids.begin(); - REQUIRE(it->id == 1); ++it; - REQUIRE(it->id == 2); ++it; - REQUIRE(it->id == 3); + REQUIRE(*it == first); ++it; + REQUIRE(*it == second); ++it; + REQUIRE(*it == third); } SECTION("std::hash enables use in unordered containers") { std::unordered_set ids; - ids.insert(Id(10)); - ids.insert(Id(20)); - ids.insert(Id(10)); // duplicate: same hash and equality + ids.insert(first); + ids.insert(second); + ids.insert(first); // duplicate: same hash and equality REQUIRE(ids.size() == 2); - REQUIRE(ids.count(Id(10)) == 1); - REQUIRE(ids.count(Id(99)) == 0); + REQUIRE(ids.count(first) == 1); + REQUIRE(ids.count(Id::invalid()) == 0); // Equal ids must hash equally std::hash hasher; - REQUIRE(hasher(Id(42)) == hasher(Id(42))); + REQUIRE(hasher(first) == hasher(first)); } SECTION("explicit conversion and invalid() sentinel") { - Id id(12345); - REQUIRE(static_cast(id) == 12345); - REQUIRE(id.is_valid()); + REQUIRE(static_cast(first) == first.id); + REQUIRE(first.is_valid()); Id invalid = Id::invalid(); REQUIRE_FALSE(invalid.is_valid()); REQUIRE(invalid.id == static_cast(-1)); + REQUIRE(invalid != first); } }