erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions 1

fix(render): S3 chunk 1 round 2 — one weather OC per frame (pinned), literal EC/OC comparison, comment/cost/wording items

Browse source 
Campaign OVERHAUL S3 chunk 1 fix round 2 (docs/research/2026-09-01-overhaul/
s3-walk-ownership-map.md §11.6), applied on top of 36be6b598 after the
three-lens re-review. H1-H6, the last round the plan allows.

H1 — the weather OC printed 2-4 times per interior-rooted frame instead of
once: the print (and the `_sky.RenderWeather` mesh draw it sits beside) lived
inside `DrawLandscapeSliceLate`, which `RetailPViewRenderer` calls once per
active `OutsideViewSlices` entry. Moved both into a new
`RetailPViewPassExecutor.DrawWeatherOnce`, called ONCE, unclipped/no-scissor,
after the slice loop in `DrawLandscapeDynamicsPhase` — retail's own
`GameSky::Draw(1)` runs once, after `LScape::draw`'s whole landblock loop.
The gate is extracted as a pure `ShouldDrawWeatherOnce(bool,bool,uint)`
predicate (retail's `SmartBox::is_player_outside` ANDed with the two render
toggles) so `RetailPViewPassExecutorTests` can pin "no OC while the player
stands indoors" without a live GL/DAT `SkyRenderer`; two structural
(CompiledCallGraph) tests prove the call moved out of the per-slice loop and
that the mesh draw + print each fire exactly once per invocation — this
codebase has no existing runtime-construction fixture for
`RetailPViewPassExecutor`, so the pin is structural + a testable pure gate
rather than an end-to-end GL drive. One deliberate deviation from the literal
"gated exactly as today": the print now runs through `_sky?.RenderWeather(...)`
(null-conditional, matching every other `_sky?.RenderSky(...)` call site in
this codebase) instead of an explicit `if (_sky is not null)` wrapper — a
missing sky asset no longer also suppresses the transcript print, since the
print's only job is trace fidelity and retail's own `GameSky` is never null.

H2 — WalkFrameDriver's EC-print comment still claimed "EC and OC counts are
always exactly equal"; replaced with the real citation
(holtburg-doorway-still.walk.log:1126,1131,1134,1137 — four EC prints for one
cell across four look-in DC turns), matching WalkTranscriptDump.
PrintEnvCellShell's own comment (already corrected in round 1).

H3 — the eight-kind signature's ORACLE side
(WalkTraceReplayContext.Signature8(WalkOracleFrame)) derived EC/OC from each
DC's cell list — the SAME derivation the REPLAY side's Recorder already used,
so the comparison could never disagree with itself on EC/OC placement or
content (how G7's SC-ordering regression shipped green with LC/SC). Now reads
its own literally captured EC/OC events, excluding only the trailing
per-frame weather OC — refined beyond the spec's literal "last event + P-cell
id" rule with an additional "P cell itself looks outdoor" check, after
cathedral-arrival and foundry-deep both proved the naive rule also strips an
interior root's OWN real trailing object-list turn when its nearest cell (the
reversed flood loop's last draw) happens to be the camera's own root cell.
All four kit-pose rows still reproduce: three exactly; #458 (this round's new
issue, formerly an inline-only note) re-verified at the SAME token index 165
under the new literal comparison.

H4 — OnSortCellTurn ran RequireOpenFrame + two range validations before
testing the flag. Since the hook is print-only (no stream side effect, unlike
OnLandCellTurn's unconditional WalkFrameEvent record), the flag check now
runs FIRST and returns immediately when off — flag-off cost drops to one
interface dispatch per visited land cell.

H5 — launch-options.md's ACDREAM_DUMP_WALK_TRANSCRIPT row: measured
≈1,200-1,400 lines per outdoor frame (terrace-edge 1,384; cathedral-arrival
1,269; doorway 1,187), replacing the earlier "600-800" estimate; documents
H4's residual flag-off interface-dispatch cost.

H6 — filed docs/ISSUES.md #458 for round 1's LOD-boundary land-cell
divergence (previously only an inline test comment); the InstalledDat lane's
known-failure set is now four (two #383 layout tests, TowerAscent, #458) —
confirmed by a clean run.

MUTATION CHECKS (both restored after confirming failure):
- H1: deleting the OC print inside DrawWeatherOnce made
  DrawWeatherOnce_DrawsTheWeatherMeshAndPrintsExactlyOnce fail with
  "Assert.Single() Failure: The collection did not contain any matching
  items".
- H3: reversing AppendFloodTurns (OC before EC) made
  Still_fixture_first_frame_reproduces_exactly(cathedral-arrival.walk) fail
  ("walk diverged from retail (cathedral-arrival.walk)"), diverging at token
  index 1241: expected "EC:f4180112" vs actual "OC:f4180112".

Gates: hermetic App suite 6823/6823 passed; InstalledDat lane 244/249 passed
with exactly the four known failures (two #383 layout tests, TowerAscent,

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-09-03 12:14:00 +02:00
parent 9b55d78a28
commit 88c70072e0
8 changed files with 333 additions and 99 deletions
Download patch file Download diff file Expand all files Collapse all files

125
tests/AcDream.App.Tests/Rendering/RetailPViewPassExecutorTests.cs
View file

@ -1,6 +1,8 @@
using System.Reflection;
using AcDream.App.Composition;
using AcDream.App.Rendering;
using AcDream.App.Rendering.Sky;
using AcDream.App.Rendering.Walk;
using AcDream.App.Tests.Architecture;
namespace AcDream.App.Tests.Rendering;
@ -109,6 +111,129 @@ public sealed class RetailPViewPassExecutorTests
&& call.Target.Name == ".ctor");
}
/// <summary>
/// S3 chunk 1 fix round 2 (§11.6 H1): the weather MESH draw + its OC
/// print moved OUT of the per-slice loop into
/// <see cref="RetailPViewPassExecutor.DrawWeatherOnce"/>
/// (that method's own doc comment has the full decomp citation).
/// <see cref="RetailPViewPassExecutor.DrawLandscapeSliceLate"/> must no
/// longer call either <see cref="SkyRenderer.RenderWeather"/> or
/// <see cref="WalkTranscriptDump"/>'s OC print — only the per-slice
/// rain PARTICLE emitters (<c>ParticleRenderer.Draw</c>) stay here.
/// MUTATION: re-inlining either call back into this method makes the
/// corresponding <c>Assert.DoesNotContain</c> fail.
/// </summary>
[Fact]
public void DrawLandscapeSliceLate_NoLongerDrawsOrPrintsTheWeatherPass()
{
MethodInfo method = typeof(RetailPViewPassExecutor).GetMethod(
nameof(RetailPViewPassExecutor.DrawLandscapeSliceLate),
BindingFlags.Instance | BindingFlags.Public)!;
IReadOnlyList<CompiledCall> calls = CompiledCallGraph.Read(method);
Assert.DoesNotContain(
calls,
call => call.Target.DeclaringType == typeof(SkyRenderer)
&& call.Target.Name == nameof(SkyRenderer.RenderWeather));
Assert.DoesNotContain(
calls,
call => call.Target.DeclaringType == typeof(WalkTranscriptDump)
&& call.Target.Name == "PrintObjectCellTurn");
}
/// <summary>
/// S3 chunk 1 fix round 2 (§11.6 H1): the new call site itself — exactly
/// one <see cref="SkyRenderer.RenderWeather"/> draw and exactly one OC
/// print per invocation (retail draws the weather pass exactly once per
/// frame; this method is the ONE call site production now uses — see
/// <see cref="DrawLandscapeDynamicsPhase_CallsDrawWeatherOnceExactlyOnceAfterTheSliceLoop"/>).
/// MUTATION: deleting either call makes the matching
/// <c>Assert.Single</c> fail (zero matches instead of one).
/// </summary>
[Fact]
public void DrawWeatherOnce_DrawsTheWeatherMeshAndPrintsExactlyOnce()
{
MethodInfo method = typeof(RetailPViewPassExecutor).GetMethod(
nameof(RetailPViewPassExecutor.DrawWeatherOnce),
BindingFlags.Instance | BindingFlags.Public)!;
IReadOnlyList<CompiledCall> calls = CompiledCallGraph.Read(method);
Assert.Single(
calls,
call => call.Target.DeclaringType == typeof(SkyRenderer)
&& call.Target.Name == nameof(SkyRenderer.RenderWeather));
Assert.Single(
calls,
call => call.Target.DeclaringType == typeof(WalkTranscriptDump)
&& call.Target.Name == "PrintObjectCellTurn");
}
/// <summary>
/// S3 chunk 1 fix round 2 (§11.6 H1): the actual wiring fix — production
/// must call <c>DrawWeatherOnce</c> exactly ONCE, positioned AFTER the
/// (single, in-loop) <c>DrawLandscapeSliceLate</c> call site. A
/// <c>foreach</c> loop compiles to ONE call instruction regardless of
/// how many <c>OutsideViewSlices</c> it iterates at runtime — an
/// interior root's 2-4 exit views would otherwise still print 2-4 OC
/// lines even after <see cref="DrawLandscapeSliceLate_NoLongerDrawsOrPrintsTheWeatherPass"/>
/// passes, if <c>DrawWeatherOnce</c> were called FROM INSIDE that same
/// loop instead of after it. IL offsets (not list position) prove
/// source order for this straight-line method. MUTATION: moving the
/// <c>DrawWeatherOnce</c> call back inside the <c>foreach</c> (or before
/// the loop) makes the offset-ordering assertion fail; adding a second
/// call site makes <c>Assert.Single</c> fail.
/// </summary>
[Fact]
public void DrawLandscapeDynamicsPhase_CallsDrawWeatherOnceExactlyOnceAfterTheSliceLoop()
{
MethodInfo method = typeof(RetailPViewRenderer).GetMethod(
"DrawLandscapeDynamicsPhase",
BindingFlags.Instance | BindingFlags.NonPublic)!;
IReadOnlyList<CompiledCall> calls = CompiledCallGraph.Read(method);
CompiledCall sliceLate = Assert.Single(
calls,
call => call.Target.DeclaringType == typeof(RetailPViewPassExecutor)
&& call.Target.Name == nameof(RetailPViewPassExecutor.DrawLandscapeSliceLate));
CompiledCall weatherOnce = Assert.Single(
calls,
call => call.Target.DeclaringType == typeof(RetailPViewPassExecutor)
&& call.Target.Name == nameof(RetailPViewPassExecutor.DrawWeatherOnce));
Assert.True(
sliceLate.Offset < weatherOnce.Offset,
"DrawWeatherOnce must be called AFTER the per-slice loop, not from inside it.");
}
/// <summary>
/// S3 chunk 1 fix round 2 (§11.6 H1): <see
/// cref="RetailPViewPassExecutor.ShouldDrawWeatherOnce"/> is
/// <see cref="DrawWeatherOnce_DrawsTheWeatherMeshAndPrintsExactlyOnce"/>'s
/// gate, extracted as a pure predicate so this suite can pin "no OC line
/// while the player stands indoors" without a live GL/DAT
/// <see cref="SkyRenderer"/> — combined with the two structural tests
/// above (moved out of the loop; drawn/printed exactly once per call),
/// this proves both halves of the spec's pin: an outdoor root (or an
/// interior root with several exit-view slices) prints exactly one OC
/// line, and an indoor player prints none. Retail's own check:
/// <c>SmartBox::is_player_outside</c> @0x00451e80,
/// <c>(player objcell_id &amp; 0xFFFF) &lt; 0x100</c>. MUTATION: negating
/// the <c>&lt; 0x100</c> comparison (or dropping either bool AND) makes
/// one of the four rows below fail.
/// </summary>
[Theory]
[InlineData(true, true, 0xF4180003u, true)] // outdoor root, player outside a land cell -> draws
[InlineData(true, true, 0xA9B40100u, false)] // player indoors (local id >= 0x100) -> no draw
[InlineData(false, true, 0xF4180003u, false)] // RenderSky off -> no draw
[InlineData(true, false, 0xF4180003u, false)] // RenderWeather off -> no draw
public void ShouldDrawWeatherOnce_MatchesRetailIsPlayerOutsideGate(
bool renderSky, bool renderWeather, uint playerCellId, bool expected)
{
Assert.Equal(
expected,
RetailPViewPassExecutor.ShouldDrawWeatherOnce(renderSky, renderWeather, playerCellId));
}
private static int RequiredCallIndex(
IReadOnlyList<CompiledCall> calls,
Type declaringType,

19
tests/AcDream.App.Tests/Rendering/Walk/WalkTraceConformanceTests.cs
View file

@ -28,8 +28,11 @@ public sealed class WalkTraceConformanceTests
/// flood defers its EC/OC to <see cref="OnInteriorFloodDrawTurn"/>),
/// flips true forever once a building's portal pass begins
/// (<see cref="OnBuildingTurn"/>) — see
/// <see cref="WalkTraceReplayContext.Signature8(WalkOracleFrame)"/>'s own
/// doc comment for why this never resets mid-frame.</summary>
/// <see cref="WalkTraceReplayContext.AppendFloodTurns"/>'s own doc
/// comment for why this never resets mid-frame. S3 chunk 1 fix round 2
/// (§11.6 H3): this REPLAY-side derivation is unchanged — only the
/// ORACLE side (<see cref="WalkTraceReplayContext.Signature8(WalkOracleFrame)"/>)
/// moved off derivation onto its own literal captured EC/OC lines.</summary>
private sealed class Recorder : IWalkEventSink
{
public readonly List<WalkEvent> Events = new();
@ -176,9 +179,15 @@ public sealed class WalkTraceConformanceTests
/// boundary near this pose — a real <c>WalkLandscape.CheckBlocks</c>
/// visibility question, not a print-site or ordering bug, and out of
/// this round's print-only scope (§11.4) — needs the retail oracle
/// (decomp/cdb) to resolve. terrace-edge/cathedral-arrival/foundry-deep
/// all reproduce EXACTLY at this same eight-kind level; only this row's
/// specific pose hits the boundary.
/// (decomp/cdb) to resolve. Filed as docs/ISSUES.md #458.
/// terrace-edge/cathedral-arrival/foundry-deep all reproduce EXACTLY at
/// this same eight-kind level; only this row's specific pose hits the
/// boundary. S3 chunk 1 fix round 2 (§11.6 H3) switched the ORACLE side
/// of the eight-kind signature from a derived EC/OC to retail's own
/// LITERAL captured EC/OC lines — re-verified 2026-09-03 that this
/// divergence stays at the SAME token index 165 (an LC/SC-only mismatch,
/// upstream of any EC/OC content) under the new comparison; #458's
/// position is unchanged.
/// </summary>
[Fact]
[Trait("Status", "KnownFailure")]

115
tests/AcDream.App.Tests/Rendering/Walk/WalkTraceReplay.cs
View file

@ -185,41 +185,50 @@ public sealed class WalkTraceReplayContext : IWalkFrameContext, IRetailFrameWalk
/// them at this level would spuriously diverge on every frame (the
/// replay always emits LC/SC; the FW0 oracle frame never has any).
/// <para>
/// LC/SC come straight from <paramref name="tokens"/>/<paramref
/// name="frame"/>'s own literal events — real turns with a real position
/// on both sides. EC/OC do NOT: <see cref="WalkEvent"/>'s four-kind
/// vocabulary has no EC/OC analogue at all, and retail's own real
/// position for them depends on which flood a DC belongs to
/// (<c>PView::DrawCells</c>'s two complete reverse loops — shell
/// far-to-near, then object-list far-to-near). A look-in flood's DC
/// draws them IMMEDIATELY, matching <c>RetailFrameWalk.DrawLandscape</c>'s
/// own recursive <c>DrawCells</c> call; the interior root's OWN flood
/// DEFERS them past the whole landscape/look-in walk, to
/// <see cref="IWalkEventSink.OnInteriorFloodDrawTurn"/> — evidence:
/// docs/research/2026-09-01-overhaul/oh-capture/cathedral-leak.walk.log
/// lines 4 and 1328: the root DC's own EC/OC print LAST, after every
/// intervening look-in's own EC/OC pair. Both signature overloads below
/// derive EC/OC from each DC's own declared cell list under that SAME
/// rule (reversed order, twice), rather than reading the oracle's
/// literal EC/OC lines — this ALSO naturally excludes G8's trailing
/// per-frame weather OC from the comparison: it owns no DC, so no
/// derivation rule ever produces a token for it (see
/// <c>WalkOracleTraceTests</c> for a dedicated assertion that a
/// COMPLETE outdoor frame's raw event list really does end with it).
/// S3 chunk 1 fix round 2 (§11.6 H3): the two sides of this comparison
/// are DELIBERATELY ASYMMETRIC. LC/SC/EC/OC all come straight from
/// <paramref name="tokens"/>'s own literal per-hook calls — the REPLAY
/// side's real turns at their real positions, EC/OC included (<see
/// cref="WalkTraceConformanceTests"/>'s <c>Recorder</c> derives them via
/// <see cref="AppendFloodTurns"/> at the exact hook that fires for each
/// flood — see that type's own doc comment). <see
/// cref="Signature8(WalkOracleFrame)"/> below, the ORACLE side, used to
/// derive EC/OC the SAME way from each DC's declared cell list — but
/// that derivation can never disagree with itself, so it silently
/// verified nothing about EC/OC placement or content (round 1's own
/// blind spot: G7's SC-ordering regression shipped green for the same
/// reason with LC/SC). Round 2 fixes this: the oracle side now reads its
/// own CAPTURED <c>EC</c>/<c>OC</c> lines verbatim, in the order retail's
/// cdb breakpoints actually recorded them — a real comparison against a
/// real trace, not a derivation compared to itself. The one exclusion is
/// the trailing per-frame weather <c>OC</c> (§11.6 H1): the LAST event
/// of the frame's raw list, when its cell id matches the frame's own "P"
/// line (<see cref="WalkOracleFrame.Pose"/>) AND that P cell itself looks
/// outdoor (local id &lt; 0x100 — retail's own weather gate,
/// <c>SmartBox::is_player_outside</c>) — an interior root's OWN flood can
/// ALSO end the frame with a real OC matching the P cell (its nearest
/// cell's object-list turn draws last, and that nearest cell is often
/// the camera's own root cell: cathedral-arrival.walk's real trailing
/// "OC f4180108" at DI:f4180108/P-cell f4180108), so position and value
/// alone are not enough — it owns no DC and is not a walk turn at all
/// (see <c>WalkOracleTraceTests</c> for a dedicated
/// assertion that a COMPLETE outdoor frame's raw event list really does
/// end with it).
/// </para>
/// </summary>
public static string Signature8(IReadOnlyList<string> tokens) => string.Join("|", tokens);
/// <summary>The oracle-frame half of <see cref="Signature8(IReadOnlyList{string})"/> —
/// see that overload's doc comment for the shared derivation rule.</summary>
/// see that overload's doc comment for why this reads EC/OC literally
/// rather than deriving them.</summary>
public static string Signature8(WalkOracleFrame frame)
{
var tokens = new List<string>();
bool? lookIn = null;
IReadOnlyList<uint>? pendingRootCells = null;
IReadOnlyList<WalkOracleEvent> events = frame.Events;
foreach (WalkOracleEvent e in frame.Events)
for (int i = 0; i < events.Count; i++)
{
WalkOracleEvent e = events[i];
switch (e.Kind)
{
case WalkOracleEventKind.Landscape:
@ -227,19 +236,13 @@ public sealed class WalkTraceReplayContext : IWalkFrameContext, IRetailFrameWalk
break;
case WalkOracleEventKind.Building:
tokens.Add($"BLD:{e.CellId!.Value:x8}");
lookIn = true;
break;
case WalkOracleEventKind.DrawInside:
tokens.Add($"DI:{e.CellId!.Value:x8}");
lookIn = false;
break;
case WalkOracleEventKind.DrawCells:
tokens.Add(
$"DC:ov={e.OutsideViewCount}:{string.Join(',', e.Cells.Select(c => c.ToString("x8")))}");
if (lookIn == true)
AppendFloodTurns(tokens, e.Cells);
else
pendingRootCells = e.Cells;
break;
case WalkOracleEventKind.LandCell:
tokens.Add($"LC:{e.CellId!.Value:x8}");
@ -247,25 +250,55 @@ public sealed class WalkTraceReplayContext : IWalkFrameContext, IRetailFrameWalk
case WalkOracleEventKind.SortCell:
tokens.Add($"SC:{e.CellId!.Value:x8}");
break;
case WalkOracleEventKind.EnvCellShell:
tokens.Add($"EC:{e.CellId!.Value:x8}");
break;
case WalkOracleEventKind.ObjectCellTurn:
// §11.6 H1's trailing weather OC: the LAST event of the
// whole frame, matching the frame's own "P" cell — not a
// flood turn, excluded from the comparison (see this
// method's own class doc for the citation). The extra
// "P cell is itself an outdoor land cell" check matters:
// an INTERIOR root's OWN flood draws its NEAREST cell's
// object list LAST (PView::DrawCells's reversed loop,
// index 0 = nearest = last) — when that nearest cell IS
// the camera's own root cell, its real OC turn ALSO sits
// at the frame's last position AND shares the P cell's
// id (cathedral-arrival.walk: DI:f4180108, P cell
// f4180108, real trailing "OC f4180108" — position AND
// value alone would misclassify it). Retail's weather
// never draws for an interior-rooted P cell at all
// (SmartBox::is_player_outside is false whenever the
// viewer's own cell id has local part &gt;= 0x100), so
// requiring the P cell itself look outdoor closes that
// hole without reopening G8's original one.
bool trailingWeatherOc = i == events.Count - 1
&& frame.Pose is not null
&& e.CellId!.Value == frame.Pose.CellId
&& (frame.Pose.CellId & 0xFFFFu) < 0x100u;
if (!trailingWeatherOc)
tokens.Add($"OC:{e.CellId!.Value:x8}");
break;
}
}
// The interior root's OWN flood: its EC/OC derive last, matching
// OnInteriorFloodDrawTurn firing as DrawInside's terminal sink call
// (see class doc) — an outdoor-rooted frame never sets this at all.
if (pendingRootCells is not null)
AppendFloodTurns(tokens, pendingRootCells);
return string.Join("|", tokens);
}
/// <summary><c>PView::DrawCells</c>'s two complete reverse loops: every
/// EnvCell shell far-to-near, THEN every object-list turn far-to-near —
/// the SAME reversed cell order twice (not one reversed EC/OC pair per
/// cell) — see <see cref="Signature8(WalkOracleFrame)"/>'s own doc
/// comment for the evidence. Internal (not private): <see
/// cref="WalkTraceConformanceTests"/>'s own <c>Recorder</c> reuses this
/// EXACT rule for the replay side rather than re-deriving it.</summary>
/// cell). S3 chunk 1 fix round 2 (§11.6 H3): this is now ONLY the
/// REPLAY side's derivation — <see cref="WalkTraceConformanceTests"/>'s
/// <c>Recorder</c> calls this at the exact hook that fires for each
/// flood (a look-in's own <c>DrawCells</c> immediately; the interior
/// root's own flood at <c>OnInteriorFloodDrawTurn</c>), matching
/// <c>RetailFrameWalk</c>'s real two-reverse-loop order
/// (<c>WalkFrameDriver.EmitFloodTurns</c>'s own comment has the same
/// citation). <see cref="Signature8(WalkOracleFrame)"/> no longer calls
/// this — the ORACLE side reads its own captured EC/OC lines literally
/// instead of re-deriving them (see that method's own doc comment for
/// why: a derivation compared to itself proves nothing).</summary>
internal static void AppendFloodTurns(List<string> tokens, IReadOnlyList<uint> cells)
{
for (int i = cells.Count - 1; i >= 0; i--)