From b73b406f92b64b99982803ad3ef6a61703753297 Mon Sep 17 00:00:00 2001 From: mledour Date: Mon, 22 Jun 2026 14:08:16 +0200 Subject: [PATCH] refactor(overlay): derive histogram quad slots, fix stale geometry comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- openxr-api-layer/shaders/overlay_bars_ps.hlsl | 2 +- openxr-api-layer/utils/overlay_renderer.cpp | 15 ++++++++++----- openxr-api-layer/utils/overlay_renderer.h | 11 +++++++---- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/openxr-api-layer/shaders/overlay_bars_ps.hlsl b/openxr-api-layer/shaders/overlay_bars_ps.hlsl index bf630ce..0cf2971 100644 --- a/openxr-api-layer/shaders/overlay_bars_ps.hlsl +++ b/openxr-api-layer/shaders/overlay_bars_ps.hlsl @@ -64,7 +64,7 @@ float4 PSMain(VSOutput i) : SV_TARGET // a bar chart and removes that artifact; the shared baseline at the // strip floor stays crisp for the same reason. // - // CONTRACT: bar geometry stays integer-aligned (4-px width, integer + // CONTRACT: bar geometry stays integer-aligned (5-px width, integer // xLeft — see HistogramBarRenderer::drawPanel). With that, covX // collapses to 1 everywhere and the sides ALSO render crisp — the // bars look pixel-perfect uniform. If a future layout reintroduces diff --git a/openxr-api-layer/utils/overlay_renderer.cpp b/openxr-api-layer/utils/overlay_renderer.cpp index cc5afa5..654ee76 100644 --- a/openxr-api-layer/utils/overlay_renderer.cpp +++ b/openxr-api-layer/utils/overlay_renderer.cpp @@ -2464,12 +2464,17 @@ namespace openxr_api_layer::detail { // bars; the count derives from kQuadSlots so adding a slot can't // silently desync it. kSlotGridEnd (one past the grid run) stays // separate from the budget slot that immediately follows it. - static constexpr UINT kSlotGridFirst = 1; // interior gridlines [1..4] + static constexpr UINT kSlotGridFirst = 1; // interior gridlines [kSlotGridFirst, kSlotGridEnd) static constexpr UINT kSlotGridEnd = 5; // one past the last grid slot - static constexpr UINT kSlotBudget = 5; // budget line - static constexpr UINT kSlotBaseline = 6; // 0-ms baseline - static constexpr UINT kSlotAxis = 7; // left vertical ms-axis - static constexpr UINT kQuadSlots = 8; // total + // Trailing single-quad slots derive sequentially from kSlotGridEnd + // so they can never silently collide with the grid run: bump + // kSlotGridEnd (e.g. to add an interior gridline) and budget / + // baseline / axis / total all shift up in lockstep instead of one + // quietly overwriting a grid slot. Values are unchanged (5/6/7/8). + static constexpr UINT kSlotBudget = kSlotGridEnd; // budget line + static constexpr UINT kSlotBaseline = kSlotBudget + 1; // 0-ms baseline + static constexpr UINT kSlotAxis = kSlotBaseline + 1; // left vertical ms-axis + static constexpr UINT kQuadSlots = kSlotAxis + 1; // total // The [kSlotGridFirst, kSlotGridEnd) gridline slots must cover every // non-zero tick (kMaxMsAxisTicks counts the 0 tick, drawn as the // baseline rather than an interior line). Catches a kMaxMsAxisTicks diff --git a/openxr-api-layer/utils/overlay_renderer.h b/openxr-api-layer/utils/overlay_renderer.h index 487e6a1..c0ade4c 100644 --- a/openxr-api-layer/utils/overlay_renderer.h +++ b/openxr-api-layer/utils/overlay_renderer.h @@ -171,10 +171,13 @@ namespace openxr_api_layer::detail { // code can declare matching rings without hard-coding the value // (and the cpp's static_assert below guards against drift). // - // 133 = the bar count that fills the GPU histogram strip exactly - // with the fixed 4-px-bar / 1-px-gap layout: 133×4 + 132×1 = 664 px - // (the strip's inner width). Picking it leaves zero margin and keeps - // every bar pixel-aligned. ~133 samples ≈ 1.5 s @ 90 Hz. + // 133 = the histogram ring size. Originally chosen so the strip filled + // exactly with 4-px bars + 1-px gaps (133×4 + 132×1 = 664 px). The bars + // are now 5-px wide at a 5-px step with no gap (see overlay_renderer.cpp), + // so 133 bars span 133×5 = 665 px and the .cpp centres or left-clips the + // run within the strip's actual width rather than filling it exactly. The + // count is retained for the time window it captures: ~133 samples ≈ + // 1.5 s @ 90 Hz. constexpr std::size_t kOverlayHistoRingSize = 133; // -------- GPU-path snapshot entry point (Task 18) ---------------------