Shadow hygiene: one shared limits authority, and recording gated by e… - #139
Merged
Conversation
…xplicit map validity Arc 1's four independent shadow-hygiene items, landed together because three of them are one change. "Skip a family's recording" and "tell the receiver that family is invalid" are the same fact: skipping without the signal leaves the shader sampling a depth image the frame never wrote, and the only thing making that safe was the current arrangement of the light loop — an accident, not a property. ONE DECLARATION OF THE SHARED LIMITS shaders/gpu_limits.glsl holds every value a C++ block and a shader block must agree on, written in the subset that is simultaneously valid GLSL and valid C++: the shaders #include it, and graphics/gpu_limits.hpp includes it inside a shader_limits namespace and re-exports each value under its k-name, so the C++ names and types are unchanged while the numbers exist once. SHADOW_TOTAL_MATRIX_COUNT = 32 and SHADOW_POINT_MATRIX_BASE = 8 are gone as literals, and so is the arithmetic behind them — the matrix bases are derived in the shared file, so moving a family's capacity moves every index after it in one edit. The semantic literals went too: shader.frag's cascade search and self-slot bound, light_ubo.glsl's cascadeViewProj[4], the second-depth pass's slot bound, and the C++ LightUBO::cascadeViewProj. cascadeSplits is the one field that cannot scale — it packs one split per vec4 component — which is now a static_assert rather than a trap. That closes codereview.md's Tier 1 finding 2 rather than half of it, so the other four data-layout limits moved onto the same mechanism: joints, morph weights, particle emitters and the SSAO kernel. Morph is not a literal swap. The block packs weights as vec4s, so the shader's array length is MORPH_WEIGHT_VEC4_COUNT = MAX_MORPH_TARGETS / 4, derived in the shared file, with the divisibility asserted C++-side because GLSL cannot say it: a ninth weight would need a third vec4 the shader never declared, and the write would land outside the block. Scope stops at shared data layout. GLSL-only algorithm constants — the VDPM scan and workgroup sizes — are deliberately not claimed. RECORDING GATED BY EXPLICIT VALIDITY graphics/shadow_map_validity.hpp is a pure, Vulkan-free law: from --no-shadows, whether a primary directional light exists, and the per-family active view counts, it yields one bit per family. The renderer derives it once in uploadFrameLighting — after the COMPLETED view set, since world-only is enabled last — and uses that single value twice: it gates the families in Shadows::recordPass, and its packed form is uploaded as LightUBO::shadowMapValidMask. Neither side can be right while the other is wrong, because there is only one of them. The upload moved out of assignSelfShadowSlots for the same reason. Whole-family where the family is addressed as a unit: a fragment picks its cascade layer from its view depth, so three fitted cascades out of four is a hole rather than three quarters of a map, and the cascade and world-only families require all of them plus a real directional light (fitted to a fallback direction they would describe a sun that is not in the scene). Point requires whole cubes, leaning on setPointLight's atomic six-face installation; self and spot slots are addressed by an index the draw or the light carries, so any active slot validates them. Every receiver path asks its bit first and answers fully lit otherwise — including the raw-depth debug view, which samples the cascade image directly and reads lights[0] as the sun, both wrong without one, so it now shows magenta instead of a plausible readout of stale data. environmentParams.w, the old "disable shadow lookups" flag, is retired: it suppressed only the sampling while the pass kept rendering. shadowMapValidMask occupies what was padding, so no UBO offset moved. GUARDS gpu_limits_guard pins the arrangement from both sides: no shader may declare a shared name (the whole shaders/ tree is swept), each consumer must include the file and use each name as many times as it does today, the shared file must keep declaring everything and stay in the common subset, and gpu_limits.hpp must define each k-constant AS the shared declaration — without that half, C++ could drift back to hard-coded values behind green shader checks. The comment-stripping helper is now shared with shadow_bias_guard rather than copied. Mutation testing earned two checks and found a bug in a third. Presence was not enough: reverting ssao.frag's loop bound to 16 passed while the UBO array above it still named the constant. Counting uses fixed that — and the counter itself was wrong, because MATCHALL returns a CMake list and a match spanning a ';' split into two elements, inflating every count enough to hide a lost use. Semicolons are neutralised before counting. Fourteen mutations, fourteen failures. VERIFICATION --no-shadows is now observably suppressed, which validation-clean rendering alone cannot show: FE_LOG=render:debug prints a per-family recording line on the cascade-fit cadence, pairing one frame's decision with that same frame's counters (the validity rides the stats ring). On ShadowLodDemo it goes from "cascade recorded passes=4 0.718ms | world-only recorded passes=4 0.260ms | self 2 | spot 1 | point 6" to every family "skipped passes=0 0.000ms | no family recorded". With shadows on the reference capture is unchanged: zero pixels differ by more than 8/255 from docs/images/shadow-lod-selected.png.
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
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.
…xplicit map validity
Arc 1's four independent shadow-hygiene items, landed together because three of them are one change. "Skip a family's recording" and "tell the receiver that family is invalid" are the same fact: skipping without the signal leaves the shader sampling a depth image the frame never wrote, and the only thing making that safe was the current arrangement of the light loop — an accident, not a property.
ONE DECLARATION OF THE SHARED LIMITS
shaders/gpu_limits.glsl holds every value a C++ block and a shader block must agree on, written in the subset that is simultaneously valid GLSL and valid C++: the shaders #include it, and graphics/gpu_limits.hpp includes it inside a shader_limits namespace and re-exports each value under its k-name, so the C++ names and types are unchanged while the numbers exist once. SHADOW_TOTAL_MATRIX_COUNT = 32 and SHADOW_POINT_MATRIX_BASE = 8 are gone as literals, and so is the arithmetic behind them — the matrix bases are derived in the shared file, so moving a family's capacity moves every index after it in one edit. The semantic literals went too: shader.frag's cascade search and self-slot bound, light_ubo.glsl's cascadeViewProj[4], the second-depth pass's slot bound, and the C++ LightUBO::cascadeViewProj. cascadeSplits is the one field that cannot scale — it packs one split per vec4 component — which is now a static_assert rather than a trap.
That closes codereview.md's Tier 1 finding 2 rather than half of it, so the other four data-layout limits moved onto the same mechanism: joints, morph weights, particle emitters and the SSAO kernel. Morph is not a literal swap. The block packs weights as vec4s, so the shader's array length is MORPH_WEIGHT_VEC4_COUNT = MAX_MORPH_TARGETS / 4, derived in the shared file, with the divisibility asserted C++-side because GLSL cannot say it: a ninth weight would need a third vec4 the shader never declared, and the write would land outside the block.
Scope stops at shared data layout. GLSL-only algorithm constants — the VDPM scan and workgroup sizes — are deliberately not claimed.
RECORDING GATED BY EXPLICIT VALIDITY
graphics/shadow_map_validity.hpp is a pure, Vulkan-free law: from --no-shadows, whether a primary directional light exists, and the per-family active view counts, it yields one bit per family. The renderer derives it once in uploadFrameLighting — after the COMPLETED view set, since world-only is enabled last — and uses that single value twice: it gates the families in Shadows::recordPass, and its packed form is uploaded as LightUBO::shadowMapValidMask. Neither side can be right while the other is wrong, because there is only one of them. The upload moved out of assignSelfShadowSlots for the same reason.
Whole-family where the family is addressed as a unit: a fragment picks its cascade layer from its view depth, so three fitted cascades out of four is a hole rather than three quarters of a map, and the cascade and world-only families require all of them plus a real directional light (fitted to a fallback direction they would describe a sun that is not in the scene). Point requires whole cubes, leaning on setPointLight's atomic six-face installation; self and spot slots are addressed by an index the draw or the light carries, so any active slot validates them.
Every receiver path asks its bit first and answers fully lit otherwise — including the raw-depth debug view, which samples the cascade image directly and reads lights[0] as the sun, both wrong without one, so it now shows magenta instead of a plausible readout of stale data. environmentParams.w, the old "disable shadow lookups" flag, is retired: it suppressed only the sampling while the pass kept rendering. shadowMapValidMask occupies what was padding, so no UBO offset moved.
GUARDS
gpu_limits_guard pins the arrangement from both sides: no shader may declare a shared name (the whole shaders/ tree is swept), each consumer must include the file and use each name as many times as it does today, the shared file must keep declaring everything and stay in the common subset, and gpu_limits.hpp must define each k-constant AS the shared declaration — without that half, C++ could drift back to hard-coded values behind green shader checks. The comment-stripping helper is now shared with shadow_bias_guard rather than copied.
Mutation testing earned two checks and found a bug in a third. Presence was not enough: reverting ssao.frag's loop bound to 16 passed while the UBO array above it still named the constant. Counting uses fixed that — and the counter itself was wrong, because MATCHALL returns a CMake list and a match spanning a ';' split into two elements, inflating every count enough to hide a lost use. Semicolons are neutralised before counting. Fourteen mutations, fourteen failures.
VERIFICATION
--no-shadows is now observably suppressed, which validation-clean rendering alone cannot show: FE_LOG=render:debug prints a per-family recording line on the cascade-fit cadence, pairing one frame's decision with that same frame's counters (the validity rides the stats ring). On ShadowLodDemo it goes from "cascade recorded passes=4 0.718ms | world-only recorded passes=4 0.260ms | self 2 | spot 1 | point 6" to every family "skipped passes=0 0.000ms | no family recorded". With shadows on the reference capture is unchanged: zero pixels differ by more than 8/255 from docs/images/shadow-lod-selected.png.