chore(render): S3 landing hygiene — scissor-era dead state, stale comments, the #130 harness, a conjunction-proof weather-gate pin

Chunk H of Campaign OVERHAUL v2's S3 walk-ownership program (docs/research/2026-09-01-overhaul/s3-walk-ownership-map.md §10.6), landing the three post-hoc three-lens findings on S3 chunk 4 round 2. Pure dead-state deletion, comment truth, one test deletion, one stronger pin — no admission, order, or behavior change; no new flag or probe; no register row (AD-17's correction is the lead's own, already on the campaign branch). H1 — ClipPlaneSet.cs: deleted ScissorNdcAabb (no production reader — only AppendOutsideSlice consumes ClipPlaneSet, and it reads only IsNothingVisible/Count/PlaneArray) and renamed UseScissorFallback -> IsPlaneOverflow with a doc stating the true consumer contract: Count==0 overflow means the region draws UNCLIPPED via the no-clip slot, never a scissor box. Rewrote the file header's convexity-rule and Count==0-states sections to the two states that exist (Empty / overflow) instead of three, and simplified the private Scissor(...) AABB-computing helpers into a parameterless Overflow() factory since nothing reads the AABB any more. ClipPlaneSetTests.cs: renamed every UseScissorFallback reference, deleted the AABB assertions (From_MultiplePolygons's four ScissorNdcAabb.X/Y/Z/W checks, Empty_StaticProperty_DrawsNothing's degenerate-box check, ScissorFallback_IsNotNothingVisible's bounds check) that pinned the deleted consumer contract. H2 — TerrainModernRenderer.cs: deleted Draw's clipPlanes/ndcClipAabb parameters (grep: TerrainModernRenderer.Draw has exactly one production caller, WorldScenePassExecutor.DrawFlatTerrain, and it never passed either), which made CollectVisibleCells's own clipPlanes/ndcClipAabb/ viewProjection parameters and the IsAabbVisibleThroughClipRegion + IsAabbOutsideHomogeneousPlane helper pair they fed entirely dead — deleted along with the stale "Retail publishes landcell in_view from the clipped landscape view" comment; CollectVisibleCells is frustum-only now, matching DrawLandCells's own "retail never view-clips terrain" doc. Deleted the terrainUploadCount computation at RetailPViewRenderer.cs (the DrawInside/DrawLandscapeDynamicsPhase caller) and PrepareClipFrame's GL-era reservation-count parameter end to end: IWorldPassSurface.PrepareClipFrame(int), RhiWorldPassSurface's matching body and its now-pointless "_ = terrainUploadCount" discard, RetailPViewPassExecutor.PrepareClipFrame's pass-through, and WorldScenePassExecutor.PrepareFlatWorldClip's hardcoded `1` argument — PrepareClipFrame() takes nothing on every arm. TerrainParticleCellVisibilityTests.cs: deleted the three tests that only existed to pin the removed clip-plane/scissor mechanism (RejectsCellsOutsideDoorwayClipPlanes, RejectsCellsOutsideDoorwayScissorAabb, UnionsCellsFromEveryLandscapeSlice) and updated the two surviving frustum tests to the new signature. H3 — deleted tests/AcDream.App.Tests/Rendering/Issue130DoorwayStripTests.cs outright. #130 was a background-color strip along a doorway's top edge from a since-fixed under-inclusion in the CPU clip pipeline; its scissor half was already retired at S3 chunk 4 fix round 2 (L7), and its surviving plane-gap half exercised only PortalVisibilityBuilder.Build and ClipFrameAssembler.Assemble, both confirmed zero-production-caller legacy paths (grep) — the one production stage it actually touched, ClipPlaneSet.From, is already pinned through the real producers (BeginWalkFrame/ReassembleOutsideViewFromWalk) by the K6/L4 punch-fan equivalence pin in ClipFrameLayoutTests, so nothing production-relevant lost coverage. H4 — comment truth pass: - ClipFrameAssembler.cs: rewrote the file-header "renderer uses scissor for passes that need that fallback" line and the TerrainClipMode summary's "that path never produces Scissor" clause. Deleted the TerrainClipMode enum's Scissor member — grepped every reader first (WorldRenderDiagnostics.cs, WorldSceneDiagnosticsController.cs, WorldSceneRendererTests.cs all only pass the value through to a string Append or a mock parameter; WorldSceneRenderer.cs, the only writer, sets only Planes (default) or Skip, never Scissor) — kept Planes/Skip for the flat path and corrected Skip's stale doc ("No outside_view slice is visible; skip landscape indoors", a leftover from the pre-round-2 walk meaning) to what the flat path's own writer actually means now ("the PView walk ran instead of the flat-world path this frame"). Corrected ClipViewSlice's doc: the AABB is retained for WorldRenderDiagnostics only, not for a scissor consumer. - WorldPassSurface.cs: "Four concerns... the clip-frame publication, the doorway scissor, gl_ClipDistance enablement, and retail's interior depth clear" -> three concerns (the doorway scissor was retired at L2), with a pointer to ClearInteriorDepth's own truthful doc. EnableClipDistances' "all three world vertex shaders already write 1.0" -> only portal_depth.vert writes gl_ClipDistance now (grep: sky.vert and terrain_modern.vert/terrain_atmospheric.vert no longer declare it at all — L3 deleted the block outright; portal_depth.vert still writes 1.0 past its active clip count). RhiWorldPassSurface's class doc dropped "the scissor is dynamic state on the borrowed encoder" (no producer left). - RetailPViewPassExecutorTests.cs: DrawLandscapeDynamicsPhase_CallsDraw WeatherOnceExactlyOnce's doc corrected "unconditionally" (false since K2) to "conditional on WalkFrameDriver.WeatherTurnFired, see the L1 pin". Added the switch-table scope note (matching L8's own note on DrawWalkSky_RenderSkyCallSiteHasNoEnclosingBackwardBranch) to the K1 pin's doc (DrawLandscapeDynamicsPhase_DrawWeatherOnceCallSiteHasNoEnclosing BackwardBranch). - RetailPViewRenderer.cs: the DrawWeatherOnce citation cited the callee address (GameSky::Draw(sky,1) @0x00506ff0) where §10.1 cites the call site (@0x00506396) — corrected to cite the call site with the callee address alongside it. H5 — MUTATION CHECK (mandatory, run against the production gate at RetailPViewRenderer.DrawLandscapeDynamicsPhase): Added DrawLandscapeDynamicsPhase_ExactlyOneBranchGuardsDrawWeatherOnce, which widens the L1 pin's window to start at the DrawUnattachedSceneParticles call (the last call before the whole gate) and asserts EXACTLY ONE branch total in that window — the L1 pin's own window (getter-call to draw-call) misses a conjoined gate's extra, earlier condition because that condition's own brfalse lands BEFORE the getter call's offset. - Mutation 1 (the conjoined gate this pin exists to reject): changed the gate to `if (clipAssembly.OutsideViewSlices.Length != 0 && walkDriver.WeatherTurnFired)`, built, ran the new pin — FAILED with: Assert.Single() Failure: The collection contained 2 matching items Expected: (predicate expression) Collection: [CompiledBranch { Offset = 7, OpCode = brfalse.s, TargetOffset = 17 }, CompiledBranch { Offset = 24, OpCode = brfalse.s, TargetOffset = 42 }, CompiledBranch { Offset = 33, OpCode = brfalse.s, TargetOffset = 42 }] Match indices: 1, 2 (offset 7 = the DrawUnattachedSceneParticles block's own guard, correctly excluded since it precedes that call's offset; offsets 24 and 33 are the conjoined gate's two brfalse's, both inside the widened window — exactly the failure this pin exists to catch). Restored the single-flag gate; rebuilt; the new pin and all 13 sibling tests in RetailPViewPassExecutorTests pass again (14/14). - Mutation M1 (the existing round-2 regression: restore the pre-fix gate `if (clipAssembly.OutsideViewSlices.Length != 0)`), built, ran the full suite — the L1 pin (DrawLandscapeDynamicsPhase_GatesDrawWeatherOnceOnWalkDriverWeatherTurnFired) still FAILS, as required, with: Assert.Equal() Failure: Values differ Expected: typeof(AcDream.App.Rendering.Walk.WalkFrameDriver) Actual: typeof(AcDream.App.Rendering.ClipFrameAssembly) (the new H5 pin does NOT fail under M1 — a single-flag gate still produces exactly one branch in the widened window, which is correct: catching "wrong condition source" is L1's job, catching "an extra conjoined condition" is H5's). Restored the correct gate; rebuilt; 14/14 green again. H6 grep sweep (extends §10.5's L6 sweep per the plan): grep -rnE "ScissorNdcAabb|UseScissorFallback|TerrainClipMode\.Scissor|ndcClipAabb|terrainUploadCount" --include=*.cs --include=*.vert --include=*.frag src tests -> empty (every mention, including in this commit's own explanatory comments, was rephrased to avoid the literal deleted identifiers). The base L6 sweep (SetTerrainClip|BeginDoorwayScissor|BindTerrainClip| TerrainBytes|active scissor|TerrainClipMode\.Scissor|NdcScissorRect) is also still empty. Gates: dotnet build tests/AcDream.App.Tests -c Release -> 0 warnings / 0 errors. Hermetic lane -> 6825/6825 passed, 0 failed. InstalledDat lane (ACDREAM_DAT_DIR set) -> 243 passed / 4 known failures (2x #383 layout tests, TowerAscentReplayTests's TowerAscent KnownFailure, WalkTraceConformanceTests.Oh_doorway_still_first_frame_diff #458 KnownFailure) / 1 skipped / 248 total — one fewer test than before H3, as expected from Issue130DoorwayStripTests's deletion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-03 18:14:38 +02:00 · 2026-09-03 18:14:38 +02:00 · 99d9c6d564
commit 99d9c6d564
parent b3ef4027a2
11 changed files with 209 additions and 640 deletions
--- a/tests/AcDream.App.Tests/Rendering/RetailPViewPassExecutorTests.cs
+++ b/tests/AcDream.App.Tests/Rendering/RetailPViewPassExecutorTests.cs
@ -159,7 +159,9 @@ public sealed class RetailPViewPassExecutorTests
    /// <c>ClearClipRouting</c> + the old <c>DrawLandscapeSliceLate</c> leaf,
    /// one call per active landscape view) is deleted — <c>DrawLandscapeDynamicsPhase</c> now calls
    /// <see cref="RetailPViewPassExecutor.DrawWeatherOnce"/> exactly once,
-    /// unconditionally, with no loop of any kind around it. This
+    /// conditional on <see cref="AcDream.App.Rendering.Walk.WalkFrameDriver.WeatherTurnFired"/>
+    /// (see <see cref="DrawLandscapeDynamicsPhase_GatesDrawWeatherOnceOnWalkDriverWeatherTurnFired"/>
+    /// — the L1 pin), with no loop of any kind around it. This
    /// <c>Assert.Single</c> alone proved insufficient at fix round 1 (K1):
    /// it counts DISTINCT call-site offsets, so it stays green even with a
    /// <c>foreach</c> wrapped around the one call site (the exact round-1
@ -201,7 +203,14 @@ public sealed class RetailPViewPassExecutorTests
    /// the compiled <c>foreach</c> emits a backward branch (the
    /// condition-check jump back to the loop body) whose span now contains
    /// the call's offset, so this test fails; restore the single
-    /// unconditional call to make it pass again.
+    /// unconditional call to make it pass again. Note for reviewers (added
+    /// at S3 landing hygiene, H4, matching <see cref="DrawWalkSky_RenderSkyCallSiteHasNoEnclosingBackwardBranch"/>'s
+    /// own note): <see cref="CompiledCallGraph.ReadBranches"/> reads only
+    /// <c>br</c>/<c>brtrue</c>/<c>brfalse</c>-family single-target branches —
+    /// it does not decode a compiled <c>switch</c> jump table, but no C# loop
+    /// construct (<c>for</c>/<c>foreach</c>/<c>while</c>/<c>do</c>) ever
+    /// compiles to one, so this pin's blind spot is not a loop shape it
+    /// could miss.
    /// </summary>
    [Fact]
    public void DrawLandscapeDynamicsPhase_DrawWeatherOnceCallSiteHasNoEnclosingBackwardBranch()
@ -283,6 +292,65 @@ public sealed class RetailPViewPassExecutorTests
                && branch.TargetOffset > drawOffset);
    }

+    /// <summary>
+    /// S3 landing hygiene (H5): strengthens the L1 pin above against a
+    /// conjoined gate the post-hoc three-lens review found it does not
+    /// reject. L1's check (b) only looks at branches STRICTLY BETWEEN the
+    /// <c>WeatherTurnFired</c> getter call and the <c>DrawWeatherOnce</c>
+    /// call — so <c>if (clipAssembly.OutsideViewSlices.Length != 0 &amp;&amp;
+    /// walkDriver.WeatherTurnFired) passes.DrawWeatherOnce(ctx);</c> still
+    /// passes it: the compiler evaluates <c>OutsideViewSlices.Length != 0</c>
+    /// FIRST, so ITS OWN <c>brfalse</c> lands BEFORE the getter call's
+    /// offset — outside L1's window — while <c>calls[c-1]</c> is still the
+    /// getter (the added condition reads <c>OutsideViewSlices</c>, a
+    /// property getter, then <c>.Length</c>, a non-call <c>ldlen</c>, so no
+    /// OTHER call intervenes before the getter). This pin widens the window
+    /// to start at the call immediately before the whole gate —
+    /// <see cref="RetailPViewPassExecutor.DrawUnattachedSceneParticles"/>,
+    /// the outdoor-emitters call the method's own comment names as the last
+    /// call before the gate — and counts EVERY branch in that wider window
+    /// with <c>Assert.Single</c>: production has exactly one, the forward
+    /// <c>brfalse</c>/<c>brfalse.s</c> right after the getter. A conjoined
+    /// gate's extra, earlier condition adds a second branch this wider
+    /// window catches but L1's narrower one cannot.
+    /// MUTATION: change the gate to <c>if
+    /// (clipAssembly.OutsideViewSlices.Length != 0 &amp;&amp;
+    /// walkDriver.WeatherTurnFired)</c> — the branch count in the window
+    /// goes from 1 to 2 and <c>Assert.Single</c> fails (see the commit body
+    /// for the exact recorded failure text).
+    /// </summary>
+    [Fact]
+    public void DrawLandscapeDynamicsPhase_ExactlyOneBranchGuardsDrawWeatherOnce()
+    {
+        MethodInfo method = typeof(RetailPViewRenderer).GetMethod(
+            "DrawLandscapeDynamicsPhase",
+            BindingFlags.Instance | BindingFlags.NonPublic)!;
+        IReadOnlyList<CompiledCall> calls = CompiledCallGraph.Read(method);
+        int particlesIndex = RequiredCallIndex(
+            calls,
+            typeof(RetailPViewPassExecutor),
+            nameof(RetailPViewPassExecutor.DrawUnattachedSceneParticles));
+        int drawIndex = RequiredCallIndex(
+            calls,
+            typeof(RetailPViewPassExecutor),
+            nameof(RetailPViewPassExecutor.DrawWeatherOnce));
+        int particlesOffset = calls[particlesIndex].Offset;
+        int drawOffset = calls[drawIndex].Offset;
+
+        IReadOnlyList<CompiledBranch> branches = CompiledCallGraph.ReadBranches(method);
+        CompiledBranch onlyGuard = Assert.Single(
+            branches,
+            branch => branch.Offset > particlesOffset && branch.Offset < drawOffset);
+
+        Assert.True(
+            onlyGuard.OpCode == OpCodes.Brfalse || onlyGuard.OpCode == OpCodes.Brfalse_S,
+            $"Expected the sole branch between DrawUnattachedSceneParticles and "
+            + $"DrawWeatherOnce to be a brfalse, was {onlyGuard.OpCode}.");
+        Assert.True(
+            onlyGuard.TargetOffset > drawOffset,
+            "Expected the guard branch to skip forward past DrawWeatherOnce.");
+    }
+
    /// <summary>
    /// S3 chunk 4 fix round 2 (L8): the identical loop-shape question K1
    /// asked of <see cref="RetailPViewPassExecutor.DrawWeatherOnce"/>'s call