diff --git a/CLAUDE.md b/CLAUDE.md index 27c2ec1..f9ba939 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 ebad6e0..ee587a6 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 475697b..6bfe024 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 0000000..8d5126e --- /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 c4592fd..6921103 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 0000000..147d6dc --- /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 1719c5e..7feb843 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 431c63b..355c353 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 0183ef6..65e8ee9 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 dc5e503..1588f35 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 c7f3fb1..3068fb2 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 7448558..0f84344 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 389afc2..be71bd7 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 0000000..f8f6be3 --- /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 9f5aef8..e44422c 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 2239d18..e283a1b 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 155264c..a70d48a 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 0000000..7147d4a --- /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 e98144b..1b87d36 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 621d3a6..fbec2fd 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 4b699f2..3a20574 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 b7a90e0..3c4a3a9 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 42771a4..36fbf1d 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 d47f814..044bff6 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 fe8364a..b4d018c 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 e6cdf45..b931ac3 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 c4ed241..f0f51d6 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 0000000..1d53601 --- /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 e9f13bc..9930056 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 d0dde67..5dcb6a2 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 0000000..0e20696 --- /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); +}