fix(app): Phase A.1 — encode landblock IDs with 0xFFFF terminator, not 0xFFFE
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 inc991fb2, synchronous streamer in531c9f9) 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) <noreply@anthropic.com>
This commit is contained in:
parent
531c9f9349
commit
f83a8c1674
2 changed files with 31 additions and 8 deletions
|
|
@ -21,10 +21,18 @@ public sealed class StreamingRegion
|
||||||
private readonly HashSet<uint> _resident = new();
|
private readonly HashSet<uint> _resident = new();
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Landblock IDs (8.8 coordinate form: <c>(lbX << 24) | (lbY << 16) | 0xFFFE</c>)
|
/// Landblock IDs in the current visible window in the AC 8.8 coordinate
|
||||||
/// in the current visible window. This is strictly the (2r+1)×(2r+1) set;
|
/// form: <c>(lbX << 24) | (lbY << 16) | 0xFFFF</c>. The trailing
|
||||||
/// it does NOT include hysteresis-retained landblocks outside the window.
|
/// <c>0xFFFF</c> selects the LandBlock dat (terrain heightmap); the
|
||||||
/// Use <see cref="Resident"/> to enumerate everything actually loaded.
|
/// matching LandBlockInfo (static-object metadata) is at <c>0xFFFE</c>
|
||||||
|
/// on the same coordinates and is resolved internally by
|
||||||
|
/// <c>LandblockLoader</c>.
|
||||||
|
///
|
||||||
|
/// <para>
|
||||||
|
/// This set is strictly the (2r+1)×(2r+1) window; it does NOT include
|
||||||
|
/// hysteresis-retained landblocks outside the window. Use
|
||||||
|
/// <see cref="Resident"/> to enumerate everything actually loaded.
|
||||||
|
/// </para>
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public IReadOnlyCollection<uint> Visible => _visible;
|
public IReadOnlyCollection<uint> Visible => _visible;
|
||||||
|
|
||||||
|
|
@ -61,8 +69,17 @@ public sealed class StreamingRegion
|
||||||
_resident.UnionWith(_visible);
|
_resident.UnionWith(_visible);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Encode a landblock at (lbX, lbY) into the AC dat id form. Always uses
|
||||||
|
/// the <c>0xFFFF</c> terminator (LandBlock = terrain). The earlier
|
||||||
|
/// version of this method used <c>0xFFFE</c> by mistake — that's the
|
||||||
|
/// LandBlockInfo id, and asking <c>LandblockLoader.Load</c> to read a
|
||||||
|
/// LandBlock at the LandBlockInfo coords corrupts the dat reader's
|
||||||
|
/// buffer position, returning a half-populated <c>LandBlock.Height[]</c>
|
||||||
|
/// array which renders as wildly distorted "ball with spikes" terrain.
|
||||||
|
/// </summary>
|
||||||
internal static uint EncodeLandblockId(int lbX, int lbY)
|
internal static uint EncodeLandblockId(int lbX, int lbY)
|
||||||
=> ((uint)lbX << 24) | ((uint)lbY << 16) | 0xFFFEu;
|
=> ((uint)lbX << 24) | ((uint)lbY << 16) | 0xFFFFu;
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Recompute the visible window around a new center and return the
|
/// Recompute the visible window around a new center and return the
|
||||||
|
|
|
||||||
|
|
@ -103,11 +103,17 @@ public class StreamingRegionTests
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Constructor_SmallRadius_IDsMatchEncodingRule()
|
public void Constructor_SmallRadius_IDsMatchEncodingRule()
|
||||||
{
|
{
|
||||||
// Verify EncodeLandblockId is correct (not swapped shifts).
|
// Verify EncodeLandblockId emits the LandBlock terminator (0xFFFF),
|
||||||
// radius=0 at (0x12, 0x34) → exactly one entry, id = 0x1234FFFE.
|
// 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);
|
var region = new StreamingRegion(cx: 0x12, cy: 0x34, radius: 0);
|
||||||
|
|
||||||
Assert.Single(region.Visible);
|
Assert.Single(region.Visible);
|
||||||
Assert.Contains(0x1234FFFEu, region.Visible);
|
Assert.Contains(0x1234FFFFu, region.Visible);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue