From e8c1892cf53a4e66f37a698e8c9d171b4bb4093a Mon Sep 17 00:00:00 2001 From: keithlostracco Date: Sat, 25 Jul 2026 17:32:43 -0700 Subject: [PATCH 1/4] fix: return references from Animation Id lookups and drop the glad dependency Closes #52. channel(Id) and operator[](Id), const and non-const, returned Channel* but could never return nullptr: they resolve through unordered_map::at, which throws std::out_of_range on a miss. The pointer return only invited null checks that are dead code, and it was inconsistent with the index and name overloads, which return references. They now return references too. Miss behavior is unchanged; callers replace -> with . at the call site. Closes #53. glad 0.1.36 is the final 0.1.x release and declares cmake_minimum_required(VERSION 3.0), which CMake 4 rejects, so the examples carried a CMAKE_POLICY_VERSION_MINIMUM workaround. glad 2.x is not a drop-in: it has no root CMakeLists.txt and needs Python at configure time. Removing glad entirely turned out to be simpler than migrating. The ImGui backend already bundles its own loader and uses it when no loader macro is defined, and the only direct GL calls in curve_visualization are glViewport, glClearColor and glClear, which are OpenGL 1.1 core and come from the GL header GLFW includes. The workaround goes with it. Verified against CMake 4.4 with no policy override: library, tests and both examples build, and all 70 tests pass. The viewer was run before and after; both write an identical imgui.ini window layout, so the change is behavior-neutral. Also ignores imgui.ini, which the example writes into the working directory, and build-*/ for alternate build trees. --- .gitignore | 5 +++ CHANGELOG.md | 26 +++++++++++ examples/CMakeLists.txt | 22 ++------- examples/curve_visualization.cpp | 10 ++--- include/anim/animation.hpp | 8 ++-- src/animation.cpp | 12 ++--- tests/test_animation.cpp | 26 +++++------ tests/test_id_functionality.cpp | 76 ++++++++++++++++---------------- 8 files changed, 99 insertions(+), 86 deletions(-) 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..e392bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,32 @@ 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]). + +### Removed + +- 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 + +- `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..7ccb76c 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,11 +158,10 @@ 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; - } + // No explicit loader is needed: the ImGui OpenGL3 backend resolves the + // modern entry points it uses through its own bundled loader, and the few + // calls made directly below (glViewport / glClearColor / glClear) are + // OpenGL 1.1 core, provided by the GL header GLFW includes. // Setup Dear ImGui context IMGUI_CHECKVERSION(); diff --git a/include/anim/animation.hpp b/include/anim/animation.hpp index 66d09f5..c706b5d 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(); } diff --git a/src/animation.cpp b/src/animation.cpp index 7d625b5..689b038 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); } diff --git a/tests/test_animation.cpp b/tests/test_animation.cpp index 9624d63..eeede25 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,11 +1150,11 @@ 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") { @@ -1167,7 +1167,7 @@ TEST_CASE("Animation Id-based access error paths", "[Animation][Id]") { 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); } diff --git a/tests/test_id_functionality.cpp b/tests/test_id_functionality.cpp index dcc40ca..776f727 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") { @@ -156,14 +156,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 +178,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") { @@ -218,9 +218,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 +237,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 +264,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 +291,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 +315,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 +357,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 +378,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"); } } @@ -458,7 +458,7 @@ TEST_CASE("Edge cases and error conditions", "[Animation][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 +484,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"); From 26c7f5d6e4a071c45e6f48cc2b9dfb436642140f Mon Sep 17 00:00:00 2001 From: keithlostracco Date: Sat, 25 Jul 2026 18:17:44 -0700 Subject: [PATCH 2/4] fix: give the curve_visualization windows a usable first-run layout Running the example from a clean checkout showed no plot at all. The plot window was opened with no size, so ImGui auto-fitted it to its content. That content is ImPlot sized ImVec2(-1, -1), meaning fill the available space. On the first frame neither had a size to work from, so both resolved to nothing and the window collapsed to 32x39 behind the curve editor, which is docked at the same default position. ImGui then wrote that size to imgui.ini, so it stayed collapsed on every later run until the window was found and dragged out by hand. Both windows now get a first-run position and size derived from the viewport work area, side by side. ImGuiCond_FirstUseEver means a layout the user has arranged is still loaded from imgui.ini as before. This is long-standing behavior, not a regression: a build of bd5f9f9, before any change in this branch, writes the same 32x39 plot window. It was only ever masked by a saved imgui.ini. --- CHANGELOG.md | 7 +++++++ examples/curve_visualization.cpp | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e392bac..2e54f8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,13 @@ While the major version is `0`, breaking changes may land in a minor release. ### 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. diff --git a/examples/curve_visualization.cpp b/examples/curve_visualization.cpp index 7ccb76c..52f2d66 100644 --- a/examples/curve_visualization.cpp +++ b/examples/curve_visualization.cpp @@ -233,6 +233,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 @@ -513,6 +528,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) { From 85de52649f87e225cedf49ea93428cd5f26ec7fb Mon Sep 17 00:00:00 2001 From: keithlostracco Date: Sat, 25 Jul 2026 18:43:36 -0700 Subject: [PATCH 3/4] feat: add Animation::sort_channels and make the Id constructor private Ids could be fabricated by any caller. That was never a safety hole -- a made-up id resolves through the same map as a real one and simply throws when absent -- but ids come from a single counter shared by every Animation, so a hand-made id could silently match an unrelated channel. The constructor is now private, with Animation and Channel as friends. Callers obtain ids from Channel::id(), or Id::invalid() for a sentinel. No test needed friending. The tests that fabricated ids now use real ones, and the 'valid but not in this animation' cases take an id from a second Animation, which is what that case actually looks like in practice. A static_assert pins the constructor down. sort_channels() sorts by name; the overload taking a comparator covers any other ordering. Both are stable, so channels sharing a name keep their relative order. Only the owning pointers are reordered, so the Channel objects stay put: the id map remains valid and references taken before the sort keep working. That invariant is covered by a test. 72 tests pass, up from 70. --- CHANGELOG.md | 12 ++++ include/anim/animation.hpp | 18 +++++ include/anim/id.hpp | 17 ++++- src/animation.cpp | 21 +++++- tests/test_animation.cpp | 119 ++++++++++++++++++++++++++++++-- tests/test_id_functionality.cpp | 78 ++++++++++++++------- 6 files changed, 231 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e54f8c..b14f9ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,20 @@ While the major version is `0`, breaking changes may land in a minor release. `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`. diff --git a/include/anim/animation.hpp b/include/anim/animation.hpp index c706b5d..495375e 100644 --- a/include/anim/animation.hpp +++ b/include/anim/animation.hpp @@ -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 689b038..81796e7 100644 --- a/src/animation.cpp +++ b/src/animation.cpp @@ -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 eeede25..8401906 100644 --- a/tests/test_animation.cpp +++ b/tests/test_animation.cpp @@ -1158,9 +1158,9 @@ TEST_CASE("Animation Id-based access error paths", "[Animation][Id]") { } 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") { @@ -1173,12 +1173,12 @@ TEST_CASE("Animation Id-based access error paths", "[Animation][Id]") { } 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 776f727..220ec1f 100644 --- a/tests/test_id_functionality.cpp +++ b/tests/test_id_functionality.cpp @@ -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); } @@ -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); } @@ -453,8 +457,9 @@ 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 @@ -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); } } From ba9e01fb1e0b5aedbb595d197b7a4353379b1a50 Mon Sep 17 00:00:00 2001 From: keithlostracco Date: Sat, 25 Jul 2026 18:55:02 -0700 Subject: [PATCH 4/4] Clean up comments in curve_visualization.cpp Removed comments about ImGui OpenGL3 backend loader. --- examples/curve_visualization.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/examples/curve_visualization.cpp b/examples/curve_visualization.cpp index 52f2d66..4e86105 100644 --- a/examples/curve_visualization.cpp +++ b/examples/curve_visualization.cpp @@ -158,11 +158,6 @@ int main() { glfwMakeContextCurrent(window); glfwSwapInterval(1); // Enable vsync - // No explicit loader is needed: the ImGui OpenGL3 backend resolves the - // modern entry points it uses through its own bundled loader, and the few - // calls made directly below (glViewport / glClearColor / glClear) are - // OpenGL 1.1 core, provided by the GL header GLFW includes. - // Setup Dear ImGui context IMGUI_CHECKVERSION(); ImGui::CreateContext();