From bf31e59805516397b2d3f8722e7738af2700e998 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 10 May 2026 16:03:16 +0200 Subject: [PATCH] =?UTF-8?q?fix(streaming):=20close=20#54=20=E2=80=94=20plu?= =?UTF-8?q?mb=20JobKind=20through=20BuildLandblockForStreaming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` factory with `Func` 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()`. 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) --- src/AcDream.App/Rendering/GameWindow.cs | 37 ++++++++++++-- .../Streaming/LandblockStreamer.cs | 51 ++++++++++++------- 2 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 5226921..149084d 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -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 (kind == LoadFar) 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 + /// with an + /// early-out at the source. /// - 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(landblockId); + if (heightmapOnly is null) return null; + return new AcDream.Core.World.LoadedLandblock( + landblockId, + heightmapOnly, + System.Array.Empty()); + } + 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) diff --git a/src/AcDream.App/Streaming/LandblockStreamer.cs b/src/AcDream.App/Streaming/LandblockStreamer.cs index 0811c8e..f71e0c0 100644 --- a/src/AcDream.App/Streaming/LandblockStreamer.cs +++ b/src/AcDream.App/Streaming/LandblockStreamer.cs @@ -52,7 +52,7 @@ public sealed class LandblockStreamer : IDisposable /// public const int DefaultDrainBatchSize = 4; - private readonly Func _loadLandblock; + private readonly Func _loadLandblock; private readonly Func _buildMeshOrNull; private readonly Channel _inbox; private readonly Channel _outbox; @@ -60,8 +60,15 @@ public sealed class LandblockStreamer : IDisposable private Thread? _worker; private int _disposed; + /// + /// Primary ctor — the factory takes the job's + /// 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 + /// LandBlockInfo + SceneryGenerator work. + /// public LandblockStreamer( - Func loadLandblock, + Func loadLandblock, Func? buildMeshOrNull = null) { _loadLandblock = loadLandblock; @@ -74,6 +81,19 @@ public sealed class LandblockStreamer : IDisposable new UnboundedChannelOptions { SingleReader = true, SingleWriter = true }); } + /// + /// 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. + /// + public LandblockStreamer( + Func loadLandblock, + Func? buildMeshOrNull = null) + : this((id, _) => loadLandblock(id), buildMeshOrNull) + { + } + /// /// 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,