Merge branch 'claude/cranky-varahamihira-fe423f' — Post-A.5 polish: close #52 (lifestone) + #54 (JobKind plumbing)

This commit is contained in:
Erik 2026-05-10 16:08:32 +02:00
commit da08490ab0
7 changed files with 175 additions and 71 deletions

View file

@ -525,10 +525,14 @@ acdream's plan lives in two files committed to the repo:
acceptance criteria. Do not drift from the spec without explicit user
approval.
**Currently in flight: Post-A.5 polish — Tier 1 retry + lifestone fix + JobKind plumbing.**
Open issues: #52 (lifestone missing), #53 (Tier 1 entity cache redo with animation-mutation
audit), #54 (JobKind plumbing through BuildLandblockForStreaming for proper far-tier skip).
After those three close, the next planned phase is N.6 (perf polish) — see roadmap for scope.
**Currently in flight: Post-A.5 polish — Tier 1 retry (only remaining priority).**
Open issues: #53 (Tier 1 entity cache redo with animation-mutation audit).
ISSUES #52 (lifestone missing) and #54 (JobKind plumbing) closed 2026-05-10. #52 by
commit `e40159f` — three real bugs in the WB rendering migration's translucent pass
(alpha-test discard, missing cull state, missing `uDrawIDOffset` uniform). #54 by
commit `bf31e59``LandblockStreamJobKind` plumbed through `BuildLandblockForStreaming`,
far-tier worker now does heightmap-only load (no `LandBlockInfo`, no `SceneryGenerator`).
After #53 closes, the next planned phase is N.6 (perf polish) — see roadmap for scope.
**Phase A.5 (Two-tier Streaming + Horizon LOD) shipped 2026-05-10.**
N₁=4 near-tier (81 LBs, full detail) + N₂=12 far-tier (544 LBs, terrain only); fog

View file

@ -46,24 +46,6 @@ Copy this block when adding a new issue:
# Active issues
## #54 — A.5/jobkind-plumbing: far-tier worker loads full entity layer then strips
**Status:** OPEN
**Severity:** LOW (correctness/perf; worker wastes CPU on far-tier LandBlockInfo + scenery generation that is immediately discarded)
**Filed:** 2026-05-10
**Component:** streaming / LandblockStreamer
**Description:** Bug A's fix (commit `9217fd9`) patches at the worker output — after a far-tier job completes the full `LoadNear` path, the result's entity list is stripped before posting to the completion queue. This means far-tier LBs still load `LandBlockInfo` + run `SceneryGenerator` + call `LandblockLoader.BuildEntitiesFromInfo` even though those results are thrown away. At N₂=12, that is ~544 far-tier LBs × unnecessary dat reads + scenery math on promotion sequences.
**Proper fix:** plumb `LandblockStreamJobKind` through `BuildLandblockForStreaming` so far-tier jobs call only `LandBlock` heightmap read + `LandblockMesh.Build`, skipping `LandBlockInfo` + `SceneryGenerator` entirely. The function signature change is ~5 lines; wiring is ~10 lines. Estimated 30 min1 hour total.
**Files:**
- `src/AcDream.App/Streaming/LandblockStreamer.cs``HandleJob` + `BuildLandblockForStreaming`
**Acceptance:** Far-tier LB worker path reads only the `LandBlock` dat file (no `LandBlockInfo`, no `SceneryGenerator` call). Verified by adding a counter diagnostic or via dotnet-trace showing the dat-read call count per job kind.
---
## #53 — A.5/tier1-redo: entity-classification cache broke animation (reverted)
**Status:** OPEN
@ -90,30 +72,6 @@ Copy this block when adding a new issue:
---
## #52 — A.5/lifestone-missing: Holtburg lifestone not rendering
**Status:** OPEN
**Severity:** MEDIUM (visible missing landmark; lifestone is the player's respawn anchor and should always be visible)
**Filed:** 2026-05-10
**Component:** streaming / rendering
**Description:** The Holtburg lifestone (spinning blue crystal) has not rendered since earlier in A.5 development. Reproduce: launch live client, walk to Holtburg town center, look toward the lifestone position. Should see the spinning blue crystal; instead see nothing.
**Root cause (suspected, two candidates):**
1. Bug A's far-tier strip (commit `9217fd9`) may be incorrectly stripping a near-tier entity. The lifestone's server GUID is `0x5000000A`; its dat object may be registering via the `LandBlockInfo` path but getting stripped as if it were a far-tier entity due to a tier-classification race or incorrect LB-tier tracking.
2. Separate regression from earlier in the A.5 development chain — possibly introduced when entity registration was restructured during T13/T16 streaming controller wiring.
**Investigation approach:**
1. Add a `[STREAMING-DIAG]` log line when far-tier stripping drops an entity — log the entity's GfxObj ID and LB address so the lifestone's GfxObj ID appears in the log if it is being stripped.
2. If not in the strip log, check whether the lifestone's LB is registering as near-tier at all during first-tick bootstrap.
3. Bisect to find the commit that broke it if the above two checks don't isolate the cause.
**Acceptance:** Launch live, walk to Holtburg center, spinning blue crystal visible at the lifestone position. No regression on other static entities in the area.
---
## #50 — Road-edge tree at 0xA9B1 visible in acdream but not retail
**Status:** OPEN
@ -1745,6 +1703,38 @@ Unverified. The likely culprits, ranked by suspected probability:
# Recently closed
## #54 — [DONE 2026-05-10 · bf31e59] A.5/jobkind-plumbing: far-tier worker loads full entity layer then strips
**Closed:** 2026-05-10
**Commits:** `bf31e59` (factory signature change to 2-arg + back-compat overload + far-tier early-out)
**Component:** streaming / LandblockStreamer
**Resolution.** `LandblockStreamer.cs` primary ctor now takes `Func<uint, LandblockStreamJobKind, LoadedLandblock?>` so the factory can branch on the job kind. A back-compat overload preserves the old single-arg signature for existing test code (5 ctor sites in `LandblockStreamerTests.cs` resolved to the overload with no test changes). `BuildLandblockForStreaming(uint, JobKind)` in `GameWindow.cs` early-outs for `LoadFar` with a heightmap-only path (`_dats.Get<LandBlock>(landblockId)` + `Array.Empty<WorldEntity>()`); near-tier path is unchanged. The Bug A post-load entity strip in `LandblockStreamer.HandleJob` is retained as a `Debug.Assert` + Release safety net. Per-LB worker cost on far-tier dropped from ~tens of ms (LandBlockInfo + scenery + interior) to ~sub-ms (single LandBlock dat read).
**Verification.** Build green; 1688/1696 tests pass (8 pre-existing physics/input failures unchanged); 30 streaming-targeted tests (LandblockStreamer + StreamingController + StreamingRegion) all green via the back-compat overload.
---
## #52 — [DONE 2026-05-10 · e40159f] A.5/lifestone-missing: Holtburg lifestone not rendering
**Closed:** 2026-05-10
**Commits:** `e40159f` (alpha-test discard removal + cull state restoration + uDrawIDOffset uniform)
**Component:** rendering / WbDrawDispatcher / shaders
**Resolution.** Three independent root causes regressed with the WB rendering migration (Phase N.5 retirement amendment, commit `dcae2b6`, 2026-05-08). The original ISSUE #52 hypothesis (Bug A far-tier strip catching the lifestone) was wrong — the lifestone is server-spawned (WCID 509, Setup `0x020002EE`) and never goes through the far-tier strip. Real causes:
1. **Alpha-test discard.** `mesh_modern.frag` transparent pass discarded fragments with `α >= 0.95`. The lifestone crystal core surface `0x080011DE` decoded with α≥0.95 across its visible surface, so 100% of the crystal's fragments were discarded — invisible. The original N.5 §2 rationale ("high-α belongs in opaque pass") doesn't hold for surfaces dat-flagged transparent: those pixels can't reach the opaque pass at all. Fix: remove the high-α discard from the transparent pass; keep `α < 0.05` as a fragment-cost optimization.
2. **Cull state regression.** Legacy `StaticMeshRenderer` had Phase 9.2's `Enable(CullFace) + Back + CCW` setup at the top of its translucent pass (commit `6f1971a`, 2026-04-11) — fix for "lifestone crystal one face missing" reported at the time. When `dcae2b6` deleted the legacy renderer, the new `WbDrawDispatcher` never inherited that GL state, so closed-shell translucents composited back-faces over front-faces in iteration order under `DepthMask(false)`. Fix: re-establish Phase 9.2's exact setup at the top of Phase 8.
3. **`uDrawIDOffset` indexing bug.** `gl_DrawIDARB` resets to 0 at the start of each `glMultiDrawElementsIndirect` call. The transparent pass starts at byte offset `_opaqueDrawCount * stride` in the indirect buffer, but the vertex shader read `Batches[gl_DrawIDARB]` directly — so transparent draws read from `Batches[0..transparentCount)` (the OPAQUE section) instead of `Batches[opaqueCount..end)`. The lifestone crystal's apparent texture flickered to whatever opaque batch sorted to index 0 each frame; with the player character in view, this often appeared as a lifestone wearing the player's body / face textures. Fix: add `uniform int uDrawIDOffset` to `mesh_modern.vert`, change `Batches[gl_DrawIDARB]` to `Batches[uDrawIDOffset + gl_DrawIDARB]`, and set the uniform per-pass in `WbDrawDispatcher` (0 for opaque, `_opaqueDrawCount` for transparent). Mirrors WorldBuilder's `BaseObjectRenderManager.cs:845`.
**Verification.** User-confirmed visually via `+Acdream` test character at the Holtburg outdoor lifestone (Z=94 platform). Tests 1688/1696 passing (8 pre-existing physics/input failures unchanged). N.5b conformance sentinel 94/94 clean.
**Lesson.** The WB rendering migration's "lift legacy state into the new dispatcher" was incomplete in two non-obvious ways: (a) GL state setup that lived inside legacy per-pass blocks, and (b) shader uniforms that the legacy per-draw flow didn't need but the multi-draw-indirect flow does. Future WB-migration work should systematically diff the legacy renderer's GL setup + shader I/O against the new dispatcher's. The `uDrawIDOffset` bug was particularly hidden because it only manifested for entities that mixed transparent draws with the visible opaque sort order — single-pass content (pure opaque or pure transparent) was unaffected.
---
## #13 — [DONE 2026-05-10 · d3b58c9..078919c] PlayerDescription trailer past enchantments
**Closed:** 2026-05-10

View file

@ -1652,7 +1652,7 @@ public sealed class GameWindow : IDisposable
// it can call LandblockMesh.Build without a dat read — _heightTable and
// _blendCtx are read-only after init, _surfaceCache is ConcurrentDictionary (T9).
_streamer = new AcDream.App.Streaming.LandblockStreamer(
loadLandblock: id => BuildLandblockForStreaming(id),
loadLandblock: (id, kind) => BuildLandblockForStreaming(id, kind),
buildMeshOrNull: (id, lb) =>
{
if (lb is null || _heightTable is null || _blendCtx is null || _surfaceCache is null)
@ -4639,8 +4639,18 @@ public sealed class GameWindow : IDisposable
/// DatReaderWriter) and pure CPU work. No GL calls here.
///
/// MVP scope: stabs only. Scenery + interior added in Task 8.
///
/// ISSUE #54 (post-A.5): far-tier loads (<c>kind == LoadFar</c>) skip
/// LandBlockInfo + scenery + interior hydration. They return only the
/// LandBlock heightmap dat record + an empty entity list — enough for
/// terrain-mesh build on the next phase. Near-tier loads run the full
/// path. This replaces Bug A's post-load entity strip in
/// <see cref="AcDream.App.Streaming.LandblockStreamer"/> with an
/// early-out at the source.
/// </summary>
private AcDream.Core.World.LoadedLandblock? BuildLandblockForStreaming(uint landblockId)
private AcDream.Core.World.LoadedLandblock? BuildLandblockForStreaming(
uint landblockId,
AcDream.App.Streaming.LandblockStreamJobKind kind)
{
if (_dats is null) return null;
@ -4653,14 +4663,31 @@ public sealed class GameWindow : IDisposable
// contention by pre-building render-thread work on the worker.
lock (_datLock)
{
return BuildLandblockForStreamingLocked(landblockId);
return BuildLandblockForStreamingLocked(landblockId, kind);
}
}
private AcDream.Core.World.LoadedLandblock? BuildLandblockForStreamingLocked(uint landblockId)
private AcDream.Core.World.LoadedLandblock? BuildLandblockForStreamingLocked(
uint landblockId,
AcDream.App.Streaming.LandblockStreamJobKind kind)
{
if (_dats is null) return null;
// ISSUE #54: far-tier early-out — heightmap only, empty entities.
// Skips the LandBlockInfo dat read AND all entity hydration (stabs
// + buildings) AND the SceneryGenerator AND interior cells. Cuts
// worker-thread cost per far-tier LB from ~tens of ms to a single
// dat read.
if (kind == AcDream.App.Streaming.LandblockStreamJobKind.LoadFar)
{
var heightmapOnly = _dats.Get<DatReaderWriter.DBObjs.LandBlock>(landblockId);
if (heightmapOnly is null) return null;
return new AcDream.Core.World.LoadedLandblock(
landblockId,
heightmapOnly,
System.Array.Empty<AcDream.Core.World.WorldEntity>());
}
var baseLoaded = AcDream.Core.World.LandblockLoader.Load(_dats, landblockId);
if (baseLoaded is null) return null;
@ -8157,7 +8184,7 @@ public sealed class GameWindow : IDisposable
_streamer.Dispose();
_streamer = new AcDream.App.Streaming.LandblockStreamer(
loadLandblock: id => BuildLandblockForStreaming(id),
loadLandblock: (id, kind) => BuildLandblockForStreaming(id, kind),
buildMeshOrNull: (id, lb) =>
{
if (lb is null || _heightTable is null || _blendCtx is null || _surfaceCache is null)

View file

@ -86,8 +86,24 @@ void main() {
if (uRenderPass == 0) {
if (color.a < 0.05) discard; // opaque pass — kill truly empty only (A2C)
} else {
if (color.a >= 0.95) discard; // transparent pass
if (color.a < 0.05) discard; // skip totally-empty
// Transparent pass.
//
// Phase Post-A.5 (ISSUE #52, 2026-05-10): do NOT discard α≥0.95 here.
// Native AC transparent-flagged surfaces routinely include
// effectively-opaque pixels — e.g. the Holtburg lifestone crystal core
// (surface 0x080011DE) which the spawn manifest classifies as
// transparent (batch.IsTransparent=True) but whose decoded texture
// alpha lands ≥0.95 across the visible surface. Those pixels still
// compose correctly under (SrcAlpha, 1-SrcAlpha) alpha-blending, so
// discarding them here threw away the whole crystal. The original
// N.5 §2 rationale (high-α fragments belong in the opaque pass) does
// not apply when the SURFACE is dat-flagged transparent — those
// pixels can't reach the opaque pass at all.
//
// Keep the α<0.05 short-circuit as a fragment-cost optimization
// (skip fully-empty pixels — saves blend bandwidth on alpha-keyed
// sprites with large transparent margins).
if (color.a < 0.05) discard;
}
vec3 N = normalize(vNormal);

View file

@ -39,6 +39,21 @@ layout(std430, binding = 1) readonly buffer BatchBuffer {
uniform mat4 uViewProjection;
// Phase Post-A.5 (ISSUE #52, 2026-05-10): per-pass offset into Batches[].
// 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 (a transparent draw)
// ended up reading the FIRST OPAQUE batch's TextureHandle every frame. As
// the camera moved and the opaque front-to-back sort reordered which group
// landed at BatchData[0], the lifestone's apparent texture flickered to
// whatever was first — frequently the player character's body parts.
//
// WbDrawDispatcher.Draw sets this to 0 before the opaque MDI call and to
// _opaqueDrawCount before the transparent MDI call, matching WorldBuilder's
// uDrawIDOffset pattern in BaseObjectRenderManager.cs line 845.
uniform int uDrawIDOffset;
out vec3 vNormal;
out vec2 vTexCoord;
out vec3 vWorldPos;
@ -56,7 +71,7 @@ void main() {
vNormal = normalize(mat3(model) * aNormal);
vTexCoord = aTexCoord;
BatchData b = Batches[gl_DrawIDARB];
BatchData b = Batches[uDrawIDOffset + gl_DrawIDARB];
vTextureHandle = b.textureHandle;
vTextureLayer = b.textureLayer;
}

View file

@ -544,6 +544,10 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
// (no MSAA) skip the unnecessary GL state change.
if (AlphaToCoverage) _gl.Enable(EnableCap.SampleAlphaToCoverage);
_shader.SetInt("uRenderPass", 0);
// Phase Post-A.5 (ISSUE #52, 2026-05-10): opaque section of
// Batches[] starts at index 0. See uDrawIDOffset comment in
// mesh_modern.vert for why this is needed.
_shader.SetInt("uDrawIDOffset", 0);
_gl.BindBuffer(BufferTargetARB.DrawIndirectBuffer, _indirectBuffer);
if (diag && _gpuQueriesInitialized) _gl.BeginQuery(QueryTarget.TimeElapsed, _gpuQueryOpaque);
_gl.MultiDrawElementsIndirect(
@ -562,6 +566,37 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
_gl.Enable(EnableCap.Blend);
_gl.BlendFunc(BlendingFactor.SrcAlpha, BlendingFactor.OneMinusSrcAlpha);
_gl.DepthMask(false);
// Phase Post-A.5 (ISSUE #52, 2026-05-10): transparent section of
// Batches[] starts at index _opaqueDrawCount. Without this offset,
// each transparent draw reads BatchData[0..transparentCount) — the
// OPAQUE section — and the lifestone crystal's apparent texture
// flickers to whatever opaque batch sorted first that frame. See
// uDrawIDOffset comment in mesh_modern.vert.
_shader.SetInt("uDrawIDOffset", _opaqueDrawCount);
// Phase Post-A.5 (ISSUE #52, 2026-05-10): re-establish Phase 9.2's
// back-face cull setup. The legacy StaticMeshRenderer had this
// (commit 6f1971a, 2026-04-11) until the N.5 retirement amendment
// (commit dcae2b6, 2026-05-08) deleted that renderer; the new
// WbDrawDispatcher never inherited the cull-face state.
//
// Closed-shell translucent meshes — lifestone crystal, glow gems,
// any convex blended mesh — NEED back-face culling in the
// translucent pass. Without it, back faces composite OVER front
// faces in arbitrary iteration order, because DepthMask(false)
// means nothing records depth within the translucent set. The
// result is the user-visible "one face missing, see into the
// hollow interior" + frame-to-frame color flicker as rotation
// shifts the triangle order.
//
// Our fan triangulation emits pos-side polygons as (0, i, i+1) —
// CCW in standard OpenGL conventions — so GL_BACK + CCW-front is
// the correct state. Matches WorldBuilder's per-batch CullMode
// handling. Neg-side polygons (rare on translucent AC content)
// use reversed winding and get culled here, matching the opaque
// pass and the original Phase 9.2 fix's known limitation.
_gl.Enable(EnableCap.CullFace);
_gl.CullFace(TriangleFace.Back);
_gl.FrontFace(FrontFaceDirection.Ccw);
_shader.SetInt("uRenderPass", 1);
if (diag && _gpuQueriesInitialized) _gl.BeginQuery(QueryTarget.TimeElapsed, _gpuQueryTransparent);
_gl.MultiDrawElementsIndirect(

View file

@ -52,7 +52,7 @@ public sealed class LandblockStreamer : IDisposable
/// </summary>
public const int DefaultDrainBatchSize = 4;
private readonly Func<uint, LoadedLandblock?> _loadLandblock;
private readonly Func<uint, LandblockStreamJobKind, LoadedLandblock?> _loadLandblock;
private readonly Func<uint, LoadedLandblock?, AcDream.Core.Terrain.LandblockMeshData?> _buildMeshOrNull;
private readonly Channel<LandblockStreamJob> _inbox;
private readonly Channel<LandblockStreamResult> _outbox;
@ -60,8 +60,15 @@ public sealed class LandblockStreamer : IDisposable
private Thread? _worker;
private int _disposed;
/// <summary>
/// Primary ctor — the factory takes the job's <see cref="LandblockStreamJobKind"/>
/// so it can branch on far-tier vs near-tier and skip entity hydration on far-tier
/// loads (heightmap-only). See ISSUE #54: prior to this signature the worker always
/// called the full-load path and stripped entities at the output, wasting per-LB
/// <c>LandBlockInfo</c> + <c>SceneryGenerator</c> work.
/// </summary>
public LandblockStreamer(
Func<uint, LoadedLandblock?> loadLandblock,
Func<uint, LandblockStreamJobKind, LoadedLandblock?> loadLandblock,
Func<uint, LoadedLandblock?, AcDream.Core.Terrain.LandblockMeshData?>? buildMeshOrNull = null)
{
_loadLandblock = loadLandblock;
@ -74,6 +81,19 @@ public sealed class LandblockStreamer : IDisposable
new UnboundedChannelOptions { SingleReader = true, SingleWriter = true });
}
/// <summary>
/// Back-compat overload — wraps a kind-agnostic factory so existing test code
/// that doesn't care about the JobKind branch keeps compiling. The wrapper
/// ignores the kind and calls the factory once per LB regardless of tier.
/// New production code should use the primary 2-arg ctor.
/// </summary>
public LandblockStreamer(
Func<uint, LoadedLandblock?> loadLandblock,
Func<uint, LoadedLandblock?, AcDream.Core.Terrain.LandblockMeshData?>? buildMeshOrNull = null)
: this((id, _) => loadLandblock(id), buildMeshOrNull)
{
}
/// <summary>
/// Activate the dedicated background worker thread. Idempotent and
/// thread-safe: concurrent callers will only spawn one worker; subsequent
@ -177,22 +197,15 @@ public sealed class LandblockStreamer : IDisposable
switch (job)
{
case LandblockStreamJob.Load load:
// A.5 T26 follow-up (Bug A): far-tier LBs must NOT contribute
// entities to GpuWorldState — that defeats the whole purpose of
// the two-tier split. The factory still builds the full entity
// layer (LandblockLoader + scenery generation + interior cells)
// regardless of Kind because it doesn't know about JobKind today.
// We strip Entities here for far-tier results so the render-
// thread dispatcher walks only near-tier (~10K) entities, not
// all (~71K) entities at radius=12.
//
// Wasted worker-thread CPU is acceptable (it's off the render
// thread). A future optimization (TODO N.6 or A.6) plumbs Kind
// through BuildLandblockForStreaming so the dat read + scenery
// generation are skipped entirely for far-tier.
// ISSUE #54 (post-A.5): JobKind is now plumbed through to the
// factory, so far-tier loads can skip LandBlockInfo + scenery
// + interior hydration on the worker thread (heightmap-only).
// The post-load entity-strip below is retained as a Debug
// assertion + Release safety net for the case where a buggy
// factory returns far-tier with entities anyway.
try
{
var lb = _loadLandblock(load.LandblockId);
var lb = _loadLandblock(load.LandblockId, load.Kind);
if (lb is null)
{
_outbox.Writer.TryWrite(new LandblockStreamResult.Failed(
@ -210,7 +223,11 @@ public sealed class LandblockStreamer : IDisposable
? LandblockStreamTier.Far : LandblockStreamTier.Near;
if (tier == LandblockStreamTier.Far && lb.Entities.Count > 0)
{
// Strip entities — far-tier ships terrain only.
// Belt-and-suspenders: factory should have skipped
// entity hydration for LoadFar. If it didn't, fail
// loud in Debug builds and strip in Release.
System.Diagnostics.Debug.Assert(false,
$"Far-tier factory should skip entity hydration; got {lb.Entities.Count} entities for LB 0x{load.LandblockId:X8}");
lb = new LoadedLandblock(
lb.LandblockId,
lb.Heightmap,