Shadow pass: decide in preparation, record from the plan

CMakeLists.txt

@@ -105,6 +105,8 @@ add_library(fireengine SHARED
     src/graphics/shadow_caster_alpha.cpp
     src/graphics/shadow_bias.cpp
     src/graphics/shadow_map_validity.cpp
+    src/graphics/shadow_pass_plan.cpp
+    src/graphics/shadow_pass_prepare.cpp
     src/graphics/shadow_caster_deformation.cpp
     src/graphics/shadow_render_view.cpp
     src/graphics/shadow_diagnostics.cpp
@@ -447,6 +449,8 @@ add_executable(test_fire_engine
     tests/graphics/test_shadow_caster_alpha.cpp
     tests/graphics/test_shadow_bias.cpp
     tests/graphics/test_shadow_map_validity.cpp
+    tests/graphics/test_shadow_pass_plan.cpp
+    tests/graphics/test_shadow_pass_prepare.cpp
     tests/graphics/test_shadow_render_view.cpp
     tests/graphics/test_texture.cpp
     tests/graphics/test_sampler_settings.cpp
@@ -522,6 +526,7 @@ add_custom_target(tests-full
   COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R shader_block_guards
     COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R shadow_bias_guard
   COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R gpu_limits_guard
+  COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R shadow_matrix_guard
   DEPENDS test_fire_engine
   COMMENT "Running the full test suite, including [slow] tests"
 )
@@ -567,6 +572,16 @@ add_test(
     -P ${PROJECT_SOURCE_DIR}/cmake/check_gpu_limits.cmake
 )
 
+# Arc 2 #4 step 1: the shadow transform reaches the GPU once, in the recorded view's push constants.
+# A reintroduced per-draw matrix table renders correctly on the day and silently detaches the shadow
+# cache's comparison from what the GPU actually rasterises with.
+add_test(
+  NAME shadow_matrix_guard
+  COMMAND ${CMAKE_COMMAND}
+    -DSHADER_DIR=${PROJECT_SOURCE_DIR}/shaders
+    -P ${PROJECT_SOURCE_DIR}/cmake/check_shadow_matrix.cmake
+)
+
 find_program(CLANG_TIDY_EXE NAMES clang-tidy)
 if(CLANG_TIDY_EXE)
     get_target_property(FIRE_ENGINE_TIDY_SOURCES fireengine SOURCES)

README.md

@@ -232,7 +232,7 @@ The transient pipelines are destroyed once the bake completes; only the resultin
   - 13 SSAO/contact-shadow texture
 
   On swapchain resize, only the `kMaxFramesInFlight` set-1 descriptors need rewriting (sceneColor, post-process targets, and any future recreated globals) via `Descriptors::updateGlobalDescriptors`; the global set-2 bindless descriptors are untouched, and forward set 0 is never allocated (pushed per draw).
-- Separate descriptor layouts for the skybox (SkyboxUBO + samplerCube + LightUBO), shadow (ShadowUBO with `lightViewProj[]` + SkinUBO + MorphUBO + MorphTargets SSBO + first self-shadow depth/sampler, plus `ShadowPushConstants` on the vertex/fragment stages), post-process (HDR sampler at 0 + bloom mip 0 sampler at 1, plus `PostProcessPushConstants { float bloomStrength }`), and bloom-down / bloom-up (single input mip sampler + `BloomPushConstants` on the fragment stage)
+- Separate descriptor layouts for the skybox (SkyboxUBO + samplerCube + LightUBO), shadow (ShadowUBO — per-object model + hasSkin — plus SkinUBO + MorphUBO + MorphTargets SSBO + first self-shadow depth/sampler, with the view's own transform arriving in `ShadowPushConstants` on the vertex/fragment stages), post-process (HDR sampler at 0 + bloom mip 0 sampler at 1, plus `PostProcessPushConstants { float bloomStrength }`), and bloom-down / bloom-up (single input mip sampler + `BloomPushConstants` on the fragment stage)
 - Two forward pipeline variants share the shader + binding layout:
   - **opaque** (no blend, depth write) — OPAQUE and MASK materials. Cull mode is a **dynamic state** (`VK_DYNAMIC_STATE_CULL_MODE`, core Vulkan 1.3) set per draw, so single-sided (cull back) and double-sided (cull none) geometry share this one pipeline; `DrawCommand::doubleSided` carries the choice.
   - **blend** (cull none, `SRC_ALPHA / ONE_MINUS_SRC_ALPHA` blend, no depth write) — BLEND materials. Kept as a separate static-blend pipeline because dynamic blend state isn't available on MoltenVK (see [Limitations](#limitations)).

cmake/check_gpu_limits.cmake

@@ -1,12 +1,13 @@
 # Guard: the limits shared by C++ and GLSL are declared ONCE, in shaders/gpu_limits.glsl, and both
 # sides actually read them.
 #
-# These numbers size UBO arrays and index the shadow matrix table. A one-sided change — a shader
-# raising a caster count that the C++ struct still writes at the old size, or the reverse — compiles
-# cleanly in both languages and then reads the wrong region of a bound buffer: every index stays in
-# range, so there is no validation error and no crash, just a shadow matrix taken from another
-# family's slot. `SHADOW_TOTAL_MATRIX_COUNT = 32`, `SHADOW_POINT_MATRIX_BASE = 8` and the caster
-# counts were each hand-transcribed exactly that way before this guard existed.
+# These numbers size the arrays in blocks both languages bind. A one-sided change — a shader raising
+# a caster count that the C++ struct still writes at the old size, or the reverse — compiles cleanly
+# in both languages and then reads the wrong region of a bound buffer: every index stays in range, so
+# there is no validation error and no crash, just one light's matrix read from another's slot. The
+# caster counts, the cascade count and the SSAO kernel size were each hand-transcribed exactly that
+# way before this guard existed. (The shadow MATRIX-TABLE constants it also used to cover are gone:
+# every shadow path rasterises with its view's pushed matrix — see `shadow_matrix_guard`.)
 #
 # Two halves, and BOTH are needed. The GLSL half fails if a shader re-declares a shared name or
 # stops including the file. The C++ half fails if graphics/gpu_limits.hpp stops including the shared
@@ -46,10 +47,6 @@ set(shared_names
     MAX_SPOT_SHADOW_CASTERS
     MAX_POINT_SHADOW_CASTERS
     CUBE_FACE_COUNT
-    SHADOW_CASCADE_MATRIX_BASE
-    SHADOW_SPOT_MATRIX_BASE
-    SHADOW_POINT_MATRIX_BASE
-    SHADOW_TOTAL_MATRIX_COUNT
     SHADOW_MAP_VALID_CASCADES
     SHADOW_MAP_VALID_WORLD_ONLY
     SHADOW_MAP_VALID_SELF
@@ -67,10 +64,6 @@ set(cpp_names
     kMaxSpotShadowCasters
     kMaxPointShadowCasters
     kCubeFaceCount
-    kShadowCascadeMatrixBase
-    kShadowSpotMatrixBase
-    kShadowPointMatrixBase
-    kShadowTotalMatrixCount
     kShadowMapValidCascades
     kShadowMapValidWorldOnly
     kShadowMapValidSelf
@@ -100,8 +93,6 @@ set(consumers
     "light_ubo.glsl:MAX_SPOT_SHADOW_CASTERS:2"
     "light_ubo.glsl:MAX_SKINNED_SELF_SHADOW_CASTERS:2"
     "light_ubo.glsl:MAX_POINT_SHADOW_CASTERS:1"
-    "shadow.vert:SHADOW_TOTAL_MATRIX_COUNT:1"
-    "shadow_depth.glsl:SHADOW_POINT_MATRIX_BASE:1"
     "self_shadow_second.glsl:MAX_SKINNED_SELF_SHADOW_CASTERS:1"
     "shader.frag:SHADOW_CASCADE_COUNT:4" # cascade search init + bound, blend factor, debug divisor
     "shader.frag:MAX_SKINNED_SELF_SHADOW_CASTERS:1"

cmake/check_shadow_matrix.cmake

@@ -0,0 +1,82 @@
+# Guard: ONE shadow transform reaches the GPU, and it is the recorded view's.
+#
+# The shadow pass used to push a 32-matrix table of every shadow view into every draw's per-object
+# UBO and select a row with a push constant. That made the transform a per-DRAW lookup rather than a
+# property of the view being recorded, and it left two descriptions of the same value: the table the
+# vertex shader indexed, and the matrix everything else reasoned about. Arc 2 #4 needs those to be
+# one thing — a cached shadow map may only be reused if the matrix compared is the matrix rasterised
+# with — so `pc.lightViewProj` is now the only shadow transform, for every family.
+#
+# Reintroducing the table would compile and render correctly on the frame it was added; the damage
+# is that the cache's comparison would silently stop describing what the GPU does. Nothing else can
+# catch that, hence a build-time check.
+#
+# The same reasoning covers the depth discriminator: the point path branched on "is this matrix
+# index at or past the point base", inferring a depth mode from where a matrix happened to live.
+# It now reads `pc.radialDepth`, which is `PreparedShadowView::depthMode()` and nothing else.
+#
+# Invoked as a CTest case; needs SHADER_DIR.
+
+if(NOT DEFINED SHADER_DIR)
+  message(FATAL_ERROR "SHADER_DIR must be set (path to shaders/)")
+endif()
+
+include("${CMAKE_CURRENT_LIST_DIR}/strip_glsl_comments.cmake")
+
+set(offenders "")
+
+# Every shader, so a NEW shadow stage cannot quietly reintroduce either pattern.
+file(GLOB shader_sources
+     "${SHADER_DIR}/*.glsl" "${SHADER_DIR}/*.vert" "${SHADER_DIR}/*.frag" "${SHADER_DIR}/*.comp")
+foreach(shader IN LISTS shader_sources)
+  file(READ "${shader}" shader_text)
+  strip_glsl_comments("${shader_text}" shader_code)
+  get_filename_component(shader_name "${shader}" NAME)
+
+  # An ARRAY of light matrices — the table itself, wherever it is declared. `mat4 lightViewProj[N]`
+  # in a block, or an index into one.
+  if(shader_code MATCHES "lightViewProj[ \t]*\\[")
+    list(APPEND offenders
+         "${shader_name} indexes or declares lightViewProj[] — the per-draw shadow matrix table is retired; use pc.lightViewProj, the matrix of the view being recorded")
+  endif()
+  # The ShadowUBO member it used to live in.
+  if(shader_code MATCHES "shadow[ \t]*\\.[ \t]*lightViewProj")
+    list(APPEND offenders
+         "${shader_name} reads shadow.lightViewProj — ShadowUBO carries the object's model matrix only")
+  endif()
+  # The selector. Its absence is what forces a depth mode to be stated rather than inferred.
+  if(shader_code MATCHES "matrixIndex")
+    list(APPEND offenders
+         "${shader_name} mentions matrixIndex — the push block carries radialDepth (the view's depth mode) in its place")
+  endif()
+endforeach()
+
+# And the positive half: the paths that must consume the pushed values still do. Without these the
+# checks above would pass on a shader that had stopped drawing anything at all.
+set(required
+    "shadow.vert:pc[ \t]*\\.[ \t]*lightViewProj:the shadow vertex stage must rasterise with the pushed view matrix"
+    "shadow_push.glsl:int[ \t]+radialDepth:the shared push block must declare the depth-mode discriminator"
+    "shadow_depth.glsl:pc[ \t]*\\.[ \t]*radialDepth:the point-face depth path must branch on the pushed depth mode")
+foreach(entry IN LISTS required)
+  string(REPLACE ":" ";" parts "${entry}")
+  list(GET parts 0 required_file)
+  list(GET parts 1 required_pattern)
+  list(GET parts 2 required_reason)
+  set(path "${SHADER_DIR}/${required_file}")
+  if(NOT EXISTS "${path}")
+    list(APPEND offenders "${required_file} is missing from ${SHADER_DIR}")
+    continue()
+  endif()
+  file(READ "${path}" required_text)
+  strip_glsl_comments("${required_text}" required_code)
+  if(NOT required_code MATCHES "${required_pattern}")
+    list(APPEND offenders "${required_file}: ${required_reason}")
+  endif()
+endforeach()
+
+if(offenders)
+  string(REPLACE ";" "\n  " report "${offenders}")
+  message(FATAL_ERROR "shadow matrix guard failed:\n  ${report}")
+endif()
+
+message(STATUS "shadow matrix guard: pc.lightViewProj is the only shadow transform; depth mode is stated, not inferred")

docs/architecturalreview.md

@@ -376,7 +376,7 @@ exists; the tiered review inherited the filename.)*
 | 1 | ✅ Skip redundant world-CSM when no skinned casters *(branch `review-shadow-taa-fixes`)* | B | S | §2.2 |
 | 2 | ✅ Skip empty self-shadow slots *(same branch; implementation is simpler than proposed — unassigned slots are provably never sampled, so no clear-once/dirty bit is needed, they are skipped outright)* | B | S | §2.1 |
 | 3 | ✅ `static_assert(kMaxFramesInFlight == 2)` on the TAA history index *(same branch)* | A | XS | §2.4 |
-| 4 | Static-scene CSM caching (light+fit+caster epoch) | B | M | §2.1 |
+| 4 | Static-scene CSM caching (light+fit+caster epoch) — **in progress, branch `shadow-static-cascade-cache`**. The review's "coarse validity check" was rejected on inspection: light dir + snapped origin + a caster epoch explains a matrix without being one, and a wrong answer here is a silently wrong image. What landed instead is the exact content descriptor (`graphics/shadow_pass_plan.hpp`, structural comparison, no hash) and the PREPARATION phase that builds it (`graphics/shadow_pass_prepare.hpp`) — filtering, LOD resolution and diagnostics moved out of recording, so `Shadows::recordPass` now consumes the plan alone. Every view is still `Recorded` (no residency store yet), verified by a byte-identical per-view diagnostic dump across three scenes and all five families. The residency store + reuse is the remaining half. | B | M | §2.1 |
 | 5 | Compute pre-skinning pass (unify with cloth pattern) | B | L | §1.3 |
 | 6 | Batch image barriers into single `DependencyInfo`s | C | S | §1.2 |
 | 7 | Physics per-step scratch persistence | B | S | §3.1 |

docs/codereview.md

@@ -561,17 +561,21 @@ hash specialisation, so ordinary callers cannot accidentally manufacture a plaus
 Several constants in [`gpu_limits.hpp`](include/fire_engine/graphics/gpu_limits.hpp) and
 [`constants.hpp`](include/fire_engine/render/constants.hpp) encode relationships manually:
 
-- `kShadowSpotMatrixBase` is the literal `4` rather than
-  `kShadowCascadeMatrixBase + kShadowCascadeCount`;
+- ~~`kShadowSpotMatrixBase` is the literal `4` rather than
+  `kShadowCascadeMatrixBase + kShadowCascadeCount`~~ — **moot** (branch
+  `shadow-static-cascade-cache`): the shadow matrix TABLE is gone. The transform now reaches the GPU
+  once, in the recorded view's push constants, so there are no bases to derive and no table to index
+  the wrong row of; `shadow_matrix_guard` fails the build if one comes back;
 - cubemap mip counts repeat values derivable from their extents;
-- `kShadowTotalMatrixCount` depends on several signed and unsigned values with no range assertion;
+- ~~`kShadowTotalMatrixCount` depends on several signed and unsigned values with no range
+  assertion~~ — **moot**, same change: the constant no longer exists;
 - projection, shadow range, TAA sample count, and mip settings have no compile-time relational
   validation.
 
 Use C++23/standard-library derivation where possible, for example `std::bit_width(extent)` for a
 full power-of-two mip chain. Add `static_assert`s or a `consteval` validator for relationships such
-as positive near planes, far > near, non-zero TAA cycle, power-of-two extents, and non-overlapping
-shadow matrix ranges.
+as positive near planes, far > near, non-zero TAA cycle and power-of-two extents. (Non-overlapping
+shadow matrix ranges are no longer expressible — the table they indexed is gone.)
 
 Group defaults by domain (`CameraDefaults`, `ShadowDefaults`, `IblDefaults`, etc.) or at least put
 them in nested namespaces. The current global `k...` list is manageable today, but grouping would

docs/lod.md

@@ -81,9 +81,13 @@ Object::writeForwardUniforms()            [per draw, per frame]
   `proj[1][1]`, and viewport height. It sets the chosen level's `indexBuffer`/`indexCount` on the
   `DrawCommand`, plus `lodLevel` (used only by the LOD-tint debug view). This is the FORWARD draw
   only. Since SH-03 a caster's *shadow* level is not chosen here at all: the shadow command carries a
-  `ShadowGeometryRequest` (LOD span, conservative world scale, caster id + generation) with no index
-  buffer, and each shadow view resolves its own level in its own shadow-map texels — see
-  [`shadowplans.md`](shadowplans.md) § SH-03 and `graphics/shadow_lod_resolver.hpp`. Two caster
+  `ShadowGeometryRequest` (LOD span, a `ShadowCasterPose` pairing the caster's world matrix with the
+  conservative world scale derived from it, caster id + generation) with no index buffer, and each
+  shadow view resolves its own level in its own shadow-map texels — see
+  [`shadowplans.md`](shadowplans.md) § SH-03 and `graphics/shadow_lod_resolver.hpp`. Since arc 2 #4
+  that resolution happens in the shadow pass's PREPARATION phase
+  (`graphics/shadow_pass_prepare.hpp`), before anything is recorded, because the same walk decides
+  whether the view's map can be reused at all. Two caster
   properties override that selection outright, and both are classified at this same seam: a caster
   that DEFORMS after the simplifier measured it (SH-04) and one whose coverage is an ALPHA CUTOUT
   (SH-05) resolve to the whole mesh, with `DeformableFallback` / `AlphaMaskedFallback` as the reason