From fccba8390d3d63e0b02d14a1e1d8d0b76a65c510 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 23 Aug 2026 03:03:56 +0200 Subject: [PATCH] refactor(render): one group-creation seam, required foliage key field, first-frame wind snap (Campaign VM VM6 review 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrow re-review of a82959f1: APPROVE, with follow-ups. All items landed. N2 (structural): (a) extracted the ONE shared InstanceGroup-from-key construction seam, WbDrawDispatcher.CreateGroupFromKey(key, registration, frame) — before this there were two near-identical `new InstanceGroup { ... }` initializers (GetOrCreateInstanceGroup and GetOrCreatePackedGroup) that had already drifted once (the round-2 F1 bug). Both routes call it now; CreateGroupFromKey's own `new()` is the only production InstanceGroup construction site repo-wide, same precedent as AppendPackedInstance. (b) GroupKey.FoliageFlags lost its `= 0u` default and moved before CullMode in the declaration (CullMode keeps its default, C# requires optional params to trail required ones), so a `new GroupKey(...)` that omits it is a compile error. Fixed every real construction site the reorder/requirement touched: the 2 production sites, ToKey (a reconstruction from InstanceGroup the review didn't count but the reorder broke), and 5 test sites (one more than the review's "4" — InstanceGroupClearTests had a second, implicit target-typed `MakeKey` factory the original count missed). Verified by a full solution build. N1: added CreateGroupFromKey_CopiesFoliageFlagsFromTheKey (InstanceGroupClearTests) — a key carrying FoliageFlags 0x2 in, the created group's FoliageFlags 0x2 out. That test plus N2b's required field are what actually guard the round-2 F1 blocker; reworded PackedDispatcherOracleTests' existing test comment to say what IT proves (the classification-to- BuildIndirectArrays-to-BatchData.flags path), not that it guards the classifier. N3: corrected the plan's round-2 paragraph — folding FoliageFlags into the G2/G3 digest is correct and symmetric, but CompareClassifiedOutput only runs from RenderScenePViewFrameProductController.BuildAndCompare, which has no production caller anywhere in src/AcDream.App/, and both of RenderScenePViewFrameProductTests's own callers construct the controller without the optional dispatcher argument — so the fold catches nothing until that oracle is wired to an actual caller. N4: the plan's F6 note now names both classification caches — the classic route's EntityClassificationCache.EntityCacheEntry (self-heals per entity on its own next eviction) and the packed route's PackedProjectionClassificationEntry/PackedClassifiedBatch.Key (PackedProjectionClassificationCache.BeginFrame clears its entire cache in one shot on a RenderSceneGeneration change) — and notes neither mechanism is keyed to a pack switch specifically. N5: deleted the now-unused single-generic ComputeEntityHasCutoutSubset overload; its 4 test call sites now use the two-generic, zero-alloc overload with an unused int context and a static (_, value) => value lambda, so there is exactly one ComputeEntityHasCutoutSubset to keep correct. A6 (reviewer-filed): ResolveFoliageWind's _windMean/_windGust started at 0 and always eased toward the weather target by clock delta, with no distinction for a graph's first-ever advance. A pinned clock (ACDREAM_SKY_PHASE_SECONDS, the offline pixel gate's determinism pin) never advances between calls, so the wind reached only whatever fraction the first (1-second-clamped) step produced and sat there forever; live, the first 10 s after a graph is constructed (pack selection / login) spun up from dead calm even though the weather already IS what it is. Fixed at the root: the first advance (_windFrameSerial == -1, the constructor sentinel) now snaps _windMean/_windGust straight to the target; every later advance eases over WeatherSystem.TransitionSeconds exactly as before. Added a SetWindClockSecondsOverrideForTesting seam (_windClockSecondsOverride is no longer readonly) so a hermetic test can advance the pinned clock by an exact amount between two resolves without a real-time Thread.Sleep; two new tests prove the first-advance snap is exact and a second advance still eases at the normal rate. The three existing indoor/wind-disabled/amplitude gate tests pass unchanged. Verify: Release build 0 warnings/0 errors. App hermetic-lane filter 6,046/0 failed. Core.Tests 4,695/0 failed. Full hermetic-filtered solution: 15,274/0 failed across 15 projects. Co-Authored-By: Claude Sonnet 5 --- .../plans/2026-08-22-visualmaster-campaign.md | 63 +++++++++++--- .../Packs/AtmosphericPostProcessGraph.cs | 82 +++++++++++++++---- .../Rendering/Wb/FoliageWindClassification.cs | 61 ++++++-------- src/AcDream.App/Rendering/Wb/GroupKey.cs | 12 ++- .../Wb/WbDrawDispatcher.PackedOracle.cs | 37 +++------ .../Rendering/Wb/WbDrawDispatcher.cs | 68 +++++++++------ .../Packs/AtmosphericPostProcessGraphTests.cs | 61 ++++++++++++++ .../Wb/FoliageWindClassificationTests.cs | 12 ++- .../Rendering/Wb/InstanceGroupClearTests.cs | 50 ++++++++++- .../Wb/PackedDispatcherOracleTests.cs | 36 ++++---- .../Wb/EntityClassificationCacheTests.cs | 3 +- .../Wb/WbDrawDispatcherBucketingTests.cs | 3 +- 12 files changed, 347 insertions(+), 141 deletions(-) diff --git a/docs/plans/2026-08-22-visualmaster-campaign.md b/docs/plans/2026-08-22-visualmaster-campaign.md index 1bd046b5..e2421b09 100644 --- a/docs/plans/2026-08-22-visualmaster-campaign.md +++ b/docs/plans/2026-08-22-visualmaster-campaign.md @@ -580,8 +580,19 @@ copies it exactly like `GetOrCreateInstanceGroup` always has. The G2/G3 classified-output digest (`AddOpaqueSubmissionGroup`/ `BuildTransparentSubmissionDigest`) now also folds `GroupKey.FoliageFlags` into its hash — previously present in the key but never actually read by -either digest function, so a content-level (not just count-level) classic- -vs-packed divergence is now caught. (F2, medium) The delayed-alpha replay +either digest function, so the fold is correct and symmetric between the +two functions. **Review fix round 3 (N3) correction:** this does NOT mean a +classic-vs-packed divergence is caught today. `CompareClassifiedOutput` (the +method that reads this digest) only runs from +`RenderScenePViewFrameProductController.BuildAndCompare`, which has no +production caller anywhere in `src/AcDream.App/` — `FrameRootComposition.cs` +constructs the controller with a real dispatcher, but nothing ever calls +`BuildAndCompare` on it — and both of `RenderScenePViewFrameProductTests`'s +own callers construct the controller without the optional `dispatcher` +argument, so `_dispatcher` is `null` and `CompareClassifiedOutput` short- +circuits before reaching the digest at all. The fold is correct and ready +for the day this oracle is wired to a caller; it catches nothing until then. +(F2, medium) The delayed-alpha replay path (`PrepareDeferredAlphaDraws`) hardcoded `Flags = 1`, dropping bits 1/2 for any group replayed through it — a trunk instance promoted into the alpha-blend group mid-fade (the `#188` translucency-promotion case) would @@ -602,15 +613,45 @@ Binding` has its seven ABI v1 members zero/Identity by construction — only the two v2 wind members are valid — safe today because `mesh_atmospheric .vert` reads this binding solely for wind displacement, a footgun for a future v1-reading addition to that shader) also landed. F6, noted rather -than fixed: `EntityClassificationCache`'s `EntityCacheEntry` bakes -`GroupKey.FoliageFlags` (hence exclusion membership) in at classification -time and is not proactively invalidated when `FoliageWindExclusions` -changes — a newly-excluded or newly-included object can show its previous -classification until the entity's cache entry is next evicted (e.g. a -landblock demote/reload) rather than immediately on a pack switch. Harmless -with the pack off (exclusions are pack-scoped) and self-heals on the next -natural cache eviction; not worth a proactive invalidation sweep for a -rarely-changing, pack-scoped list. +than fixed, applies to BOTH of the classifier's caches: the classic route's +`EntityClassificationCache`'s `EntityCacheEntry`, and the packed +production route's `PackedProjectionClassificationEntry`/ +`PackedClassifiedBatch.Key` (`PackedProjectionClassificationCache`). Both +bake `GroupKey.FoliageFlags` (hence exclusion membership) in at +classification time and neither is proactively invalidated when +`FoliageWindExclusions` changes — a newly-excluded or newly-included object +can show its previous classification until its cache entry is next evicted +rather than immediately on a pack switch. The two caches differ in HOW they +eventually recover: `EntityCacheEntry` self-heals per entity, on that +entity's own next eviction (e.g. a landblock demote/reload); +`PackedProjectionClassificationCache.BeginFrame` +(`PackedProjectionClassificationCache.cs:133-137`) instead clears its +ENTIRE cache in one shot whenever `RenderSceneGeneration` changes. Neither +mechanism is keyed to a pack switch specifically, so both are staleness +windows of unknown-but-bounded length, not an immediate reclassification. +Harmless with the pack off (exclusions are pack-scoped); not worth a +proactive invalidation sweep for a rarely-changing, pack-scoped list. + +**Review nit A6 (reviewer-filed, landed round 3):** `ResolveFoliageWind`'s +`_windMean`/`_windGust` started at 0 and ALWAYS eased toward the weather +target by clock delta, with no distinction for a graph's first-ever +advance. Two consequences: a pinned clock (`ACDREAM_SKY_PHASE_SECONDS`, +the offline pixel gate's determinism pin) has delta 0 on every advance +after the first, so the wind reached only whatever fraction the first +(clamped-to-1-second) step produced and sat there forever — every offline +capture under-represented the motion; live, the first 10 s after a graph +is constructed (pack selection / login) spun up from dead calm even though +the weather already IS what it is, because there was no previous frame to +ease from. Fixed at the root: the first advance +(`_windFrameSerial == -1`, the constructor sentinel) now SNAPS `_windMean`/ +`_windGust` straight to the target; every later advance eases over +`WeatherSystem.TransitionSeconds` exactly as before. A new +`SetWindClockSecondsOverrideForTesting` test-only seam +(`AtmosphericPostProcessGraph`, `_windClockSecondsOverride` no longer +`readonly`) lets a hermetic test advance the pinned clock by an exact, +deterministic amount between two resolves — proving both the first-advance +snap and that a SECOND advance still eases normally — without a real-time +`Thread.Sleep`. ## VM7 — Closeout and merge diff --git a/src/AcDream.App/Rendering/Packs/AtmosphericPostProcessGraph.cs b/src/AcDream.App/Rendering/Packs/AtmosphericPostProcessGraph.cs index 9b17d55e..a7ada34d 100644 --- a/src/AcDream.App/Rendering/Packs/AtmosphericPostProcessGraph.cs +++ b/src/AcDream.App/Rendering/Packs/AtmosphericPostProcessGraph.cs @@ -184,7 +184,7 @@ internal sealed class AtmosphericPostProcessGraph : // frame (see D2/D4 — the shadow must move with the leaf). private readonly FoliageWindSettings _foliageWind; private readonly IReadOnlySet _foliageWindExclusions; - private readonly float? _windClockSecondsOverride; + private float? _windClockSecondsOverride; private readonly System.Diagnostics.Stopwatch _windClock = System.Diagnostics.Stopwatch.StartNew(); private long _windFrameSerial = -1; @@ -545,6 +545,26 @@ internal sealed class AtmosphericPostProcessGraph : /// cell transition reads exactly zero on the very next frame, and /// resuming outdoors/enabled picks the smoothed state back up without a /// spin-up glitch. + /// + /// The first advance. Review fix round 3 (A6): the very + /// first advance for a graph instance ( + /// still its -1 constructor sentinel) SNAPS / + /// straight to that frame's target instead of + /// easing toward it — there is no previous state to ease FROM. Without + /// this, two things went wrong: (1) a pinned clock + /// (ACDREAM_SKY_PHASE_SECONDS, the determinism pin the offline + /// pixel gate uses) never advances between frames, so + /// deltaSeconds is 0 on every call AFTER the first — the wind + /// would ease only once, by whatever the first call's clamped + /// deltaSeconds happened to be (up to the 1 s clamp, ~10% of the + /// way at the 10 s transition rate), then sit there forever, so every + /// offline capture under-represented the motion; (2) live, the first + /// 10 s after this graph is constructed (pack selection / login) spun + /// up from dead calm even though the weather already IS what it is — + /// there was no "previous frame" to have been easing from. Every + /// advance AFTER the first still eases over + /// + /// exactly as before; only the very first one is exact. /// private (Vector4 ClockWind, Vector4 WindAmplitude) ResolveFoliageWind( long frameSerial, @@ -555,26 +575,44 @@ internal sealed class AtmosphericPostProcessGraph : ?? (float)_windClock.Elapsed.TotalSeconds; if (_windFrameSerial != frameSerial) { - float deltaSeconds = Math.Clamp( - clockSeconds - _windLastAdvanceClockSeconds, - 0f, - 1f); (float targetMean, float targetGust) = RenderPackAtmospherePolicyEvaluation .FoliageWind( Descriptor.AtmospherePolicy?.FoliageWindByWeather, weather); targetMean *= _foliageWind.Strength; targetGust *= _foliageWind.Strength; - _windMean = RenderPackAtmospherePolicyEvaluation.EaseTowardTarget( - _windMean, - targetMean, - deltaSeconds, - AcDream.Core.World.WeatherSystem.TransitionSeconds); - _windGust = RenderPackAtmospherePolicyEvaluation.EaseTowardTarget( - _windGust, - targetGust, - deltaSeconds, - AcDream.Core.World.WeatherSystem.TransitionSeconds); + + // Review fix round 3 (A6): _windFrameSerial's constructor + // sentinel (-1) means this is the FIRST advance this graph + // instance has ever done — there is no previous _windMean/ + // _windGust state to ease FROM, so snap straight to the target + // instead of computing a deltaSeconds-based ease step. See this + // method's "The first advance" doc paragraph for the two + // concrete symptoms this fixes (a pinned clock's permanent + // near-zero wind, and live's dead-calm spin-up on construction). + if (_windFrameSerial == -1) + { + _windMean = targetMean; + _windGust = targetGust; + } + else + { + float deltaSeconds = Math.Clamp( + clockSeconds - _windLastAdvanceClockSeconds, + 0f, + 1f); + _windMean = RenderPackAtmospherePolicyEvaluation.EaseTowardTarget( + _windMean, + targetMean, + deltaSeconds, + AcDream.Core.World.WeatherSystem.TransitionSeconds); + _windGust = RenderPackAtmospherePolicyEvaluation.EaseTowardTarget( + _windGust, + targetGust, + deltaSeconds, + AcDream.Core.World.WeatherSystem.TransitionSeconds); + } + _windClockSeconds = clockSeconds; _windLastAdvanceClockSeconds = clockSeconds; _windFrameSerial = frameSerial; @@ -595,6 +633,20 @@ internal sealed class AtmosphericPostProcessGraph : return (clockWind, windAmplitude); } + /// + /// Campaign VM VM6 review fix round 3 (A6 test support, N1-adjacent): + /// is normally fixed for a + /// graph's whole lifetime (constructor-supplied, from + /// ACDREAM_SKY_PHASE_SECONDS) — production never changes it + /// mid-session. This lets a hermetic test advance the pinned clock by + /// an EXACT, deterministic amount between two resolves (proving + /// 's ease-after-the-first-advance + /// behavior) without a real-time Thread.Sleep or a flaky + /// tolerance window. Test-only: nothing in production calls this. + /// + internal void SetWindClockSecondsOverrideForTesting(float seconds) => + _windClockSecondsOverride = seconds; + public IGpuRenderTarget PrepareWorldTarget( int width, int height, diff --git a/src/AcDream.App/Rendering/Wb/FoliageWindClassification.cs b/src/AcDream.App/Rendering/Wb/FoliageWindClassification.cs index 02f4a063..8b84ff1e 100644 --- a/src/AcDream.App/Rendering/Wb/FoliageWindClassification.cs +++ b/src/AcDream.App/Rendering/Wb/FoliageWindClassification.cs @@ -58,42 +58,31 @@ internal static class FoliageWindClassification /// no cutout batches of its own (the leaves are a DIFFERENT part), so /// classifying it from only its own HasCutoutSubset never gives /// it . Both the world-receiver and - /// directional-shadow-caster Setup-part walks call this ONCE per entity - /// per frame (not per batch) to OR every currently-resolved part's - /// HasCutoutSubset into one entity-scoped value before - /// classifying each part against it. Generic over the caller's own part - /// representation so it needs no dependency on ObjectRenderData - /// or the mesh adapter — the caller supplies - /// to look each part's value up however it already does. Short-circuits - /// on the first ; a part the caller cannot - /// currently resolve simply contributes nothing (the caller marks the - /// entity incomplete separately and reclassifies once every part loads). - /// - internal static bool ComputeEntityHasCutoutSubset( - IReadOnlyList setupParts, - Func hasCutoutSubset) - { - for (int i = 0; i < setupParts.Count; i++) - { - if (hasCutoutSubset(setupParts[i])) - return true; - } - return false; - } - - /// - /// Campaign VM VM6 review fix round 2 (F3): zero-alloc production - /// overload of . The - /// single-generic overload above is exactly what a caller needs to - /// predicate on an instance field (e.g. the mesh adapter) — but doing so - /// with a lambda that closes over this allocates a fresh closure - /// object AND a fresh delegate on every call, and this is called once - /// per Setup entity per frame from the world receiver, the shadow - /// caster, and the packed production classifier. Passing the would-be- - /// captured value as an explicit - /// argument to a lambda (no captures at all) - /// lets the C# compiler cache a single delegate instance for the - /// method's lifetime instead of allocating one per call. + /// directional-shadow-caster Setup-part walks, and the packed production + /// classifier, call this ONCE per entity per frame (not per batch) to OR + /// every currently-resolved part's HasCutoutSubset into one + /// entity-scoped value before classifying each part against it. Generic + /// over the caller's own part representation so it needs no dependency + /// on ObjectRenderData or the mesh adapter — the caller supplies + /// to look each part's value up + /// however it already does. Short-circuits on the first + /// ; a part the caller cannot currently resolve + /// simply contributes nothing (the caller marks the entity incomplete + /// separately and reclassifies once every part loads). + /// + /// Campaign VM VM6 review fix round 2 (F3), round 3 (N5): takes + /// the would-be-captured value (e.g. the mesh adapter) as an explicit + /// argument to a + /// lambda instead of letting a caller's lambda + /// close over an instance field — a closure-capturing lambda would + /// allocate a fresh closure object AND delegate on every call, and this + /// runs once per Setup entity per frame from all three production + /// classifiers. A lambda with no captures lets + /// the C# compiler cache a single delegate instance for the method's + /// lifetime instead. Round 3 deleted the single-generic predecessor of + /// this overload — every call site (production and test) now goes + /// through this one, so there is exactly one + /// ComputeEntityHasCutoutSubset to keep correct. /// internal static bool ComputeEntityHasCutoutSubset( IReadOnlyList setupParts, diff --git a/src/AcDream.App/Rendering/Wb/GroupKey.cs b/src/AcDream.App/Rendering/Wb/GroupKey.cs index 8d01842c..e5283094 100644 --- a/src/AcDream.App/Rendering/Wb/GroupKey.cs +++ b/src/AcDream.App/Rendering/Wb/GroupKey.cs @@ -32,6 +32,14 @@ namespace AcDream.App.Rendering.Wb; /// between frames and let the caster (keyed correctly from the start) and /// the receiver (previously keyed without this field) disagree about the /// same subset. +/// +/// Campaign VM VM6 review fix round 3 (N2b): +/// is REQUIRED (no default) and declared before (which +/// keeps its default) so that a new GroupKey(...) omitting it is a +/// compile error, not a silent fall-back to 0. The round-2 packed-classifier +/// blocker (F1) was exactly a silently-defaulted FoliageFlags reaching +/// production; a caller can no longer forget this field the way that one +/// could. /// internal readonly record struct GroupKey( uint FirstIndex, @@ -40,5 +48,5 @@ internal readonly record struct GroupKey( GpuTextureSlot TextureSlot, uint TextureLayer, TranslucencyKind Translucency, - CullMode CullMode = CullMode.CounterClockwise, - uint FoliageFlags = 0u); + uint FoliageFlags, + CullMode CullMode = CullMode.CounterClockwise); diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.PackedOracle.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.PackedOracle.cs index c955713d..06ea7b44 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.PackedOracle.cs +++ b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.PackedOracle.cs @@ -669,8 +669,8 @@ public sealed unsafe partial class WbDrawDispatcher texture.Slot, texture.Layer, translucency, - batch.CullMode, - foliageFlags); + FoliageFlags: foliageFlags, + CullMode: batch.CullMode); var classified = new PackedClassifiedBatch( key, restPose, @@ -796,29 +796,16 @@ public sealed unsafe partial class WbDrawDispatcher "Packed instance-group registration space was exhausted."); } - group = new InstanceGroup - { - FirstIndex = key.FirstIndex, - BaseVertex = key.BaseVertex, - IndexCount = key.IndexCount, - TextureSlot = key.TextureSlot, - TextureLayer = key.TextureLayer, - Translucency = key.Translucency, - CullMode = key.CullMode, - // Campaign VM VM6 review fix round 2 (F1 BLOCKER): this was the - // missing copy. GetOrCreateInstanceGroup (the classic route) has - // set FoliageFlags from key.FoliageFlags since the round-1 - // fix; this packed-route sibling never did, so every packed - // production InstanceGroup carried FoliageFlags == 0 regardless - // of what ClassifyPackedBatches computed into the key — - // BatchData.flags bits 1/2 were always 0 for production world - // geometry even though the independently-classified shadow - // caster set them correctly, so casters swayed while the - // meshes they shadowed did not. - FoliageFlags = key.FoliageFlags, - Registration = _nextPackedGroupRegistration++, - LastUsedFrame = _packedGroupFrame, - }; + // Campaign VM VM6 review fix round 3 (N2a): both routes share the + // ONE InstanceGroup-from-key seam now — see CreateGroupFromKey's + // doc comment. This is exactly the seam that closed the F1 bug + // (FoliageFlags copied on the classic side, dropped here) by + // construction: there is now exactly one place either route can + // build an InstanceGroup, and it always copies every GroupKey field. + group = CreateGroupFromKey( + key, + registration: _nextPackedGroupRegistration++, + frame: _packedGroupFrame); _packedGroups.Add(key, group); return group; } diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs index d45fc48e..9568f34b 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +++ b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs @@ -2401,8 +2401,40 @@ public sealed partial class WbDrawDispatcher : IDisposable g.TextureSlot, g.TextureLayer, g.Translucency, - g.CullMode, - g.FoliageFlags); + g.FoliageFlags, + g.CullMode); + + /// + /// Campaign VM VM6 review fix round 3 (N2a): the ONE shared + /// InstanceGroup-from-key construction seam. Before this there were two + /// near-identical new InstanceGroup { ... } initializers — the + /// classic route's and the packed + /// route's GetOrCreatePackedGroup — that had already drifted once + /// (the packed route's F1 bug: FoliageFlags copied on one side, dropped + /// on the other). and + /// are the only two values that legitimately + /// differ per route (each owns its own registration counter and frame + /// serial); every other field comes from , so a + /// field neither site remembers to set can no longer exist. Same + /// precedent as — one writer, tested + /// once, called from both routes. + /// + internal static InstanceGroup CreateGroupFromKey( + GroupKey key, + long registration, + long frame) => new() + { + FirstIndex = key.FirstIndex, + BaseVertex = key.BaseVertex, + IndexCount = key.IndexCount, + TextureSlot = key.TextureSlot, + TextureLayer = key.TextureLayer, + Translucency = key.Translucency, + CullMode = key.CullMode, + FoliageFlags = key.FoliageFlags, + Registration = registration, + LastUsedFrame = frame, + }; private void ObserveCurrentDispatcherSubmission( int visibleInstanceCount, @@ -3214,25 +3246,14 @@ public sealed partial class WbDrawDispatcher : IDisposable "Instance-group registration space was exhausted before a safe identity could be assigned."); } - group = new InstanceGroup - { - FirstIndex = key.FirstIndex, - BaseVertex = key.BaseVertex, - IndexCount = key.IndexCount, - TextureSlot = key.TextureSlot, - TextureLayer = key.TextureLayer, - Translucency = key.Translucency, - CullMode = key.CullMode, - // Campaign VM VM6 review fix round: FoliageFlags is now part of - // GroupKey (see GroupKey's doc comment), so it is set exactly - // once here, at group creation, from the SAME key that decides - // group identity — never re-stamped imperatively afterward, - // which is what let a shared group's classification flicker - // between whichever caller ran last. - FoliageFlags = key.FoliageFlags, - Registration = _nextGroupRegistration++, - LastUsedFrame = _groupFrame, - }; + // Campaign VM VM6 review fix round 3 (N2a): both routes share the + // ONE InstanceGroup-from-key seam now — see CreateGroupFromKey's + // doc comment for why (the packed route's F1 bug was exactly this + // kind of drift between two near-identical initializers). + group = CreateGroupFromKey( + key, + registration: _nextGroupRegistration++, + frame: _groupFrame); _groups.Add(key, group); return group; } @@ -3442,8 +3463,9 @@ public sealed partial class WbDrawDispatcher : IDisposable entityHasCutoutSubset); var key = new GroupKey( batch.FirstIndex, (int)batch.BaseVertex, - batch.IndexCount, texSlot, texLayer, translucency, batch.CullMode, - foliageFlags); + batch.IndexCount, texSlot, texLayer, translucency, + FoliageFlags: foliageFlags, + CullMode: batch.CullMode); InstanceGroup grp = GetOrCreateInstanceGroup(key); grp.Matrices.Add(model); diff --git a/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs b/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs index 1b783c87..e84bc758 100644 --- a/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs @@ -1222,6 +1222,67 @@ public sealed class AtmosphericPostProcessGraphTests // ── Campaign VM VM6: foliage wind ──────────────────────────────────── + [Fact] + public void FirstAdvanceSnapsExactlyToTheWeatherTargetInsteadOfEasingFromZero() + { + // Campaign VM VM6 review fix round 3 (A6): before this fix, the + // very first advance treated the constructor-default _windMean/ + // _windGust (0) as the "previous" strength and eased toward the + // target by a deltaSeconds-based step clamped to 1 second. Under a + // pinned clock (ACDREAM_SKY_PHASE_SECONDS, the determinism pin the + // offline pixel gate uses) that first step is the ONLY one that + // ever happens — deltaSeconds is 0 on every later call because the + // pinned clock never advances — so the wind would sit at whatever + // fraction that one step reached (at most 10% of the target, at + // the 10 s TransitionSeconds rate) forever. The fix snaps exactly + // to the target on the very first advance instead. + var device = new RecordingGpuDevice(); + using var graph = Graph(device, "medium", windClockSecondsOverride: 12f); + graph.PrepareWorldTarget(640, 480, 1); + AtmosphericFrameInputs outdoors = Inputs(640, 480) with { Weather = WeatherKind.Storm }; + + AtmosphericFrameUniforms atmospheric = RenderAndReadFrameBlock(device, graph, outdoors); + + // Storm: mean 1.00, gust 0.75 (FoliageWindByWeather's built-in + // row), times the declared default wind-strength (1.0x) — exact, + // not a fraction of the way there. + Assert.Equal(1.00f, atmospheric.ClockWind.Y, 5); + Assert.Equal(0.75f, atmospheric.ClockWind.Z, 5); + } + + [Fact] + public void SecondAdvanceStillEasesTowardTheNewTargetFromTheFirstAdvancesSnappedValue() + { + // Campaign VM VM6 review fix round 3 (A6): the snap-on-first- + // advance fix must not turn EVERY advance into a snap — only the + // very first one. Resolve once at Clear (snaps exactly to Clear's + // target), advance the pinned clock by exactly 1 real second + // (SetWindClockSecondsOverrideForTesting gives deterministic + // control no Thread.Sleep could), then resolve again at Storm on a + // fresh frame.Serial: this is the graph's SECOND advance, so it + // eases at rate = deltaSeconds / TransitionSeconds = 1 / 10 = 10% + // of the way from Clear's snapped value toward Storm's target, + // exactly like every advance did before this fix. + var device = new RecordingGpuDevice(); + using var graph = Graph(device, "medium", windClockSecondsOverride: 0f); + graph.PrepareWorldTarget(640, 480, 1); + AtmosphericFrameInputs clearInputs = Inputs(640, 480) with { Weather = WeatherKind.Clear }; + + AtmosphericFrameUniforms first = RenderAndReadFrameBlock(device, graph, clearInputs); + Assert.Equal(0.25f, first.ClockWind.Y, 5); // Clear mean, snapped exactly + Assert.Equal(0.15f, first.ClockWind.Z, 5); // Clear gust, snapped exactly + + graph.SetWindClockSecondsOverrideForTesting(1f); + AtmosphericFrameInputs stormInputs = Inputs(640, 480) with { Weather = WeatherKind.Storm }; + AtmosphericFrameUniforms second = RenderAndReadFrameBlock(device, graph, stormInputs); + + const float rate = 1f / 10f; // deltaSeconds(1) / TransitionSeconds(10) + float expectedMean = 0.25f + ((1.00f - 0.25f) * rate); + float expectedGust = 0.15f + ((0.75f - 0.15f) * rate); + Assert.Equal(expectedMean, second.ClockWind.Y, 5); + Assert.Equal(expectedGust, second.ClockWind.Z, 5); + } + [Fact] public void IndoorGatesWindOutputToExactZeroRegardlessOfSmoothedState() { diff --git a/tests/AcDream.App.Tests/Rendering/Wb/FoliageWindClassificationTests.cs b/tests/AcDream.App.Tests/Rendering/Wb/FoliageWindClassificationTests.cs index cedfc258..25cfeadc 100644 --- a/tests/AcDream.App.Tests/Rendering/Wb/FoliageWindClassificationTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Wb/FoliageWindClassificationTests.cs @@ -189,7 +189,8 @@ public sealed class FoliageWindClassificationTests { bool result = FoliageWindClassification.ComputeEntityHasCutoutSubset( partHasCutout, - static value => value); + context: 0, + static (_, value) => value); Assert.Equal(expected, result); } @@ -200,7 +201,8 @@ public sealed class FoliageWindClassificationTests Assert.False( FoliageWindClassification.ComputeEntityHasCutoutSubset( Array.Empty(), - static value => value)); + context: 0, + static (_, value) => value)); } /// @@ -224,10 +226,12 @@ public sealed class FoliageWindClassificationTests bool partAOwnHasCutoutSubset = FoliageWindClassification.ComputeEntityHasCutoutSubset( partAHasCutoutSubset, - static value => value); + context: 0, + static (_, value) => value); bool entityScopedHasCutoutSubset = FoliageWindClassification.ComputeEntityHasCutoutSubset( setupPartsHasCutoutSubset, - static value => value); + context: 0, + static (_, value) => value); uint flagsFromPartOwnValue = FoliageWindClassification.Classify( sceneryEntityId, diff --git a/tests/AcDream.App.Tests/Rendering/Wb/InstanceGroupClearTests.cs b/tests/AcDream.App.Tests/Rendering/Wb/InstanceGroupClearTests.cs index d015244a..e64b5a25 100644 --- a/tests/AcDream.App.Tests/Rendering/Wb/InstanceGroupClearTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Wb/InstanceGroupClearTests.cs @@ -218,7 +218,8 @@ public class InstanceGroupClearTests IndexCount: 6, TextureSlot: Slot(0xAA), TextureLayer: 0, - Translucency: TranslucencyKind.Opaque); + Translucency: TranslucencyKind.Opaque, + FoliageFlags: 0u); var cached = new CachedBatch( key, Slot(0xAA), @@ -235,6 +236,49 @@ public class InstanceGroupClearTests out _)); } + /// + /// Campaign VM VM6 review fix round 3 (N1): this is the guard for the + /// round-2 F1 blocker (the packed classifier's InstanceGroup carried + /// FoliageFlags == 0 no matter what GroupKey.FoliageFlags said, because + /// GetOrCreatePackedGroup's own initializer never copied it). N2a + /// deleted that possibility structurally — CreateGroupFromKey is the + /// ONLY InstanceGroup-from-key construction site left in production, + /// used by BOTH the classic and packed routes — and N2b made + /// GroupKey.FoliageFlags a required constructor argument, so a future + /// caller cannot silently default it to 0 either. Together those two + /// changes are what actually guards F1; this test pins + /// CreateGroupFromKey's own copy so a regression there fails here + /// directly instead of only showing up as a visual "trees don't sway" + /// report. + /// + [Fact] + public void CreateGroupFromKey_CopiesFoliageFlagsFromTheKey() + { + var key = new GroupKey( + FirstIndex: 10, + BaseVertex: 2, + IndexCount: 18, + TextureSlot: Slot(0x77), + TextureLayer: 0, + Translucency: TranslucencyKind.ClipMap, + FoliageFlags: 0x2u, + CullMode: DatReaderWriter.Enums.CullMode.CounterClockwise); + + WbDrawDispatcher.InstanceGroup group = + WbDrawDispatcher.CreateGroupFromKey(key, registration: 5, frame: 9); + + Assert.Equal(0x2u, group.FoliageFlags); + Assert.Equal(5, group.Registration); + Assert.Equal(9, group.LastUsedFrame); + Assert.Equal(key.FirstIndex, group.FirstIndex); + Assert.Equal(key.BaseVertex, group.BaseVertex); + Assert.Equal(key.IndexCount, group.IndexCount); + Assert.Equal(key.TextureSlot, group.TextureSlot); + Assert.Equal(key.TextureLayer, group.TextureLayer); + Assert.Equal(key.Translucency, group.Translucency); + Assert.Equal(key.CullMode, group.CullMode); + } + [Fact] public void FramePrune_RetiresOnlyGroupsAbsentForWholePreviousFrame() { @@ -314,14 +358,14 @@ public class InstanceGroupClearTests } private static AcDream.App.Rendering.Gpu.GpuTextureSlot Slot(uint index) => new(index); - private static GroupKey MakeKey(uint textureSlot) => new( FirstIndex: 0, BaseVertex: 0, IndexCount: 6, TextureSlot: Slot(textureSlot), TextureLayer: 0, - Translucency: TranslucencyKind.Opaque); + Translucency: TranslucencyKind.Opaque, + FoliageFlags: 0u); private static WbDrawDispatcher.InstanceGroup MakeGroup( TranslucencyKind translucency, diff --git a/tests/AcDream.App.Tests/Rendering/Wb/PackedDispatcherOracleTests.cs b/tests/AcDream.App.Tests/Rendering/Wb/PackedDispatcherOracleTests.cs index c7bf6fa8..c1664b8a 100644 --- a/tests/AcDream.App.Tests/Rendering/Wb/PackedDispatcherOracleTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Wb/PackedDispatcherOracleTests.cs @@ -8,27 +8,23 @@ namespace AcDream.App.Tests.Rendering.Wb; public sealed class PackedDispatcherOracleTests { /// - /// Campaign VM VM6 review fix round 2 (F1 BLOCKER): the production - /// packed classifier (WbDrawDispatcher.PackedOracle.cs's - /// ClassifyPackedBatches/GetOrCreatePackedGroup) is a private instance - /// pipeline reached only through the full RetailPViewPassExecutor → - /// DrawPackedProductionRoute route, which needs a real IGpuDevice, - /// world-pass scope, mesh manager, and compiled pipelines to construct — - /// no test in this suite (or anywhere in the App test project) stands - /// one up, so driving ClassifyPackedBatches/GetOrCreatePackedGroup - /// directly is not a "cheap test." This proves the two halves of the - /// fix that ARE cheaply testable and, chained together, prove the exact - /// claim the review asked for: (1) FoliageWindClassification.Classify — - /// called with the SAME argument shape ClassifyPackedBatches now uses + /// Campaign VM VM6 review fix round 3 (N1): this test does NOT guard + /// the packed classifier itself — CreateGroupFromKey's own test + /// (InstanceGroupClearTests.CreateGroupFromKey_CopiesFoliageFlagsFromTheKey) + /// plus GroupKey.FoliageFlags being a required constructor argument are + /// what actually make F1 (the round-2 blocker: the packed classifier's + /// InstanceGroup silently carried FoliageFlags == 0) impossible to + /// reintroduce. What THIS test proves is the SHARED production step + /// downstream of classification: FoliageWindClassification.Classify, + /// called with the same argument shape ClassifyPackedBatches uses /// (entity.LocalEntityId, exclusion membership, batch.Translucency, - /// entity-scoped HasCutoutSubset) — resolves a real procedural-scenery - /// entity id (0x8…) to CutoutFoliageFlag (0x2); (2) that flags value, - /// carried on an IndirectGroupInput exactly like GetOrCreatePackedGroup - /// now carries it on InstanceGroup.FoliageFlags, reaches the literal - /// BatchData.flags word through BuildIndirectArrays — the same shared, - /// already-tested production step both the classic and packed group - /// lists feed into (WbDrawDispatcherIndirectBuilderTests pins bit 0; - /// this pins bits 1/2 landing alongside it for a real scenery id). + /// entity-scoped HasCutoutSubset), resolves a real procedural-scenery + /// entity id (0x8…) to CutoutFoliageFlag (0x2); and a group carrying + /// that flags value reaches the literal BatchData.flags word through + /// BuildIndirectArrays — the same already-tested production step both + /// the classic and packed group lists feed into + /// (WbDrawDispatcherIndirectBuilderTests pins bit 0; this pins bits 1/2 + /// landing alongside it for a real scenery id). /// [Fact] public void SceneryCutoutEntityClassificationReachesTheBatchDataFlagsWordAsBit0x2() diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs index 5f86aee9..7e57619a 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +++ b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs @@ -336,7 +336,8 @@ public class EntityClassificationCacheTests IndexCount: indexCount, TextureSlot: new AcDream.App.Rendering.Gpu.GpuTextureSlot(texSlot), TextureLayer: 0, - Translucency: TranslucencyKind.Opaque); + Translucency: TranslucencyKind.Opaque, + FoliageFlags: 0u); return new CachedBatch(key, new AcDream.App.Rendering.Gpu.GpuTextureSlot(texSlot), Matrix4x4.Identity); } } diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs index f0740973..f9c0c111 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +++ b/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs @@ -426,7 +426,8 @@ public sealed class WbDrawDispatcherBucketingTests IndexCount: indexCount, TextureSlot: new AcDream.App.Rendering.Gpu.GpuTextureSlot(texSlot), TextureLayer: 0, - Translucency: TranslucencyKind.Opaque); + Translucency: TranslucencyKind.Opaque, + FoliageFlags: 0u); return new CachedBatch( key, new AcDream.App.Rendering.Gpu.GpuTextureSlot(texSlot),