Mechanical refactor: GroupKey was a private nested record struct on
WbDrawDispatcher. The upcoming EntityClassificationCache (ISSUE #53) needs
to store GroupKey inside CachedBatch records, so it must be visible to
both the dispatcher and the cache. Promoting to internal at file scope is
the smallest change that achieves this.
No behavior change. 1688 tests pass; 8 pre-existing failures unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug A's fix (commit `9217fd9`) patched at the worker output by stripping
entities from far-tier `LoadedLandblock`s after the full `LoadNear` path
ran. The worker still wasted CPU on `LandBlockInfo` reads + entity
hydration + `SceneryGenerator` math + interior-cell walks for ~544
far-tier LBs at radius=12, just to throw the work away.
This commit plumbs `LandblockStreamJobKind` through to the factory so the
worker can branch at the source:
- `LandblockStreamer.cs`: replace the `Func<uint, LoadedLandblock?>`
factory with `Func<uint, LandblockStreamJobKind, LoadedLandblock?>` as
the primary ctor signature. Add a back-compat overload that wraps the
old single-arg signature (`(id, _) => loadLandblock(id)`) so existing
test code keeps compiling without modification — the 5 ctor sites in
`LandblockStreamerTests.cs` now resolve to the overload. `HandleJob`
passes `load.Kind` to the factory; the post-load entity-strip is
retained as a `Debug.Assert` + Release safety net.
- `GameWindow.cs`: `BuildLandblockForStreaming(uint, JobKind)` branches
on `kind == LoadFar` at the top — reads only the `LandBlock` heightmap
dat and returns a `LoadedLandblock` with `Array.Empty<WorldEntity>()`.
Skips `LandblockLoader.Load` (which reads `LandBlockInfo`),
`BuildSceneryEntitiesForStreaming`, and `BuildInteriorEntitiesForStreaming`
entirely. Near-tier path is unchanged. Both call sites updated to pass
the kind through the lambda: `(id, kind) => BuildLandblockForStreaming(id, kind)`.
Tests: 1688/1696 (8 pre-existing physics/input failures unchanged).
Streaming-targeted filter (30 tests covering LandblockStreamer +
StreamingController + StreamingRegion) all green via the back-compat
overload — no test code needed updating.
Per-LB worker cost on far-tier: was ~tens of ms (full hydration,
including LandBlockInfo + scenery generation + interior cells); now a
single `LandBlock` dat read (~sub-ms).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three root causes regressed the Holtburg lifestone since the WB rendering
migration (Phase N.5 retirement amendment, commit dcae2b6, 2026-05-08).
All confirmed via temporary [LIFESTONE-DIAG] instrumentation and visually
verified by the user through the +Acdream test character.
1. **Alpha-test discard** in mesh_modern.frag transparent pass killed
high-α pixels of dat-flagged transparent surfaces. Native AC
transparent surfaces routinely include effectively-opaque pixels —
e.g. the lifestone crystal core (surface 0x080011DE) — that compose
correctly under (SrcAlpha, 1-SrcAlpha) blending. The original N.5
§2 rationale ("high-α belongs in opaque pass") doesn't hold for
surfaces flagged transparent at the dat level: those pixels can't
reach the opaque pass at all. Fix: remove `α >= 0.95 discard` from
the transparent pass, keep `α < 0.05 discard` as a fragment-cost
optimization (skip totally-empty pixels).
2. **Cull state** for the transparent pass was unset by
WbDrawDispatcher after the N.5 retirement amendment deleted
StaticMeshRenderer.cs (which had the Phase 9.2 setup at commit
6f1971a, 2026-04-11). Closed-shell translucents — lifestone crystal,
glow gems — need GL_CULL_FACE + GL_BACK + GL_CCW in the transparent
pass; otherwise back faces composite over front faces in iteration
order under DepthMask(false). Fix: re-establish Phase 9.2's exact
GL state setup at the top of Phase 8.
3. **uDrawIDOffset uniform** was missing from mesh_modern.vert.
gl_DrawIDARB resets to 0 at the start of each
glMultiDrawElementsIndirect call, so the transparent pass — which
begins later in the indirect buffer — was fetching
Batches[0..transparentCount) instead of its actual section at
Batches[opaqueCount..end). The lifestone crystal ended up reading
the FIRST OPAQUE batch's TextureHandle every frame; as the camera
moved and the front-to-back opaque sort reordered which group
landed at BatchData[0], the crystal's apparent texture flickered to
whatever sat first — typically the player character's body parts.
Fix: add `uniform int uDrawIDOffset` to the vertex shader, change
Batches[gl_DrawIDARB] → Batches[uDrawIDOffset + gl_DrawIDARB], and
set the uniform per-pass in WbDrawDispatcher (0 for opaque,
_opaqueDrawCount for transparent). Mirrors WorldBuilder's
BaseObjectRenderManager.cs line 845.
Tests: 1688/1696 passing (8 pre-existing physics/input failures
unchanged). N.5b conformance sentinel 94/94 clean.
Visual: Holtburg lifestone now renders with the spinning blue crystal
correctly composed over the pedestal. Other transparent content (glass,
particle effects, NPC clothing) is unaffected — the same uniform fix
applies globally and is correct for all transparent draws.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per docs/plans/2026-05-10-perf-tiers-2-3-roadmap.md Tier 1: cache the
per-(entity, meshRef, batch) classification (TextureCache lookup,
GroupKey hash, _groups dict insert) so the per-frame Draw inner loop
becomes "look up cache → walk assignments → append matrix to group's
Matrices list."
For static entities (~95% of world: trees, rocks, buildings, scenery),
the answer never changes between frames. Cache once at first visit;
reuse permanently. Per-frame work for static drops from 4 expensive
operations per (meshRef, batch) to 1 list-append.
Estimated entity dispatcher: 3.5ms → ~1-1.5ms median at radius=12.
Should land inside the 2.0ms spec budget.
Implementation:
- New EntityClassificationCache class (per-meshRef list of cached
(group ref, baked-PartTransform) tuples) keyed by entity.Id.
- ClassifyEntity does the one-time work; result populates _groups and
the cache.
- Draw inner loop: cache lookup → for each assignment, model =
PartTransform × entityWorld; group.Matrices.Add(model).
- Cache miss when ClassifyEntity finds NO mesh loaded yet (Vao == 0)
→ don't store; retry next frame. Avoids cache thrash during the
streaming-in window.
- Public InvalidateEntity(uint id) + ClearEntityCache() for explicit
invalidation hooks. Wiring (palette swap on ObjDescEvent, MeshRefs
hot-swap) is post-A.5 follow-up — for now, cache-stale entities
show their pre-swap appearance until next respawn.
Tier 2 (static/dynamic split with persistent groups) and Tier 3 (GPU
compute culling) tracked in the roadmap doc.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After PlayerDescription is dispatched, the Inventory and Equipped lists
produced by the parser are now fed into ItemRepository via AddOrUpdate +
MoveItem so inventory/paperdoll panels see items after login.
Acceptance test PlayerDescription_RegistersInventoryEntries_InItemRepository
confirms ItemCount goes 0→2 for a synthetic PD with two inventory entries.
282 Net.Tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
T17's WalkEntities helper allocated a fresh List<(WorldEntity, int)>
per frame to hold the (entity, meshRefIndex) pairs that pass visibility
filters. At ~10K entities × ~3 mesh refs = ~30K tuples × 16 bytes =
~480 KB / frame of GC pressure on the render thread. The implementer's
self-review flagged this as a future N.6 optimization; the post-T26
diagnostic showed it materially contributing to the perf regression
(though Bug A — far-tier entity load — was the dominant factor).
Refactor: split WalkEntities into two overloads.
- WalkEntities(...) — test-friendly, allocates a fresh ToDraw list per
call. Tests keep using this signature unchanged.
- WalkEntitiesInto(..., scratch, ref result) — no-alloc, clears + populates
a caller-provided scratch list. Draw uses this with a per-dispatcher
_walkScratch field reused across frames.
Test count unchanged (40 streaming + 8 bucketing tests still pass).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase A.5's two-tier streaming spec promised that far-tier landblocks
ship terrain ONLY — no entities, no scenery, no interior cells. T13/T16
wired the controller side (RecenterTo emits ToLoadFar/ToLoadNear/ToPromote;
controller passes JobKind to the worker), but the worker's HandleJob
never branched on Kind: every load called BuildLandblockForStreaming
which runs the full hydration + scenery generation + interior cell path.
Result: at default radii (N₁=4 / N₂=12), 540 far-tier LBs each loaded
their full entity layer (~132 entities/LB → ~71K entities total) into
GpuWorldState. The dispatcher then walked all ~54K entities per frame
(post-frustum-cull), driving the entity dispatcher cpu_us from ~3.6ms
median (T24 baseline) to ~18-21ms (post-T22.5 horizon-test). User-
observed: 40 FPS / 25ms frame time at horizon-safe settings; system
crash at full High preset.
Minimum-diff fix: in LandblockStreamer.HandleJob, after
_loadLandblock returns, strip Entities to empty for LoadFar before
posting Loaded. Worker still does wasted hydration CPU (off the render
thread, harmless). Render-side dispatcher walk drops from ~54K to ~10K
entities/frame.
Math: post-fix entity dispatcher should drop to ~3-4ms median at N₁=4 /
N₂=12 (matches T24's 3.6ms at radius=5 single-tier, since N₁=4 has 33%
fewer near entities than N₁=5).
Future optimization (N.6 / A.6): plumb JobKind through
BuildLandblockForStreaming so the worker also skips the wasted CPU.
Out of A.5 scope.
Bug B (T17 WalkEntities allocation) is a smaller perf hit — defer if
post-Bug-A FPS is acceptable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GameWindow.OnLoad resolves QualitySettings.From(_persistedDisplay.Quality)
+ WithEnvOverrides() immediately after LoadAndApplyPersistedSettings, stores
result in _resolvedQuality field. All six quality dimensions applied:
- NearRadius / FarRadius: replace old T16 env-var-only block; preset drives
the radii, legacy ACDREAM_STREAM_RADIUS override still honoured.
- MsaaSamples: WindowOptions.Samples reads from startup quality resolution
in Run() (pre-window-create read from SettingsStore). MSAA cannot change
at runtime; ReapplyQualityPreset logs a restart-required warning if the
new preset would change it.
- AnisotropicLevel: TerrainAtlas.SetAnisotropic() called after Build() and
again in ReapplyQualityPreset. Temporarily removes bindless residency
before the GL TexParameter call, re-makes resident after.
- AlphaToCoverage: WbDrawDispatcher.AlphaToCoverage property gates the
glEnable/glDisable(SampleAlphaToCoverage) pair around the opaque pass.
- MaxCompletionsPerFrame: set on StreamingController after construction
and after each mid-session restart.
ReapplyQualityPreset(QualityPreset) method handles mid-session changes
(Settings panel Quality dropdown Save): rebuilds streamer + controller for
radius changes, toggles A2C and aniso immediately, logs MSAA restart caveat.
onSaveDisplay callback updated to call ReapplyQualityPreset when Quality
field changes.
TerrainModernRenderer.Atlas property added to expose the atlas for
mid-session aniso updates.
991 tests passing, 8 pre-existing failures unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add QualityPreset enum + QualitySettings readonly record struct with
From(preset) table and WithEnvOverrides() env-var override layer.
Four presets (Low/Medium/High/Ultra) drive NearRadius, FarRadius,
MsaaSamples, AnisotropicLevel, AlphaToCoverage, MaxCompletionsPerFrame.
Env vars (ACDREAM_NEAR_RADIUS, ACDREAM_FAR_RADIUS, ACDREAM_MSAA_SAMPLES,
ACDREAM_ANISOTROPIC, ACDREAM_A2C, ACDREAM_MAX_COMPLETIONS_PER_FRAME)
override individual preset fields for dev spot-testing.
DisplaySettings gains a Quality: QualityPreset field (default High);
SettingsStore persists/loads it under display."quality" as an enum
name string with Enum.TryParse fallback. 12 new QualityPresetTests
cover the preset table (radii, msaa, aniso, a2c, completions) and all
six env-var override paths. 415 UI.Abstractions tests passing.
Wiring into GameWindow / WbDrawDispatcher / TerrainAtlas follows in
commit 2 of this task.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Phase A.5 spec §2 acceptance criterion 6: entity dispatcher median
≤ 2.0ms; terrain dispatcher median ≤ 1.0ms at standstill. When the
median exceeds the budget, prefix the DIAG line with " BUDGET_OVER" so
the regression is grep-friendly during perf testing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Phase A.5 spec §4.8: fog ramp is tuned to mask the N₁ scenery
boundary. FogStart = N₁ × 192m × 0.7 ≈ 538m at default radii (4/12).
FogEnd = N₂ × 192m × 0.95 ≈ 2188m. Multipliers exposed as env vars for
fast iteration during visual gate.
Override is injected into the UBO after SceneLightingUbo.Build() so fog
color, lightning flash and mode still come from the sky keyframe. Adds
ParseEnvFloat helper (InvariantCulture) for float env-var parsing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-quality review followup on Task 2 (becbde6) — addresses I1 (the
forward-looking concern that Tasks 3-9's inner-catch will leave partial
lists visible to callers with no signal) and M1 (silent inner catch).
Changes:
- Parsed gains a trailing `bool TrailerTruncated` field. Both
construction sites pass `false` by default; the trailer try/catch
flips a local `trailerTruncated` to `true` on FormatException and
feeds it into the final return.
- Inner catch logs `pos`/`payload.Length`/exception message under
ACDREAM_DUMP_VITALS=1, mirroring the outer catch's diagnostic
pattern.
- Task 2 test strengthened to assert defaults on Options2 /
SpellbookFilters / HotbarSpells / DesiredComps / GameplayOptions /
Equipped + TrailerTruncated=false (M2 followup — gives Tasks 3-9
a regression guard if they consume into the wrong field).
- New test `TryParse_TrailerAbsent_LessThan8BytesAfterEnchantments_*`
documents the contract that <8 bytes after enchantments means the
trailer is absent (not truncated): TrailerTruncated stays false,
upstream attribute data survives.
- Plan updated in lockstep so Tasks 3-11 implementers see the
`trailerTruncated` local and the new return-arg position.
271/271 AcDream.Core.Net.Tests pass.
Per Phase A.5 spec §4.9.2: ClipMap foliage uses binary alpha-cutoff.
At N₂=12 horizon distance the pixel-stepped silhouettes are visible.
A2C with MSAA 4x produces smooth retail-faithful tree edges.
GL context now requests Samples=4. WbDrawDispatcher's opaque pass
toggles GL_SAMPLE_ALPHA_TO_COVERAGE on/off around the multi-draw
indirect call. mesh_modern.frag's opaque pass now discards only
truly-empty (α<0.05) so the GPU derives sample mask from coverage;
transparent pass boundary logic is unchanged.
MSAA audit: no custom FBOs found — all rendering uses default
framebuffer. Sky/particles/ImGui are all MSAA-compatible.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Phase A.5 spec §4.9.1: at N₂=12 distant terrain LBs occupy a few
pixels on screen and shimmer (texel-swap aliasing) without mipmaps.
Generate mips after atlas upload; sampler trilinear + 16x anisotropic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Phase A.5 spec §4.6 Change #2: WalkEntities's per-entity AABB
frustum cull was recomputing Position±5 per frame per entity. With
~10.7K entities (N1=4) at 240 FPS that is ~2.5M wasted Vector3
ops/sec.
Read the AABB from the WorldEntity cache (T8 schema) instead.
RefreshAabb runs lazily on AabbDirty=true. Populate at register time:
- LandblockLoader.BuildEntitiesFromInfo: RefreshAabb after each new
WorldEntity construction (stabs + buildings). Refactored from
inline object-initializer to named variable to enable the call.
- EntitySpawnAdapter.OnCreate: RefreshAabb after entity state init
(position/rotation already set via the WorldEntity passed in).
Dynamic entities (NPCs, players) move every frame via direct
Position writes in GameWindow.cs. Migrated all three per-frame
write sites to SetPosition() (T8 mutator) so AabbDirty propagates:
- line 5942: player entity render position update
- line 6951: remote animated entity interpolated path
- line 7279: remote animated entity landing/movement path
The lazy RefreshAabb in WalkEntities catches up on the next frame
after any SetPosition call — render thread only, no races.
Build green, 986 passed / 8 pre-existing failures unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First step of the PD trailer walk. Wraps trailer reads in their own
try/catch so a malformed trailer does not null out the upstream
attribute/skill/spell/enchantment data we already extracted.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per Phase A.5 spec §4.6 Change #1: when an LB is invisible AND
animatedEntityIds is non-empty, the inner loop walked every entity
in the LB just to find the few animated ones. At ~10.7K entities
(N1=4) that is wasted iteration cost per frame.
Extracted a pure-CPU internal static WalkEntities helper. When LB
is invisible: iterate animatedEntityIds directly and look each up
in a per-LB AnimatedById dictionary (typically <50 animated vs
~10K total). When LB is visible: walk all entities as before.
GpuWorldState.LandblockEntries now yields an AnimatedById map as a
5th tuple field alongside the AABB tuple. Dictionary is built on
each yield (cheap — ~132 entities/LB max). A caching layer is out
of A.5 scope.
WbDrawDispatcher.Draw signature updated to consume the 5-tuple.
GameWindow.cs call site passes _worldState.LandblockEntries which
now yields the 5-tuple — no change needed there.
8 new tests in WbDrawDispatcherBucketingTests cover T17 Change #1
(invisible LB / animated set / neverCull / null frustum) and
T18 Change #2 guard tests (cached AABB / dirty flag / animated bypass).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review nit-fix on top of d3b58c9 — addresses two issues from the
quality review of Task 1:
I1 (Important): the record struct `Shortcut` was a homograph with the
flag member `CharacterOptionDataFlag.Shortcut`. Both names live inside
`PlayerDescriptionParser`'s scope. Rename to `ShortcutEntry` aligns
with `InventoryEntry`/`EquippedEntry` and removes the trap before
Task 3's walker references both names in the same method body.
M2 (Minor): `EquippedEntry` had no holtburger source citation; added
one referencing events.rs:180-190. Also expanded `InventoryEntry`'s
comment with the strict reader's validation reference.
Plan doc updated in lockstep so Task 3+ implementers see the new name.
8/8 PlayerDescriptionParser tests still pass.
Code review on T13-T16 bundle (commits fb10c3f/aff35d2/b8d80fe/c4fd373/31d312a)
flagged 3 Important + 2 test-coverage gaps. Apply all 5:
Important #1: GpuWorldState.AddEntitiesToExistingLandblock didn't
canonicalize landblockId. Streaming callers always pass canonical
0xAAAA0xFFFF ids, but the public API silently key-missed for callers
that mirror AppendLiveEntity's cell-resolved-id pattern. Both new
methods now canonicalize the id on entry.
Important #2: RemoveEntitiesFromLandblock asymmetry with RemoveLandblock
re: persistent-entity rescue. Documented as intentional — demote-tier
entities are atlas-tier only (procedural scenery, dat-static stabs/
buildings; never ServerGuid != 0); the local player and live server
spawns live in their LB via RelocateEntity per frame and aren't
affected by atlas-layer demote.
Important #3: StreamingController.NearRadius / FarRadius were { get; set; }
but mutating them after the first Tick is a no-op (StreamingRegion
snapshots the values). Switched to { get; } only with XML doc warning.
Test gap #1: ToDemote routing through Tick — added test that walks
the player past hysteresis and asserts entities drop while terrain
stays.
Test gap #2: Promoted result routing through Tick — added test that
enqueues a Promoted and asserts AddEntitiesToExistingLandblock fires.
Deferred Minor: dead _streamingRadius write + style consistency on
fully-qualified IReadOnlyList — non-load-bearing, can roll into a later
cleanup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No behavior change yet — adds CharacterOptionDataFlag, Shortcut/Inventory/
EquippedEntry records, and extends Parsed with trailer fields filled with
empty defaults. Sets up the per-section TDD walk in subsequent commits.
Cosmetic follow-up flagged by spec compliance review on T13-T16 bundle
(commits fb10c3f / aff35d2 / b8d80fe / c4fd373). The debug overlay's
getStreamingRadius callback was reading _streamingRadius — the legacy
single-tier field that's only updated by ACDREAM_STREAM_RADIUS. Operators
using the new ACDREAM_NEAR_RADIUS / ACDREAM_FAR_RADIUS env vars would
see the overlay frozen at the default 2.
Switch to _nearRadius. The overlay still shows a single number (matching
its label "Streaming radius"); operators who want both tier numbers can
read the launch log.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GameWindow now constructs StreamingController with nearRadius / farRadius
defaults of 4 / 12 (per spec acceptance criterion). Env vars:
- ACDREAM_NEAR_RADIUS (default 4)
- ACDREAM_FAR_RADIUS (default 12)
- ACDREAM_STREAM_RADIUS (legacy; if set, treats as nearRadius and
bumps farRadius to max(stream, default))
Fields _nearRadius / _farRadius added alongside legacy _streamingRadius
(kept so the debug overlay's getStreamingRadius callback stays valid).
ApplyLoadedTerrainLocked routes to TerrainModernRenderer.AddLandblockWithMesh
(T15) instead of AddLandblock directly, making the two-tier entry point
the canonical call path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the single-radius Tick with a two-tier model that consumes
StreamingRegion's TwoTierDiff (5-list) and routes to the appropriate
JobKind:
- ToLoadFar -> _enqueueLoad(id, LoadFar)
- ToLoadNear -> _enqueueLoad(id, LoadNear)
- ToPromote -> _enqueueLoad(id, PromoteToNear)
- ToDemote -> _state.RemoveEntitiesFromLandblock(id) on render thread
- ToUnload -> _enqueueUnload(id)
Drain switch handles Loaded (terrain + entity layer), Promoted (entity
layer only -- terrain already loaded), Unloaded, Failed, WorkerCrashed.
Constructor signature: nearRadius/farRadius separate ints. Old single-
radius ctor removed; existing single-radius tests updated to pass
nearRadius=farRadius for backward-compat coverage.
GameWindow's enqueueLoad lambda updated from (id =>...) to (id, kind) =>
to match new Action<uint, LandblockStreamJobKind> signature; radius: arg
renamed to nearRadius:/farRadius: (both set to _streamingRadius until T16
wires the full two-tier env-var parsing).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T13 routes worker-built meshes from LandblockStreamResult.Loaded.MeshData
into the renderer. AddLandblockWithMesh accepts a prebuilt mesh + origin
and delegates to the existing AddLandblock(uint, LandblockMeshData, Vector3)
so both paths share one upload path (Approach B -- AddLandblock already
takes a prebuilt mesh; no inline build to extract).
GameWindow's T16 lambda captures liveCenterX/Y and passes the derived
origin; the renderer stays origin-agnostic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new methods on GpuWorldState, used by two-tier streaming (T13):
- RemoveEntitiesFromLandblock(id): drop all entities from an LB while
keeping the terrain. Used for Near->Far demote (player walks past the
inner ring; LB stays loaded but entities leave).
- AddEntitiesToExistingLandblock(id, entities): merge new entities into
an already-loaded LB record. Used for Far->Near promote (terrain is
already on the GPU; just streaming the entity layer in).
Falls back to the pending bucket if the LB hasn't loaded yet.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review on T10-T12 bundle (commits 0cf86bb/00bb030/0405947 + audit
fix 76e1a64) found 3 Important issues:
1. LandblockStreamer.Start() had an idempotency race — the XML doc
claimed thread-safety but the implementation checked _worker != null
before assigning, allowing two callers to both pass the check and
spawn duplicate worker threads. Fixed via Interlocked.CompareExchange.
2. No test verified the worker emits Failed when buildMeshOrNull returns
null. Added Load_WhenBuildMeshReturnsNull_ReportsFailed.
3. StreamingControllerTests.cs:81 used MeshData: default! when
constructing a Loaded result. If a future test flows MeshData
through the apply callback, the null reference would NRE rather
than producing a meaningful assertion failure. Replaced with a real
empty LandblockMeshData instance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spec compliance review on T10-T12 bundle (commits 0cf86bb/00bb030/0405947)
caught 2 unprotected dat reads that the original T10 audit missed:
- GameWindow.UpdatePlayerAnimation (line ~7546): reads Setup when the
player entity is missing from _animatedEntities (post-respawn pattern).
- GameWindow.EnterPlayerModeNow (line ~8567): reads Setup when entering
player mode to derive StepUpHeight / StepDownHeight from the dat.
Both run on the render thread post-_streamer.Start(), so they can race
with the worker thread's BuildLandblockForStreamingLocked. DatBinReader's
shared buffer position would corrupt — same class of "ball with spikes"
bug the original Phase A.1 hotfix addressed.
Wrap both reads in lock (_datLock).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the T7-temporary default! MeshData placeholder. Streamer
now takes Func<uint, LoadedLandblock?, LandblockMeshData?> at
construction; the worker calls it after _loadLandblock succeeds and
passes the pre-built mesh into LandblockStreamResult.Loaded.
GameWindow's buildMeshOrNull factory takes the already-loaded
LoadedLandblock (lb.Heightmap is the LandBlock dat object), so no
additional dat read is needed — _heightTable and _blendCtx are
read-only after init, _surfaceCache is ConcurrentDictionary (T9).
Zero dat lock needed inside the mesh-build closure.
StreamingController._applyTerrain delegate signature widened to
Action<LoadedLandblock, LandblockMeshData> so the pre-built mesh
flows render-thread-side via the Loaded result. ApplyLoadedTerrainLocked
now accepts meshData and calls _terrain.AddLandblock directly, skipping
the per-frame LandblockMesh.Build that previously ran on the render
thread (~5ms per LB at radius=12 first traversal).
StreamingControllerTests updated: all four applyTerrain lambdas
adapted to the two-arg Action signature.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase A.1 reverted to synchronous mode due to DatCollection thread-
safety; T10 documented the lock that makes concurrent reads safe. T11
activates the dedicated worker thread and switches enqueue methods
to non-blocking Channel.Writer.TryWrite.
EnqueueLoad now takes LandblockStreamJobKind (default: LoadNear from
all callers, matching previous full-load semantics). T13/T16 will
route by kind per TwoTierDiff.
Constructor gains optional buildMeshOrNull param (defaults to null-
returning stub); T12 wires the real LandblockMesh.Build factory.
GameWindow construction site updated: Action<uint> enqueueLoad
delegate now wraps a lambda (method group won't bind to Action<uint>
when the method has an optional second param).
LandblockStreamerTests updated: the synchronous-thread-pinning test
replaced by Load_ExecutesLoaderOnWorkerThread which asserts the
loader runs on a different thread; Load_FollowedByDrain now supplies
a stubMesh so the worker can produce Loaded (not Failed) results.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase A.5 T11 activates the LandblockStreamer worker thread, making
concurrent dat reads possible. DatReaderWriter's DatBinReader uses a
shared buffer position internally — concurrent _dats.Get<T> calls from
worker + render thread corrupt that state and produce half-populated
LandBlock.Height[] arrays (renders as wildly distorted terrain).
The _datLock field already existed from the Phase A.1 hotfix, and the
high-traffic worker-facing paths (BuildLandblockForStreaming,
ApplyLoadedTerrain, OnLiveEntitySpawned) already hold it. This commit
updates the field comment to precisely document the T10 contract:
all worker-thread dat reads enter via factory closures that acquire
_datLock; render-thread paths are already covered by their outer
lock wrappers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review on commits 295bce9/a0741bd/4be392b flagged 1 Important + 3
Minor issues. Apply the actionable two:
Important: 6 sites in GameWindow.cs (lines 3900, 4017-4024, 4138, 4270,
4315) wrote entity.Position = X directly, bypassing T8's SetPosition
mutator and therefore never marking AabbDirty. When T18 lands the
dispatcher's "if AabbDirty refresh" cull gate, these direct writes
would silently leave AABB stale (frustum culls dynamic entities at
their previous positions). Migrated all 6 sites to SetPosition().
Minor: Added a silent case LandblockStreamResult.Promoted arm in
StreamingController.Tick with a TODO(A.5 T13) marker. Today the
streamer never produces Promoted, so the arm is unreachable; the
explicit case prevents a future reader from wondering why the case
is missing.
Deferred Minor: surfaceCache thread-safety XML doc comment + style
consistency on System.Collections.Generic using directive — non-
load-bearing cosmetic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Widens LandblockMesh.Build's surfaceCache parameter from Dictionary to
IDictionary so any IDictionary implementation compiles at call sites.
Switches GameWindow._surfaceCache from Dictionary to ConcurrentDictionary
so T11's streaming worker can call Build off the render thread without
a lock.
The TryGetValue+assign lookup inside Build is not atomic, but BuildSurface
is deterministic (same palCode -> same SurfaceInfo), making last-write-wins
under concurrent access benign. Comment added at the pattern site.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds AabbMin/AabbMax (per-entity world-space bounding box) and AabbDirty
flag to WorldEntity. RefreshAabb() recomputes the box from Position ±5 m
(DefaultAabbRadius). SetPosition() writes Position and marks the cache
dirty so the dispatcher calls RefreshAabb on first read rather than
carrying stale bounds.
AabbDirty defaults to true on construction — freshly-built entities have
zero AabbMin/AabbMax until RefreshAabb is called. Two new conformance tests
verify the ±5 m geometry and the dirty/clean state machine.
Per Phase A.5 spec §4.6 Change #2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the Loaded result record with a LandblockStreamTier discriminator
and a LandblockMeshData payload (default! stub — T13 wires the real
off-thread mesh build). Adds the Promoted variant for Far→Near upgrades
that only need the entity layer, not a mesh rebuild.
LandblockStreamer.HandleJob passes Tier.Near + default! MeshData at the
existing synchronous load site; StreamingControllerTests updated to
match the new positional signature.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review on commits 7bcabab/fb6b61e/326b698 flagged 2 Important +
4 Minor issues. Apply all fixes:
Important:
- Two-tier RecenterTo + MarkResidentFromBootstrap now throw
InvalidOperationException on misuse — calling RecenterTo before the
bootstrap silently emitted the entire window as fresh loads (no
demotes/unloads since _tierResidence was empty), a correctness hazard
that produced no exception. Calling MarkResidentFromBootstrap twice
silently dropped accumulated tier state. Both now crash loudly via
a _bootstrapped flag.
- Dropped TierResidence.None from the enum — never assigned, never
checked; absence from the dictionary already encodes "not resident."
Minor:
- Renamed test: RecenterTo_FirstTick_* → ComputeFirstTickDiff_FirstTick_*
(the test calls ComputeFirstTickDiff, not RecenterTo).
- Strengthened RecenterTo_PlayerWalks_NullToFar_* with assertions for
ToPromote.Count==3 (the x=102 column promoting Far→Near) and
ToUnload.Empty (everything within hysteresis).
- Replaced System.Math.Abs with Math.Abs in new code to match the
file's existing `using System;` convention.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds TierResidence enum (None/Far/Near), _tierResidence dictionary seeded
by MarkResidentFromBootstrap, and the canonical two-tier RecenterTo overload
returning TwoTierDiff. Pass 1 walks the new far window and emits ToLoadFar /
ToLoadNear / ToPromote; Pass 2 walks prior residents and emits ToDemote /
ToUnload using Chebyshev hysteresis thresholds (NearRadius+2 / FarRadius+2).
EncodeLandblockIdForTest exposes the encoding rule to test assemblies.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the first-tick bootstrap diff: ToLoadNear for the (2*near+1)^2 inner
window, ToLoadFar for the outer annulus up to FarRadius. Uses Chebyshev
distance, matching existing Recenter convention.
Also renames the single-tier RecenterTo → RecenterToSingleTier to free
the canonical name for the upcoming two-tier overload (T5). Updates
StreamingRegionTests and StreamingController to call the renamed method.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add NearRadius/FarRadius properties and a four-arg constructor
(centerX, centerY, nearRadius, farRadius). Radius is set to farRadius
so existing hysteresis math (unload threshold = Radius+2) uses the
outer ring as the bookkeeping boundary. Old three-arg constructor
becomes a thin wrapper: this(cx, cy, radius, radius) — no behaviour
change, 25 pre-existing streaming tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review on commit 90a2027 flagged that HandleJob silently ignores
load.Kind. Add a TODO(A.5 T11/T16) comment at the case arm so the
unused field reads as a planned stub, not a bug.
No semantic change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds TwoTierDiff — the five-list output of StreamingRegion.RecenterTo
(ToLoadFar/Near, ToPromote, ToDemote, ToUnload) per spec §4.2. Used by
T3–T6 (StreamingRegion) and T13 (StreamingController).
Extends LandblockStreamJob.Load with a LandblockStreamJobKind parameter
so the streaming worker can route far vs near vs promote jobs differently
(spec §4.3). Patches the one call site in LandblockStreamer.EnqueueLoad
with LoadNear as a placeholder that preserves today's full-load semantics
until T11 activates the worker thread and T16 routes by tier.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
