From f83a8c1674e62aeeb3a87c6157121c58cf273054 Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 11 Apr 2026 22:59:21 +0200 Subject: [PATCH] =?UTF-8?q?fix(app):=20Phase=20A.1=20=E2=80=94=20encode=20?= =?UTF-8?q?landblock=20IDs=20with=200xFFFF=20terminator,=20not=200xFFFE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ROOT CAUSE of the "giant ball with spikes" terrain corruption that the previous two hotfix attempts (lock + synchronous loading) failed to address. Threading was a red herring all along. AC dat conventions: 0xAAAA0xFFFF — LandBlock dat (terrain heightmap) 0xAAAA0xFFFE — LandBlockInfo dat (static-object metadata) WorldView.NeighborLandblockIds correctly uses 0xFFFF. My StreamingRegion.EncodeLandblockId from Phase A.1 Task 1 used 0xFFFE by mistake. Every streaming load was therefore calling LandblockLoader.Load with the LandBlockInfo id, which makes DatCollection ask DatBinReader to read a LandBlock from the LandBlockInfo file. The reader's internal buffer position lands in the middle of the wrong file's bytes, ReadBytesInternal asks for an out-of-range slice, throws ArgumentOutOfRangeException, and the landblocks that DON'T throw return half-populated LandBlock objects whose Height[] arrays contain garbage. Garbage Z values render as the spike pattern. The kicker: my Task-1 review fix added a test (Constructor_SmallRadius_IDsMatchEncodingRule) that asserted Assert.Contains(0x1234FFFEu, region.Visible). The test was passing because it pinned the wrong value. I literally codified the bug. Fix: change EncodeLandblockId's terminator from 0xFFFEu to 0xFFFFu and update the test to assert 0x1234FFFFu. The XML doc on Visible now explicitly explains the 0xFFFF/0xFFFE distinction so this can't recur. The previous two hotfixes (_datLock in c991fb2, synchronous streamer in 531c9f9) stay in place — _datLock is defensive belt-and-suspenders that documents which entry points read dats, and synchronous loading is correct-by-default until we decide whether to reintroduce background loading (Phase A.3 may make it unnecessary anyway). 212 tests green. With this fix the streaming should actually work. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/AcDream.App/Streaming/StreamingRegion.cs | 27 +++++++++++++++---- .../Streaming/StreamingRegionTests.cs | 12 ++++++--- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/AcDream.App/Streaming/StreamingRegion.cs b/src/AcDream.App/Streaming/StreamingRegion.cs index 0bef84c..b28b547 100644 --- a/src/AcDream.App/Streaming/StreamingRegion.cs +++ b/src/AcDream.App/Streaming/StreamingRegion.cs @@ -21,10 +21,18 @@ public sealed class StreamingRegion private readonly HashSet _resident = new(); /// - /// Landblock IDs (8.8 coordinate form: (lbX << 24) | (lbY << 16) | 0xFFFE) - /// 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. + /// Landblock IDs in the current visible window in the AC 8.8 coordinate + /// form: (lbX << 24) | (lbY << 16) | 0xFFFF. The trailing + /// 0xFFFF selects the LandBlock dat (terrain heightmap); the + /// matching LandBlockInfo (static-object metadata) is at 0xFFFE + /// on the same coordinates and is resolved internally by + /// LandblockLoader. + /// + /// + /// This set is strictly the (2r+1)×(2r+1) window; it does NOT include + /// hysteresis-retained landblocks outside the window. Use + /// to enumerate everything actually loaded. + /// /// public IReadOnlyCollection Visible => _visible; @@ -61,8 +69,17 @@ public sealed class StreamingRegion _resident.UnionWith(_visible); } + /// + /// Encode a landblock at (lbX, lbY) into the AC dat id form. Always uses + /// the 0xFFFF terminator (LandBlock = terrain). The earlier + /// version of this method used 0xFFFE by mistake — that's the + /// LandBlockInfo id, and asking LandblockLoader.Load to read a + /// LandBlock at the LandBlockInfo coords corrupts the dat reader's + /// buffer position, returning a half-populated LandBlock.Height[] + /// array which renders as wildly distorted "ball with spikes" terrain. + /// internal static uint EncodeLandblockId(int lbX, int lbY) - => ((uint)lbX << 24) | ((uint)lbY << 16) | 0xFFFEu; + => ((uint)lbX << 24) | ((uint)lbY << 16) | 0xFFFFu; /// /// Recompute the visible window around a new center and return the diff --git a/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs b/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs index d65b480..741ea2b 100644 --- a/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs +++ b/tests/AcDream.Core.Tests/Streaming/StreamingRegionTests.cs @@ -103,11 +103,17 @@ public class StreamingRegionTests [Fact] public void Constructor_SmallRadius_IDsMatchEncodingRule() { - // Verify EncodeLandblockId is correct (not swapped shifts). - // radius=0 at (0x12, 0x34) → exactly one entry, id = 0x1234FFFE. + // Verify EncodeLandblockId emits the LandBlock terminator (0xFFFF), + // not the LandBlockInfo terminator (0xFFFE). The earlier version of + // this test pinned 0xFFFE and codified the bug that produced "ball + // of spikes" terrain in Phase A.1's first live run — LandblockLoader + // was being asked to read the LandBlockInfo file as a LandBlock, + // which corrupted the dat reader's buffer position and returned a + // half-populated heightmap. radius=0 at (0x12, 0x34) → exactly one + // entry, id = 0x1234FFFF. var region = new StreamingRegion(cx: 0x12, cy: 0x34, radius: 0); Assert.Single(region.Visible); - Assert.Contains(0x1234FFFEu, region.Visible); + Assert.Contains(0x1234FFFFu, region.Visible); } }