refactor(overlay): derive histogram quad slots, fix stale geometry comments#75
Merged
Merged
Conversation
…mments Code-review findings 4 & 5 on the 5-px/no-gap histogram bars (#70): - overlay_renderer.cpp: kSlotBudget / kSlotBaseline / kSlotAxis / kQuadSlots were hand-pinned integers, and the static_assert only guards grid-vs-tick count, not slot overlap — so bumping kSlotGridEnd for another gridline could silently make budget and grid write the same slot (one line vanishes, assert still green). Derive the trailing slots sequentially from kSlotGridEnd so they shift in lockstep. Values are unchanged (1/5/5/6/7/8, verified via a standalone constexpr check), so there is no render change. - overlay_renderer.h + overlay_bars_ps.hlsl: comments still described the old 4-px-bar / 1-px-gap / 664-px geometry. Update to the current 5-px/no-gap layout (133×5 = 665 px). Comment-only; the pixel shader's covX edge-AA reasoning still holds (5 is integer). No behaviour change — the bit-exact overlay snapshot golden is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the overlay findings (4–5) from the code review — both on the 5-px/no-gap histogram bars merged in #70. No behaviour change (the bit-exact snapshot golden is unaffected).
4 · Quad-slot overlap not guarded (low-med)
kSlotBudget/kSlotBaseline/kSlotAxis/kQuadSlotswere hand-assigned integers. The existingstatic_assertonly checks grid-slot count vs tick count — not whetherbudgetandgridwrite the same slot. A future change bumpingkSlotGridEnd(e.g. a 5th interior gridline) would have to remember to bump all four in lockstep, or one line silently vanishes with the assert still green.Fix: derive the trailing slots sequentially:
Values are identical to before — verified with a standalone constexpr check (
first=1 end=5 budget=5 baseline=6 axis=7 total=8, allstatic_asserts green) — so rendering is unchanged.5 · Stale geometry comments (low)
overlay_renderer.hstill documented the old4-px-bar / 1-px-gap … 133×4 + 132×1 = 664 px … fills exactlymath, contradicting the.cpp's own updated 5-px/no-gap text. The pixel-shader CONTRACT comment likewise said "4-px width". Updated both to the 5-px/no-gap geometry (133×5 = 665 px). Comment-only — thecovXedge-AA reasoning still holds since 5 is integer.Declined (rationale)
usedW = n*step − (step−bar)): this is the correct general formula for n bars of widthbarat pitchstep;kBarPx/kStepPxexist precisely so a gap can be reintroduced, wheren*stepwould be a latent off-by-one. Intentional generality, already documented.std::sin): pre-existing and a deliberate design choice (the workflow documents why the snapshot tolerates zero diff). Reviewer flagged for completeness, no action.🤖 Generated with Claude Code