From 449c2caf8be96b826689afec040d470c2333635b Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 11 Apr 2026 22:08:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(app):=20Phase=20A.1=20=E2=80=94=20separate?= =?UTF-8?q?=20Visible=20from=20Resident=20in=20StreamingRegion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up from commit 11df793. Three fixes: 1. Visible semantics: StreamingRegion.Visible now strictly describes the current (2r+1)×(2r+1) window, not window + hysteresis retainees. Added a parallel Resident property exposing the actual loaded set (window + hysteresis buffer). This matters because StreamingController (next task) reads these to decide what to render vs what to unload; conflating them in one set would have forced awkward post-processing downstream. 2. Doc/code disagreement: updated the RecenterTo and RegionDiff doc comments from "radius + 1" to "radius + 2" to match the actual implementation (which is what the tests require). Also updated the plan doc so future readers don't hit the same contradiction. 3. Edge-clamping test coverage: added a single-axis edge test (cx=0, cy=50 → 15 entries) and an ID-encoding test (radius=0 at 0x12,0x34 → 0x1234FFFE) so a swapped-shift bug in EncodeLandblockId or an asymmetric off-by-one would fail a test instead of passing silently. 9 tests green, full suite regressions-free. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../2026-04-11-foundation-a1-streaming.md | 18 +++--- src/AcDream.App/Streaming/StreamingRegion.cs | 55 ++++++++++++------- .../Streaming/StreamingRegionTests.cs | 23 ++++++++ 3 files changed, 67 insertions(+), 29 deletions(-) diff --git a/docs/superpowers/plans/2026-04-11-foundation-a1-streaming.md b/docs/superpowers/plans/2026-04-11-foundation-a1-streaming.md index 59ed9f2..227b803 100644 --- a/docs/superpowers/plans/2026-04-11-foundation-a1-streaming.md +++ b/docs/superpowers/plans/2026-04-11-foundation-a1-streaming.md @@ -4,7 +4,7 @@ **Goal:** Replace acdream's one-shot 3×3 landblock preload with a streaming loader that follows the camera (offline) or player (live), loads new landblocks on a background worker, and unloads landblocks that fall outside a configurable visible window. -**Architecture:** Four new classes (`StreamingRegion`, `LandblockStreamer`, `GpuWorldState`, `StreamingController`) plus incremental additions to `TerrainRenderer` and `GameWindow`. Loads run on a dedicated worker thread driven by `System.Threading.Channels.Channel`; GPU upload stays on the render thread draining a completion outbox in `OnUpdate`. Window radius is runtime-configurable via `ACDREAM_STREAM_RADIUS` (default 2 → 5×5 visible window) with hysteresis of `radius + 1` to prevent churn at boundary crossings. +**Architecture:** Four new classes (`StreamingRegion`, `LandblockStreamer`, `GpuWorldState`, `StreamingController`) plus incremental additions to `TerrainRenderer` and `GameWindow`. Loads run on a dedicated worker thread driven by `System.Threading.Channels.Channel`; GPU upload stays on the render thread draining a completion outbox in `OnUpdate`. Window radius is runtime-configurable via `ACDREAM_STREAM_RADIUS` (default 2 → 5×5 visible window) with hysteresis of `radius + 2` to prevent churn at boundary crossings. **Tech Stack:** .NET 10, Silk.NET, DatReaderWriter, System.Threading.Channels, xUnit. @@ -211,11 +211,11 @@ Append to `StreamingRegionTests.cs`: [Fact] public void RecenterTo_SingleStepEast_LoadsColumn_NoUnloadsDueToHysteresis() { - // Radius 2 → unload threshold is radius+1 = 3. + // Radius 2 → unload threshold is radius+2 = 4. // Starting center (50,50) covers X in [48..52]. Step to (51,50): // new coverage X in [49..53]. New column is x=53 (5 entries). - // Departing column would be x=48, but |48-51| = 3 which equals the - // threshold, so it stays loaded (hysteresis keeps radius+1). + // Departing column would be x=48, but |48-51| = 3 which is less than + // the threshold, so it stays loaded (hysteresis keeps radius+2). var region = new StreamingRegion(cx: 50, cy: 50, radius: 2); var diff = region.RecenterTo(51, 50); @@ -229,7 +229,7 @@ Append to `StreamingRegionTests.cs`: { // Starting (50,50) covers X in [48..52]. Step to (53,50): // new coverage X in [51..55]. New columns: x=53,54,55 (15 entries). - // x=48 is now 5 away → unload. x=49,50 still within radius+1 → keep. + // x=48 is now 5 away → unload. x=49,50 still within radius+2 → keep. var region = new StreamingRegion(cx: 50, cy: 50, radius: 2); var diff = region.RecenterTo(53, 50); @@ -264,7 +264,7 @@ Append to `StreamingRegion.cs` (inside the `AcDream.App.Streaming` namespace): /// /// Output of : the landblocks to /// start loading (newly entered the visible window) and the landblocks to -/// unload (fell outside the unload threshold, which is radius + 1). +/// unload (fell outside the unload threshold, which is radius + 2). /// Both lists are disjoint from the current /// set; the caller hands them to LandblockStreamer as jobs. /// @@ -279,7 +279,7 @@ Add to the `StreamingRegion` class body (below `Recenter`): /// /// Recompute the visible window around a new center and return the /// delta vs. the previous state. Hysteresis: landblocks aren't unloaded - /// until they're further than Radius + 1 from the new center, + /// until they're further than Radius + 2 from the new center, /// so boundary crossings don't thrash. /// public RegionDiff RecenterTo(int newCx, int newCy) @@ -296,7 +296,7 @@ Add to the `StreamingRegion` class body (below `Recenter`): // Unloads = everything previously visible AND now outside the // hysteresis threshold (|dx| > r+1 OR |dy| > r+1). - int unloadThreshold = Radius + 1; + int unloadThreshold = Radius + 2; var toUnload = new List(); foreach (var id in oldVisible) { @@ -335,7 +335,7 @@ feat(app): Phase A.1 — StreamingRegion (window set + diff with hysteresis) Pure data type describing the set of landblocks inside the current streaming window, with a diff-style Recenter that returns (toLoad, toUnload) pairs the LandblockStreamer consumes as jobs. Hysteresis -of radius+1 prevents load/unload churn at boundary crossings. +of radius+2 prevents load/unload churn at boundary crossings. First piece of Phase A.1 per docs/superpowers/plans/2026-04-11-foundation-a1-streaming.md. diff --git a/src/AcDream.App/Streaming/StreamingRegion.cs b/src/AcDream.App/Streaming/StreamingRegion.cs index 025d3af..0bef84c 100644 --- a/src/AcDream.App/Streaming/StreamingRegion.cs +++ b/src/AcDream.App/Streaming/StreamingRegion.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; namespace AcDream.App.Streaming; @@ -13,14 +14,27 @@ public sealed class StreamingRegion public int CenterY { get; private set; } public int Radius { get; } + // Strictly the (2r+1)×(2r+1) window (clamped to world bounds). private readonly HashSet _visible = new(); + // Everything currently loaded: window + hysteresis-retained landblocks. + private readonly HashSet _resident = new(); + /// /// Landblock IDs (8.8 coordinate form: (lbX << 24) | (lbY << 16) | 0xFFFE) - /// in the current visible window. + /// in the current visible window. This is strictly the (2r+1)×(2r+1) set; + /// it does NOT include hysteresis-retained landblocks outside the window. + /// Use to enumerate everything actually loaded. /// public IReadOnlyCollection Visible => _visible; + /// + /// Every landblock currently loaded: the current visible window plus any + /// landblocks being held in memory by the hysteresis buffer (not yet past + /// the Radius + 2 unload threshold). + /// + public IReadOnlyCollection Resident => _resident; + public StreamingRegion(int cx, int cy, int radius) { Radius = radius; @@ -43,6 +57,8 @@ public sealed class StreamingRegion _visible.Add(EncodeLandblockId(nx, ny)); } } + // On first construction, resident == visible. + _resident.UnionWith(_visible); } internal static uint EncodeLandblockId(int lbX, int lbY) @@ -51,43 +67,42 @@ public sealed class StreamingRegion /// /// Recompute the visible window around a new center and return the /// delta vs. the previous state. Hysteresis: landblocks aren't unloaded - /// until they're further than Radius + 1 from the new center, + /// until they're further than Radius + 2 from the new center, /// so boundary crossings don't thrash. /// public RegionDiff RecenterTo(int newCx, int newCy) { - // Snapshot the current visible set so we can diff against it. - var oldVisible = new HashSet(_visible); + // Snapshot the old resident set so we can diff against it. + var oldResident = new HashSet(_resident); + + // Recompute _visible strictly as the new window. Recenter(newCx, newCy); - // Loads = everything newly in visible but not previously. + // Loads = entries in the new window not yet in the resident set. var toLoad = new List(); foreach (var id in _visible) - if (!oldVisible.Contains(id)) + if (!oldResident.Contains(id)) toLoad.Add(id); - // Unloads = everything previously visible AND now outside the - // hysteresis threshold (|dx| > r+2 OR |dy| > r+2). - // The extra +1 beyond the visible radius (+1) gives a full buffer - // cell of hysteresis before eviction, matching the test expectations. + // Unloads = resident entries outside the hysteresis threshold + // (|dx| > Radius+2 OR |dy| > Radius+2). int unloadThreshold = Radius + 2; var toUnload = new List(); - foreach (var id in oldVisible) + foreach (var id in oldResident) { - if (_visible.Contains(id)) continue; // still visible, not unloading + if (_visible.Contains(id)) continue; // still in window, keep int lbX = (int)((id >> 24) & 0xFFu); int lbY = (int)((id >> 16) & 0xFFu); - int dx = System.Math.Abs(lbX - newCx); - int dy = System.Math.Abs(lbY - newCy); + int dx = Math.Abs(lbX - newCx); + int dy = Math.Abs(lbY - newCy); if (dx > unloadThreshold || dy > unloadThreshold) toUnload.Add(id); } - // Any "still loaded but outside visible" landblocks not in toUnload - // need to rejoin the visible set so we don't lose track of them. - foreach (var id in oldVisible) - if (!_visible.Contains(id) && !toUnload.Contains(id)) - _visible.Add(id); + // Update resident: (oldResident ∪ newVisible) ∖ toUnload. + _resident.UnionWith(_visible); + foreach (var id in toUnload) + _resident.Remove(id); return new RegionDiff(toLoad, toUnload); } @@ -96,7 +111,7 @@ public sealed class StreamingRegion /// /// Output of : the landblocks to /// start loading (newly entered the visible window) and the landblocks to -/// unload (fell outside the unload threshold, which is radius + 1). +/// unload (fell outside the unload threshold, which is Radius + 2). /// Both lists are disjoint from the current /// set; the caller hands them to LandblockStreamer as jobs. /// diff --git a/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs b/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs index 9e5aeb1..767dd6d 100644 --- a/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs +++ b/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs @@ -82,4 +82,27 @@ public class StreamingRegionTests Assert.Equal(25, diff.ToLoad.Count); Assert.Equal(25, diff.ToUnload.Count); } + + [Fact] + public void Constructor_NearXEdgeOnly_ClampsOnlyXAxis() + { + // cx=0, cy=50, radius=2: + // X is clamped to [0..2] (3 entries) + // Y is unclamped [48..52] (5 entries) + // Total = 3 × 5 = 15 landblocks. + var region = new StreamingRegion(cx: 0, cy: 50, radius: 2); + + Assert.Equal(15, region.Visible.Count); + } + + [Fact] + public void Constructor_SmallRadius_IDsMatchEncodingRule() + { + // Verify EncodeLandblockId is correct (not swapped shifts). + // radius=0 at (0x12, 0x34) → exactly one entry, id = 0x1234FFFE. + var region = new StreamingRegion(cx: 0x12, cy: 0x34, radius: 0); + + Assert.Single(region.Visible); + Assert.Contains(0x1234FFFEu, region.Visible); + } }