From 1f15124271681e7d362324375b5e667126f1668a Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 23 Aug 2026 04:19:05 +0200 Subject: [PATCH] test(render): foliage-wind receiver fallback guard asserts branch shape; cascade-0 publish covered (Campaign VM VM6 review 5 nits) Round-5 narrow re-review of 754d59d9: APPROVE (verified to the SPIR-V disassembly; shadows-on path proven opcode-identical). Four non-blocking nits. N1 (test robustness): T3 (ReceiverVertexShadersFallBackToThePlainLightDirectionWhenTheShadowFlagIsClear) was satisfied by comment prose -- round 5's own explanatory comments quote the exact plain-pipeline direction substring ("-uLights[i].dirAndRange.xyz, ... matched bit-for-bit"), so the substring-only assertion passed even with the code mutated. Fixed: comment lines are now stripped (new StripLineComments helper) before any assertion, and a new ordered regex per shader asserts the exact branch SHAPE -- shadowGatedOff ? -uLights[...] : normalize(uShadow...) -- not just substring presence. Mutation-tested locally against the rewritten test: (1) swapping the ternary's true/false operands -- FAILED (previously passed); (2) deleting the fallback entirely, collapsing to the pre-round-5 buggy expression -- FAILED (previously passed). Original file restored and reverified passing after each mutation. N2 (doc accuracy): "numerically the plain pipeline" overstated the round-5 fix in three places (plan doc, mesh_atmospheric.vert, terrain_atmospheric.vert). The direction expression is bit-for-bit; the SUM is not, because the atmospheric shaders' split ambient+point vs directional accumulation (and terrain's two varyings vs the plain pipeline's one) reassociates float summation order by ~1 ulp -- which is exactly the measured mean |Delta| 0.007 the coordinator's own pixel-proof evidence already recorded (well under the 65 px noise floor). All three rewritten to say the receiver "matches the plain pipeline to within float summation-order rounding (measured mean |Delta| 0.007 on the offline scene)." N3 (coverage): RenderPrepared's own cascadeCount == 0 exit (the F2 fix) had no direct test even though RenderPrepared already has 8 direct call sites in this file. Added one: ResidentMaximumReachMeters at/below CameraNearMeters, passed straight to RenderPrepared with an otherwise-fully-valid environment (so the fitter, not the environment gate, is what returns zero cascades), asserting IsBindableFor true / IsValidFor false. N4 (latent): RenderPrepared's OWN "if (!environment.ShouldRender)" exit is a fourth bufferless-disabled path -- unreachable via Render (whose own gate already validates ShouldRender first) but the same shape, and RenderPrepared is called directly by tests and any future caller. Took the preferred fix: publishes the disabled binding there too, via the same helper, so every exit on a frame that draws the world publishes when the pack's AtmosphericFrame is bound. Also made EvaluateGateAndPublishDisabledBinding self-contained: it now resets _currentFrameBinding to Disabled on its own entry instead of relying on Render having done so first (idempotent with Render's own reset). Regenerated SPIR-V: mesh_atmospheric.vert and terrain_atmospheric.vert are comment-only changes (N2), so only the manifest's source hashes changed -- compiled .spv bytes are unchanged, consistent with round 3's precedent for comment-only shader edits. Verify: Release build 0 warnings/0 errors. App hermetic-lane filter 6,055/0 failed. Full hermetic-filtered solution: 15,283/0 failed across 15 projects. Co-Authored-By: Claude Sonnet 5 --- .../plans/2026-08-22-visualmaster-campaign.md | 9 +- .../Rendering/DirectionalSunShadowRenderer.cs | 19 ++++- .../Rendering/Shaders/mesh_atmospheric.vert | 9 +- .../Shaders/spv/shaders.manifest.json | 4 +- .../Shaders/terrain_atmospheric.vert | 7 +- .../Rendering/DirectionalShadowGpuTests.cs | 47 +++++++++++ .../Packs/AtmosphericPostProcessGraphTests.cs | 84 +++++++++++++++---- 7 files changed, 154 insertions(+), 25 deletions(-) diff --git a/docs/plans/2026-08-22-visualmaster-campaign.md b/docs/plans/2026-08-22-visualmaster-campaign.md index a44290e0..a8973f0d 100644 --- a/docs/plans/2026-08-22-visualmaster-campaign.md +++ b/docs/plans/2026-08-22-visualmaster-campaign.md @@ -721,7 +721,14 @@ plain-pipeline expression (`-uLights[i].dirAndRange.xyz` in `terrain_modern.vert`) instead of the shadow block's direction. Corrected statement: with the flag bit clear the receiver shaders take visibility 1.0 AND fall back to the authored `uLights` direction (round 5), so a -gated-off frame is numerically the plain pipeline. Wind is unaffected — +gated-off frame matches the plain pipeline to within float summation- +order rounding (review fix round 6, N2: "numerically the plain pipeline" +overstated it — the direction expression is bit-for-bit, but the SUM is +not, because the atmospheric fragment's split ambient+point vs +directional accumulation reassociates the terms relative to the plain +pipeline's single varying, and terrain's two separate varyings vs the +plain pipeline's one do the same; measured mean |Δ| 0.007 on the offline +scene, well under the apparatus's 65 px floor). Wind is unaffected — `foliage_wind.glsl` reads only the AtmosphericFrame half of set 3, never the shadow block. diff --git a/src/AcDream.App/Rendering/DirectionalSunShadowRenderer.cs b/src/AcDream.App/Rendering/DirectionalSunShadowRenderer.cs index f2691404..035ffa9f 100644 --- a/src/AcDream.App/Rendering/DirectionalSunShadowRenderer.cs +++ b/src/AcDream.App/Rendering/DirectionalSunShadowRenderer.cs @@ -329,7 +329,12 @@ internal sealed class DirectionalSunShadowRenderer : IDirectionalShadowReceiverS /// are the validated values /// Render's remaining stages use). Behaviour-preserving: identical /// order, identical PublishDisabledReceiverBinding calls, identical - /// Disabled(...) construction as before the extraction. + /// Disabled(...) construction as before the extraction. Round 6 (N4): + /// self-contained — resets itself + /// on entry instead of relying on the caller to have done it first, + /// so a future direct caller of this method (not just Render) cannot + /// observe a stale binding from a previous frame. Idempotent with + /// Render's own reset immediately before calling this. /// internal DirectionalSunShadowDiagnostics? EvaluateGateAndPublishDisabledBinding( IGpuFrame frame, @@ -337,6 +342,7 @@ internal sealed class DirectionalSunShadowRenderer : IDirectionalShadowReceiverS out DirectionalShadowEnvironmentState environment, out long environmentGateTicks) { + _currentFrameBinding = DirectionalShadowFrameBinding.Disabled; long cpuStageStarted = input.MeasureCpuStages ? Stopwatch.GetTimestamp() : 0L; environment = DirectionalShadowEnvironmentGate.Evaluate( input.Environment, @@ -636,7 +642,18 @@ internal sealed class DirectionalSunShadowRenderer : IDirectionalShadowReceiverS ArgumentNullException.ThrowIfNull(worldDraws); ArgumentNullException.ThrowIfNull(terrainDraws); if (!environment.ShouldRender) + { + // Campaign VM VM6 review fix round 6 (N4): a fourth bufferless- + // disabled exit, same shape as Render's own two and the + // cascadeCount == 0 exit below (F2). Unreachable via Render + // itself (its own gate already validated ShouldRender before + // calling here), but RenderPrepared is also called directly + // (tests, and any future caller) — one rule covers every exit + // on a frame that draws the world: publish when the pack's + // AtmosphericFrame is bound. + PublishDisabledReceiverBinding(frame, atmosphericFrame); return Disabled(in environment, cpuStages); + } if (!worldDraws.Commands.IsEmpty && worldGeometry is null) throw new ArgumentNullException(nameof(worldGeometry)); if (!terrainDraws.Commands.IsEmpty && terrainGeometry is null) diff --git a/src/AcDream.App/Rendering/Shaders/mesh_atmospheric.vert b/src/AcDream.App/Rendering/Shaders/mesh_atmospheric.vert index 967b40ae..cec1ba8c 100644 --- a/src/AcDream.App/Rendering/Shaders/mesh_atmospheric.vert +++ b/src/AcDream.App/Rendering/Shaders/mesh_atmospheric.vert @@ -278,8 +278,13 @@ vec3 accumulateAmbientLocalLights( // 0, indoors, portal cover) from straight overhead. Fall back // to the EXACT plain mesh_modern.vert expression // (-uLights[i].dirAndRange.xyz, unnormalized — matched - // bit-for-bit) so a gated-off frame is numerically the plain - // pipeline. Hoisted out of the loop: a uniform branch, + // bit-for-bit) so a gated-off frame matches the plain + // pipeline to within float summation-order rounding + // (review fix round 6, N2: the atmospheric fragment's split + // ambient+point vs directional accumulation reassociates the + // sum relative to the plain pipeline's single varying — + // measured mean |Δ| 0.007 on the offline scene, not bit- + // identical). Hoisted out of the loop: a uniform branch, // evaluated once per vertex regardless of light count. bool shadowGatedOff = (uShadowTextureAndFlags.w & 1u) == 0u; int activeLights = int(uCellAmbient.w); diff --git a/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json b/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json index c8cd1094..e0c93c12 100644 --- a/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json +++ b/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json @@ -215,7 +215,7 @@ "stages": [ { "stage": "vert", - "sourceSha256": "a6b7aa2b0c1c38f92e4b35e76a58acdf5adc916f6ed0cebf94c8bea3f8843666", + "sourceSha256": "c03ba560fbc2c7ce12e67326dcc79114f740999423fade53cd2d18804bf8f81a", "compiled": true }, { @@ -327,7 +327,7 @@ "stages": [ { "stage": "vert", - "sourceSha256": "8cc199b3c6582831e5c9614e19afdff4f0f5fcdb8d96271c8cb3f427e29c68c9", + "sourceSha256": "882979f63c858760e977ecafb708416ab01f7adabe4bb38c0630020c087f1c28", "compiled": true }, { diff --git a/src/AcDream.App/Rendering/Shaders/terrain_atmospheric.vert b/src/AcDream.App/Rendering/Shaders/terrain_atmospheric.vert index a4399b9c..706a085b 100644 --- a/src/AcDream.App/Rendering/Shaders/terrain_atmospheric.vert +++ b/src/AcDream.App/Rendering/Shaders/terrain_atmospheric.vert @@ -165,8 +165,11 @@ void main() { // carries only a (0,0,1) NaN guard, NOT a real light direction. Fall // back to the EXACT plain terrain_modern.vert expression // (-uLights[0].dirAndRange.xyz, unnormalized — matched bit-for-bit) - // so a gated-off frame is numerically the plain pipeline — see the - // matching comment in mesh_atmospheric.vert's accumulateLights. + // so a gated-off frame matches the plain pipeline to within float + // summation-order rounding (review fix round 6, N2: terrain's two + // separate varyings vs the plain pipeline's one reassociate the sum + // — measured mean |Δ| 0.007 on the offline scene, not bit-identical) + // — see the matching comment in mesh_atmospheric.vert's accumulateLights. bool shadowGatedOff = (uShadowTextureAndFlags.w & 1u) == 0u; vec3 surfaceToLight = shadowGatedOff ? -uLights[0].dirAndRange.xyz diff --git a/tests/AcDream.App.Tests/Rendering/DirectionalShadowGpuTests.cs b/tests/AcDream.App.Tests/Rendering/DirectionalShadowGpuTests.cs index e99b0f34..be4bd238 100644 --- a/tests/AcDream.App.Tests/Rendering/DirectionalShadowGpuTests.cs +++ b/tests/AcDream.App.Tests/Rendering/DirectionalShadowGpuTests.cs @@ -756,6 +756,53 @@ public sealed class DirectionalShadowGpuTests bindingValid)); } + [Fact] + public void RenderPrepared_CascadeFitterReturningZeroCascadesPublishesBindableNotValidBinding() + { + // Campaign VM VM6 review fix round 5 (F2), coverage added round 6 + // (N3): RenderPrepared's OWN cascadeCount == 0 exit is reachable + // from a frame that already passed the environment gate + // (ShouldRender true) but whose cascade fitter still finds zero + // usable cascades — DirectionalShadowCascadeFitter.Fit returns 0 + // whenever min(Quality.MaximumReachMeters, + // ResidentMaximumReachMeters) <= CameraNearMeters, the same + // effective constraint Render's own early-out checks, but + // reached HERE via the fitter's own arithmetic on a frame that + // got PAST that early-out (this test drives RenderPrepared + // directly, bypassing Render's gate entirely, to isolate the + // fitter's own zero-cascade path). Proves it publishes the same + // disabled receiver binding Render's two early-outs do. + using var device = new RecordingGpuDevice(); + using var renderer = new DirectionalSunShadowRenderer(device, DirectionalShadowPreset.Low); + using IGpuBuffer atmosphericBuffer = Buffer(device, "cascade-zero-frame", GpuBufferUsage.Uniform); + var atmosphericFrame = new AtmosphericFrameBufferBinding(atmosphericBuffer, 0u, 192u); + device.Clear(); + using IGpuFrame frame = device.BeginFrame(); + WorldTransformFrameSlice transforms = PublishSharedTransforms( + frame, + ReadOnlySpan.Empty); + + DirectionalSunShadowDiagnostics diagnostics = renderer.RenderPrepared( + frame, + EnabledEnvironment(), + Matrix4x4.Identity, + Matrix4x4.CreatePerspectiveFieldOfView(1f, 1f, 0.1f, 500f), + 0.1f, + 48f, + new DirectionalShadowPreparedDraws(), + new DirectionalShadowTerrainPreparedDraws(), + null, + null, + transforms, + residentMaximumReachMeters: 0.05f, // <= cameraNearMeters (0.1f): the fitter returns 0 + atmosphericFrame: atmosphericFrame); + + Assert.Equal(DirectionalShadowGateReason.ResidentWindowUnavailable, diagnostics.GateReason); + Assert.True(renderer.TryGetCurrentFrameBinding(frame, out DirectionalShadowFrameBinding binding)); + Assert.True(binding.IsBindableFor(frame)); + Assert.False(binding.IsValidFor(frame)); + } + [Fact] public void BindDirectionalShadowReceiver_WithADisabledBindingEmitsBothShadowAndAtmosphericBinds() { diff --git a/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs b/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs index 0c8293ea..776abd6c 100644 --- a/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs @@ -1,6 +1,7 @@ using System.Globalization; using System.Numerics; using System.Runtime.InteropServices; +using System.Text.RegularExpressions; using AcDream.App.Plugins; using AcDream.App.Rendering; using AcDream.App.Rendering.Gpu; @@ -1225,28 +1226,38 @@ public sealed class AtmosphericPostProcessGraphTests [Fact] public void ReceiverVertexShadersFallBackToThePlainLightDirectionWhenTheShadowFlagIsClear() { - // Campaign VM VM6 review fix round 5 (F1 BLOCKER, test T3): pins - // the shader SOURCE TEXT so a future edit that removes the - // flag-gated fallback (reintroducing "a shadow-gated-off frame - // lights outdoor terrain/objects from the disabled block's - // (0,0,1) placeholder instead of the authored sun") fails this - // test instead of only showing up in a pixel capture. Both - // receiver vertex shaders must (1) test the SAME flags bit - // acdreamDirectionalShadowVisibility already tests, and (2) - // reference the SAME uLights-derived expression the corresponding - // plain (mesh_modern.vert / terrain_modern.vert) pipeline uses, - // proving the fallback branch is not some other, unverified - // direction. + // Campaign VM VM6 review fix round 5 (F1 BLOCKER, test T3), hardened + // round 6 (N1): the original version of this test asserted only + // that the plain-pipeline direction SUBSTRING appeared somewhere + // in the receiver file — but the round-5 fix's own explanatory + // comments ALSO quote that exact substring in prose ("(-uLights[i] + // .dirAndRange.xyz, ... matched bit-for-bit)"), so the check was + // satisfied by the COMMENT even with the CODE mutated. Mutation- + // tested locally against this rewritten test: (1) swapping the + // ternary's true/false operands — PASSED the old substring-only + // test, FAILS this one; (2) deleting the fallback entirely + // (always taking the shadow-block direction, i.e. collapsing the + // ternary to just its false arm) — PASSED the old test, FAILS + // this one. Comment lines are stripped before matching, and the + // assertion is the exact branch SHAPE — the gated-off condition, + // then the plain uLights expression on the TRUE arm, then + // uShadowLightDirectionAndSource on the FALSE arm — via an + // ordered regex, not a substring search that a comment can also + // satisfy. string shaderRoot = Path.Combine( RepositoryRoot(), "src", "AcDream.App", "Rendering", "Shaders"); - string meshAtmospheric = File.ReadAllText(Path.Combine(shaderRoot, "mesh_atmospheric.vert")); - string meshModern = File.ReadAllText(Path.Combine(shaderRoot, "mesh_modern.vert")); - string terrainAtmospheric = File.ReadAllText(Path.Combine(shaderRoot, "terrain_atmospheric.vert")); - string terrainModern = File.ReadAllText(Path.Combine(shaderRoot, "terrain_modern.vert")); + string meshAtmospheric = StripLineComments( + File.ReadAllText(Path.Combine(shaderRoot, "mesh_atmospheric.vert"))); + string meshModern = StripLineComments( + File.ReadAllText(Path.Combine(shaderRoot, "mesh_modern.vert"))); + string terrainAtmospheric = StripLineComments( + File.ReadAllText(Path.Combine(shaderRoot, "terrain_atmospheric.vert"))); + string terrainModern = StripLineComments( + File.ReadAllText(Path.Combine(shaderRoot, "terrain_modern.vert"))); const string flagTest = "(uShadowTextureAndFlags.w & 1u) == 0u"; Assert.Contains(flagTest, meshAtmospheric, StringComparison.Ordinal); @@ -1254,7 +1265,8 @@ public sealed class AtmosphericPostProcessGraphTests // mesh: both files negate the per-light uniform inline (no // intermediate variable), so the literal expression matches - // string-for-string. + // string-for-string once comments can no longer supply a false + // positive. const string meshPlainDirection = "-uLights[i].dirAndRange.xyz"; Assert.Contains(meshPlainDirection, meshModern, StringComparison.Ordinal); Assert.Contains(meshPlainDirection, meshAtmospheric, StringComparison.Ordinal); @@ -1269,8 +1281,46 @@ public sealed class AtmosphericPostProcessGraphTests "-" + terrainLightUniform, terrainAtmospheric, StringComparison.Ordinal); + + // Branch-shape guard (N1): fails on a swapped ternary or a + // deleted fallback even though the substrings above would still + // be present somewhere in the (comment-stripped) file. + var meshBranchShape = new Regex( + @"shadowGatedOff\s*\?\s*-uLights\[i\]\.dirAndRange\.xyz\s*:\s*normalize\(uShadowLightDirectionAndSource\.xyz\)", + RegexOptions.Singleline); + Assert.True( + meshBranchShape.IsMatch(meshAtmospheric), + "mesh_atmospheric.vert must branch exactly: " + + "shadowGatedOff ? -uLights[i].dirAndRange.xyz : normalize(uShadowLightDirectionAndSource.xyz)"); + + var terrainBranchShape = new Regex( + @"shadowGatedOff\s*\?\s*-uLights\[0\]\.dirAndRange\.xyz\s*:\s*normalize\(uShadowLightDirectionAndSource\.xyz\)", + RegexOptions.Singleline); + Assert.True( + terrainBranchShape.IsMatch(terrainAtmospheric), + "terrain_atmospheric.vert must branch exactly: " + + "shadowGatedOff ? -uLights[0].dirAndRange.xyz : normalize(uShadowLightDirectionAndSource.xyz)"); } + /// + /// Campaign VM VM6 review fix round 6 (N1): removes everything from + /// the first // on each line onward. GLSL has no string + /// literals in these files, so a per-line truncation is safe and + /// cannot itself corrupt real code — it exists specifically so a + /// shader-text assertion cannot be satisfied by a comment that quotes + /// the expected code as prose instead of by the code itself. + /// + private static string StripLineComments(string glsl) + { + string[] lines = glsl.Split('\n'); + for (int i = 0; i < lines.Length; i++) + { + int index = lines[i].IndexOf("//", StringComparison.Ordinal); + if (index >= 0) + lines[i] = lines[i][..index]; + } + return string.Join('\n', lines); + } [Fact] public void FirstAdvanceSnapsExactlyToTheWeatherTargetInsteadOfEasingFromZero() {