diff --git a/src/AcDream.App/Streaming/LandblockStreamer.cs b/src/AcDream.App/Streaming/LandblockStreamer.cs index 65b2007..fff7fc6 100644 --- a/src/AcDream.App/Streaming/LandblockStreamer.cs +++ b/src/AcDream.App/Streaming/LandblockStreamer.cs @@ -8,22 +8,39 @@ using AcDream.Core.World; namespace AcDream.App.Streaming; /// -/// Background worker that services landblock load and unload requests off -/// the render thread. Loads are executed on a dedicated thread via a -/// caller-supplied delegate (the production instance wraps -/// ); completed results are posted to -/// an outbox channel the render thread drains once per OnUpdate. +/// Services landblock load/unload requests by invoking a caller-supplied +/// load delegate (the production instance wraps +/// ) and posting results to an outbox +/// the render thread drains once per OnUpdate. /// /// -/// Unloads are passed through the same channel as a -/// record so the render thread can release GPU state on the next drain — -/// the worker never touches GPU resources directly. +/// Currently runs synchronously on the calling thread. The original +/// Phase A.1 design ran loads on a dedicated worker thread, but DatReaderWriter's +/// DatCollection is not thread-safe — concurrent reads from a worker +/// and the render thread (animation tick, live spawn handlers) corrupt +/// internal buffer state and produce half-populated LandBlock.Height[] +/// arrays which render as wildly distorted terrain. Until Phase A.3 introduces +/// a thread-safe dat wrapper, loads are synchronous: +/// invokes the load delegate inline and writes the result to the outbox in +/// a single call. This causes a frame hitch when crossing landblock +/// boundaries, but the rendering is correct. +/// +/// +/// +/// The Channel-based outbox + API is +/// preserved so the move back to async loading is a single-class change +/// when DatCollection thread safety lands. +/// +/// +/// +/// Unloads pass through the outbox as +/// records so the render thread can release GPU state on the next drain — +/// the streamer never touches GPU resources directly. /// /// /// -/// Threading: / may -/// be called from any thread; must be called -/// from a single consumer thread (the render thread in production). +/// Threading: synchronous mode means all methods must be called from the +/// same thread (the render thread in production). /// /// public sealed class LandblockStreamer : IDisposable @@ -39,7 +56,9 @@ public sealed class LandblockStreamer : IDisposable private readonly Channel _inbox; private readonly Channel _outbox; private readonly CancellationTokenSource _cancel = new(); +#pragma warning disable CS0649 // _worker stays declared for the future async path; unused in synchronous mode. private Thread? _worker; +#pragma warning restore CS0649 private int _disposed; public LandblockStreamer(Func loadLandblock) @@ -52,34 +71,31 @@ public sealed class LandblockStreamer : IDisposable } /// - /// Start the worker thread. Must be called before enqueueing jobs. - /// Calling twice is a no-op. + /// No-op in synchronous mode. Preserved on the API surface so callers + /// don't need to change when async loading returns in Phase A.3. /// public void Start() { if (System.Threading.Volatile.Read(ref _disposed) != 0) throw new ObjectDisposedException(nameof(LandblockStreamer)); - if (_worker is not null) return; - _worker = new Thread(WorkerLoop) - { - IsBackground = true, - Name = "acdream.landblock-streamer", - }; - _worker.Start(); + // No worker thread in synchronous mode. } public void EnqueueLoad(uint landblockId) { if (System.Threading.Volatile.Read(ref _disposed) != 0) throw new ObjectDisposedException(nameof(LandblockStreamer)); - _inbox.Writer.TryWrite(new LandblockStreamJob.Load(landblockId)); + // Synchronous mode: invoke the load delegate inline. The result lands + // in the outbox and DrainCompletions picks it up later in the same + // (or next) frame. + HandleJob(new LandblockStreamJob.Load(landblockId)); } public void EnqueueUnload(uint landblockId) { if (System.Threading.Volatile.Read(ref _disposed) != 0) throw new ObjectDisposedException(nameof(LandblockStreamer)); - _inbox.Writer.TryWrite(new LandblockStreamJob.Unload(landblockId)); + HandleJob(new LandblockStreamJob.Unload(landblockId)); } /// diff --git a/tests/AcDream.Core.Tests/Streaming/LandblockStreamerTests.cs b/tests/AcDream.Core.Tests/Streaming/LandblockStreamerTests.cs index 491ca6b..e058f81 100644 --- a/tests/AcDream.Core.Tests/Streaming/LandblockStreamerTests.cs +++ b/tests/AcDream.Core.Tests/Streaming/LandblockStreamerTests.cs @@ -104,8 +104,16 @@ public class LandblockStreamerTests } [Fact] - public async Task Load_ExecutesLoaderOnBackgroundThread() + public void Load_ExecutesLoaderSynchronously_OnCallingThread() { + // Streamer was made synchronous after Phase A.1 visual verification + // exposed concurrent dat reads as the cause of "ball of spikes" + // terrain corruption — DatReaderWriter's DatCollection isn't + // thread-safe and locking around every dat read on every render- + // thread code path was too invasive. Until Phase A.3 introduces a + // thread-safe dat wrapper, the load delegate runs on the calling + // thread and the result is in the outbox by the time EnqueueLoad + // returns. This test pins that contract. int testThreadId = System.Environment.CurrentManagedThreadId; int? loaderThreadId = null; var stubLandblock = new LoadedLandblock( @@ -122,14 +130,11 @@ public class LandblockStreamerTests streamer.Start(); streamer.EnqueueLoad(0x77770FFEu); - // Drain until we see the completion. - for (int i = 0; i < SpinMaxIterations && loaderThreadId is null; i++) - { - streamer.DrainCompletions(LandblockStreamer.DefaultDrainBatchSize); - if (loaderThreadId is null) await Task.Delay(SpinStepMs); - } + // Result is already in the outbox — no spinning needed. + var drained = streamer.DrainCompletions(LandblockStreamer.DefaultDrainBatchSize); - Assert.NotNull(loaderThreadId); - Assert.NotEqual(testThreadId, loaderThreadId.Value); + Assert.Single(drained); + Assert.IsType(drained[0]); + Assert.Equal(testThreadId, loaderThreadId); } }