Skip to content

Return references from Id lookups, and remove the glad dependency - #54

Merged
keithlostracco merged 4 commits into
mainfrom
fix/channel-id-ref-and-drop-glad
Jul 26, 2026
Merged

Return references from Id lookups, and remove the glad dependency#54
keithlostracco merged 4 commits into
mainfrom
fix/channel-id-ref-and-drop-glad

Conversation

@keithlostracco

Copy link
Copy Markdown
Contributor

Closes #52. Closes #53.

#52channel(Id) returned a pointer that could never be null

Both Id overloads resolve through unordered_map::at, which throws on a
miss, so the only outcomes were a valid non-null pointer or an exception. The
Channel* return invited null checks that are dead code, and it was the odd
one out — the index and name overloads return references.

All four (channel(Id) / operator[](Id), const and non-const) now return
references. Miss behavior is unchanged: still std::out_of_range.

Breaking for callers, mechanically: -> becomes ..

The test updates deliberately keep &a == &b identity comparisons rather than
rewriting to a == bChannel::operator== is value equality, so the
simpler-looking form would have silently weakened 49 assertions from identity
to equality.

#53 — glad removal, and the CMake 4 workaround goes with it

glad 0.1.36 is the last 0.1.x release and declares
cmake_minimum_required(VERSION 3.0), which CMake 4 rejects — hence the
CMAKE_POLICY_VERSION_MINIMUM 3.5 workaround. glad 2.x is not a drop-in: no
root CMakeLists.txt, and it needs Python at configure time.

Removing glad was simpler than migrating to it:

  • ImGui already bundles imgui_impl_opengl3_loader.h and uses it when no
    loader macro is defined — the IMGUI_IMPL_OPENGL_LOADER_GLAD definition was
    the only thing redirecting it to glad
  • the sole direct GL calls in curve_visualization are glViewport,
    glClearColor and glClear, all OpenGL 1.1 core, available from the GL
    header GLFW includes and already linked via OpenGL::GL

One dependency and one workaround removed together.

Verification

Against CMake 4.4 with no policy override: configure, build of library +
tests + both examples, and 70/70 tests pass.

The viewer was launched from a build with glad and a build without, each
from a clean layout. Both write an identical imgui.ini:

Window with glad without glad
###CurvesPlot Pos=60,60 Size=32,39 Pos=60,60 Size=32,39
Curve Editor Pos=60,60 Size=156,116 Pos=60,60 Size=156,116

So the change is behavior-neutral. Note this also documents a pre-existing
quirk, present on both sides and not addressed here: on a first run with no
saved layout the plot window auto-sizes to 32x39 and sits behind the Curve
Editor, so a new user sees no plot until they resize or dock it. Worth a
separate issue.

Also

imgui.ini is written into the working directory by the example and was not
ignored, so it appeared as untracked noise in the repo root. Now ignored, along
with build-*/.

…pendency

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.
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.
@keithlostracco

Copy link
Copy Markdown
Contributor Author

Added: the example rendered no plot on a clean checkout

Investigated whether this branch caused it. It did not — but it is a real bug
and it is fixed here.

Root cause. Curves Plot was opened with no size, so ImGui auto-fitted it
to its content. That content is ImPlot::BeginPlot(..., ImVec2(-1,-1), ...)
"fill the available space". On the first frame neither has a size to work from,
so both resolve to nothing, the window collapses to 32x39, and it lands behind
Curve Editor, which defaults to the same position. ImGui then persists that
size to imgui.ini, so it stays collapsed on every later run.

Not a regression. A build of bd5f9f9 — before anything in this branch —
writes an identical layout:

Build ###CurvesPlot
bd5f9f9 (pre-branch) Pos=60,60 Size=32,39
with glad (post-#50) Pos=60,60 Size=32,39
without glad Pos=60,60 Size=32,39

It was only ever masked by a saved imgui.ini from having dragged the window
out once.

Fix. Both windows get a first-run position and size derived from the
viewport work area, laid out side by side. ImGuiCond_FirstUseEver means an
arranged layout is still restored from imgui.ini as before.

After the fix, a clean first run gives:

[Window][###CurvesPlot]   Pos=12,12      Size=2460,1256
[Window][Curve Editor]    Pos=2484,12    Size=704,1256

Verified visually: the plot renders with axes, all five curves, and their
keyframes and tangent handles.

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.
Removed comments about ImGui OpenGL3 backend loader.
@keithlostracco
keithlostracco merged commit 6026ca2 into main Jul 26, 2026
8 checks passed
@keithlostracco
keithlostracco deleted the fix/channel-id-ref-and-drop-glad branch July 26, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the CMAKE_POLICY_VERSION_MINIMUM workaround by dropping glad Animation::channel(Id) returns a pointer that can never be null

1 participant