From 71d0edc3d7642fad3ad833e86adc45935edebdf0 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 10 May 2026 20:07:19 +0200 Subject: [PATCH] fix(world #53): namespace stab Ids globally for Tier 1 cache safety LandblockLoader.BuildEntitiesFromInfo restarted nextId at 1 per landblock, producing colliding entity.Id values across landblocks. EntityClassificationCache keys by entity.Id alone, so cross-LB collisions caused cache pollution: multiple stabs sharing id=1 -> cache entry for id=1 ended up with the CONCATENATION of multiple entities' batches -> buildings rendered up in the air with wrong textures (visual gate observation 2026-05-10). Audit at docs/research/2026-05-10-tier1-mutation-audit.md did not verify entity.Id uniqueness - that was an unchecked assumption. Cache design trusted entity.Id was globally unique; for stabs it wasn't. Fix: optional landblockId parameter on BuildEntitiesFromInfo. When non-zero, stab Ids are namespaced as 0xC0XXYY00 + nextId, matching the scenery (0x80XXYY00) and interior (0x40XXYY00) namespacing already in GameWindow.cs. The 0xC0 top byte distinguishes stabs from those. Existing tests pass landblockId=0 and keep their legacy starting-from-1 behavior. Known latent: if any one landblock has >256 stabs, nextId overflows the low byte. Same pattern + same limitation as scenery/interior. Out of scope for the immediate Tier 1 cache bug; not affecting current Holtburg play. Adds 2 regression tests pinning the namespacing + the legacy fallback. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/AcDream.Core/World/LandblockLoader.cs | 23 ++++++++-- .../World/LandblockLoaderTests.cs | 46 +++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/AcDream.Core/World/LandblockLoader.cs b/src/AcDream.Core/World/LandblockLoader.cs index fc3d30e..b18608a 100644 --- a/src/AcDream.Core/World/LandblockLoader.cs +++ b/src/AcDream.Core/World/LandblockLoader.cs @@ -22,7 +22,7 @@ public static class LandblockLoader var info = dats.Get((landblockId & 0xFFFF0000u) | 0xFFFEu); var entities = info is null ? Array.Empty() - : BuildEntitiesFromInfo(info); + : BuildEntitiesFromInfo(info, landblockId); return new LoadedLandblock(landblockId, block, entities); } @@ -33,10 +33,27 @@ public static class LandblockLoader /// (neither GfxObj 0x01xxxxxx nor Setup 0x02xxxxxx) are silently skipped. /// MeshRefs is left empty at this stage — Task 5 populates it. /// - public static IReadOnlyList BuildEntitiesFromInfo(LandBlockInfo info) + public static IReadOnlyList BuildEntitiesFromInfo(LandBlockInfo info, uint landblockId = 0) { var result = new List(info.Objects.Count + info.Buildings.Count); - uint nextId = 1; + + // When landblockId is non-zero, namespace stab Ids globally: + // 0xC0XXYY00 + n, where XX = lbX byte, YY = lbY byte + // matching the scenery (0x80XXYY00) and interior (0x40XXYY00) patterns + // in GameWindow.cs. The 0xC0 top byte distinguishes stabs from those. + // + // Pre-Tier-1 callers (existing tests) pass landblockId=0 and get the + // legacy starting-from-1 monotonic Ids — compatible with their assertions + // which check uniqueness within a single landblock. + // + // Latent: if a landblock has >256 stabs (rare), nextId overflows the + // low byte and bleeds into the lbY byte → cross-LB collision. Same + // pattern + same limitation as scenery/interior. Document but don't + // fix in this commit — out of scope for the Tier 1 cache bug fix. + uint stabIdBase = landblockId == 0 + ? 0u + : 0xC0000000u | ((landblockId >> 24) & 0xFFu) << 16 | ((landblockId >> 16) & 0xFFu) << 8; + uint nextId = stabIdBase == 0 ? 1u : stabIdBase + 1u; foreach (var stab in info.Objects) { diff --git a/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs b/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs index af68b01..d1d24b8 100644 --- a/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs +++ b/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs @@ -116,4 +116,50 @@ public class LandblockLoaderTests var entities = LandblockLoader.BuildEntitiesFromInfo(new LandBlockInfo()); Assert.Empty(entities); } + + [Fact] + public void BuildEntitiesFromInfo_WithLandblockId_NamespacesIdsForGlobalUniqueness() + { + // Regression: cross-LB stab Id collision was the cause of visual + // glitches in Tier 1 cache (commit ) — buildings rendered + // up in the air with wrong textures because cache was keyed by + // entity.Id and stab Ids restarted at 1 per landblock. + var info = new LandBlockInfo + { + Objects = + { + new Stab { Id = 0x01000001u, Frame = new Frame() }, + new Stab { Id = 0x01000002u, Frame = new Frame() }, + }, + }; + + var entitiesLbA = LandblockLoader.BuildEntitiesFromInfo(info, landblockId: 0xA9B40000u); + var entitiesLbB = LandblockLoader.BuildEntitiesFromInfo(info, landblockId: 0xA9B50000u); + + // No two entities across LB A and LB B share the same Id. + var idsA = entitiesLbA.Select(e => e.Id).ToArray(); + var idsB = entitiesLbB.Select(e => e.Id).ToArray(); + Assert.Empty(idsA.Intersect(idsB)); + + // The namespace top byte is 0xC0 for stabs (distinct from 0x80 scenery, + // 0x40 interior, low-range live entities). + Assert.All(idsA, id => Assert.Equal(0xC0u, (id >> 24) & 0xFFu)); + Assert.All(idsB, id => Assert.Equal(0xC0u, (id >> 24) & 0xFFu)); + } + + [Fact] + public void BuildEntitiesFromInfo_LegacyZeroLandblockId_StartsAtOne() + { + // Backward compat: existing callers (tests pre-fix) call without a + // landblockId and get the legacy "starts at 1" behavior. + var info = new LandBlockInfo + { + Objects = { new Stab { Id = 0x01000001u, Frame = new Frame() } }, + }; + + var entities = LandblockLoader.BuildEntitiesFromInfo(info); + + Assert.Single(entities); + Assert.Equal(1u, entities[0].Id); + } }