fix(render): production packed classifier carries the foliage flags; deferred-alpha replay keeps them (Campaign VM VM6 review 2)

Narrow re-review of 43e3abed found every A1-A5/A7/A8 item resolved but
one new blocker in the production packed classifier.

F1 (BLOCKER): RetailPViewPassExecutor.DrawPackedProductionRoute — the
route production world geometry actually draws from — never computed
FoliageFlags at all. WbDrawDispatcher.PackedOracle.cs's
ClassifyPackedBatches built its GroupKey with the field defaulting to
0u, and GetOrCreatePackedGroup never copied it onto the created
InstanceGroup, so production BatchData.flags bits 1/2 were always zero
for every scenery entity: the world geometry never swayed even though
the independently-classified shadow caster did, so shadows visibly
swayed under rigid trees. Both classifier call sites now compute
FoliageFlags via the identical FoliageWindClassification.Classify call
and entity-scoped HasCutoutSubset OR the classic (non-packed) path
uses, and GetOrCreatePackedGroup copies it exactly like
GetOrCreateInstanceGroup always has. The G2/G3 classified-output
digest (AddOpaqueSubmissionGroup/BuildTransparentSubmissionDigest) now
also folds GroupKey.FoliageFlags into its hash — present in the key
since round 1 but never actually read by either digest function, so a
content-level (not just group-count-level) classic-vs-packed
divergence is now caught.

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 stop swaying for the
duration of its fade. Now 1u | key.FoliageFlags.

F3 (nit): ComputeEntityHasCutoutSubset's three call sites (classic,
caster, and the newly-fixed packed classifier) each allocated a
closure over _meshAdapter per Setup entity per frame. A new
context-taking overload passes the mesh adapter as an explicit
argument to a static lambda instead, letting the compiler cache one
delegate for the method's lifetime rather than allocating fresh ones.

A3 test gap: WbDrawDispatcher.BindDirectionalShadowReceiver is now
internal so DirectionalShadowGpuTests can drive it directly with a
bare RecordingGpuDevice pass encoder, proving it emits
UniformAtmosphericFrame with the exact buffer/offset/size a
DirectionalShadowFrameBinding carries — paired with the existing test
proving that binding carries the caster's real bind forward untouched.

F1's missing test: PackedDispatcherOracleTests chains
FoliageWindClassification.Classify (called with the packed
classifier's exact argument shape) for a real 0x8... scenery entity id
through BuildIndirectArrays — the same shared, already-tested
production step both classic and packed group lists feed into BatchData —
proving the resulting flags word carries bit 0x2. Driving
ClassifyPackedBatches/GetOrCreatePackedGroup directly was not a "cheap
test": both are private instance methods reachable only through the
full RetailPViewPassExecutor route, which needs a real IGpuDevice,
world-pass scope, mesh manager, and compiled pipelines to construct —
no test anywhere in the App test project stands one up.

Nits: F4 corrects foliage_wind.glsl's header comment from "bit 31" to
the top-nibble test; F5 documents at the receiver bind site that the
caster's own AtmosphericFrameBufferBinding 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 that
binding solely for wind displacement, flagged as a footgun for a
future v1-reading addition to that shader; F6 notes in the plan
(rather than fixes) that EntityCacheEntry does not proactively
invalidate when FoliageWindExclusions changes on a pack switch —
harmless with the pack off, self-heals on the entry's next natural
eviction.

foliage_wind.glsl's F4 comment-only change updated the SPIR-V
manifest's source hashes for the five includers (mesh_atmospheric.vert
+ four directional_shadow_world_* casters); the compiled .spv bytes
are byte-identical since comments do not affect bytecode.

Verify: Release build 0 warnings/0 errors. App hermetic-lane filter
6,043/0 failed. Core.Tests 4,695/0 failed. RenderPackValidator 30/30.
Full hermetic-filtered solution: 15,271/0 failed across 15 projects.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-23 02:28:17 +02:00
parent 43e3abed4d
commit a82959f1b7
10 changed files with 332 additions and 16 deletions

View file

@ -564,6 +564,54 @@ precision at far landblock corners) and A8 (`FoliageWindExclusions` as a
`FrozenSet`) also landed; A7 is the divergence note on the `max(maxHeight,
0.5)` guard above.
**Review fix round 2 (2026-08-23):** a narrow re-review found one more
blocker. (F1) The PRODUCTION packed classifier (`WbDrawDispatcher
.PackedOracle.cs`'s `ClassifyPackedBatches`/`GetOrCreatePackedGroup` — the
route `RetailPViewPassExecutor.DrawPackedProductionRoute` actually draws
from) never computed `FoliageFlags` at all: it built its `GroupKey` with the
field defaulting to `0u` and never copied it onto the created
`InstanceGroup`, so production `BatchData.flags` bits 1/2 were always zero
for every scenery entity — the world geometry never swayed even though the
independently-classified shadow caster did, so shadows visibly swayed under
rigid trees. Both classifier call sites now compute `FoliageFlags` via the
identical `FoliageWindClassification.Classify` call and entity-scoped
`HasCutoutSubset` OR the classic path uses, and `GetOrCreatePackedGroup`
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
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
stop swaying for the duration of its fade; now `1u | key.FoliageFlags`. (F3)
`ComputeEntityHasCutoutSubset`'s three call sites (classic, caster, and the
newly-fixed packed classifier) each allocated a closure over `_meshAdapter`
per Setup entity per frame; a new context-taking overload passes the mesh
adapter as an explicit argument to a `static` lambda instead, letting the
compiler cache one delegate for the method's lifetime. A3's test gap also
closed: `DirectionalShadowGpuTests` now has a companion test proving
`WbDrawDispatcher.BindDirectionalShadowReceiver` (made `internal` for this)
actually emits the `UniformAtmosphericFrame` bind with the exact
buffer/offset/size a `DirectionalShadowFrameBinding` carries, pairing with
the existing test that proves that binding carries the caster's real bind
forward untouched. F4 (the `foliage_wind.glsl` header comment) and F5 (a
comment at the receiver bind site: the caster's own `AtmosphericFrameBuffer-
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.
## VM7 — Closeout and merge
- Full gates: `tools/run-release-gate.ps1` (hermetic lanes), the AR