From 7aa11f69e41c194cbc5ab225147a5eb5597c5f44 Mon Sep 17 00:00:00 2001 From: Nick Newson Date: Sat, 8 Aug 2026 11:03:03 +0100 Subject: [PATCH] Shadow hygiene: one shared limits authority, and recording gated by explicit map validity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CLAUDE.md | 7 +- CMakeLists.txt | 26 ++- README.md | 7 +- cmake/check_gpu_limits.cmake | 221 ++++++++++++++++++ cmake/check_shadow_bias.cmake | 12 +- cmake/strip_glsl_comments.cmake | 23 ++ docs/acceptance-testing.md | 20 ++ docs/codereview.md | 44 +++- docs/onboarding.md | 26 ++- docs/review-order.md | 8 +- docs/roadmap.md | 22 +- docs/shadowplans.md | 92 ++++++-- include/fire_engine/graphics/gpu_limits.hpp | 90 +++++-- .../graphics/shadow_map_validity.hpp | 87 +++++++ include/fire_engine/render/renderer.hpp | 23 ++ include/fire_engine/render/shadows.hpp | 18 +- include/fire_engine/render/ubo.hpp | 24 +- shaders/gpu_limits.glsl | 87 +++++++ shaders/light_ubo.glsl | 21 +- shaders/particle.vert | 6 +- shaders/particle_simulate.comp | 7 +- shaders/self_shadow_second.glsl | 11 +- shaders/shader.frag | 83 +++++-- shaders/shader.vert | 10 +- shaders/shadow.vert | 8 +- shaders/shadow_depth.glsl | 6 +- shaders/ssao.frag | 12 +- src/graphics/shadow_map_validity.cpp | 63 +++++ src/render/renderer.cpp | 114 +++++++-- src/render/shadows.cpp | 98 ++++---- tests/graphics/test_shadow_map_validity.cpp | 179 ++++++++++++++ 31 files changed, 1252 insertions(+), 203 deletions(-) create mode 100644 cmake/check_gpu_limits.cmake create mode 100644 cmake/strip_glsl_comments.cmake create mode 100644 include/fire_engine/graphics/shadow_map_validity.hpp create mode 100644 shaders/gpu_limits.glsl create mode 100644 src/graphics/shadow_map_validity.cpp create mode 100644 tests/graphics/test_shadow_map_validity.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 27c2ec12..f9ba9393 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,6 +129,8 @@ assets/ glTF samples + HDR skyboxes - **render/constants.hpp** — single source of truth for scalar render tunables (shadow biases, IBL strengths, shadow/IBL extents, camera FOV, bloom config). GPU data-layout limits (frames-in-flight, joint/morph/light counts, shadow caster caps + matrix layout) live in **graphics/gpu_limits.hpp** so the Vulkan-free graphics layer can size its arrays; constants.hpp includes it, so all constants stay reachable through one include. - **Layering guard** — `graphics/` and `scene/` headers must not include `render/`, and `render/` headers must not include `scene/` (`render`↔`scene` meet only through the Vulkan-free `graphics/renderable_scene.hpp` seam). Enforced by the `layering_guards` CTest case (`cmake/check_layering.cmake`). Graphics `.cpp` files may include `render/resources.hpp` — that's the documented GPU-allocation bridge. - **GPU resource model** — three orthogonal layers; never conflate them. **(1) Identity:** opaque handles (`graphics/gpu_handle.hpp`) into `Resources`' tables — an index packed with a generation so a stale handle to a reused slot is detectably invalid. **(2) Lifetime:** off-the-shelf `vk::raii` owns every Vulkan object whose lifetime is *independent of a device-memory allocation* (image views, samplers, pipelines, descriptor layouts, command pools). `vk::raii` is a lifetime tool, not a memory allocator — it imposes nothing on allocation strategy. **(3) Memory / sub-allocation:** VMA is the arena, used the idiomatic way (`vmaCreateBuffer`/`vmaCreateImage`) so it owns the resource + its sub-allocation and frees them together. Because `vmaDestroy*` couples the two, buffers and images are the *one* exception to layer 2: their `VkBuffer`/`VkImage` lifetime lives inside a small custom move-only VMA RAII wrapper (`UniqueVmaBuffer`/`UniqueVmaImage`) — still RAII, just VMA-backed. This is **not** a migration away from `vk::raii`; revisit layer 2 only if runtime streaming (mid-frame create/destroy needing a deletion queue) ever arrives. +- **One declaration of every shared GPU data-layout limit** — the sizes and indices that a C++ block and a shader block must agree on (caster/light/joint/morph/emitter/kernel counts, the shadow matrix bases, the map-validity bits). Purely GLSL-side algorithm constants are NOT in scope and this mechanism does not own them: a compute workgroup size, a scan radix, a tap count with no C++ counterpart stays where it is used. `shaders/gpu_limits.glsl` is written in the subset that is valid GLSL *and* valid C++; shaders `#include` it and `graphics/gpu_limits.hpp` includes it inside a `shader_limits` namespace, re-exporting each value under its `k`-name. Add a shader-visible limit **there**, never as a literal on either side, and keep the file inside the common subset (no `constexpr`, `inline`, `namespace`, `static_cast`, unsigned suffixes — each breaks the *other* language, in files that never mention this one). The `gpu_limits_guard` CTest case sweeps `shaders/` for a re-declaration, requires each consumer to use the name rather than a literal, and requires each `k`-constant to be defined *as* the shared declaration, so C++ cannot drift back to hard-coded values behind green shader checks. +- **A shadow family's recording and its uploaded validity are one value** — `ShadowMapValidity` (`graphics/shadow_map_validity.hpp`) is derived once per frame in `Renderer::uploadFrameLighting`, from the COMPLETED view set, and used twice: it gates the families in `Shadows::recordPass` and it is uploaded as `LightUBO::shadowMapValidMask` for every sampling path in `shader.frag`. Never skip a family's recording without routing the decision through it — a skipped family's depth image holds an earlier frame's content, and sampling it produces no error, no crash, and shadows from a frame that is gone. - **GPU data-layout discipline** — every CPU struct shared with a shader (UBO/SSBO) lives in `render/ubo.hpp` with `alignas` + `static_assert`s pinning its std140/std430 offsets and size. Preserve this: when you change a shader-visible struct, update both sides and keep the static_asserts — they are the only thing catching a silent host↔GPU layout mismatch. Mapped host-visible writes go through `graphics/mapped_buffer.hpp` `writeMapped` (a bounds-checked `std::span`), never a raw `void*`. **And a block bound by more than one shader is declared ONCE, in a shared `shaders/*.glsl` include** (`light_ubo.glsl` for `LightUBO`, `material.glsl` for the bindless `Materials` SSBO + `MaterialData`, `shadow_push.glsl` for the `ShadowPushConstants` push block), never hand-copied per shader: field offsets depend on every field before them, so a copy missing an inserted field misreads everything after it, with no validation error and no crash. That is how the sky came to be multiplied by a shadow matrix — `selfShadowViewProj` was added to the struct and `shader.frag`, not to `skybox.frag`, and the wrong value read 1.0 until a scene had two skinned self-shadow casters. The `shader_block_guards` CTest case (`cmake/check_shader_blocks.cmake`) fails on a re-declared block *and* on a shared include that stops declaring it. ## Code Style @@ -167,8 +169,9 @@ Reference class: `include/fire_engine/graphics/image.hpp`. Catch2 (v3, `Catch2::Catch2WithMain`). Single binary `test_fire_engine`. CTest registers `test_fire_engine` as the fast `~[slow]` entry, so plain `ctest` and `ctest --preset fast` -stay fast. The `tests-full` target runs the all-tags Catch2 binary plus the graphics-layer -include guard and the shared-shader-block guard; from the source root use `cmake --build --preset full`. Test files mirror +stay fast. The `tests-full` target runs the all-tags Catch2 binary plus the four build-time guards — +graphics-layer includes, shared shader blocks, the shadow bias law, and the shared GPU limits; from +the source root use `cmake --build --preset full`. Test files mirror source paths. Shared helpers/traits live in `tests/support/`. Test assets in `tests/assets/` → copied to `build/test_assets/`. diff --git a/CMakeLists.txt b/CMakeLists.txt index ebad6e03..ee587a6d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -104,6 +104,7 @@ add_library(fireengine SHARED src/graphics/shadow_lod_resolver.cpp src/graphics/shadow_caster_alpha.cpp src/graphics/shadow_bias.cpp + src/graphics/shadow_map_validity.cpp src/graphics/shadow_caster_deformation.cpp src/graphics/shadow_render_view.cpp src/graphics/shadow_diagnostics.cpp @@ -178,7 +179,14 @@ add_library(fireengine SHARED src/scene/transform.cpp ) -target_include_directories(fireengine PUBLIC ${PROJECT_SOURCE_DIR}/include ${Stb_INCLUDE_DIR}) +# `shaders/` is on the C++ include path because a PUBLIC header needs it: graphics/gpu_limits.hpp +# includes shaders/gpu_limits.glsl, the one file where the limits shared with GLSL are written. +# BUILD_INTERFACE-only — nothing is installed, and a consumer of an installed tree would have no +# shaders/ to point at. +target_include_directories(fireengine PUBLIC + ${PROJECT_SOURCE_DIR}/include + $ + ${Stb_INCLUDE_DIR}) target_compile_features(fireengine PUBLIC cxx_std_23) target_compile_options(fireengine PRIVATE -Wall -Wextra -Wpedantic) if(FIRE_ENGINE_WARNINGS_AS_ERRORS) @@ -291,6 +299,9 @@ endif() # any of them rebuilds every shader — includes are rare + shared, so this over-approximation is cheap # and keeps the SPV outputs correct. set(SHADER_INCLUDES + # Read by BOTH languages: the shaders include it directly, and the public C++ header + # graphics/gpu_limits.hpp includes it to re-export the same values under the engine's k-names. + ${PROJECT_SOURCE_DIR}/shaders/gpu_limits.glsl ${PROJECT_SOURCE_DIR}/shaders/vdpm_repair_classify.glsl ${PROJECT_SOURCE_DIR}/shaders/vdpm_apply_classify.glsl ${PROJECT_SOURCE_DIR}/shaders/light_ubo.glsl @@ -435,6 +446,7 @@ add_executable(test_fire_engine tests/graphics/test_shadow_caster_deformation.cpp tests/graphics/test_shadow_caster_alpha.cpp tests/graphics/test_shadow_bias.cpp + tests/graphics/test_shadow_map_validity.cpp tests/graphics/test_shadow_render_view.cpp tests/graphics/test_texture.cpp tests/graphics/test_sampler_settings.cpp @@ -509,6 +521,7 @@ add_custom_target(tests-full COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R layering_guards 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 DEPENDS test_fire_engine COMMENT "Running the full test suite, including [slow] tests" ) @@ -543,6 +556,17 @@ add_test( -P ${PROJECT_SOURCE_DIR}/cmake/check_shadow_bias.cmake ) +# The limits shared by C++ and GLSL are declared once and read by both sides. A one-sided edit +# compiles cleanly and then indexes the wrong region of a bound buffer — in range, so no validation +# error — which is why this is a build-time check rather than a runtime one. +add_test( + NAME gpu_limits_guard + COMMAND ${CMAKE_COMMAND} + -DSHADER_DIR=${PROJECT_SOURCE_DIR}/shaders + -DINCLUDE_DIR=${PROJECT_SOURCE_DIR}/include + -P ${PROJECT_SOURCE_DIR}/cmake/check_gpu_limits.cmake +) + find_program(CLANG_TIDY_EXE NAMES clang-tidy) if(CLANG_TIDY_EXE) get_target_property(FIRE_ENGINE_TIDY_SOURCES fireengine SOURCES) diff --git a/README.md b/README.md index 475697b4..6bfe0245 100644 --- a/README.md +++ b/README.md @@ -189,7 +189,7 @@ The transient pipelines are destroyed once the bake completes; only the resultin 4. `PhysicsWorld::step(1.0f / 60.0f)` runs zero or more fixed substeps from the frame accumulator. 5. `scene_.applyPhysics(physics_, alpha)` pulls dynamic and corrected kinematic transforms back into scene nodes — interpolated by `alpha = accumulator / fixedDt` between the last two simulated poses for smooth motion above 60 Hz — and resolves composed-world matrices. 6. `Renderer::drawFrame()` acquires a swapchain image and records the frame passes: - - **Shadow passes** — directional cascades render both the full CSM and a world-only CSM that excludes skinned casters. Each skinned self-shadow slot renders two tightly-fit passes: the first captures the nearest light-facing surface, and the second samples that first depth and discards it so the forward shader can sample the next useful self-occluder. Spot and point shadow passes replay the same compatible shadow draw commands through their per-layer/per-face depth attachment views. Skin and morph still apply in the shadow vertex shader + - **Shadow passes** — directional cascades render both the full CSM and a world-only CSM that excludes skinned casters. Each skinned self-shadow slot renders two tightly-fit passes: the first captures the nearest light-facing surface, and the second samples that first depth and discards it so the forward shader can sample the next useful self-occluder. Spot and point shadow passes replay the same compatible shadow draw commands through their per-layer/per-face depth attachment views. Skin and morph still apply in the shadow vertex shader. **Which families record is one decision per frame** (`ShadowMapValidity`): the slot-addressed families disappear when nothing assigns them — no self-shadow caster, no spot or point caster, no skinned draw for the world-only CSM — while the main cascades still record (and clear) whenever there is a sun to fit them to, casters or not. The directional families additionally require a primary directional light, and `--no-shadows` clears everything. A skipped family costs nothing at all — no draws, no clears, no timestamps — and the *same* value is uploaded as a bit mask the forward shader reads, so its sampling path answers fully lit instead of reading depth left over from an earlier frame - **Forward pass** — begin the HDR offscreen pass, draw the skybox (LEQUAL depth, no write), then call `scene.buildDrawCommands(frameInfo, frustums, out)` through the Vulkan-free `RenderableScene` interface (the scene culls internally and emits draws); Mesh/Object emit `DrawCommand`s that the Renderer buckets into opaque, transmissive, and blend lists, sorts the blend bucket back-to-front by `sortDepth`, and replays through the same bind/draw loop resolving handles via `Resources` - **Transmission pass** — when transmissive draws are present, capture the opaque scene colour mip chain and replay transmissive draws so the shader can sample scene-behind-glass data - **TAA resolve** — the forward/transmission passes also write a screen-space velocity (motion-vector) attachment; the resolve reprojects the previous frame's accumulated history along that buffer, neighbourhood-clamps it against the current 3×3 to kill ghosting, blends, and blits the result back into the HDR target. Sub-pixel projection jitter (Halton(2,3)) drives the accumulation; particles render afterwards with the un-jittered projection so they stay out of history. Skipped under `--no-taa` @@ -396,6 +396,11 @@ FE_LOG=render:debug ./fireEngineApp Current categories are `app`, `general`, `gltf`, `physics`, `ragdoll`, and `render`. +`render:debug` also prints a periodic **shadow recording** line — per family, whether it was recorded +or skipped, its raster passes and its GPU milliseconds — which is how `--no-shadows` is checked: it +suppresses the *recording*, not only the sampling, so every family must read `skipped passes=0 +0.000ms`. A frame that still rendered into maps nobody samples would look identical on screen. + ## Dependencies Managed via the vcpkg manifest (`vcpkg.json`): diff --git a/cmake/check_gpu_limits.cmake b/cmake/check_gpu_limits.cmake new file mode 100644 index 00000000..8d5126e2 --- /dev/null +++ b/cmake/check_gpu_limits.cmake @@ -0,0 +1,221 @@ +# 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. +# +# 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 +# file, or re-exports a name as anything other than the shared declaration — otherwise C++ could +# quietly return to hard-coded values while every shader-side check stayed green, which is the same +# drift with the sides swapped. +# +# COMMENTS ARE STRIPPED FIRST, both forms, in both languages: a check that a file "uses" a name is +# otherwise satisfied by the name appearing in prose, and this file is full of prose about it. +# +# Invoked as a CTest case; needs SHADER_DIR and INCLUDE_DIR. + +if(NOT DEFINED SHADER_DIR) + message(FATAL_ERROR "SHADER_DIR must be set (path to shaders/)") +endif() +if(NOT DEFINED INCLUDE_DIR) + message(FATAL_ERROR "INCLUDE_DIR must be set (path to include/)") +endif() + +include("${CMAKE_CURRENT_LIST_DIR}/strip_glsl_comments.cmake") + +set(offenders "") +set(shared "${SHADER_DIR}/gpu_limits.glsl") +set(cpp_authority "${INCLUDE_DIR}/fire_engine/graphics/gpu_limits.hpp") + +# The shared names, paired with the C++ constant each must be re-exported as. Adding a limit that +# both languages need means adding it here too — deliberately, which is the point. +set(shared_names + MAX_LIGHTS + MAX_JOINTS + MAX_MORPH_TARGETS + MORPH_WEIGHT_VEC4_COUNT + MAX_PARTICLE_EMITTERS + SSAO_KERNEL_SIZE + SHADOW_CASCADE_COUNT + MAX_SKINNED_SELF_SHADOW_CASTERS + 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 + SHADOW_MAP_VALID_SPOT + SHADOW_MAP_VALID_POINT) +set(cpp_names + kMaxLights + kMaxJoints + kMaxMorphTargets + kMorphWeightVec4Count + kMaxParticleEmitters + kSsaoKernelSize + kShadowCascadeCount + kMaxSkinnedSelfShadowCasters + kMaxSpotShadowCasters + kMaxPointShadowCasters + kCubeFaceCount + kShadowCascadeMatrixBase + kShadowSpotMatrixBase + kShadowPointMatrixBase + kShadowTotalMatrixCount + kShadowMapValidCascades + kShadowMapValidWorldOnly + kShadowMapValidSelf + kShadowMapValidSpot + kShadowMapValidPoint) + +# Each consumer as `file:NAME:uses` — the shared name it must be READING, and HOW MANY TIMES. A file +# that keeps the #include but goes back to a literal at the point of use is the failure this pins; +# the include alone proves nothing, and neither does a single mention when the file uses the value in +# several places. (Mutation-testing found exactly that gap: reverting `ssao.frag`'s loop bound to 16 +# passed a presence check, because the UBO array above it still named the constant.) +# +# A MINIMUM, unlike the bias guard's exact call count. There the count IS the contract — five +# receiver paths, no more — whereas here a new legitimate use of a size is unremarkable and losing +# one is the regression. Raise a number when a file gains a use; never lower one to make a red check +# pass, since that is the drift arriving. +set(consumers + "shader.vert:MAX_JOINTS:2" # SkinUBO + PrevSkinUBO + "shader.vert:MORPH_WEIGHT_VEC4_COUNT:1" + "shadow.vert:MAX_JOINTS:1" + "shadow.vert:MORPH_WEIGHT_VEC4_COUNT:1" + "particle.vert:MAX_PARTICLE_EMITTERS:1" + "particle_simulate.comp:MAX_PARTICLE_EMITTERS:1" + "ssao.frag:SSAO_KERNEL_SIZE:3" # kernel[] + the loop bound + the occlusion divisor + "light_ubo.glsl:SHADOW_CASCADE_COUNT:2" # cascadeViewProj[] + cascadeBiasMetrics[] + "light_ubo.glsl:MAX_LIGHTS:1" + "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" + # EVERY family's validity bit is consulted by the receiver. A family the renderer skips has a + # stale depth image, so a sampling path that stops asking reads last-frame's shadows with + # nothing to report it — no error, no crash, just shadows from a frame that is gone. The cascade + # bit is asked twice: by the sampler and by the raw-depth debug view, which reads lights[0] as + # the sun and would have no valid one. + "shader.frag:SHADOW_MAP_VALID_CASCADES:2" + "shader.frag:SHADOW_MAP_VALID_WORLD_ONLY:1" + "shader.frag:SHADOW_MAP_VALID_SELF:1" + "shader.frag:SHADOW_MAP_VALID_SPOT:1" + "shader.frag:SHADOW_MAP_VALID_POINT:1") + +if(NOT EXISTS "${shared}") + list(APPEND offenders "shaders/gpu_limits.glsl is missing — it is the single declaration both languages read") +else() + file(READ "${shared}" shared_text) + strip_glsl_comments("${shared_text}" shared_code) + # Vacuity check first: if the shared file stopped declaring a name, the "nobody else declares it" + # sweep below would pass while the value lived somewhere else entirely. + foreach(name IN LISTS shared_names) + if(NOT shared_code MATCHES "const[ \t]+int[ \t]+${name}[ \t]*=") + list(APPEND offenders "gpu_limits.glsl no longer declares ${name} — the checks for it would pass vacuously") + endif() + endforeach() + # The file has to stay in the GLSL/C++ common subset, and the C++-only keywords are what a C++ + # author reaches for first. A `constexpr` here breaks every shader compile in files that never + # mention this one, so name the cause here instead. + foreach(keyword constexpr inline namespace static_cast unsigned) + if(shared_code MATCHES "(^|[^A-Za-z_])${keyword}[^A-Za-z_]") + list(APPEND offenders "gpu_limits.glsl uses `${keyword}` — it must stay in the subset that is valid GLSL *and* valid C++") + endif() + endforeach() +endif() + +# No shader may declare a shared name itself. The whole shaders/ tree is swept, not a list of known +# consumers: a NEW shader with its own `const int SHADOW_CASCADE_COUNT = 4;` is exactly the drift +# this exists to stop, and it would not be on any list. +file(GLOB shader_sources + "${SHADER_DIR}/*.glsl" "${SHADER_DIR}/*.vert" "${SHADER_DIR}/*.frag" "${SHADER_DIR}/*.comp") +foreach(shader IN LISTS shader_sources) + if(shader STREQUAL "${shared}") + continue() + endif() + file(READ "${shader}" shader_text) + strip_glsl_comments("${shader_text}" shader_code) + get_filename_component(shader_name "${shader}" NAME) + foreach(name IN LISTS shared_names) + if(shader_code MATCHES "const[ \t]+int[ \t]+${name}[ \t]*=") + list(APPEND offenders + "${shader_name} declares ${name} itself — include \"gpu_limits.glsl\" instead; a second declaration is how the two sides drift") + endif() + endforeach() +endforeach() + +foreach(consumer IN LISTS consumers) + string(REPLACE ":" ";" parts "${consumer}") + list(GET parts 0 consumer_file) + list(GET parts 1 consumer_name) + list(GET parts 2 expected_uses) + set(path "${SHADER_DIR}/${consumer_file}") + if(NOT EXISTS "${path}") + list(APPEND offenders "${consumer_file} is missing from ${SHADER_DIR}") + continue() + endif() + file(READ "${path}" consumer_text) + strip_glsl_comments("${consumer_text}" consumer_code) + if(NOT consumer_code MATCHES "#include[ \t]+\"gpu_limits.glsl\"") + list(APPEND offenders "${consumer_file} does not #include \"gpu_limits.glsl\"") + endif() + # The include line itself is code, not a use, and it does not contain the name — so every match + # below is a real read of the value. + # + # SEMICOLONS ARE NEUTRALISED FIRST, and that is not cosmetic: `MATCHALL` returns a CMake LIST, and + # a match that spans a `;` (`for (i < NAME; ++i)`) becomes TWO elements, so `list(LENGTH)` counted + # one use as two and a lost use could hide behind the inflation. A `;` only ever terminates a GLSL + # statement, so replacing it with a space cannot merge two identifiers. + string(REPLACE ";" " " consumer_scan "${consumer_code}") + string(REGEX MATCHALL "[^A-Za-z_0-9]${consumer_name}[^A-Za-z_0-9]" uses "${consumer_scan}") + list(LENGTH uses use_count) + if(use_count LESS expected_uses) + list(APPEND offenders + "${consumer_file} uses ${consumer_name} ${use_count} time(s); expected at least ${expected_uses}. A literal in its place is the drift this guard exists for") + endif() +endforeach() + +# The C++ half. +if(NOT EXISTS "${cpp_authority}") + list(APPEND offenders "include/fire_engine/graphics/gpu_limits.hpp is missing — it is the C++ side of the shared limits") +else() + file(READ "${cpp_authority}" cpp_text) + strip_glsl_comments("${cpp_text}" cpp_code) + if(NOT cpp_code MATCHES "#include[ \t]+\"gpu_limits.glsl\"") + list(APPEND offenders + "gpu_limits.hpp does not #include \"gpu_limits.glsl\" — the C++ side must READ the shared declarations, not restate them") + endif() + list(LENGTH shared_names shared_count) + math(EXPR last_index "${shared_count} - 1") + foreach(index RANGE ${last_index}) + list(GET shared_names ${index} shared_name) + list(GET cpp_names ${index} cpp_name) + # The INITIALISER, not merely a mention: `kMaxLights = 8;` beside an unused include is the exact + # regression this half catches. + if(NOT cpp_code MATCHES "${cpp_name}[ \t]*=[ \t]*shader_limits::${shared_name}[ \t]*;") + list(APPEND offenders + "gpu_limits.hpp does not define ${cpp_name} as shader_limits::${shared_name} — a hard-coded value here drifts from the shaders with nothing to catch it") + endif() + endforeach() +endif() + +if(offenders) + string(REPLACE ";" "\n " report "${offenders}") + message(FATAL_ERROR "gpu limits guard failed:\n ${report}") +endif() + +message(STATUS "gpu limits guard: one declaration in shaders/gpu_limits.glsl, read by every shader consumer and re-exported by gpu_limits.hpp") diff --git a/cmake/check_shadow_bias.cmake b/cmake/check_shadow_bias.cmake index c4592fd7..69211032 100644 --- a/cmake/check_shadow_bias.cmake +++ b/cmake/check_shadow_bias.cmake @@ -24,16 +24,8 @@ if(NOT DEFINED SHADER_DIR) message(FATAL_ERROR "SHADER_DIR must be set (path to shaders/)") endif() -# Both GLSL comment forms, block first so a `//` inside a block cannot survive it. The block pattern -# is the classic non-greedy-free form — CMake's regex has no lazy quantifier, and `/\*.*\*/` would -# swallow everything between the FIRST and LAST comment in the file, silently blanking the shader and -# making every check "fail" for the wrong reason. The line pattern anchors on start-of-string as well -# as newline, so a comment at byte zero is stripped too. -function(strip_glsl_comments in_text out_var) - string(REGEX REPLACE "/\\*[^*]*\\*+([^/*][^*]*\\*+)*/" "" stripped "${in_text}") - string(REGEX REPLACE "//[^\n]*" "" stripped "${stripped}") - set(${out_var} "${stripped}" PARENT_SCOPE) -endfunction() +# Comment stripping is shared with the other shader guards — see the file for why both forms matter. +include("${CMAKE_CURRENT_LIST_DIR}/strip_glsl_comments.cmake") set(offenders "") set(law "${SHADER_DIR}/shadow_bias.glsl") diff --git a/cmake/strip_glsl_comments.cmake b/cmake/strip_glsl_comments.cmake new file mode 100644 index 00000000..147d6dca --- /dev/null +++ b/cmake/strip_glsl_comments.cmake @@ -0,0 +1,23 @@ +# `strip_glsl_comments(in_text out_var)` — remove both comment forms before a textual guard reads a +# file. Shared by the shader guards rather than copied into each, because the block-comment pattern +# below is easy to get subtly wrong and a wrong version fails open. +# +# Every shader guard asks whether the CODE does something, and an unstripped match is satisfied by +# commented-out text — which is exactly how a call gets disabled while appearing to survive. Line +# comments alone are not enough: wrapping a whole path in /* ... */ keeps its calls visible to a +# naive match, and can hide a shared file's own declarations so every later check passes vacuously. +# Both gaps were found by mutation-testing a guard, which is the only way to know a textual check +# bites. +# +# The comment syntax is identical in GLSL and C++, so this serves the C++-side checks too. + +# Block first, so a `//` inside a block cannot survive it. The block pattern is the classic +# non-greedy-free form — CMake's regex has no lazy quantifier, and `/\*.*\*/` would swallow +# everything between the FIRST and LAST comment in the file, silently blanking the input and making +# every check "fail" for the wrong reason. The line pattern anchors on start-of-string as well as +# newline, so a comment at byte zero is stripped too. +function(strip_glsl_comments in_text out_var) + string(REGEX REPLACE "/\\*[^*]*\\*+([^/*][^*]*\\*+)*/" "" stripped "${in_text}") + string(REGEX REPLACE "//[^\n]*" "" stripped "${stripped}") + set(${out_var} "${stripped}" PARENT_SCOPE) +endfunction() diff --git a/docs/acceptance-testing.md b/docs/acceptance-testing.md index 1719c5ec..7feb843a 100644 --- a/docs/acceptance-testing.md +++ b/docs/acceptance-testing.md @@ -625,6 +625,26 @@ them. Flags are position-independent. Images are `placeholder.jpg` for now. ## Feature checks (overlay-driven) +### Shadow-map recording — `--no-shadows` must suppress the PASS, not just the lookups +`--no-shadows` clears every bit of the frame's `ShadowMapValidity`, which skips the recording of +every family as well as the sampling. That cannot be judged from the image — a frame that still +renders into maps nobody samples looks identical, and stays validation-clean — so read the counters: + +```bash +FE_LOG=render:debug ./fireEngineApp shadow_lod/ShadowLodDemo.gltf skybox.hdr +FE_LOG=render:debug ./fireEngineApp shadow_lod/ShadowLodDemo.gltf skybox.hdr --no-shadows +``` + +Each prints a periodic `shadow recording:` line, one entry per family, on the cascade-fit cadence. +Without the flag every family that has casters reads `recorded passes=N `; with it **every** +family must read `skipped passes=0 0.000ms`, and the line must end `| no family recorded`. The same +line is how a partially-fitted family is spotted: a `skipped` entry in a scene that should have +casters means the whole-family rule rejected it, not that the geometry vanished. + +A scene with no skinned draw legitimately shows `world-only skipped` and `self skipped` +(`DamagedHelmet`); a skinned one shows both recorded (`BrainStem`). Spot and point follow their own +casters — `LightsPunctualLamp` records 24 point passes (four cubes) and no spot. + ### Mesh LOD — discrete / VIPM / VDPM ```bash ./fireEngineApp DamagedHelmet/DamagedHelmet.gltf skybox.hdr --overlay diff --git a/docs/codereview.md b/docs/codereview.md index 431c63b4..355c353d 100644 --- a/docs/codereview.md +++ b/docs/codereview.md @@ -347,17 +347,21 @@ Recommended change: VDPM front/mesh lookup already validates generation before dereferencing its table; use that as the minimum owner-side contract. -#### 2. High: GPU layout limits have no machine-enforced C++/GLSL authority - -[`gpu_limits.hpp`](include/fire_engine/graphics/gpu_limits.hpp) calls itself the shared source of -truth, but shader-visible values are independently repeated as literals: - -- `kMaxJoints = 64` versus `mat4 joints[64]` in two vertex shaders; -- `kMaxMorphTargets = 8` versus `vec4 weights[2]`; -- `kMaxLights = 8` versus `MAX_LIGHTS = 8`; -- shadow cascade/spot/self-shadow counts and the total matrix count; -- `kMaxParticleEmitters = 4` versus `emitters[4]` in two particle shaders; -- `kSsaoKernelSize = 16` versus `KERNEL_SIZE = 16`. +#### 2. ~~High: GPU layout limits have no machine-enforced C++/GLSL authority~~ ✅ **landed** (`shadow-hygiene`) + +[`gpu_limits.hpp`](include/fire_engine/graphics/gpu_limits.hpp) called itself the shared source of +truth while every shader-visible value was independently repeated as a literal. All six are now read +from one declaration: + +- ~~`kMaxJoints = 64` versus `mat4 joints[64]` in two vertex shaders~~ ✅; +- ~~`kMaxMorphTargets = 8` versus `vec4 weights[2]`~~ ✅ (the GLSL length is DERIVED — + `MORPH_WEIGHT_VEC4_COUNT = MAX_MORPH_TARGETS / 4` — with the divisibility asserted C++-side, since + the block packs weights as vec4s and a ninth weight would need a third vec4 the shader never + declared); +- ~~`kMaxLights = 8` versus `MAX_LIGHTS = 8`~~ ✅; +- ~~shadow cascade/spot/self-shadow counts and the total matrix count~~ ✅; +- ~~`kMaxParticleEmitters = 4` versus `emitters[4]` in two particle shaders~~ ✅; +- ~~`kSsaoKernelSize = 16` versus `KERNEL_SIZE = 16`~~ ✅. The C++ layout assertions prove only the CPU layout. A one-sided constant change can still compile both languages successfully while making array bounds, UBO sizes, or shader loops disagree. @@ -370,6 +374,24 @@ making drift impossible. Keep device-dependent capacity validation in `Device`, but generate the compile-time ABI values. This is a correctness boundary, not a documentation convention. +**How** (`shadow-hygiene`, with arc 1's hygiene item — see [`shadowplans.md`](shadowplans.md) +§ Independent shadow hygiene): `shaders/gpu_limits.glsl` holds the values once, written in the +subset that is valid GLSL *and* valid C++, and `graphics/gpu_limits.hpp` includes it and re-exports +them under the engine's `k`-names. That is stronger than the "parser test comparing literals" this +finding warned against and cheaper than generation: there is nothing to compare, because there is +one declaration and both languages read it. + +`gpu_limits_guard` enforces both directions — no shader may declare a shared name itself (the whole +`shaders/` tree is swept, so a new shader is covered), each consumer must include the file and use +the name **as many times as it is used today**, and the C++ header must define each `k`-constant *as* +the shared declaration, so C++ cannot drift back to hard-coded values behind green shader checks. +The use COUNT rather than mere presence came out of mutation-testing: reverting `ssao.frag`'s loop +bound to `16` passed a presence check, because the UBO array above it still named the constant. + +**Scope, stated honestly**: this owns shared GPU DATA-LAYOUT values — the sizes and indices a C++ +block and a shader block must agree on. GLSL-only algorithm constants with no C++ counterpart (VDPM +scan/workgroup sizes, for instance) are not in scope and are not claimed by the mechanism. + #### 3. High: `ColliderId` registration state is inconsistent between broadphases The value type in [`collider_id.hpp`](include/fire_engine/collision/collider_id.hpp) is reasonable, diff --git a/docs/onboarding.md b/docs/onboarding.md index 0183ef69..65e8ee93 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -649,7 +649,11 @@ and resolve pass: requested only when capture is asked for and rejected loudly if the surface lacks it, and supports 8-bit BGRA/RGBA swapchain formats only (anything else throws rather than guessing the channel order). The captured extent is the SWAPCHAIN's — 2× the 800×600 window on a HiDPI display. -- `--no-shadows`: disable shadow-map visibility lookups. +- `--no-shadows`: record no shadow maps and sample none. It clears every bit of the frame's + `ShadowMapValidity`, which both skips the pass's families (no draws, no clears, no timestamps) and + tells the forward shader to answer fully lit — `FE_LOG=render:debug` prints the per-family + recording line that shows it. Re-enabling mid-run is safe: the pass records before anything + samples a map, so the frame that turns shadows back on renders them first. - `--no-taa`: skip the projection jitter and TAA resolve — reverts to the raw aliased image. - `--overlay`: start with the ImGui debug overlay visible (also toggled at runtime with **F1**). - `-f`: add a receiver-only floor plane at y=0. @@ -912,6 +916,26 @@ the same change — most have a test or guard that will catch you, but not all. shader must match the corresponding `ForwardBinding` / `ForwardGlobalBinding` / `ShadowBinding` / `SkyboxBinding` / `PostProcessBinding` enumerator. `tests/render/test_pipeline_config.cpp` checks the C++ side; the GLSL side is on you. +- **A shared GPU data-layout limit is declared ONCE, in `shaders/gpu_limits.glsl`.** "Shared + data-layout" is the scope: a size or index that a C++ block and a shader block must agree on. A + GLSL-only algorithm constant with no C++ counterpart — a compute workgroup size, a scan radix — + is not in scope and stays where it is used. That file is + written in the subset that is valid GLSL *and* valid C++; shaders `#include` it, and + `graphics/gpu_limits.hpp` includes it inside a `shader_limits` namespace and re-exports each value + under its `k`-name. Adding a shader-visible limit means adding it there and re-exporting it here — + never writing the number twice, and never a `constexpr` in the shared file (it must stay in the + common subset, or shaders that never mention it stop compiling). `gpu_limits_guard` enforces both + directions: no shader may declare a shared name, each consumer must use the name rather than a + literal, and each `k`-constant must be defined *as* the shared declaration. +- **A shadow family's recording and its uploaded validity are one value** (`ShadowMapValidity`). + `Renderer::uploadFrameLighting` derives it once, from the COMPLETED view set — every producer has + run, including the world-only enablement that only `anySkinned` decides — then gates + `Shadows::recordPass` with it and uploads its packed form as `LightUBO::shadowMapValidMask`. If + you add a shadow family, or a new place that decides whether a family renders, route it through + that value: a family skipped without the bit leaves the receiver sampling a depth image this frame + never wrote, and nothing in the pipeline will complain. Every sampling path in `shader.frag` asks + its bit first (the guard pins that too), including the raw-depth debug view, which has no valid + `lights[0]` to read when the cascade family is invalid. - **A shadow caster that deforms after the simplifier measured it may not select a level** (SH-04). The deviation channel is measured on the mesh as authored — bind pose, base weights, the vertex buffer at build time — so for skinned, morph-capable or storage-vertex geometry it describes a mesh diff --git a/docs/review-order.md b/docs/review-order.md index dc5e503b..1588f35a 100644 --- a/docs/review-order.md +++ b/docs/review-order.md @@ -17,7 +17,7 @@ Read these first when a change touches build configuration, CI, or local tooling | File | Pay attention to | |---|---| | `CMakePresets.json` | The `vcpkg` preset pins Apple Clang on macOS, selects `Dev`, and exports `compile_commands.json` for `clangd`. | -| `CMakeLists.txt` | `Dev` is the default build type (`-O2 -g`, no `NDEBUG`); `FIRE_ENGINE_WARNINGS_AS_ERRORS` is CI-only by default; CTest registers `test_fire_engine` (`~[slow]`) so plain CTest stays fast; `tests-full` runs the all-tags Catch2 binary plus the layering and shared-shader-block guards; `run-clang-tidy` appears only when `clang-tidy` is installed. **Read the include-order block near the top before touching dependency includes**: `CMAKE_NO_SYSTEM_FROM_IMPORTED` + `include_directories(BEFORE …)` force vcpkg's headers to arrive as `-I`, because clang searches `/usr/local/include` ahead of every `-isystem` path — with a Vulkan SDK installed there, our TUs compiled against ITS vulkan-hpp while includes resolved relative to a vcpkg header got vcpkg's, and the first RAII call aborted on a header-version assert with no bad C++ behind it (real: SDK 1.4.357 beside the pinned 1.4.335). The guard checks both `VCPKG_INSTALLED_DIR` spellings and hard-errors if the directory is missing, since a silent miss restores the mixed-header build. `SHADER_INCLUDES` lists the shared GLSL includes (`light_ubo.glsl`, `material.glsl`, `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl`) so editing one rebuilds every shader — a stale `.spv` against a changed block is the layout bug the include exists to prevent. Consequence: third-party headers are no longer warning-suppressed, so a finding gets a narrow `#pragma` at the include site (`src/graphics/frame_capture.cpp`), never a weakened flag. | +| `CMakeLists.txt` | `Dev` is the default build type (`-O2 -g`, no `NDEBUG`); `FIRE_ENGINE_WARNINGS_AS_ERRORS` is CI-only by default; CTest registers `test_fire_engine` (`~[slow]`) so plain CTest stays fast; `tests-full` runs the all-tags Catch2 binary plus the layering, shared-shader-block, shadow-bias and GPU-limits guards; `shaders/` is on the C++ include path (`BUILD_INTERFACE`, PUBLIC) because the public header `graphics/gpu_limits.hpp` includes `shaders/gpu_limits.glsl` — the dual-language limits file; `run-clang-tidy` appears only when `clang-tidy` is installed. **Read the include-order block near the top before touching dependency includes**: `CMAKE_NO_SYSTEM_FROM_IMPORTED` + `include_directories(BEFORE …)` force vcpkg's headers to arrive as `-I`, because clang searches `/usr/local/include` ahead of every `-isystem` path — with a Vulkan SDK installed there, our TUs compiled against ITS vulkan-hpp while includes resolved relative to a vcpkg header got vcpkg's, and the first RAII call aborted on a header-version assert with no bad C++ behind it (real: SDK 1.4.357 beside the pinned 1.4.335). The guard checks both `VCPKG_INSTALLED_DIR` spellings and hard-errors if the directory is missing, since a silent miss restores the mixed-header build. `SHADER_INCLUDES` lists the shared GLSL includes (`gpu_limits.glsl`, `light_ubo.glsl`, `material.glsl`, `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl`) so editing one rebuilds every shader — a stale `.spv` against a changed block is the layout bug the include exists to prevent. Consequence: third-party headers are no longer warning-suppressed, so a finding gets a narrow `#pragma` at the include site (`src/graphics/frame_capture.cpp`), never a weakened flag. | | `.clang-format` / `.clang-tidy` | Formatting is CI-gated. Tidy is the first-pass static-analysis config for engine `src/` plus `include/fire_engine/`; disabled checks are documented inline. | | `.github/workflows/ci.yml` | Builds with vcpkg + warnings-as-errors, runs `run-clang-tidy`, runs the full `tests-full` target, and has a separate clang-format dry-run job. | | `tools/assetgen/geometry.py` | Primitive builders, all returning the same `(positions, normals, indices)` triple: box (24 verts, per-face normals), tetrahedron, UV sphere, flat-shaded mesh-from-triangles, and `combine_geometry` for compounds. Plus the shared vector helpers. Everything is authored at true size with node scale left at 1. | @@ -33,6 +33,7 @@ Read these first when a change touches build configuration, CI, or local tooling | `graphics/frame_capture.hpp` + `frame_capture.cpp` | The Vulkan-free half of `--capture`: swapchain readback → tightly-packed RGBA8, plus the PNG write (stb_image_write). Two things it exists to get right, both silent when wrong: the BGRA/RGBA channel order, and the row PITCH (a linear image may pad rows — assuming `width * 4` shears the picture). An undersized mapping returns empty by contract, so the caller reports a failed capture rather than encoding whatever followed the buffer. Covered by `tests/graphics/test_frame_capture.cpp`. | | `tools/assetgen/png.py` | Byte-deterministic RGBA PNG writer. Note WHY it exists: `zlib.compress` output varies with the zlib build, so a committed asset embedding a compressed texture could differ between machines. Image data goes into DEFLATE **stored** blocks (RFC 1951, fully specified) with a hand-written Adler-32. Don't "optimise" it into real compression. | | `graphics/shadow_bias.hpp` + `.cpp` | **SH-07's bias law — the EXECUTABLE SPECIFICATION** for what `shaders/shadow_bias.glsl` runs per fragment. Pure and headless-tested because the arithmetic is where the mistakes live: the law takes `worldUnitsPerTexel`, `normalizedDepthPerWorldUnit`, `nDotL` and `filterRadiusTexels`, works in WORLD units and converts once at the end, which is what keeps it projection-independent and stops a scale being applied twice (the defect: `exp2(cascade)` stood in for a texel footprint AND a depth-range conversion simultaneously). The per-projection metrics beneath it are where the geometry differs — note the spot conversion carries the RAY-FORWARD COSINE (the slope term is measured along the light ray, the stored depth is projected onto the cone axis; omitting it over-converts ~41% at 45° off-axis) and the point footprint follows the MAJOR AXIS while its comparison depth stays radial. Degenerate metrics return 0, never infinity: 0 means acne, which is visible and locatable; infinity detaches every shadow in the frame. | +| `graphics/shadow_map_validity.hpp` + `.cpp` | **Which map families a frame RECORDS — one decision, used twice.** Pure and Vulkan-free: 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 — world-only is enabled last), gates `Shadows::recordPass` with it, and uploads `packedMask()` as `LightUBO::shadowMapValidMask`; the receiver answers fully lit for a clear bit. The two halves are the same value on purpose — skipping a family without telling the shader leaves it sampling depth this frame never wrote, which no validation layer or crash will report. Read the WHOLE-FAMILY rule carefully: cascades and world-only need EVERY cascade (a fragment picks its layer by depth, so a missing layer is a hole, not three-quarters of a map) plus a real directional light (they are fitted to a fallback direction otherwise, describing a sun that is not in the scene); point needs whole cubes, leaning on `setPointLight`'s atomicity; self and spot are per-slot and any active slot validates them. | | `graphics/shadow_caster_alpha.hpp` + `.cpp` | **SH-05's classifier — the one place that decides whether a caster's shadow is its triangles or its cutout.** Returns `Masked` for `AlphaMode::Mask` alone: keyed on the DECLARED mode, not on whether a base-colour texture happens to be bound (a textureless MASK material still tests its base-colour factor's alpha, and a shadow that ignored it would occlude where the surface draws nothing), and BLEND maps to `Opaque` deliberately, leaving blend-shadow semantics an open decision. Same shape and same reasons as the SH-04 classifier below; both consumers (the resolver's LOD pin, the shadow pass' fragment path) read ONE derivation made in `Object::buildDrawCommands`. | | `graphics/shadow_caster_deformation.hpp` + `.cpp` | **SH-04's classifier — the one place that decides whether a caster's error claim is about the mesh that gets rasterised.** Returns `Deformable` for three carriers: a skinned/morph-weighted INSTANCE, morph-CAPABLE geometry (deliberately independent of current weights — classifying by weights would swap a caster's error model mid-animation), and storage-vertex geometry whose vertices a compute pass rewrites (cloth). A free function, not an `Object` member, so it is testable against a real `Geometry` without a GPU — classification and the resolver's response to it are pinned by separate tests, so neither is proven only by the other. | | `tools/shadow_lod_sweep.sh` | SH-03's calibration procedure as a script, so the budget and dead band can be re-derived rather than trusted. Read the header first: it names the three references that were rejected and why (`--no-lod` changes the visible geometry; a tiny budget still runs selection; whole-image PSNR dilutes a localised silhouette error). The metric compares the `--debug-shadow` visibility image and reports differing pixels against the reference's SHADOWED area, plus the worst pixel and an amplified difference image so "edge slivers" is something you look at. The dead-band half aggregates the per-frame `FE_LOG=render:debug` movement record over a whole animated run, and REVERSALS — not transitions — are the column that can justify a ratio. | @@ -185,7 +186,7 @@ Read these first when a change touches build configuration, CI, or local tooling | `graphics/renderable_scene.hpp` | The Vulkan-free render↔scene seam (CR-09): the `RenderableScene` interface (`activeCamera`/`gatherLights`/`gatherEmitters`/`buildDrawCommands`) that `SceneGraph` implements and `Renderer` pulls through. The **active camera crosses here too** — `activeCamera()` returns a `CameraView{position,target}`, so `drawFrame` takes no camera arg (the scene owns which node is active; `SceneGraph::update` must run first). The renderer builds a `graphics/frame_info.hpp` `FrameInfo` per frame (retired the old `RenderContext`) and passes it plus cull frustums in; the scene emits `DrawCommand`s back. | | `render/viewport.hpp`, `cubemap_basis.hpp` | Small render helpers. | | `render/descriptor_bindings.hpp` | **Cross-file lynchpin.** `ForwardBinding`/`ForwardGlobalBinding`/`ShadowBinding`/`SkyboxBinding`/`PostProcessBinding` must match every `layout(set,binding)` in GLSL. | -| `render/ubo.hpp` | **High-attention.** Structs memcpy'd into mapped GPU memory — field order/`alignas`/padding must mirror GLSL std140 exactly. `test_ubo.cpp` guards sizes/offsets. **`LightUBO` now carries its own `offsetof` asserts** — it was the one block here without them, and that is exactly where a layout drift went unnoticed: a field inserted mid-struct moves every field after it, which `skybox.frag`'s hand-copied block did not follow (see `shaders/light_ubo.glsl`). Shaders share one declaration of it, so the two sides can only drift by editing both. **`ShadowPushConstants` carries the same asserts since SH-05** gave it a `materialIndex` (in what was padding, so no offset moved) — a push block has no driver-side reflection at all, so a resized member silently reinterprets the shader's fields, and the shader side is shared via `shaders/shadow_push.glsl`. The forward pass splits its data across **`ObjectUBO`** (per-object: model/hasSkin/previousModel — set 0 binding 0) and **`CameraUBO`** (per-frame: view/proj/cameraPos/view-projections — set 0 binding 29, written once/frame by the Renderer, pushed per draw so the depth prepass gets it too); both must match the blocks in `shader.vert`/`shader.frag`. Note `LightUBO::environmentParams` packs debug-view + no-shadows. | +| `render/ubo.hpp` | **High-attention.** Structs memcpy'd into mapped GPU memory — field order/`alignas`/padding must mirror GLSL std140 exactly. `test_ubo.cpp` guards sizes/offsets. **`LightUBO` now carries its own `offsetof` asserts** — it was the one block here without them, and that is exactly where a layout drift went unnoticed: a field inserted mid-struct moves every field after it, which `skybox.frag`'s hand-copied block did not follow (see `shaders/light_ubo.glsl`). Shaders share one declaration of it, so the two sides can only drift by editing both. **`ShadowPushConstants` carries the same asserts since SH-05** gave it a `materialIndex` (in what was padding, so no offset moved) — a push block has no driver-side reflection at all, so a resized member silently reinterprets the shader's fields, and the shader side is shared via `shaders/shadow_push.glsl`. The forward pass splits its data across **`ObjectUBO`** (per-object: model/hasSkin/previousModel — set 0 binding 0) and **`CameraUBO`** (per-frame: view/proj/cameraPos/view-projections — set 0 binding 29, written once/frame by the Renderer, pushed per draw so the depth prepass gets it too); both must match the blocks in `shader.vert`/`shader.frag`. Note `LightUBO::environmentParams` packs the debug-view selector; its `w` is now RESERVED — the old "disable shadow lookups" flag was replaced by `shadowMapValidMask` (in what was `_pad0`, so no offset moved), because that flag suppressed only the sampling while the pass kept recording. | | `render/descriptors.hpp` + `descriptors.cpp` | Forward set 0 (seven per-object/per-draw buffers: frame, camera, skin, previous-skin, morph, morph-target, VIPM) is **pushed inline per draw** via `pushForwardObjectDescriptors` (core 1.4 push descriptors) — not allocated. The shadow set 0 is **pushed inline per draw** too (`pushShadowObjectDescriptors`: per-object ShadowUBO + reused skin/morph buffers + the shared self-shadow image/sampler from `Resources`) — no per-object allocation in either pass. Set 1 (forward globals): `createGlobalDescriptors` once at startup; **`updateGlobalDescriptors` after resize** so recreated sceneColor sampler isn't dangling. Watch info-object lifetime around `updateDescriptorSets`. Set 2 (bindless materials) is owned by `Resources`, not here. **`makeForwardPushConstants(dc)`** lives beside them — the single `DrawCommand` → `ForwardPushConstants` packer, consumed by both the forward and transmission recorders (they used to each build the struct by hand, and the transmission one silently dropped `lodLevel`). Add a push-constant field here, never at a call site; covered by `tests/render/test_descriptors.cpp`. | | `render/pipeline.hpp` + `pipeline.cpp` | Forward pipelines declare `bindings` (set 0), `globalBindings` (set 1), and `bindlessSet` (set 2: a partially-bound `sampler2D[kMaxBindlessTextures]` + materials SSBO, with update-after-bind binding flags); the SHADOW pipelines opt into bindless too (SH-05's masked fragment paths read the same materials SSBO + texture array), which is why a config that wants bindless without globals gets an EMPTY set-1 layout — descriptor sets must be contiguous, and that empty layout is what keeps bindless at set 2 in every pipeline instead of at whatever index each one's set list happens to reach. Skybox / post-process / IBL-precompute leave both off. Fullscreen/fragment-only config factories share private helpers; `test_pipeline_config.cpp` guards the returned state. | | `render/resources` (bindless) | Owns the global set-2 descriptor (update-after-bind pool). `registerBindlessTexture` writes each 2D material texture into the array at its handle index (called from the 2D `createTexture` paths). `registerMaterial` assigns a material its slot in the persistently-mapped materials[] SSBO (dedup by `Material*`), returning the index drawn with. | @@ -226,9 +227,10 @@ Read paired with `ubo.hpp`, `descriptor_bindings.hpp`, `gpu_limits.hpp`. | `shader.vert` / `shader.frag` | Main forward PBR. `shader.vert` also emits jitter-free current/previous clip positions for TAA motion vectors — skinned meshes reproject through **last frame's joints** (`PrevSkin` UBO, set-0 binding 30) for exact per-vertex deformation velocity; rigid nodes use `previousModel` (unified via `prevTransform`). **`shader.frag` is the densest shader**: writes `outVelocity` (location 1) before any early return, then IBL, CSM/spot/point shadow sampling, double-sided normal flip (`if (!gl_FrontFacing) N = -N;` for `N`, `shadowNormal`, clearcoat `N_cc`), debug-view branches (incl. velocity = 5, SSAO = 6, **LOD tint = 7** — colours by `pc.lodLevel` — and **shadow-LOD tint = 8** — colours by `pc.shadowLodLevel`, grey at the `0xFFFFFFFF` no-shadow sentinel; both share `lodTint()`), transmission. Array sizes must equal `gpu_limits.hpp`. | | `taa.frag` | TAA resolve: `historyUV = uv − velocity`, 3×3 neighbourhood clamp, `mix(current, history, historyBlend)`; falls back to current when history is invalid or the reprojected UV is off-screen. Fullscreen triangle (`postprocess.vert`). | | `shadow.vert`/`.frag`, `shadow_masked.frag`, `self_shadow_second.frag`, `self_shadow_second_masked.frag` | Skinning+morph still run here so animated geo casts matching shadows; second pass = back-faces only. **SH-05 split each fragment stage into an opaque and a masked path** — the masked ones apply the visible material's alpha cutout from `material.glsl` (bindless set 2, indexed by `ShadowPushConstants::materialIndex`), so a cutout casts its silhouette rather than its quad; the second-depth masked path tests the cutout FIRST, since a masked-out fragment is not a surface and cannot be anybody's second depth. One vertex shader serves all four: it emits both UV sets unconditionally (skinning moves positions, never UVs) rather than duplicating the skin/morph maths in a second stage. | -| `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl` | The shadow stages' shared includes (SH-05). `shadow_push.glsl` is **the single declaration of `ShadowPushConstants`** — guarded, because a push block is a raw byte range with no reflection, and it had been hand-copied into three stages before a fourth arrived. `shadow_depth.glsl` holds the point-face linear distance/range `gl_FragDepth` write, so the opaque and masked paths cannot record different depths for the same face. `self_shadow_second.glsl` holds the dual-depth rejection test, which IS the pass — a drifted copy would show as acne on cutout characters only, looking like a bias problem. Each still hand-copies a shadow limit (`SHADOW_TOTAL_MATRIX_COUNT`, `SHADOW_POINT_MATRIX_BASE`, the self-slot count) from `gpu_limits.hpp`; generating them is the open hygiene item in the roadmap. | +| `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl` | The shadow stages' shared includes (SH-05). `shadow_push.glsl` is **the single declaration of `ShadowPushConstants`** — guarded, because a push block is a raw byte range with no reflection, and it had been hand-copied into three stages before a fourth arrived. `shadow_depth.glsl` holds the point-face linear distance/range `gl_FragDepth` write, so the opaque and masked paths cannot record different depths for the same face. `self_shadow_second.glsl` holds the dual-depth rejection test, which IS the pass — a drifted copy would show as acne on cutout characters only, looking like a bias problem. The shadow limits they use (`SHADOW_POINT_MATRIX_BASE`, the self-slot count — and `SHADOW_TOTAL_MATRIX_COUNT` in `shadow.vert` beside them) are no longer transcribed: each `#include`s `gpu_limits.glsl`, the one declaration `gpu_limits.hpp` also reads, and `gpu_limits_guard` fails if one goes back to a literal. | | `depth_prepass.frag` | Depth-only, and **no longer empty**: it applies the material's ALPHA CUTOUT through the shared `material.glsl` test, gated on the material's own `alphaCutoff` so an opaque draw pays one scalar SSBO read and no texture fetch (the gate is exactly equivalent — `toMaterialUBO` publishes a cutoff only for MASK, and at cutoff 0 the test can never discard, in this stage or the forward one). Before that it wrote depth across a cutout's holes, so the forward pass' `LESS_OR_EQUAL` test rejected whatever stood behind them and SSAO — which reconstructs position and normal from this depth alone — treated the cutout as a solid sheet. The prepass and the forward pass MUST discard the same fragments: one keeping what the other drops leaves either a depth-only occluder or a shaded fragment whose depth nobody wrote, which is why both call one implementation on the same UVs from the same vertex path. | | `forward_push.glsl` | **The single declaration of `ForwardPushConstants`**, included by `shader.frag` and `depth_prepass.frag`. Guarded (`shader_block_guards`), for the reason a push block always is: no driver-side reflection, so a member added to one copy silently reinterprets every field after it — and a shifted `materialIndex` is a wrong bindless material rather than an error. Note the pipeline layout must declare the WHOLE block even for a stage that reads one field of it. | +| `gpu_limits.glsl` | **The one declaration of every shared GPU data-layout value** — light/caster/joint/morph/emitter/SSAO-kernel counts, the derived shadow-matrix bases and total, and the `SHADOW_MAP_VALID_*` bits. Scope is "a C++ block and a shader block must agree on it"; GLSL-only algorithm constants (compute workgroup sizes, scan radices) are deliberately out. Note `MORPH_WEIGHT_VEC4_COUNT` is DERIVED from `MAX_MORPH_TARGETS` — the block packs weights as vec4s, and the divisibility is asserted C++-side because GLSL cannot. Written in the subset that is valid GLSL *and* valid C++, because it is compiled by both: shaders `#include` it, and `graphics/gpu_limits.hpp` includes it inside a `shader_limits` namespace and re-exports each value as its `k`-name. Keep it in that subset — a `constexpr`, a namespace or an unsigned suffix here breaks shader compiles in files that never mention this one, which is why `gpu_limits_guard` rejects those keywords by name. The guard also sweeps the whole `shaders/` tree for a re-declaration, requires each consumer to USE the name **as many times as it does today** rather than a literal beside a decorative include, and requires the C++ header to define each `k`-constant *as* the shared declaration — without that last check C++ could drift back to hard-coded values with every shader-side check still green. The count, not mere presence, is what catches one of several uses reverting; and the counter neutralises `;` first, because `MATCHALL` returns a LIST and a match spanning a semicolon splits in two, inflating the count enough to hide a lost use. Fourteen mutations tested. | | `shadow_bias.glsl`, `poisson_taps.inl` | **The single production bias implementation** (SH-07), shared by all five receiver call sites, and the PCF taps. `shadow_bias.glsl` mirrors `graphics/shadow_bias.hpp`, which is the arbiter — the split exists only because `nDotL` and the punctual depths are per-fragment, so no CPU precomputation can supply them; both sides sanitise a malformed policy identically. `poisson_taps.inl` is a list of `POISSON_TAP(x, y)` invocations rather than a table, so `shader.frag` and the unit test expand the SAME numbers and the unit-support check pins what ships. Both are protected by `shadow_bias_guard` (`cmake/check_shadow_bias.cmake`), which strips both GLSL comment forms, requires exactly five law calls and all four per-family metrics reads, and rejects any `exp2(` — every one of those checks is mutation-tested, because a textual guard's failure mode is passing quietly. | | `material.glsl` | **The single declaration of the bindless material authority** — `MaterialData`, the `Materials` SSBO and the `textures[]` array at `set = 2`, plus `matTex`, `materialSlotUv`, `materialBaseColourTexel`, `materialAlpha` and `materialAlphaCutoutFails`. Guarded (`shader_block_guards`), and shared since SH-05 gave the shadow pass a path that must apply the SAME cutout as `shader.frag`: a second copy of this struct is a second cutoff, a second UV-set choice and a second transform. An including shader must declare its own push block named `pc` with a `uint materialIndex` FIRST — forward stages use `ForwardPushConstants`, shadow stages `shadow_push.glsl` — which is what lets one file serve both. | | `light_ubo.glsl` | **The single declaration of the `LightUBO` block**, `#include`d by `shader.frag` and `skybox.frag` (each `#define`s `LIGHT_UBO_SET`/`LIGHT_UBO_BINDING` first — the buffer sits at a different descriptor address in each). It exists because the block used to be hand-copied: `selfShadowViewProj` was added to the C++ struct and `shader.frag` but not `skybox.frag`, which then read `environmentParams` 256 bytes early — inside `selfShadowViewProj[1]` — so the sky was multiplied by a shadow-matrix element. Silent for months (0 VUIDs, no crash) because unused self slots are IDENTITY, making the misread exactly 1.0 until a scene supplied a *second* skinned self-shadow caster. Any shader binding this buffer must include this file, not restate it — a partial block is not a smaller mistake, it shifts every field after the omission. `shader_block_guards` (CTest) enforces it; `LightUBO`'s `offsetof` asserts in `ubo.hpp` pin the C++ side. | diff --git a/docs/roadmap.md b/docs/roadmap.md index c7f3fb16..3068fb2e 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -73,15 +73,6 @@ quality limit. The honest next steps are the follow-ups below, or another arc en - **SH-08** — shadow VIPM, *if* discrete transitions remain visibly popping. - **SH-09** — shadow VDPM checkpoint; highest complexity, requires evidence before committing. -**Independent shadow hygiene** (independent of everything above; see the plan's § Independent shadow -hygiene): make `noShadows` suppress *recording*, not just sampling; skip directional/world/self maps -with no active primary directional light; generate the GLSL shadow-limits include from the C++ -authority instead of repeating `SHADOW_TOTAL_MATRIX_COUNT` / `SHADOW_POINT_MATRIX_BASE` / the -self-shadow slot count (**same fix as arc 3's Tier 1 finding 2**; SH-05 reduced each of the three to -ONE hand-written copy — `shadow.vert`, `shaders/shadow_depth.glsl`, `shaders/self_shadow_second.glsl` -— but they are still hand-written, so the finding stands); keep map validity explicit when a -family is skipped. - --- ## Arc 2 — Architectural-review remainder ([`architecturalreview.md`](architecturalreview.md) §6) @@ -154,13 +145,14 @@ conventions, duplicated conversion authority) + a standardisation list. Sequence 3. **Transform & API redesign** — `Affine3` + direct TRS, split affine point/vector/normal from projective, explicit projection conventions, standardise the access/operator surface. -**Tier 1 — handles, limits, tunables.** 4 high (texture generations not enforced on lookup/release; -GPU layout limits have no machine-enforced C++/GLSL authority; `ColliderId` registration inconsistent -between broadphases; handle packing silently aliases invalid inputs), 5 medium, 2 low. Sequenced: +**Tier 1 — handles, limits, tunables.** 3 high open (texture generations not enforced on +lookup/release; `ColliderId` registration inconsistent between broadphases; handle packing silently +aliases invalid inputs) — the fourth, the missing C++/GLSL limits authority, landed — plus 5 medium +and 2 low. Sequenced: 1. **Close correctness holes** — enforce texture generation everywhere, fix dynamic-tree collider-ID - clearing/re-registration + broadphase parity tests, derive the shadow matrix ranges with - compile-time relationship asserts, add the build-enforced C++/GLSL limits authority (**the same - authority arc 1's hygiene item needs**). + clearing/re-registration + broadphase parity tests. (The shadow matrix ranges and the + build-enforced C++/GLSL limits authority — finding 2 — **landed** with arc 1's hygiene item: + `shaders/gpu_limits.glsl` + `gpu_limits_guard`.) 2. **Identity foundation** — neutral raw-index + generational strong-handle primitives, move `GenerationalSlotPool` out of `graphics/` with occupancy tracking and invalid-release rejection, pick a no-resurrection generation policy, migrate the GPU/physics handle families. diff --git a/docs/shadowplans.md b/docs/shadowplans.md index 74485580..0f843446 100644 --- a/docs/shadowplans.md +++ b/docs/shadowplans.md @@ -1003,21 +1003,83 @@ contracts from this work: Already-landed fixes—skipping the redundant world CSM when no skinned draw exists and skipping unassigned self-shadow slots—remain valid under this design. -## Independent shadow hygiene - -These small items are useful but need not block the LOD milestones: - -- Make `noShadows` suppress shadow pass recording, not only sampling in the forward shader. - Re-enabling is safe because the maps are rendered before their next sample. -- Do not render directional/world/self maps when there is no active primary directional light. - Likewise, avoid clearing an unused punctual family merely because its texture exists. -- Remove manually repeated shader limits such as `SHADOW_TOTAL_MATRIX_COUNT = 32`, - `SHADOW_POINT_MATRIX_BASE = 8`, and self-shadow slot count `4`. Generate a GLSL limits include - from the C++ authority or make shader compilation validate both sides. A one-sided count change - can otherwise compile cleanly and index the wrong UBO region. -- Preserve explicit map validity when skipping a map family. Sampling code should either know that - the map is valid for this frame or return fully lit; it should never rely on stale depth being - harmless by accident. +## Independent shadow hygiene — ✅ landed (`shadow-hygiene`) + +All four items landed together, because three of them are one change. "Skip a family's recording" +and "tell the receiver the family is invalid" are the same fact: skipping without the signal means +the shader samples a depth image the frame never wrote, and the only thing making that safe is the +current arrangement of the light loop — an accident, not a property. + +**One declaration of the limits both languages need** (`shaders/gpu_limits.glsl`). The file is +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 every +value under the engine's `k`-names, so the C++ types and names 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 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 does NOT +scale (it packs one split per `vec4` component), which is now a `static_assert` rather than a trap. + +The same mechanism then took the other four data-layout limits the tiered review's Tier 1 finding 2 +listed, which closes that finding: joints, morph weights, particle emitters and the SSAO kernel. The +morph one is not a literal swap — the block packs weights as `vec4`s, 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 (GLSL cannot say it). A ninth weight would otherwise need a third `vec4` the shader +never declared. Scope stops there: this owns values a C++ block and a shader block must AGREE on, not +GLSL-only algorithm constants such as the VDPM workgroup sizes. + +The `gpu_limits_guard` CTest case pins the arrangement from **both** sides: no shader may declare a +shared name itself (the whole `shaders/` tree is swept, so a new shader is covered too), every +consumer must include the file and use the name **as many times as it uses it today**, the shared +file must still declare everything and stay inside the common subset (a stray `constexpr` there +breaks shader compiles in files that never mention it), and `gpu_limits.hpp` must define each +`k`-constant *as* the shared declaration. Without that last half, C++ could drift back to hard-coded +values with every shader-side check still green. + +Mutation-testing earned two of those checks. Presence was not enough — reverting `ssao.frag`'s loop +bound to `16` passed, because the UBO array above it still named the constant — so the check counts +uses, and the counting itself had a bug worth recording: `MATCHALL` returns a CMake *list*, and a +match spanning a `;` (`for (i < NAME; ++i)`) split into two elements, inflating the count enough to +hide a lost use. Semicolons are neutralised before counting. Fourteen mutations, fourteen failures. + +**Recording gated by an explicit, uploaded 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 produces one bit per family. The renderer computes it **once**, after the +view set is complete — the world-only views are enabled last, so anything earlier reads a set still +being written — 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` to a `uploadFrameLighting` step at the end of `collectDrawCommands` 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/world-only families require *all* of them plus a real primary directional light (they +are fitted to a fallback direction otherwise, which produces a map describing 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 those families. + +Every receiver path asks its family's 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 a directional light, so it now shows magenta ("no map this frame") instead of a +plausible readout of stale data. `environmentParams.w`, the old "disable shadow lookups" flag, is +retired: it only ever suppressed the sampling, while the pass kept rendering. + +**Verification.** `--no-shadows` is now *observably* suppressed, which validation-clean rendering +alone would not show: `FE_LOG=render:debug` prints a per-family recording line on the cascade-fit +cadence, and on `ShadowLodDemo` it goes from `cascade recorded passes=4 0.718ms | world-only +recorded passes=4 0.260ms | self recorded passes=2 | spot recorded passes=1 | point recorded +passes=6` to every family `skipped passes=0 0.000ms | no family recorded`. The line pairs one +frame's decision with that same frame's counters (the validity rides the stats ring). With shadows +on, the reference capture is byte-identical under the sweep's own metric: **zero** pixels differ by +more than 8/255 from `docs/images/shadow-lod-selected.png`. + +One honest limitation: the "no primary directional light" branch is exercised by the headless tests, +not by a scene, because the app seeds a default sun whenever an asset authors none +(`FireEngine::loadScene`). It is still the right guard — a light removed at runtime, or a loader +regression like the one that dropped lights on animated nodes, produces exactly that frame. ## Verification gates diff --git a/include/fire_engine/graphics/gpu_limits.hpp b/include/fire_engine/graphics/gpu_limits.hpp index 389afc29..be71bd7e 100644 --- a/include/fire_engine/graphics/gpu_limits.hpp +++ b/include/fire_engine/graphics/gpu_limits.hpp @@ -12,52 +12,94 @@ // Pure scalar tunables (biases, strengths, FOV, IBL/shadow extents) stay in // render/constants.hpp, which includes this header so existing render-side // users keep seeing every constant through a single include. +// +// The values the SHADERS also need are not written here: they live in +// `shaders/gpu_limits.glsl`, a file in the common subset of GLSL and C++ that +// is included below and re-exported under the engine's `k`-names. Add a limit +// there — not here — whenever a shader must know it too, so the two sides +// cannot drift. The rest (frames in flight, joints, bindless capacities…) is +// C++-only and stays in this file. namespace fire_engine { +// The shared declarations, parked in their own namespace: they are GLSL-style +// SHOUTING_CASE and this header exists to give them engine names and engine +// types. Nothing outside this file should name them. +namespace shader_limits +{ +#include "gpu_limits.glsl" // NOLINT(bugprone-suspicious-include): the shared GLSL/C++ limits +} // namespace shader_limits + // Frames-in-flight: how many copies of per-frame GPU resources exist. inline constexpr int kMaxFramesInFlight = 2; // Skinning joint matrices per SkinUBO. -inline constexpr std::size_t kMaxJoints = 64; - -// Morph target weights per MorphUBO. -inline constexpr int kMaxMorphTargets = 8; +inline constexpr std::size_t kMaxJoints = shader_limits::MAX_JOINTS; + +// Morph target weights per MorphUBO. The GPU block packs them as vec4s, so the shader's array +// length is kMorphWeightVec4Count — derived, not written twice. The packing REQUIRES a multiple of +// four: a ninth weight would need a third vec4 the shader would not have declared, and the write +// would land outside the block. GLSL cannot say that, so it is asserted here, on the shared value +// both sides read. +inline constexpr int kMaxMorphTargets = shader_limits::MAX_MORPH_TARGETS; +inline constexpr int kMorphWeightVec4Count = shader_limits::MORPH_WEIGHT_VEC4_COUNT; +static_assert(kMaxMorphTargets % 4 == 0, + "MorphUBO packs weights as vec4s; a non-multiple of four would not fit the block"); +static_assert(kMorphWeightVec4Count * 4 == kMaxMorphTargets); // Cap on lights consumed by the forward shader's main lighting loop. Sized so // the LightUBO array fits comfortably under any sane Vulkan UBO limit. Bump // when scenes routinely exceed this; or swap to an SSBO at that point. -inline constexpr int kMaxLights = 8; +inline constexpr int kMaxLights = shader_limits::MAX_LIGHTS; // Per-skinned-object self-shadow slots (LightUBO::selfShadowViewProj). -inline constexpr int kMaxSkinnedSelfShadowCasters = 4; +inline constexpr int kMaxSkinnedSelfShadowCasters = shader_limits::MAX_SKINNED_SELF_SHADOW_CASTERS; // Directional cascade layers in the 2D-array shadow map. -inline constexpr uint32_t kShadowCascadeCount = 4; +inline constexpr uint32_t kShadowCascadeCount = shader_limits::SHADOW_CASCADE_COUNT; // Shadow casters for punctual lights. Caps are independent of kMaxLights; // excess punctual lights remain unshadowed. First-N policy in gather order. -inline constexpr int kMaxSpotShadowCasters = 4; -inline constexpr int kMaxPointShadowCasters = 4; +inline constexpr int kMaxSpotShadowCasters = shader_limits::MAX_SPOT_SHADOW_CASTERS; +inline constexpr int kMaxPointShadowCasters = shader_limits::MAX_POINT_SHADOW_CASTERS; // Faces of a cube map — ONE authority, because this value participates in four separate things: // logical-view key validation, shadow matrix indexing, image layer indexing, and the flat // point-view slot arithmetic. Two definitions drifting apart would corrupt all of them at once, // and quietly: every index would still be in range, just pointing at the wrong face. -inline constexpr std::uint32_t kCubeFaceCount = 6; +inline constexpr std::uint32_t kCubeFaceCount = shader_limits::CUBE_FACE_COUNT; // Shadow vertex shader projects each vertex into light-space using one of the // ShadowUBO::lightViewProj matrices, picked via ShadowPushConstants::matrixIndex. -// [0..3] directional cascades 0..3 -// [4..] spot lights, layout 4 + spotIndex -// [4+S..] point lights, layout (4 + S) + 6 * cubeIndex + face -// where S = kMaxSpotShadowCasters. -inline constexpr int kShadowCascadeMatrixBase = 0; -inline constexpr int kShadowSpotMatrixBase = 4; -inline constexpr int kShadowPointMatrixBase = kShadowSpotMatrixBase + kMaxSpotShadowCasters; -inline constexpr int kShadowTotalMatrixCount = - kShadowPointMatrixBase + static_cast(kCubeFaceCount) * kMaxPointShadowCasters; +// [0..C-1] directional cascades +// [C..] spot lights, layout C + spotIndex +// [C+S..] point lights, layout (C + S) + 6 * cubeIndex + face +// where C = kShadowCascadeCount and S = kMaxSpotShadowCasters. The arithmetic itself is shared with +// the shaders, not repeated here — see shaders/gpu_limits.glsl. +inline constexpr int kShadowCascadeMatrixBase = shader_limits::SHADOW_CASCADE_MATRIX_BASE; +inline constexpr int kShadowSpotMatrixBase = shader_limits::SHADOW_SPOT_MATRIX_BASE; +inline constexpr int kShadowPointMatrixBase = shader_limits::SHADOW_POINT_MATRIX_BASE; +inline constexpr int kShadowTotalMatrixCount = shader_limits::SHADOW_TOTAL_MATRIX_COUNT; + +// Which shadow-map families a frame recorded, packed into `LightUBO::shadowMapValidMask`. The +// producer is `ShadowMapValidity::packedMask()` (graphics/shadow_map_validity.hpp); the consumer is +// every sampling path in shader.frag. Bit values shared with the shader for the same reason the +// sizes are: a bit that means one family on one side and another on the other is a silent misread. +inline constexpr std::int32_t kShadowMapValidCascades = shader_limits::SHADOW_MAP_VALID_CASCADES; +inline constexpr std::int32_t kShadowMapValidWorldOnly = shader_limits::SHADOW_MAP_VALID_WORLD_ONLY; +inline constexpr std::int32_t kShadowMapValidSelf = shader_limits::SHADOW_MAP_VALID_SELF; +inline constexpr std::int32_t kShadowMapValidSpot = shader_limits::SHADOW_MAP_VALID_SPOT; +inline constexpr std::int32_t kShadowMapValidPoint = shader_limits::SHADOW_MAP_VALID_POINT; + +// The layout the comment above describes, asserted rather than trusted: these are the relations the +// matrix table's users assume, and they must survive any future change to a family's capacity. +static_assert(kShadowCascadeMatrixBase == 0); +static_assert(kShadowSpotMatrixBase == + kShadowCascadeMatrixBase + static_cast(kShadowCascadeCount)); +static_assert(kShadowPointMatrixBase == kShadowSpotMatrixBase + kMaxSpotShadowCasters); +static_assert(kShadowTotalMatrixCount == + kShadowPointMatrixBase + static_cast(kCubeFaceCount) * kMaxPointShadowCasters); // Bindless material textures: capacity of the global combined-image-sampler // array (forward set 2). Indexed directly by TextureHandle value, so it caps the @@ -79,12 +121,12 @@ inline constexpr uint32_t kMaxMaterials = 256; // Particle system pool sizing. The GPU particle pool holds // kMaxParticleEmitters * kMaxParticlesPerEmitter particles; each active emitter // owns a contiguous slice (emitterIndex = particleIndex / kMaxParticlesPerEmitter). -inline constexpr int kMaxParticleEmitters = 4; +inline constexpr int kMaxParticleEmitters = shader_limits::MAX_PARTICLE_EMITTERS; inline constexpr int kMaxParticlesPerEmitter = 4096; -// SSAO hemisphere kernel size. Mirrored by the kernel[] array in SsaoUBO and the -// matching loop bound in ssao.frag — keep all three in lockstep. TAA denoises the -// per-pixel rotation noise, so a modest count suffices. -inline constexpr uint32_t kSsaoKernelSize = 16; +// SSAO hemisphere kernel size — the SsaoUBO kernel[] length and ssao.frag's loop bound, which read +// the same shared declaration rather than being "kept in lockstep". TAA denoises the per-pixel +// rotation noise, so a modest count suffices. +inline constexpr uint32_t kSsaoKernelSize = shader_limits::SSAO_KERNEL_SIZE; } // namespace fire_engine diff --git a/include/fire_engine/graphics/shadow_map_validity.hpp b/include/fire_engine/graphics/shadow_map_validity.hpp new file mode 100644 index 00000000..f8f6be3b --- /dev/null +++ b/include/fire_engine/graphics/shadow_map_validity.hpp @@ -0,0 +1,87 @@ +#pragma once + +#include "fire_engine/graphics/gpu_limits.hpp" + +#include +#include + +// Which shadow-map families a frame actually RECORDS — one decision, used twice. +// +// The shadow pass may legitimately skip a whole family: shadows switched off, no primary +// directional light for the cascades to be fitted to, no punctual caster. Skipping is only correct +// while the receiver knows it happened. A skipped family's depth image still holds whatever the +// last frame that did render it left behind, and "nothing samples it anyway" is an argument about +// the CURRENT arrangement of the forward shader, not a property of the data — the kind of accident +// that survives until someone adds a sampler and gets last-second-of-last-frame shadows. +// +// So the recording decision and the signal the shader reads are the SAME VALUE: the renderer +// computes this once per frame, gates the pass's families on it, and uploads its packed mask in +// LightUBO. Neither side can be right while the other is wrong, because there is only one of them. +// +// Vulkan-free and pure, so the policy is testable without a device — which matters here because the +// interesting cases (a partially fitted cascade set, a cube with a missing face) are exactly the +// ones that are awkward to stage against a GPU. + +namespace fire_engine +{ + +// What the frame knows by the time the view set is complete. Counts are ACTIVE VIEWS from the +// authoritative set, not requests: what the pass will iterate over. +struct ShadowMapValidityInputs +{ + // The `--no-shadows` tunable. Suppresses recording as well as sampling, so nothing is drawn + // into any map and nothing reads one. + bool shadowsDisabled{false}; + // A primary directional light exists this frame. The cascade, world-only and self families are + // all fitted to it; with no sun there is nothing for them to describe. + bool primaryDirectionalLight{false}; + std::size_t activeCascadeViews{0}; + std::size_t activeWorldOnlyViews{0}; + std::size_t activeSelfViews{0}; + std::size_t activeSpotViews{0}; + // Flattened cube faces (lightSlot * kCubeFaceCount + face), as the set stores them. + std::size_t activePointViews{0}; +}; + +// One bit per family. Deliberately a struct of named bools rather than the mask itself: the callers +// that matter ask "is this family valid" (the pass, per group) and only the upload wants the packed +// form, so the packing is a projection instead of the currency. +struct ShadowMapValidity +{ + bool cascades{false}; + bool worldOnly{false}; + bool self{false}; + bool spot{false}; + bool point{false}; + + [[nodiscard]] bool operator==(const ShadowMapValidity&) const noexcept = default; + + // The LightUBO field, using the bit values shared with the shader (shaders/gpu_limits.glsl). + [[nodiscard]] std::int32_t packedMask() const noexcept; + + // True when the pass records nothing at all — the shadow groups can then skip their timestamps + // as well as their draws. + [[nodiscard]] bool none() const noexcept; +}; + +// The law. +// +// WHOLE-FAMILY, not "some slot is active", wherever the family is rendered as a unit: +// +// * the cascades and their world-only twin are sampled by cascade INDEX, chosen per fragment from +// the view depth. A fragment landing in cascade 2 samples layer 2 whether or not layer 2 was +// fitted, so three fitted cascades out of four is not "three quarters valid" — it is a family +// with a hole in it, and the honest answer is to record nothing and report invalid. +// * a point light's six faces are installed atomically (`ShadowRenderViewSet::setPointLight`), so +// a non-zero count that is not a whole number of cubes means that invariant has been broken +// upstream; validity reports false rather than rendering part of a cube. +// * self-shadow slots ARE independent — one caster per slot, sampled only by draws carrying that +// slot — so the family is valid when any slot is active. Same for spot lights, which are +// addressed by their own shadow index. +// +// The directional families additionally require a primary directional light: their fit has no +// meaning without one, and a fallback direction would produce a map that looks valid and describes +// a sun that is not in the scene. +[[nodiscard]] ShadowMapValidity shadowMapValidity(const ShadowMapValidityInputs& inputs) noexcept; + +} // namespace fire_engine diff --git a/include/fire_engine/render/renderer.hpp b/include/fire_engine/render/renderer.hpp index 9f5aef8c..e44422ce 100644 --- a/include/fire_engine/render/renderer.hpp +++ b/include/fire_engine/render/renderer.hpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -297,6 +298,15 @@ class Renderer // engine-wide constants plus the debug-flag members. void writeIblAndDebugParams(LightUBO& out) const; void assignSelfShadowSlots(std::span drawCommands); + // Derives this frame's shadow-map validity from the COMPLETED view set and performs the single + // per-frame LightUBO upload. Must run after every view producer — cascades, punctual, self + // layers, and the world-only enablement that only `anySkinned` decides — because the mask it + // uploads and the families the shadow pass records are the same value. + void uploadFrameLighting(); + // Reports which shadow families the completed ring slot recorded, with their raster counts and + // GPU time. This is how "`--no-shadows` suppresses RECORDING" is checked: a frame that still + // renders into maps nobody samples looks identical on screen and to the validation layers. + void logShadowRecordingSample() const; static void clearDrawBuckets(DrawBuckets& buckets) noexcept; void buildDrawBuckets(std::span drawCommands, DrawBuckets& buckets) const; void recordDrawBucket(vk::CommandBuffer cmd, std::span bucket, @@ -486,6 +496,15 @@ class Renderer std::optional pendingShadowFocus_{}; LightUBO lightData_{}; Vec3 directionalLightDir_{1.0f, -1.0f, 1.0f}; + // Whether `directionalLightDir_` came from a real light or from the fallback direction. The + // cascades are fitted either way — the shadow views must stay well-formed — but a fit against a + // sun that is not in the scene describes nothing, which is why the directional families' map + // validity asks this rather than asking whether a matrix exists. + bool hasPrimaryDirectional_{false}; + // Which map families this frame records, decided ONCE (after the view set is complete) and used + // twice: it gates the families in `Shadows::recordPass` and it is uploaded in + // `LightUBO::shadowMapValidMask` for the receiver. See graphics/shadow_map_validity.hpp. + ShadowMapValidity shadowMapValidity_{}; int activeSpotCasters_{0}; int activePointCasters_{0}; std::array pointCasters_{}; @@ -509,6 +528,10 @@ class Renderer // Per-slot "collected AND submitted" bit, consumed on publication. Independent of the GPU // timestamp validity: a device without timestamp support still produces valid CPU counters. std::array shadowStatsSlotUsed_{}; + // The validity that decided each slot's recording, ringed for the same reason the counters are: + // the diagnostic that reports "this family was skipped and cost nothing" must read one frame's + // decision beside that same frame's raster counts. + std::array shadowValidityRing_{}; // Per-frame camera matrices (set at the top of drawFrame). view_ + jitteredProj_ // drive rasterisation; currentViewProj_/previousViewProj_ are jitter-free for // TAA motion vectors. previousViewProj_ persists across frames. diff --git a/include/fire_engine/render/shadows.hpp b/include/fire_engine/render/shadows.hpp index 2239d183..e283a1b5 100644 --- a/include/fire_engine/render/shadows.hpp +++ b/include/fire_engine/render/shadows.hpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -125,9 +126,16 @@ class Shadows // // `activeSelfShadowCasters` bounds the self-shadow slot loop (slots are assigned densely, and // an unassigned slot's layers are never sampled — no fragment carries that slot index — so they - // need no clear). `renderWorldShadow` gates the world-only CSM: only skinned fragments sample - // it (shader.frag gates on hasSkin), so a frame with no skinned draw skips those iterations, - // and the frame that reintroduces one re-renders the map before anything samples it. + // need no clear). + // + // `validity` decides WHICH FAMILIES RECORD, and it is the same value the receiver read in + // `LightUBO::shadowMapValidMask`. A family whose bit is clear draws nothing, clears nothing and + // stamps no timestamp, so its diagnostic rows and its GPU time both stay at zero — that is the + // honest report, since the views were not rasterised. Re-enabling is safe in the same frame: + // this pass runs before anything samples a map, so the frame that turns a family back on + // re-renders it before its first read. What makes SKIPPING safe is the other half of the same + // value: the receiver is told the family is invalid and answers fully lit, rather than sampling + // depth left behind by whichever frame last rendered it. // // `stats` (SH-01) is MUTATED: every iteration marks its view rasterised and observes every // command it walks, so a view that renders nothing is still reported. Rows are keyed by @@ -144,8 +152,8 @@ class Shadows int activeSpotCasters, std::span pointCasters, const ShadowRenderViewSet& views, ShadowLodResolver& resolver, float lodBudgetTexels, ShadowLodHysteresis hysteresis, bool cullingEnabled, - bool renderWorldShadow, ShadowFrameStats& stats, const GpuProfiler& profiler, - uint32_t frameIndex) const; + ShadowMapValidity validity, ShadowFrameStats& stats, + const GpuProfiler& profiler, uint32_t frameIndex) const; private: Resources* resources_{nullptr}; diff --git a/include/fire_engine/render/ubo.hpp b/include/fire_engine/render/ubo.hpp index 155264c8..a70d48ac 100644 --- a/include/fire_engine/render/ubo.hpp +++ b/include/fire_engine/render/ubo.hpp @@ -122,6 +122,10 @@ struct MorphUBO alignas(4) int morphTargetCount{0}; alignas(4) int vertexCount{0}; int _pad0{0}; + // A flat float array here, `vec4 weights[MORPH_WEIGHT_VEC4_COUNT]` in GLSL — the SAME bytes, + // because std140 gives a scalar array a 16-byte stride and packing them as vec4s is what avoids + // paying it. Both lengths come from the shared `MAX_MORPH_TARGETS`, whose divisibility by four + // `gpu_limits.hpp` asserts; the shader indexes `weights[i / 4][i % 4]`. float weights[kMaxMorphTargets]{}; // VIPM geomorph (Continuous LOD): the vertex shader slides each drawn vertex whose removal // level equals vipmTargetLevel toward its target by morphFactor. Both 0 in Discrete mode / for @@ -179,13 +183,15 @@ struct LightUBO // Per-cascade light-space view-projection matrices. Computed against the // first directional light in `lights[]` if any; otherwise against a // default direction so the matrices stay valid for the shadow pass. - alignas(16) Mat4 cascadeViewProj[4]{}; + alignas(16) Mat4 cascadeViewProj[kShadowCascadeCount]{}; // Spot-light view-projection matrices for shadow sampling. Indexed by // LightData::cone.z (shadow index). Identity when the slot is unused. alignas(16) Mat4 spotViewProj[kMaxSpotShadowCasters]{}; // Per-skinned-object self-shadow matrices. Indexed by ForwardPushConstants::selfShadowSlot. alignas(16) Mat4 selfShadowViewProj[kMaxSkinnedSelfShadowCasters]{}; - // View-space far-plane distances for each cascade (x..w = cascades 0..3). + // View-space far-plane distances for each cascade (x..w = cascades 0..3). A single vec4 on the + // shader side, so unlike the matrix arrays it cannot grow with the cascade count — hence the + // assertion below rather than a sizing expression that would silently misdescribe the block. alignas(16) float cascadeSplits[4]{}; alignas(16) float iblParams[4]{}; // x = maxReflectionLod, y/z = IBL strengths // SH-07 BIAS POLICY, in texels of each view's own world footprint — not normalised depth, and @@ -200,7 +206,8 @@ struct LightUBO // 4=directional raw depth: red=receiver, green=stored, blue=cascade, // 5=velocity, 6=SSAO, 7=LOD tint, 8=shadow-LOD tint). See DebugView, whose // shader-backed values 0..8 are exactly this range. - // w = disable all shadow-map visibility lookups when > 0.5. + // w = RESERVED (0). It was the "disable shadow lookups" flag; `shadowMapValidMask` replaced it, + // because that flag suppressed only the SAMPLING while the pass kept recording. alignas(16) float environmentParams[4]{}; // x = the cascade cross-fade fraction (`kShadowCascadeBlendFraction`). UPLOADED rather than // duplicated as a shader literal because SH-06 made it a FITTING constraint as well as a @@ -217,7 +224,12 @@ struct LightUBO // the primary directional (CSM source) when one exists. The shader loops // 0..lightCount-1 and only applies CSM shadow at i==0 with type==0. alignas(16) int lightCount{0}; - int _pad0{0}; + // Which shadow-map families this frame RECORDED, as `ShadowMapValidity::packedMask()` (bit + // values in shaders/gpu_limits.glsl). A cleared bit means the family was skipped and its depth + // image is stale, so the receiver must answer "fully lit" instead of sampling it. It occupies + // what was padding, so no offset below moved — and it is padding well spent: the alternative + // was a sixth vec4 carrying one integer. + int shadowMapValidMask{0}; int _pad1{0}; int _pad2{0}; LightData lights[kMaxLights]{}; @@ -244,6 +256,10 @@ struct LightUBO // multiplied the sky by a shadow matrix element. These asserts pin the C++ side; shaders share one // declaration (shaders/light_ubo.glsl) so they cannot drift from it independently. static_assert(sizeof(Mat4) == 64, "LightUBO offsets below assume a 4x4 float matrix"); +// `cascadeSplits` is one vec4 on both sides, so it is the field that does NOT scale with the +// cascade count: raising kShadowCascadeCount past four needs a second field (or a different +// packing) here and in the shader, not just a wider matrix array. +static_assert(kShadowCascadeCount <= 4, "cascadeSplits packs one split per vec4 component"); static_assert(offsetof(LightUBO, cascadeViewProj) == 0, "LightUBO std140 layout"); static_assert(offsetof(LightUBO, spotViewProj) == 256, "LightUBO std140 layout"); static_assert(offsetof(LightUBO, selfShadowViewProj) == 512, "LightUBO std140 layout"); diff --git a/shaders/gpu_limits.glsl b/shaders/gpu_limits.glsl new file mode 100644 index 00000000..7147d4a6 --- /dev/null +++ b/shaders/gpu_limits.glsl @@ -0,0 +1,87 @@ +// The values C++ and GLSL must agree on — ONE declaration, read by both languages. Data-layout +// limits, mostly, plus the encoding of any field whose meaning is split across the seam. +// +// This file is written in the subset that is simultaneously valid GLSL and valid C++, and it is +// included by both: shaders take it through the shaders/ include path, and +// `include/fire_engine/graphics/gpu_limits.hpp` includes it inside a namespace and re-exports every +// value under the engine's `k`-names. There is therefore no second copy to drift — the numbers, and +// the arithmetic deriving one from another, exist here and nowhere else. +// +// It exists because they DID drift-by-hand for a long time: `SHADOW_TOTAL_MATRIX_COUNT = 32` in +// shadow.vert, `SHADOW_POINT_MATRIX_BASE = 8` in shadow_depth.glsl and the caster counts in +// light_ubo.glsl were each a transcription of gpu_limits.hpp. A one-sided change to any of them +// compiles cleanly on both sides and then indexes the wrong region of a UBO: every index stays in +// range, so there is no validation error and no crash, just a shadow matrix read from a slot that +// belongs to another family. +// +// KEEP IT IN THE COMMON SUBSET. Only `const int NAME = ;`, `//` +// comments, and this include guard are portable between GLSL 450 and C++. No `constexpr`, no +// `inline`, no namespaces, no unsigned suffixes, no `static_cast` — anything else compiles in one +// language and fails in the other, and the failure surfaces as a shader-compile error in a file +// that never mentions this one. + +#ifndef FIRE_ENGINE_GPU_LIMITS_GLSL +#define FIRE_ENGINE_GPU_LIMITS_GLSL + +// Cap on lights consumed by the forward shader's main lighting loop. +const int MAX_LIGHTS = 8; + +// Skinning joint matrices per SkinUBO. +const int MAX_JOINTS = 64; + +// Morph target weights per MorphUBO. The block stores them PACKED as vec4s, so the shader-side +// array length is the derived count below rather than the weight count itself — writing `weights[2]` +// by hand is how a ninth weight would land outside the block with the C++ side none the wiser. The +// division is exact by construction; `gpu_limits.hpp` asserts the divisibility, which GLSL cannot. +const int MAX_MORPH_TARGETS = 8; +const int MORPH_WEIGHT_VEC4_COUNT = MAX_MORPH_TARGETS / 4; + +// GPU particle pool: each active emitter owns a contiguous slice of the pool. +const int MAX_PARTICLE_EMITTERS = 4; + +// SSAO hemisphere kernel size — the UBO array length and the loop bound in ssao.frag. +const int SSAO_KERNEL_SIZE = 16; + +// Directional cascade layers in the 2D-array shadow map. +const int SHADOW_CASCADE_COUNT = 4; + +// Per-skinned-object self-shadow slots (LightUBO::selfShadowViewProj). +const int MAX_SKINNED_SELF_SHADOW_CASTERS = 4; + +// Shadow casters for punctual lights. Independent of MAX_LIGHTS; excess punctual lights remain +// unshadowed. +const int MAX_SPOT_SHADOW_CASTERS = 4; +const int MAX_POINT_SHADOW_CASTERS = 4; + +// Faces of a cube map. +const int CUBE_FACE_COUNT = 6; + +// The shadow matrix table (ShadowUBO::lightViewProj), selected per draw by +// ShadowPushConstants::matrixIndex: +// [0 .. C-1] directional cascades +// [C .. C+S-1] spot lights +// [C+S ..] point lights, 6 * cubeIndex + face +// The bases are DERIVED here rather than written out, so moving a family's capacity moves every +// index that follows it in one edit. +const int SHADOW_CASCADE_MATRIX_BASE = 0; +const int SHADOW_SPOT_MATRIX_BASE = SHADOW_CASCADE_MATRIX_BASE + SHADOW_CASCADE_COUNT; +const int SHADOW_POINT_MATRIX_BASE = SHADOW_SPOT_MATRIX_BASE + MAX_SPOT_SHADOW_CASTERS; +const int SHADOW_TOTAL_MATRIX_COUNT = + SHADOW_POINT_MATRIX_BASE + CUBE_FACE_COUNT * MAX_POINT_SHADOW_CASTERS; + +// Which shadow-map families were RECORDED this frame, packed as a bitmask in +// LightUBO::shadowMapValidMask. Not a limit, but it lives here for the same reason the limits do: +// the renderer sets the bits and the receiver reads them, so a bit that means one thing on one side +// and another on the other is a silent misread. Both sides take these declarations from here. +// +// A family whose bit is CLEAR was not rendered this frame and its depth is stale. The receiver must +// answer "fully lit" for it rather than sample: the whole point of skipping a family is that its +// texture no longer describes the scene, and "the stale content happens to be harmless" is not a +// property anything checks. +const int SHADOW_MAP_VALID_CASCADES = 1; +const int SHADOW_MAP_VALID_WORLD_ONLY = 2; +const int SHADOW_MAP_VALID_SELF = 4; +const int SHADOW_MAP_VALID_SPOT = 8; +const int SHADOW_MAP_VALID_POINT = 16; + +#endif // FIRE_ENGINE_GPU_LIMITS_GLSL diff --git a/shaders/light_ubo.glsl b/shaders/light_ubo.glsl index e98144b2..1b87d364 100644 --- a/shaders/light_ubo.glsl +++ b/shaders/light_ubo.glsl @@ -19,13 +19,10 @@ #error "define LIGHT_UBO_SET and LIGHT_UBO_BINDING before including light_ubo.glsl" #endif -// Mirrors gpu_limits.hpp: kMaxLights, kMaxSpotShadowCasters, kMaxSkinnedSelfShadowCasters, -// kMaxPointShadowCasters, kShadowCascadeCount. -const int MAX_LIGHTS = 8; -const int MAX_SPOT_SHADOW_CASTERS = 4; -const int MAX_SKINNED_SELF_SHADOW_CASTERS = 4; -const int MAX_POINT_SHADOW_CASTERS = 4; -const int SHADOW_CASCADE_COUNT = 4; +// MAX_LIGHTS, MAX_SPOT_SHADOW_CASTERS, MAX_SKINNED_SELF_SHADOW_CASTERS, MAX_POINT_SHADOW_CASTERS +// and SHADOW_CASCADE_COUNT — the same declarations graphics/gpu_limits.hpp re-exports, not a +// transcription of them. +#include "gpu_limits.glsl" struct LightData { // .xyz = world position (point/spot), .w = type (0=dir, 1=point, 2=spot) @@ -40,7 +37,7 @@ struct LightData { }; layout(set = LIGHT_UBO_SET, binding = LIGHT_UBO_BINDING) uniform LightUBO { - mat4 cascadeViewProj[4]; + mat4 cascadeViewProj[SHADOW_CASCADE_COUNT]; mat4 spotViewProj[MAX_SPOT_SHADOW_CASTERS]; // Per-skinned-object self-shadow matrices, indexed by ForwardPushConstants::selfShadowSlot. // Unused slots are identity, NOT zero — see selfShadowViewProjArray. @@ -54,7 +51,8 @@ layout(set = LIGHT_UBO_SET, binding = LIGHT_UBO_BINDING) uniform LightUBO { // x = kSkyboxIntensity, y = kEnvironmentShadowStrength, // z = debug view (0=off, 1=normals, 2=NdotL, 3=shadow visibility, // 4=directional raw depth, 5=velocity, 6=SSAO, 7=LOD tint, 8=shadow-LOD tint), - // w = disable all shadow-map visibility lookups when > 0.5. + // w = RESERVED (0) — the old "disable shadow lookups" flag, replaced by shadowMapValidMask + // below, which suppresses the RECORDING as well and so cannot disagree with the pass. vec4 environmentParams; // x = cascade cross-fade fraction (kShadowCascadeBlendFraction). Uploaded, not a literal here: // the renderer expands each cascade's fitted slice to cover the previous cascade's blend band, @@ -64,7 +62,10 @@ layout(set = LIGHT_UBO_SET, binding = LIGHT_UBO_BINDING) uniform LightUBO { // z/w reserved. vec4 cascadeParams; int lightCount; - int _pad0; + // Which shadow-map families were RECORDED this frame — SHADOW_MAP_VALID_* bits from + // gpu_limits.glsl. A cleared bit means that family's depth image was not rendered and is stale; + // its sampling path must return fully lit rather than read it. `--no-shadows` clears every bit. + int shadowMapValidMask; int _pad1; int _pad2; LightData lights[MAX_LIGHTS]; diff --git a/shaders/particle.vert b/shaders/particle.vert index 621d3a6b..fbec2fd2 100644 --- a/shaders/particle.vert +++ b/shaders/particle.vert @@ -4,6 +4,10 @@ // per pool slot; the quad is generated from gl_VertexIndex (6 verts = 2 tris) and // faced toward the camera using the view matrix's world-space right/up axes. +// MAX_PARTICLE_EMITTERS, sizing the emitter array below from the same declaration +// graphics/gpu_limits.hpp re-exports as kMaxParticleEmitters. +#include "gpu_limits.glsl" + struct Particle { vec4 posAge; // xyz position, w age vec4 velLife; // xyz velocity, w lifetime @@ -28,7 +32,7 @@ layout(std140, binding = 1) uniform Frame { uint frameCounter; uint emitterCount; uint particlesPerEmitter; - EmitterGpu emitters[4]; + EmitterGpu emitters[MAX_PARTICLE_EMITTERS]; } frame; layout(location = 0) out vec2 fragUv; diff --git a/shaders/particle_simulate.comp b/shaders/particle_simulate.comp index 4b699f2d..3a205744 100644 --- a/shaders/particle_simulate.comp +++ b/shaders/particle_simulate.comp @@ -5,6 +5,11 @@ // their emitter up to a per-frame spawn budget claimed via an atomic counter. // The pool is partitioned per emitter (emitterIndex = i / particlesPerEmitter). +// MAX_PARTICLE_EMITTERS, sizing the emitter array below from the same declaration +// graphics/gpu_limits.hpp re-exports as kMaxParticleEmitters. (The workgroup size below is a +// compute tuning choice with no C++ counterpart, so it stays a literal here.) +#include "gpu_limits.glsl" + layout(local_size_x = 64) in; struct Particle { @@ -36,7 +41,7 @@ layout(std140, binding = 2) uniform Frame { uint frameCounter; uint emitterCount; uint particlesPerEmitter; - EmitterGpu emitters[4]; // kMaxParticleEmitters + EmitterGpu emitters[MAX_PARTICLE_EMITTERS]; } frame; uint hashU(uint x) { diff --git a/shaders/self_shadow_second.glsl b/shaders/self_shadow_second.glsl index b7a90e03..3c4a3a98 100644 --- a/shaders/self_shadow_second.glsl +++ b/shaders/self_shadow_second.glsl @@ -10,13 +10,16 @@ layout(binding = 4) uniform texture2DArray selfShadowFirstMapTex; layout(binding = 5) uniform sampler selfShadowDepthSampler; +#include "gpu_limits.glsl" + // True when this fragment is the SAME surface the first (light-facing) layer already recorded, so it -// must not become the second depth. The slot bound mirrors kMaxSkinnedSelfShadowCasters -// (graphics/gpu_limits.hpp): a fragment carrying a slot outside the array has no layer to compare -// against, and rejecting it is the only answer that cannot sample someone else's caster. +// must not become the second depth. The slot bound is the shared +// MAX_SKINNED_SELF_SHADOW_CASTERS (re-exported to C++ as kMaxSkinnedSelfShadowCasters): a fragment +// carrying a slot outside the array has no layer to compare against, and rejecting it is the only +// answer that cannot sample someone else's caster. bool selfShadowSecondRejects() { - if (pc.selfShadowSlot < 0 || pc.selfShadowSlot >= 4) { + if (pc.selfShadowSlot < 0 || pc.selfShadowSlot >= MAX_SKINNED_SELF_SHADOW_CASTERS) { return true; } vec2 extent = vec2(textureSize(selfShadowFirstMapTex, 0).xy); diff --git a/shaders/shader.frag b/shaders/shader.frag index 42771a4a..36fbf1d2 100644 --- a/shaders/shader.frag +++ b/shaders/shader.frag @@ -4,6 +4,12 @@ // as a sampledImage so all shadow maps can share one comparison sampler. #extension GL_EXT_samplerless_texture_functions : require +// SHADOW_CASCADE_COUNT and MAX_SKINNED_SELF_SHADOW_CASTERS, bounding the cascade search and the +// self-shadow slot. Included DIRECTLY rather than leant on through light_ubo.glsl: this file uses +// the names in its own code, and an include it can see is what keeps that true if the block's +// includes are ever rearranged. The file is idempotent, so the second include costs nothing. +#include "gpu_limits.glsl" + // Per-object data (set 0, pushed per draw) — must match ObjectUBO in shader.vert / render/ubo.hpp. layout(binding = 0) uniform ObjectUBO { mat4 model; @@ -127,6 +133,19 @@ const vec2 poissonDisk[16] = vec2[16]( ); #undef POISSON_TAP +// Did the renderer RECORD this map family this frame? A family whose bit is clear was skipped — +// `--no-shadows`, no primary directional light for the directional families to be fitted to, no +// caster of that kind — and its depth image still holds whatever the last frame that rendered it +// left behind. Every sampling path asks this first and answers "fully lit" when the answer is no. +// +// Reading a stale map is not a theoretical worry: the previous arrangement skipped families and +// relied on nothing sampling them, which was true only because of how the light loop happened to be +// written. The bit makes it a property of the frame instead. +bool shadowMapValid(int familyBit) +{ + return (light.shadowMapValidMask & familyBit) != 0; +} + // Per-pixel rotation hash so neighbouring fragments use different rotations // of the same Poisson kernel. Stops the kernel pattern from showing as moiré. mat2 poissonRotation(vec3 worldPos) @@ -180,7 +199,10 @@ float sampleDirectionalShadowFrom(texture2DArray shadowTex, vec3 worldPos, vec3 float sampleSelfShadow(vec3 worldPos, vec3 normal, vec3 lightDir, int slot) { - if (slot < 0 || slot >= 4) { + if (!shadowMapValid(SHADOW_MAP_VALID_SELF)) { + return 1.0; + } + if (slot < 0 || slot >= MAX_SKINNED_SELF_SHADOW_CASTERS) { return 1.0; } @@ -206,8 +228,8 @@ float sampleSelfShadow(vec3 worldPos, vec3 normal, vec3 lightDir, int slot) int selectCascade(float viewDepth) { - int cascade = 3; - for (int i = 0; i < 4; ++i) + int cascade = SHADOW_CASCADE_COUNT - 1; + for (int i = 0; i < SHADOW_CASCADE_COUNT; ++i) { if (viewDepth < light.cascadeSplits[i]) { @@ -222,7 +244,7 @@ int selectCascade(float viewDepth) // current cascade; 1.0 = pure next cascade. Always 0.0 for the last cascade. float cascadeBlendFactor(int cascade, float viewDepth) { - if (cascade >= 3) + if (cascade >= SHADOW_CASCADE_COUNT - 1) return 0.0; float cascadeStart = cascade == 0 ? 0.0 : light.cascadeSplits[cascade - 1]; float cascadeEnd = light.cascadeSplits[cascade]; @@ -233,6 +255,11 @@ float cascadeBlendFactor(int cascade, float viewDepth) float computeShadow(vec3 worldPos, vec3 normal, vec3 lightDir, int cascade, float viewDepth) { + // Gated at the CALLER of the sampler rather than inside it: the two directional maps share one + // sampling function but are separate families with separate bits, and a check inside would have + // to guess which map it was handed. + if (!shadowMapValid(SHADOW_MAP_VALID_CASCADES)) + return 1.0; float current = sampleDirectionalShadowFrom(shadowMapTex, worldPos, normal, lightDir, cascade); float t = cascadeBlendFactor(cascade, viewDepth); if (t <= 0.0) @@ -243,6 +270,8 @@ float computeShadow(vec3 worldPos, vec3 normal, vec3 lightDir, int cascade, floa float computeWorldShadow(vec3 worldPos, vec3 normal, vec3 lightDir, int cascade, float viewDepth) { + if (!shadowMapValid(SHADOW_MAP_VALID_WORLD_ONLY)) + return 1.0; float current = sampleDirectionalShadowFrom(worldShadowMapTex, worldPos, normal, lightDir, cascade); float t = cascadeBlendFactor(cascade, viewDepth); @@ -448,7 +477,12 @@ void main() { } int shIdx = int(L.cone.z + 0.5); - if (shIdx >= 0 && attenuation > 0.0 && light.environmentParams.w <= 0.5) { + // The family this light would sample must have been RECORDED this frame. Spot and point + // are separate bits: a scene can legitimately record one and skip the other, and + // `--no-shadows` clears both. + bool punctualMapValid = (type == 2) ? shadowMapValid(SHADOW_MAP_VALID_SPOT) + : shadowMapValid(SHADOW_MAP_VALID_POINT); + if (shIdx >= 0 && attenuation > 0.0 && punctualMapValid) { // The GEOMETRIC normal, not the shaded one. `N` carries normal-map detail, and // biasing or displacing a receiver by it would let a texture physically move the // surface the shadow test is performed against — crawling and leaks that track the @@ -543,19 +577,21 @@ void main() { primaryDirectionalNdotL = NdotL; int cascade = selectCascade(fragViewDepth); float shadow = 1.0; - if (light.environmentParams.w <= 0.5) { - if (ubo.hasSkin == 1) { - float worldShadow = - computeWorldShadow(fragWorldPos, shadowNormal, lightVec, cascade, - fragViewDepth); - float selfShadow = sampleSelfShadow(fragWorldPos, shadowNormal, lightVec, - pc.selfShadowSlot); - shadow = min(worldShadow, selfShadow); - } else { - shadow = - computeShadow(fragWorldPos, shadowNormal, lightVec, cascade, - fragViewDepth); - } + // No blanket "shadows off" flag here any more: each path asks its own family's bit + // (inside computeWorldShadow / sampleSelfShadow / computeShadow), and `--no-shadows` is + // simply the case where every bit is clear. One mechanism, so suppressing a family and + // telling the receiver about it cannot diverge — and a skinned receiver whose + // world-only map was skipped still gets its self-shadow, which a shared gate here + // would have thrown away with it. + if (ubo.hasSkin == 1) { + float worldShadow = computeWorldShadow(fragWorldPos, shadowNormal, lightVec, + cascade, fragViewDepth); + float selfShadow = sampleSelfShadow(fragWorldPos, shadowNormal, lightVec, + pc.selfShadowSlot); + shadow = min(worldShadow, selfShadow); + } else { + shadow = computeShadow(fragWorldPos, shadowNormal, lightVec, cascade, + fragViewDepth); } primaryDirectionalVisibility = shadow; // Contact shadows (screen-space) further occlude the *direct* sun, @@ -583,10 +619,19 @@ void main() { } if (light.environmentParams.z > 3.5 && light.environmentParams.z < 4.5) { + // NO MAP, NO READOUT. This view samples the cascade image directly and reads lights[0] as + // the sun — both are wrong when the cascade family was not recorded: lights[0] is then + // whatever light happened to pack first (a point light's `direction` is its range and a + // forward of zero), and the image holds another frame's depth. Magenta says "this frame has + // no directional shadow map" instead of drawing a plausible-looking readout of stale data. + if (!shadowMapValid(SHADOW_MAP_VALID_CASCADES)) { + outColor = vec4(1.0, 0.0, 1.0, alpha); + return; + } int cascade = selectCascade(fragViewDepth); vec3 lightVec = normalize(-light.lights[0].direction.xyz); vec2 depths = directionalShadowDepths(fragWorldPos, shadowNormal, lightVec, cascade); - float cascadeDebug = float(cascade) / 3.0; + float cascadeDebug = float(cascade) / float(SHADOW_CASCADE_COUNT - 1); outColor = vec4(depths.x, depths.y, cascadeDebug, alpha); return; } diff --git a/shaders/shader.vert b/shaders/shader.vert index d47f8143..044bff66 100644 --- a/shaders/shader.vert +++ b/shaders/shader.vert @@ -1,5 +1,9 @@ #version 450 +// MAX_JOINTS and MORPH_WEIGHT_VEC4_COUNT, sizing the skin and morph blocks below from the same +// declarations graphics/gpu_limits.hpp re-exports as kMaxJoints / kMorphWeightVec4Count. +#include "gpu_limits.glsl" + // Per-object data (set 0, pushed per draw). previousModel is last frame's world for motion vectors. layout(binding = 0) uniform ObjectUBO { mat4 model; @@ -22,13 +26,13 @@ layout(binding = 29) uniform CameraUBO { } camera; layout(binding = 3) uniform SkinUBO { - mat4 joints[64]; + mat4 joints[MAX_JOINTS]; } skin; // Previous-frame joint matrices (TAA motion vectors for skinned meshes). Identity for non-skinned // draws (unread there). Same layout as SkinUBO. layout(binding = 30) uniform PrevSkinUBO { - mat4 joints[64]; + mat4 joints[MAX_JOINTS]; } prevSkin; layout(binding = 4) uniform MorphUBO { @@ -36,7 +40,7 @@ layout(binding = 4) uniform MorphUBO { int morphTargetCount; int vertexCount; int _pad0; - vec4 weights[2]; + vec4 weights[MORPH_WEIGHT_VEC4_COUNT]; float morphFactor; // VIPM geomorph amount (0 = discrete / no morph) int vipmTargetLevel; // vertices removed by this 1-based LOD level morph in this transition } morph; diff --git a/shaders/shadow.vert b/shaders/shadow.vert index fe8364a3..b4d018c8 100644 --- a/shaders/shadow.vert +++ b/shaders/shadow.vert @@ -1,6 +1,8 @@ #version 450 -const int SHADOW_TOTAL_MATRIX_COUNT = 32; +// SHADOW_TOTAL_MATRIX_COUNT — the shared declaration graphics/gpu_limits.hpp re-exports as +// kShadowTotalMatrixCount, which is what sizes the C++ ShadowUBO this block must match. +#include "gpu_limits.glsl" layout(binding = 0) uniform ShadowUBO { mat4 model; @@ -11,7 +13,7 @@ layout(binding = 0) uniform ShadowUBO { #include "shadow_push.glsl" layout(binding = 1) uniform SkinUBO { - mat4 joints[64]; + mat4 joints[MAX_JOINTS]; } skin; layout(binding = 2) uniform MorphUBO { @@ -19,7 +21,7 @@ layout(binding = 2) uniform MorphUBO { int morphTargetCount; int vertexCount; int _pad0; - vec4 weights[2]; + vec4 weights[MORPH_WEIGHT_VEC4_COUNT]; } morph; layout(std430, binding = 3) readonly buffer MorphTargets { diff --git a/shaders/shadow_depth.glsl b/shaders/shadow_depth.glsl index e6cdf45d..b931ac30 100644 --- a/shaders/shadow_depth.glsl +++ b/shaders/shadow_depth.glsl @@ -6,9 +6,9 @@ // the main shader's samplerCubeArrayShadow on one of them only, and cutout casters would lose their // point shadows for a reason that looks like a bias problem. // -// Mirrors kShadowPointMatrixBase (graphics/gpu_limits.hpp) — the matrix-slot layout that puts the -// point faces last. -const int SHADOW_POINT_MATRIX_BASE = 8; +// SHADOW_POINT_MATRIX_BASE — the shared matrix-slot layout that puts the point faces last, from the +// one file graphics/gpu_limits.hpp also reads. +#include "gpu_limits.glsl" // Point faces store linear distance / range into gl_FragDepth so the main pass' comparison sampler // tests the same ratio. Cascade, spot and self views write nothing here and keep the fixed-function diff --git a/shaders/ssao.frag b/shaders/ssao.frag index c4ed241d..f0f51d6c 100644 --- a/shaders/ssao.frag +++ b/shaders/ssao.frag @@ -5,13 +5,15 @@ // G-buffer); a hemisphere kernel estimates occlusion. R = AO, G = contact term. // Mirrors SsaoUBO in include/fire_engine/render/ubo.hpp. -const int KERNEL_SIZE = 16; // == kSsaoKernelSize +// SSAO_KERNEL_SIZE — the shared declaration graphics/gpu_limits.hpp re-exports as kSsaoKernelSize, +// so the UBO array here and the C++ block it must match are one value, not two that agree today. +#include "gpu_limits.glsl" layout(set = 0, binding = 0) uniform sampler2D depthTex; layout(set = 0, binding = 1) uniform SsaoUBO { - mat4 proj; // jittered projection the depth was rendered with - vec4 kernel[KERNEL_SIZE]; // hemisphere samples (xyz), tangent space (+Z = normal) + mat4 proj; // jittered projection the depth was rendered with + vec4 kernel[SSAO_KERNEL_SIZE]; // hemisphere samples (xyz), tangent space (+Z = normal) vec4 params; // x=radius y=bias z=intensity (0=off) w=power vec4 contact; // x=length y=steps z=sunEnabled(>0.5) w=edgeThreshold vec4 sunViewDir; // xyz sun direction in view space @@ -133,7 +135,7 @@ void main() float radius = ssao.params.x; float bias = ssao.params.y; float occlusion = 0.0; - for (int i = 0; i < KERNEL_SIZE; ++i) + for (int i = 0; i < SSAO_KERNEL_SIZE; ++i) { vec3 samplePos = viewPos + (TBN * ssao.kernel[i].xyz) * radius; vec4 clip = ssao.proj * vec4(samplePos, 1.0); @@ -154,7 +156,7 @@ void main() float rangeCheck = smoothstep(0.0, 1.0, radius / max(abs(viewPos.z - sampleViewZ), 1e-4)); occlusion += (sampleViewZ >= samplePos.z + bias ? 1.0 : 0.0) * rangeCheck; } - float ao = 1.0 - (occlusion / float(KERNEL_SIZE)) * ssao.params.z; + float ao = 1.0 - (occlusion / float(SSAO_KERNEL_SIZE)) * ssao.params.z; ao = pow(clamp(ao, 0.0, 1.0), ssao.params.w); // Contact shadows are unreliable at depth silhouettes — the ray skims the diff --git a/src/graphics/shadow_map_validity.cpp b/src/graphics/shadow_map_validity.cpp new file mode 100644 index 00000000..1d536014 --- /dev/null +++ b/src/graphics/shadow_map_validity.cpp @@ -0,0 +1,63 @@ +#include "fire_engine/graphics/shadow_map_validity.hpp" + +namespace fire_engine +{ + +std::int32_t ShadowMapValidity::packedMask() const noexcept +{ + std::int32_t mask = 0; + if (cascades) + { + mask |= kShadowMapValidCascades; + } + if (worldOnly) + { + mask |= kShadowMapValidWorldOnly; + } + if (self) + { + mask |= kShadowMapValidSelf; + } + if (spot) + { + mask |= kShadowMapValidSpot; + } + if (point) + { + mask |= kShadowMapValidPoint; + } + return mask; +} + +bool ShadowMapValidity::none() const noexcept +{ + return !cascades && !worldOnly && !self && !spot && !point; +} + +ShadowMapValidity shadowMapValidity(const ShadowMapValidityInputs& inputs) noexcept +{ + // One early return, not a `shadowsDisabled` term repeated in five expressions: "no shadows" + // means no family records and no family is sampled, and stating it once makes that total. + if (inputs.shadowsDisabled) + { + return ShadowMapValidity{}; + } + + constexpr auto cascadeCount = static_cast(kShadowCascadeCount); + constexpr auto faceCount = static_cast(kCubeFaceCount); + + ShadowMapValidity validity{}; + // EVERY cascade, not a non-zero count — a fragment picks its layer by depth and would sample an + // unfitted one. See the header. + validity.cascades = inputs.primaryDirectionalLight && inputs.activeCascadeViews == cascadeCount; + validity.worldOnly = + inputs.primaryDirectionalLight && inputs.activeWorldOnlyViews == cascadeCount; + validity.self = inputs.primaryDirectionalLight && inputs.activeSelfViews > 0; + validity.spot = inputs.activeSpotViews > 0; + // Whole cubes only. A remainder means the atomic six-face installation was bypassed, and half a + // cube is a light whose shadow depends on which way the receiver happens to face. + validity.point = inputs.activePointViews > 0 && inputs.activePointViews % faceCount == 0; + return validity; +} + +} // namespace fire_engine diff --git a/src/render/renderer.cpp b/src/render/renderer.cpp index e9f13bc9..99300564 100644 --- a/src/render/renderer.cpp +++ b/src/render/renderer.cpp @@ -401,6 +401,7 @@ void Renderer::updateLightData(Vec3 cameraPosition, Vec3 cameraTarget, float asp // (glTF/KHR convention), so the shadow camera must look down that vector. // The forward shader negates it separately when it needs surface-to-light. const Lighting* primaryDirectional = primaryDirectionalLight(lights); + hasPrimaryDirectional_ = primaryDirectional != nullptr; directionalLightDir_ = primaryDirectional != nullptr ? Vec3::normalise(primaryDirectional->worldDirection) : Vec3::normalise(Vec3{1.0f, -1.0f, 1.0f}); @@ -470,10 +471,11 @@ void Renderer::updateLightData(Vec3 cameraPosition, Vec3 cameraTarget, float asp writeIblAndDebugParams(lightData); lightData_ = lightData; - // No upload here: assignSelfShadowSlots — which runs later this frame in collectDrawCommands, - // before any submit — fills the self-shadow matrices and writes the whole struct once. That is - // the single authoritative per-frame LightUBO upload; a write here would be immediately - // overwritten (the struct is multi-KB), so don't reinstate one. + // No upload here. The struct is not COMPLETE yet: the self-shadow matrices need the draws, and + // `shadowMapValidMask` needs the world-only views, which are enabled last of all. The single + // authoritative per-frame upload therefore sits at the end of collectDrawCommands — see + // `uploadFrameLighting`. A write here would be overwritten anyway (the struct is multi-KB), so + // don't reinstate one. } void Renderer::computeShadowCascades(LightUBO& out, Vec3 cameraPosition, Vec3 cameraTarget, @@ -736,7 +738,10 @@ void Renderer::writeIblAndDebugParams(LightUBO& out) const const DebugView shaderView = tunables_.debugView == DebugView::Joints ? DebugView::None : tunables_.debugView; out.environmentParams[2] = static_cast(shaderView); - out.environmentParams[3] = tunables_.noShadows ? 1.0f : 0.0f; + // environmentParams[3] is RESERVED. It carried the `noShadows` flag until the family validity + // mask subsumed it: `--no-shadows` now clears every bit in `shadowMapValidMask`, which both + // suppresses recording and tells the receiver, where this flag only ever did the second half. + out.environmentParams[3] = 0.0f; // The SAME constant the cascade fit expands each slice by. Uploaded rather than duplicated as a // shader literal: the fit covers the band, the shader decides who is in it, and two hand-kept // copies would disagree about where it starts. @@ -808,9 +813,34 @@ void Renderer::assignSelfShadowSlots(std::span drawCommands) dc.selfShadowViewProj = selfMatrices[static_cast(it->second)]; } - // The single authoritative per-frame LightUBO upload (updateLightData deliberately does not - // write — see the note there). Runs unconditionally, even with no self-shadow casters, so the - // rest of lightData_ (lights, cascades, IBL) still reaches the GPU. + // No upload here either: `uploadFrameLighting` runs once the world-only views are enabled, so + // the mask it writes describes the families the pass will actually record. +} + +void Renderer::uploadFrameLighting() +{ + // The frame's map validity, decided ONCE from the completed view set — every producer has run: + // cascades and punctual views in updateFrameLighting, self layers in assignSelfShadowSlots, + // world-only last. Deriving it earlier would read a set that is still being written, and the + // two consumers (this upload and the pass's family gates) would then be answering different + // questions with the same name. + shadowMapValidity_ = shadowMapValidity(ShadowMapValidityInputs{ + .shadowsDisabled = tunables_.noShadows, + .primaryDirectionalLight = hasPrimaryDirectional_, + .activeCascadeViews = shadowViews_.activeCount(ShadowViewGroup::Cascade), + .activeWorldOnlyViews = shadowViews_.activeCount(ShadowViewGroup::WorldOnly), + .activeSelfViews = shadowViews_.activeCount(ShadowViewGroup::Self), + .activeSpotViews = shadowViews_.activeCount(ShadowViewGroup::Spot), + .activePointViews = shadowViews_.activeCount(ShadowViewGroup::Point), + }); + lightData_.shadowMapValidMask = shadowMapValidity_.packedMask(); + // Into the frame ring beside the counters it explains: the diagnostics publish a slot a + // ring-cycle later, and a report pairing this frame's decision with that frame's raster counts + // would be two frames' answers under one heading. + shadowValidityRing_[currentFrame_] = shadowMapValidity_; + + // The single authoritative per-frame LightUBO upload. Runs unconditionally — with no shadows at + // all the rest of the struct (lights, IBL, debug params) still has to reach the GPU. writeMapped(lightUbo_.mapped[currentFrame_], lightData_); } @@ -1199,6 +1229,9 @@ const Renderer::DrawBuckets& Renderer::collectDrawCommands(RenderableScene& scen } } + // The view set is complete: derive this frame's map validity and upload the lighting block. + uploadFrameLighting(); + // GPU-driven VDPM (Stage B5b): distil the request sink down to the fronts that are actually // camera-visible this frame, and (B5b-2) resolve each visible forward draw's buffers to the GPU // output. Object appended a request for every front on a coarse-cull survivor (camera ∪ @@ -1313,22 +1346,69 @@ void Renderer::recordShadowPass(vk::CommandBuffer cmd, const DrawBuckets& bucket pointCasters_.data(), static_cast(activePointCasters_)}; // Self-shadow slots are assigned densely (assignSelfShadowSlots), so the // scratch map's size is the number of slots the pass must render. - // THE SET decides whether the world-only CSM runs, not `buckets.anySkinned` — that was the - // request, made before the views existed. Every world-only slot must be active: the pass loops - // all cascades, so a partially enabled set would rasterise cascades the set reports as - // inactive. (`anySkinned` false leaves them all inactive, which is the same answer by a - // shorter route.) - const bool renderWorldShadow = shadowViews_.activeCount(ShadowViewGroup::WorldOnly) == - shadowViewSlotCount(ShadowViewGroup::WorldOnly); + // WHICH FAMILIES RECORD is `shadowMapValidity_`, the same value the receiver was told about in + // `LightUBO::shadowMapValidMask` — including the world-only decision, which used to be re-read + // from the set here. `anySkinned` was only ever the request; the set's whole-family answer is + // what the validity law consumes, so the pass and the shader cannot disagree about which maps + // this frame's depth belongs to. shadows_.recordPass(cmd, buckets.shadow, buckets.worldShadow, buckets.selfShadow, static_cast(selfShadowSlotsScratch_.size()), activeSpotCasters_, pointCasterSpan, shadowViews_, shadowLodResolver_, tunables_.shadowLodPixelBudget, ShadowLodHysteresis{.coarsenRatio = tunables_.shadowLodCoarsenRatio}, - tunables_.cullingEnabled, renderWorldShadow, + tunables_.cullingEnabled, shadowMapValidity_, shadowStatsRing_[currentFrame_], profiler_, currentFrame_); } +// What the shadow pass actually RECORDED, per family — the observable half of the validity +// contract. `--no-shadows` (or a scene with no light a family is fitted to) must show zero raster +// passes and zero GPU milliseconds for every family, and only a report like this can show it: a +// frame that still records into maps nobody samples looks identical on screen and identical to the +// validation layers, while costing exactly as much as it did before. +// +// Same cadence and category as the cascade-fit diagnostics (`FE_LOG=render:debug`), and the same +// reason: periodic so a long run stays readable, plus the `--capture-frame` frame so a capture and +// its explanation describe one submitted frame. The counters and timings are a ring-cycle old — +// they were published above from the frame that has completed — so they describe a real submission +// rather than the frame being built. +void Renderer::logShadowRecordingSample() const +{ + if (!logShadowPlacementThisFrame_) + { + return; + } + if (!stats_.shadowValid) + { + log::debug(log::category::render, + "shadow recording: no completed frame's counters yet (ring warm-up)"); + return; + } + + // The validity that DECIDED this slot's recording, not the decision being made for the frame + // under construction — it rides the same ring as the counters for exactly that reason, and the + // publish above happens before this frame's `uploadFrameLighting` overwrites the slot. + const ShadowMapValidity& recorded = shadowValidityRing_[currentFrame_]; + + const auto familyLine = [&](ShadowViewGroup group, bool valid) -> std::string + { + const ShadowViewStats totals = stats_.shadow.groupTotal(group); + const ProfilePass pass = shadowProfilePass(group); + const float ms = + pass == ProfilePass::Count ? 0.0f : stats_.passMs[static_cast(pass)]; + return std::format("{} {} passes={} {:.3f}ms", toString(group), + valid ? "recorded" : "skipped", totals.rasterPasses, ms); + }; + + log::debug(log::category::render, "shadow recording: {} | {} | {} | {} | {}{}{}", + familyLine(ShadowViewGroup::Cascade, recorded.cascades), + familyLine(ShadowViewGroup::WorldOnly, recorded.worldOnly), + familyLine(ShadowViewGroup::Self, recorded.self), + familyLine(ShadowViewGroup::Spot, recorded.spot), + familyLine(ShadowViewGroup::Point, recorded.point), + recorded.none() ? " | no family recorded" : "", + stats_.gpuValid() ? "" : " | timings unavailable on this device"); +} + void Renderer::resolveShadowFocusRequest() { if (!pendingShadowFocus_) @@ -1463,6 +1543,8 @@ void Renderer::drawFrame(Window& display, RenderableScene& scene, float dt) profiler_.resolve(currentFrame_, stats_); stats_.cpuFrameMs = dt * 1000.0f; + logShadowRecordingSample(); + // Start the ImGui frame before recording. GLFW events were already polled // this frame (Input::update), so the overlay's input state is current. overlay_.beginFrame(); diff --git a/src/render/shadows.cpp b/src/render/shadows.cpp index d0dde67f..5dcb6a24 100644 --- a/src/render/shadows.cpp +++ b/src/render/shadows.cpp @@ -379,23 +379,32 @@ void Shadows::recordPass(vk::CommandBuffer cmd, std::span sha int activeSpotCasters, std::span pointCasters, const ShadowRenderViewSet& views, ShadowLodResolver& resolver, float lodBudgetTexels, ShadowLodHysteresis hysteresis, bool cullingEnabled, - bool renderWorldShadow, ShadowFrameStats& stats, + ShadowMapValidity validity, ShadowFrameStats& stats, const GpuProfiler& profiler, uint32_t frameIndex) const { + // Nothing to record at all — `--no-shadows`, or a scene with no light any family is fitted to. + // Returning here is what makes suppression OBSERVABLE: no draws, no clears, no timestamps, so + // every shadow row in the diagnostics and every shadow group in the GPU timings reads zero. + if (validity.none()) + { + return; + } // Bottom-to-bottom group timing: both boundaries are bottom-of-pipe stamps, so adjacent // sub-millisecond groups cannot overlap and inflate each other the way top-to-bottom spans do. // Every pass in the engine stamps this way now — begin() itself is bottom-of-pipe — so this is // no longer a shadow-only convention, just the convention. A group that records nothing leaves - // its two stamps unwritten and reports 0. `active` gates the STAMPS, not the body: a family - // that renders nothing this frame must leave its two timestamps unwritten so the pass reports - // 0, rather than recording an empty span that reads as a small real cost and inflates the frame - // total. An active family with zero candidate draws is still timed — its clears and layout - // barriers are real GPU work. - const auto timeGroup = [&](ProfilePass pass, bool active, auto&& body) + // its two stamps unwritten and reports 0, rather than an empty span that reads as a small real + // cost and inflates the frame total. An active family with zero candidate draws is still timed + // — its clears and layout barriers are real GPU work. + // + // `recording` gates the body AND the stamps together, which is the point: a family this frame + // does not record must draw nothing, clear nothing and time nothing, and one gate is what makes + // those three the same answer. It comes from `ShadowMapValidity`, the same value the receiver + // was told, so a skipped family's diagnostics read zero and its shader path reads "fully lit". + const auto timeGroup = [&](ProfilePass pass, bool recording, auto&& body) { - if (!active) + if (!recording) { - body(); // no-op for an inactive family, but keeps the control flow in one place return; } profiler.begin(cmd, frameIndex, pass); @@ -501,7 +510,7 @@ void Shadows::recordPass(vk::CommandBuffer cmd, std::span sha // per cascade. Nothing depends on the interleaving (each layer is independently barriered), and // grouping them is what lets each family carry one bottom-to-bottom timestamp boundary in the // per-group GPU timing — interleaved, the two families' costs could not be separated at all. - timeGroup(ProfilePass::ShadowCascades, /*active=*/true, + timeGroup(ProfilePass::ShadowCascades, validity.cascades, [&] { for (uint32_t cascade = 0; cascade < kShadowCascadeCount; ++cascade) @@ -524,46 +533,43 @@ void Shadows::recordPass(vk::CommandBuffer cmd, std::span sha }); // The world-only CSM exists so skinned receivers can sample a cascade without their own - // geometry; with no skinned draw this frame nothing samples it, so skip the duplicate - // 4-cascade render entirely (stale content is unreachable — see the recordPass contract in - // shadows.hpp). Skipping leaves its diagnostic rows untouched, which is the honest report: the - // views were not rasterised. - timeGroup( - ProfilePass::ShadowWorldOnly, renderWorldShadow, - [&] - { - if (renderWorldShadow) - { - for (uint32_t cascade = 0; cascade < kShadowCascadeCount; ++cascade) - { - // The set's world-only entry ALIASES the cascade's, so this iteration - // resolves against the same logical view — the resolver returns the - // cascade's cached answer, which is what makes the two CSMs agree for a - // rigid caster rather than agreeing by coincidence. - const ShadowRenderView* view = viewFor(ShadowViewGroup::WorldOnly, cascade); - if (view == nullptr) - { - continue; - } - ShadowPushConstants pc{}; - pc.matrixIndex = kShadowCascadeMatrixBase + static_cast(cascade); - const std::optional frustum = frustumFor(*view, cullingEnabled); - const ShadowDrawFilter filter{.frustum = frustum ? &*frustum : nullptr}; - layeredIteration( - worldShadowMapHandle_, cascade, kShadowMapExtent, pc, worldOnlyShadowDraws, - filter, lodContextFor(*view), shadowPipelines_, ShadowFaceCull::PerCaster, - kDirectionalShadowRasterBiasConstant, kDirectionalShadowRasterBiasSlope, - ShadowViewTarget{stats, ShadowViewGroup::WorldOnly, cascade, true}); - } - } - }); + // geometry; with no skinned draw this frame no world-only view is enabled, so the duplicate + // 4-cascade render is skipped entirely. Skipping leaves its diagnostic rows untouched, which is + // the honest report: the views were not rasterised. The receiver is told (the WORLD_ONLY bit) + // rather than left to rely on nothing sampling it. + timeGroup(ProfilePass::ShadowWorldOnly, validity.worldOnly, + [&] + { + for (uint32_t cascade = 0; cascade < kShadowCascadeCount; ++cascade) + { + // The set's world-only entry ALIASES the cascade's, so this iteration + // resolves against the same logical view — the resolver returns the cascade's + // cached answer, which is what makes the two CSMs agree for a rigid caster + // rather than agreeing by coincidence. + const ShadowRenderView* view = viewFor(ShadowViewGroup::WorldOnly, cascade); + if (view == nullptr) + { + continue; + } + ShadowPushConstants pc{}; + pc.matrixIndex = kShadowCascadeMatrixBase + static_cast(cascade); + const std::optional frustum = frustumFor(*view, cullingEnabled); + const ShadowDrawFilter filter{.frustum = frustum ? &*frustum : nullptr}; + layeredIteration( + worldShadowMapHandle_, cascade, kShadowMapExtent, pc, + worldOnlyShadowDraws, filter, lodContextFor(*view), shadowPipelines_, + ShadowFaceCull::PerCaster, kDirectionalShadowRasterBiasConstant, + kDirectionalShadowRasterBiasSlope, + ShadowViewTarget{stats, ShadowViewGroup::WorldOnly, cascade, true}); + } + }); // Only the densely-assigned slots render; an unassigned slot's layers are // never sampled (no fragment carries its index), so they need no clear. An // assigned slot whose caster produced no shadow draw still clears here — // correctly reading "no occluder" (depth 1.0) for its forward fragments. timeGroup( - ProfilePass::ShadowSelf, activeSelfShadowCasters > 0, + ProfilePass::ShadowSelf, validity.self, [&] { for (int slot = 0; @@ -603,7 +609,7 @@ void Shadows::recordPass(vk::CommandBuffer cmd, std::span sha } }); - timeGroup(ProfilePass::ShadowSpot, activeSpotCasters > 0, + timeGroup(ProfilePass::ShadowSpot, validity.spot, [&] { for (int s = 0; s < activeSpotCasters && s < kMaxSpotShadowCasters; ++s) @@ -629,7 +635,7 @@ void Shadows::recordPass(vk::CommandBuffer cmd, std::span sha } }); - timeGroup(ProfilePass::ShadowPoint, !pointCasters.empty(), + timeGroup(ProfilePass::ShadowPoint, validity.point, [&] { for (std::size_t p = 0; p < pointCasters.size() && diff --git a/tests/graphics/test_shadow_map_validity.cpp b/tests/graphics/test_shadow_map_validity.cpp new file mode 100644 index 00000000..0e206963 --- /dev/null +++ b/tests/graphics/test_shadow_map_validity.cpp @@ -0,0 +1,179 @@ +#include + +#include + +using namespace fire_engine; + +namespace +{ + +constexpr auto kCascades = static_cast(kShadowCascadeCount); +constexpr auto kFaces = static_cast(kCubeFaceCount); + +// A frame with everything present, so each test can spoil exactly one thing and attribute the +// result to it. +ShadowMapValidityInputs everything() +{ + return ShadowMapValidityInputs{ + .shadowsDisabled = false, + .primaryDirectionalLight = true, + .activeCascadeViews = kCascades, + .activeWorldOnlyViews = kCascades, + .activeSelfViews = 2, + .activeSpotViews = 1, + .activePointViews = kFaces, + }; +} + +} // namespace + +TEST_CASE("a complete frame validates every family", "[ShadowMapValidity]") +{ + const ShadowMapValidity validity = shadowMapValidity(everything()); + CHECK(validity.cascades); + CHECK(validity.worldOnly); + CHECK(validity.self); + CHECK(validity.spot); + CHECK(validity.point); + CHECK_FALSE(validity.none()); +} + +TEST_CASE("disabling shadows clears every family", "[ShadowMapValidity]") +{ + // Not "clears the directional ones": --no-shadows means the pass records nothing at all, and + // `none()` is what the pass returns on. + ShadowMapValidityInputs inputs = everything(); + inputs.shadowsDisabled = true; + const ShadowMapValidity validity = shadowMapValidity(inputs); + CHECK(validity == ShadowMapValidity{}); + CHECK(validity.none()); + CHECK(validity.packedMask() == 0); +} + +TEST_CASE("the directional families need a real primary directional light", "[ShadowMapValidity]") +{ + // The cascades are FITTED whether or not a sun exists — the views must stay well-formed — so + // the active counts alone cannot distinguish a real fit from one against the fallback + // direction. Only this flag can, which is why validity asks for it rather than for a matrix. + ShadowMapValidityInputs inputs = everything(); + inputs.primaryDirectionalLight = false; + const ShadowMapValidity validity = shadowMapValidity(inputs); + CHECK_FALSE(validity.cascades); + CHECK_FALSE(validity.worldOnly); + CHECK_FALSE(validity.self); + // Punctual maps are fitted to their own lights and are unaffected. + CHECK(validity.spot); + CHECK(validity.point); +} + +TEST_CASE("a partial cascade set is invalid, not partially valid", "[ShadowMapValidity]") +{ + // A fragment picks its cascade layer from its view depth. Three fitted cascades out of four is + // a family with a hole, and sampling the hole reads another frame's depth. + for (std::size_t active = 0; active < kCascades; ++active) + { + ShadowMapValidityInputs inputs = everything(); + inputs.activeCascadeViews = active; + CHECK_FALSE(shadowMapValidity(inputs).cascades); + } + ShadowMapValidityInputs complete = everything(); + complete.activeCascadeViews = kCascades; + CHECK(shadowMapValidity(complete).cascades); +} + +TEST_CASE("a partial world-only set is invalid too", "[ShadowMapValidity]") +{ + ShadowMapValidityInputs inputs = everything(); + inputs.activeWorldOnlyViews = kCascades - 1; + const ShadowMapValidity validity = shadowMapValidity(inputs); + CHECK_FALSE(validity.worldOnly); + // Independent families: the main CSM is untouched by the world-only twin's absence, which is + // the ordinary case for a frame with no skinned draw. + CHECK(validity.cascades); +} + +TEST_CASE("point validity requires whole cubes", "[ShadowMapValidity]") +{ + // The six faces are installed atomically (ShadowRenderViewSet::setPointLight). A remainder + // means that invariant was bypassed upstream; half a cube is a light whose shadow depends on + // which way the receiver happens to face, so the family reports invalid rather than rendering + // the faces it has. + for (std::size_t partial = 1; partial < kFaces; ++partial) + { + ShadowMapValidityInputs inputs = everything(); + inputs.activePointViews = partial; + CHECK_FALSE(shadowMapValidity(inputs).point); + } + ShadowMapValidityInputs twoCubes = everything(); + twoCubes.activePointViews = 2 * kFaces; + CHECK(shadowMapValidity(twoCubes).point); + + ShadowMapValidityInputs cubeAndAHalf = everything(); + cubeAndAHalf.activePointViews = kFaces + 3; + CHECK_FALSE(shadowMapValidity(cubeAndAHalf).point); +} + +TEST_CASE("self and spot families are per-slot, so any active slot validates them", + "[ShadowMapValidity]") +{ + // Unlike the cascades, these are addressed by an index the draw or the light carries: a slot + // that was never assigned is never sampled, so one active slot is a valid family rather than a + // partial one. + ShadowMapValidityInputs oneEach = everything(); + oneEach.activeSelfViews = 1; + oneEach.activeSpotViews = 1; + CHECK(shadowMapValidity(oneEach).self); + CHECK(shadowMapValidity(oneEach).spot); + + ShadowMapValidityInputs none = everything(); + none.activeSelfViews = 0; + none.activeSpotViews = 0; + CHECK_FALSE(shadowMapValidity(none).self); + CHECK_FALSE(shadowMapValidity(none).spot); +} + +TEST_CASE("an empty frame records nothing", "[ShadowMapValidity]") +{ + const ShadowMapValidity validity = shadowMapValidity(ShadowMapValidityInputs{}); + CHECK(validity.none()); + CHECK(validity.packedMask() == 0); +} + +TEST_CASE("the packed mask uses the bits the shader reads", "[ShadowMapValidity]") +{ + // The values come from shaders/gpu_limits.glsl through gpu_limits.hpp — the same declarations + // the receiver compiles against. Pinning them here as well would be a transcription; what this + // checks is that each family maps to its OWN bit and to no other. + const std::int32_t cascades = + ShadowMapValidity{ + .cascades = true, .worldOnly = false, .self = false, .spot = false, .point = false} + .packedMask(); + const std::int32_t worldOnly = + ShadowMapValidity{ + .cascades = false, .worldOnly = true, .self = false, .spot = false, .point = false} + .packedMask(); + const std::int32_t self = + ShadowMapValidity{ + .cascades = false, .worldOnly = false, .self = true, .spot = false, .point = false} + .packedMask(); + const std::int32_t spot = + ShadowMapValidity{ + .cascades = false, .worldOnly = false, .self = false, .spot = true, .point = false} + .packedMask(); + const std::int32_t point = + ShadowMapValidity{ + .cascades = false, .worldOnly = false, .self = false, .spot = false, .point = true} + .packedMask(); + + CHECK(cascades == shader_limits::SHADOW_MAP_VALID_CASCADES); + CHECK(worldOnly == shader_limits::SHADOW_MAP_VALID_WORLD_ONLY); + CHECK(self == shader_limits::SHADOW_MAP_VALID_SELF); + CHECK(spot == shader_limits::SHADOW_MAP_VALID_SPOT); + CHECK(point == shader_limits::SHADOW_MAP_VALID_POINT); + + // Distinct, non-overlapping bits: an OR of all five must be recoverable term by term. + const std::int32_t all = cascades | worldOnly | self | spot | point; + CHECK(shadowMapValidity(everything()).packedMask() == all); + CHECK((cascades & worldOnly & self & spot & point) == 0); + CHECK(cascades + worldOnly + self + spot + point == all); +}