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) <noreply@anthropic.com>
This commit is contained in:
parent
4df19146ff
commit
71d0edc3d7
2 changed files with 66 additions and 3 deletions
|
|
@ -22,7 +22,7 @@ public static class LandblockLoader
|
||||||
var info = dats.Get<LandBlockInfo>((landblockId & 0xFFFF0000u) | 0xFFFEu);
|
var info = dats.Get<LandBlockInfo>((landblockId & 0xFFFF0000u) | 0xFFFEu);
|
||||||
var entities = info is null
|
var entities = info is null
|
||||||
? Array.Empty<WorldEntity>()
|
? Array.Empty<WorldEntity>()
|
||||||
: BuildEntitiesFromInfo(info);
|
: BuildEntitiesFromInfo(info, landblockId);
|
||||||
|
|
||||||
return new LoadedLandblock(landblockId, block, entities);
|
return new LoadedLandblock(landblockId, block, entities);
|
||||||
}
|
}
|
||||||
|
|
@ -33,10 +33,27 @@ public static class LandblockLoader
|
||||||
/// (neither GfxObj 0x01xxxxxx nor Setup 0x02xxxxxx) are silently skipped.
|
/// (neither GfxObj 0x01xxxxxx nor Setup 0x02xxxxxx) are silently skipped.
|
||||||
/// MeshRefs is left empty at this stage — Task 5 populates it.
|
/// MeshRefs is left empty at this stage — Task 5 populates it.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public static IReadOnlyList<WorldEntity> BuildEntitiesFromInfo(LandBlockInfo info)
|
public static IReadOnlyList<WorldEntity> BuildEntitiesFromInfo(LandBlockInfo info, uint landblockId = 0)
|
||||||
{
|
{
|
||||||
var result = new List<WorldEntity>(info.Objects.Count + info.Buildings.Count);
|
var result = new List<WorldEntity>(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)
|
foreach (var stab in info.Objects)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -116,4 +116,50 @@ public class LandblockLoaderTests
|
||||||
var entities = LandblockLoader.BuildEntitiesFromInfo(new LandBlockInfo());
|
var entities = LandblockLoader.BuildEntitiesFromInfo(new LandBlockInfo());
|
||||||
Assert.Empty(entities);
|
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 <THIS_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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue