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

refactor(render): one group-creation seam, required foliage key field, first-frame wind snap (Campaign VM VM6 review 3)

Browse source 
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<T>
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 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-23 03:03:56 +02:00
parent a82959f1b7
commit fccba8390d
12 changed files with 347 additions and 141 deletions
Download patch file Download diff file Expand all files Collapse all files

82
src/AcDream.App/Rendering/Packs/AtmosphericPostProcessGraph.cs
View file

@ -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<uint> _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.</para>
///
/// <para><b>The first advance.</b> Review fix round 3 (A6): the very
/// first advance for a graph instance (<see cref="_windFrameSerial"/>
/// still its <c>-1</c> constructor sentinel) SNAPS <see cref="_windMean"/>/
/// <see cref="_windGust"/> 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
/// (<c>ACDREAM_SKY_PHASE_SECONDS</c>, the determinism pin the offline
/// pixel gate uses) never advances between frames, so
/// <c>deltaSeconds</c> is 0 on every call AFTER the first — the wind
/// would ease only once, by whatever the first call's clamped
/// <c>deltaSeconds</c> 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
/// <see cref="AcDream.Core.World.WeatherSystem.TransitionSeconds"/>
/// exactly as before; only the very first one is exact.</para>
/// </summary>
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);
}
/// <summary>
/// Campaign VM VM6 review fix round 3 (A6 test support, N1-adjacent):
/// <see cref="_windClockSecondsOverride"/> is normally fixed for a
/// graph's whole lifetime (constructor-supplied, from
/// <c>ACDREAM_SKY_PHASE_SECONDS</c>) — production never changes it
/// mid-session. This lets a hermetic test advance the pinned clock by
/// an EXACT, deterministic amount between two resolves (proving
/// <see cref="ResolveFoliageWind"/>'s ease-after-the-first-advance
/// behavior) without a real-time <c>Thread.Sleep</c> or a flaky
/// tolerance window. Test-only: nothing in production calls this.
/// </summary>
internal void SetWindClockSecondsOverrideForTesting(float seconds) =>
_windClockSecondsOverride = seconds;
public IGpuRenderTarget PrepareWorldTarget(
int width,
int height,

61
src/AcDream.App/Rendering/Wb/FoliageWindClassification.cs
View file

@ -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 <c>HasCutoutSubset</c> never gives
/// it <see cref="TrunkFlag"/>. 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
/// <c>HasCutoutSubset</c> 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 <c>ObjectRenderData</c>
/// or the mesh adapter — the caller supplies <paramref name="hasCutoutSubset"/>
/// to look each part's value up however it already does. Short-circuits
/// on the first <see langword="true"/>; a part the caller cannot
/// currently resolve simply contributes nothing (the caller marks the
/// entity incomplete separately and reclassifies once every part loads).
/// </summary>
internal static bool ComputeEntityHasCutoutSubset<T>(
IReadOnlyList<T> setupParts,
Func<T, bool> hasCutoutSubset)
{
for (int i = 0; i < setupParts.Count; i++)
{
if (hasCutoutSubset(setupParts[i]))
return true;
}
return false;
}
/// <summary>
/// Campaign VM VM6 review fix round 2 (F3): zero-alloc production
/// overload of <see cref="ComputeEntityHasCutoutSubset{T}"/>. 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 <c>this</c> 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 <typeparamref name="TContext"/>
/// argument to a <see langword="static"/> 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 <c>HasCutoutSubset</c> 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 <c>ObjectRenderData</c> or the mesh adapter — the caller supplies
/// <paramref name="hasCutoutSubset"/> to look each part's value up
/// however it already does. Short-circuits on the first
/// <see langword="true"/>; a part the caller cannot currently resolve
/// simply contributes nothing (the caller marks the entity incomplete
/// separately and reclassifies once every part loads).
///
/// <para>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
/// <typeparamref name="TContext"/> argument to a
/// <see langword="static"/> 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 <see langword="static"/> 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
/// <c>ComputeEntityHasCutoutSubset</c> to keep correct.</para>
/// </summary>
internal static bool ComputeEntityHasCutoutSubset<T, TContext>(
IReadOnlyList<T> setupParts,

12
src/AcDream.App/Rendering/Wb/GroupKey.cs
View file

@ -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.</para>
///
/// <para>Campaign VM VM6 review fix round 3 (N2b): <see cref="FoliageFlags"/>
/// is REQUIRED (no default) and declared before <see cref="CullMode"/> (which
/// keeps its default) so that a <c>new GroupKey(...)</c> 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.</para>
/// </summary>
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);

37
src/AcDream.App/Rendering/Wb/WbDrawDispatcher.PackedOracle.cs
View file

@ -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;
}

68
src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs
View file

@ -2401,8 +2401,40 @@ public sealed partial class WbDrawDispatcher : IDisposable
g.TextureSlot,
g.TextureLayer,
g.Translucency,
g.CullMode,
g.FoliageFlags);
g.FoliageFlags,
g.CullMode);
/// <summary>
/// Campaign VM VM6 review fix round 3 (N2a): the ONE shared
/// InstanceGroup-from-key construction seam. Before this there were two
/// near-identical <c>new InstanceGroup { ... }</c> initializers — the
/// classic route's <see cref="GetOrCreateInstanceGroup"/> and the packed
/// route's <c>GetOrCreatePackedGroup</c> — that had already drifted once
/// (the packed route's F1 bug: FoliageFlags copied on one side, dropped
/// on the other). <paramref name="registration"/> and
/// <paramref name="frame"/> are the only two values that legitimately
/// differ per route (each owns its own registration counter and frame
/// serial); every other field comes from <paramref name="key"/>, so a
/// field neither site remembers to set can no longer exist. Same
/// precedent as <see cref="AppendPackedInstance"/> — one writer, tested
/// once, called from both routes.
/// </summary>
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);