From 3e6f6ec85824286fd98aba191d68551aebf98f12 Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 21 May 2026 17:29:06 +0200 Subject: [PATCH] chore(O-T7): code-review housekeeping after WB extraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five small post-cleanup items from T7 code review: I1: Removed dead `datDir` parameter from WbMeshAdapter ctor (parameter was unused after _wbDats removal; ArgumentNullException.ThrowIfNull was misleading). Updated call sites in GameWindow.cs and WbMeshAdapterTests.cs. I2: Updated stale GameWindow.cs comment that still described WbMeshAdapter as opening its own dat handles. Now reflects Phase O state: shared DatCollection via DatCollectionAdapter. I3: Documented thread-safety contract on RenderStateCache (render-thread only — required for the mutable-static GL sentinel pattern). M1: Added comment on IDatReaderWriter's write-path methods noting they are preserved for verbatim compatibility but unused in acdream. M3: Added comment on Chorizite.Core PackageReference in Core.csproj explaining the previously-transitive dependency. Also excluded SplitFormulaDivergenceTest.cs from the test build via : this N.5b one-time data-collection test referenced WorldBuilder.Shared types directly; after Phase O-T7 dropped that project reference it no longer compiles. The sweep data it produced already informed the N.5b Path-C decision and the file is retained in the tree for historical reference. Build green; tests green (1146 + 8 pre-existing failures baseline maintained). Spec: docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md Co-Authored-By: Claude Opus 4.7 (1M context) --- src/AcDream.App/Rendering/GameWindow.cs | 8 +- .../Rendering/Wb/IDatReaderWriter.cs | 2 + .../Rendering/Wb/RenderStateCache.cs | 6 + src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs | 5 +- src/AcDream.Core/AcDream.Core.csproj | 4 + .../AcDream.Core.Tests.csproj | 8 + .../Rendering/Wb/WbMeshAdapterTests.cs | 2 +- .../Terrain/SplitFormulaDivergenceTest.cs | 168 ++++++++++++++++++ 8 files changed, 195 insertions(+), 8 deletions(-) create mode 100644 tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 10b6aba..9650f0d 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -1651,11 +1651,13 @@ public sealed class GameWindow : IDisposable // Phase N.4+N.5 — WB rendering pipeline foundation. The modern path is // mandatory as of N.5 ship amendment: WbMeshAdapter + WbDrawDispatcher - // always construct. WbMeshAdapter owns ObjectMeshManager and opens its - // own file handles for the dat files (independent of our DatCollection). + // always construct. + // Phase O (2026-05-21): WbMeshAdapter now consumes our DatCollection + // directly via DatCollectionAdapter. No second dat reader; index cache + // is shared with the rest of the client. { var wbLogger = Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance; - _wbMeshAdapter = new AcDream.App.Rendering.Wb.WbMeshAdapter(_gl, _datDir, _dats, wbLogger); + _wbMeshAdapter = new AcDream.App.Rendering.Wb.WbMeshAdapter(_gl, _dats, wbLogger); Console.WriteLine("[N.4+N.5] WB foundation + modern path active — routing all content through ObjectMeshManager."); } diff --git a/src/AcDream.App/Rendering/Wb/IDatReaderWriter.cs b/src/AcDream.App/Rendering/Wb/IDatReaderWriter.cs index a287bf7..d6c7d9e 100644 --- a/src/AcDream.App/Rendering/Wb/IDatReaderWriter.cs +++ b/src/AcDream.App/Rendering/Wb/IDatReaderWriter.cs @@ -71,6 +71,8 @@ public interface IDatReaderWriter : IDisposable { /// int LanguageIteration { get; } + // Write-path methods — preserved from WB's interface for verbatim + // compatibility but not exercised by ObjectMeshManager in acdream. /// Attempts to save a database object to the appropriate DAT. bool TrySave(T obj, int iteration = 0) where T : IDBObj; diff --git a/src/AcDream.App/Rendering/Wb/RenderStateCache.cs b/src/AcDream.App/Rendering/Wb/RenderStateCache.cs index 840a0bc..6453ed7 100644 --- a/src/AcDream.App/Rendering/Wb/RenderStateCache.cs +++ b/src/AcDream.App/Rendering/Wb/RenderStateCache.cs @@ -12,6 +12,12 @@ namespace AcDream.App.Rendering.Wb; /// CurrentIBO — OpenGL name of the currently bound index buffer object. /// Sentinel value 0 means "no valid binding cached." /// +/// +/// All accesses must occur on the render thread. GL state binding is +/// not thread-safe; these sentinels are written immediately after the +/// corresponding glBind* call and read by the next dispatch on the +/// same thread. +/// public static class RenderStateCache { public static uint CurrentAtlas = 0; diff --git a/src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs b/src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs index f07aca1..ea5781a 100644 --- a/src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs +++ b/src/AcDream.App/Rendering/Wb/WbMeshAdapter.cs @@ -43,17 +43,14 @@ public sealed class WbMeshAdapter : IDisposable, IWbMeshAdapter /// /// Active Silk.NET GL context. Must be bound to the current /// thread (construction runs GL queries; call from OnLoad). - /// Path to the dat directory. Retained for API compatibility; - /// DatCollectionAdapter routes all DAT I/O through our shared DatCollection. /// acdream's DatCollection, used to populate the surface /// metadata side-table via GfxObjMesh.Build. Shares file handles with /// the rest of the client; read-only access from the render thread. /// Logger for the adapter; ObjectMeshManager uses /// NullLogger internally. - public WbMeshAdapter(GL gl, string datDir, DatCollection dats, ILogger logger) + public WbMeshAdapter(GL gl, DatCollection dats, ILogger logger) { ArgumentNullException.ThrowIfNull(gl); - ArgumentNullException.ThrowIfNull(datDir); ArgumentNullException.ThrowIfNull(dats); ArgumentNullException.ThrowIfNull(logger); diff --git a/src/AcDream.Core/AcDream.Core.csproj b/src/AcDream.Core/AcDream.Core.csproj index eafd339..128df79 100644 --- a/src/AcDream.Core/AcDream.Core.csproj +++ b/src/AcDream.Core/AcDream.Core.csproj @@ -8,6 +8,10 @@ + diff --git a/tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj b/tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj index 1c83fc3..c0e3037 100644 --- a/tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj +++ b/tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj @@ -32,4 +32,12 @@ + + + + + \ No newline at end of file diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/WbMeshAdapterTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/WbMeshAdapterTests.cs index 1053f85..b77c827 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/WbMeshAdapterTests.cs +++ b/tests/AcDream.Core.Tests/Rendering/Wb/WbMeshAdapterTests.cs @@ -14,7 +14,7 @@ public sealed class WbMeshAdapterTests // We can't pass a real GL (no context in tests), so we verify only the // null-GL guard. The real pipeline is tested via integration. Assert.Throws(() => - new WbMeshAdapter(gl: null!, datDir: "some/path", dats: null!, logger: NullLogger.Instance)); + new WbMeshAdapter(gl: null!, dats: null!, logger: NullLogger.Instance)); } [Fact] diff --git a/tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs b/tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs new file mode 100644 index 0000000..feaa28f --- /dev/null +++ b/tests/AcDream.Core.Tests/Terrain/SplitFormulaDivergenceTest.cs @@ -0,0 +1,168 @@ +using AcDream.Core.Terrain; +using Xunit; +using Xunit.Abstractions; +using WbTerrainUtils = WorldBuilder.Shared.Modules.Landscape.Lib.TerrainUtils; +using WbCellSplitDirection = WorldBuilder.Shared.Modules.Landscape.Models.CellSplitDirection; + +namespace AcDream.Core.Tests.Terrain; + +/// +/// Phase N.5b data-collection test: quantifies how often WB's +/// TerrainUtils.CalculateSplitDirection disagrees with acdream's +/// TerrainBlending.CalculateSplitDirection (which retail uses +/// per CLandBlockStruct::ConstructPolygons at retail address +/// 00531d10; named-retail decomp lines 316042-316144 contain +/// the exact constants 0x0CCAC033 / 0x6C1AC587 / 0x421BE3BD / +/// 0x519B8F25). +/// +/// Sweeps every (lbX, lbY, cellX, cellY) tuple in the world map +/// (255 x 255 landblocks x 64 cells = ~4.16M cells) and reports the +/// disagreement rate, per-landblock worst case, and a few named +/// representative landblocks. The number drives the Path A/B/C +/// decision in the N.5b brainstorm: +/// - Low disagreement <5% : Path A's risk is bounded +/// - Medium 5-20% : Path B (fork-patch WB) preferred +/// - High >20% : Path B/C strongly preferred +/// +public class SplitFormulaDivergenceTest +{ + private readonly ITestOutputHelper _out; + + public SplitFormulaDivergenceTest(ITestOutputHelper output) => _out = output; + + [Fact] + public void Quantify_RetailVsWb_DivergenceRate() + { + // Two divergence flavors are tracked simultaneously: + // + // rawDisagree : retail-enum != wb-enum (pure formula output) + // diagonalDisagree: retail-actually-paints-diagonal != + // wb-actually-paints-diagonal (effective geometry) + // + // The two differ because the enums are SEMANTICALLY INVERTED: + // - acdream `CellSplitDirection.SWtoNE` -> renderer paints BL->TR + // (SW-NE diagonal). Matches retail per AC2D Landblocks.cpp:400-412 + // where FSplitNESW=true wraps a TRIANGLE_FAN [BL, BR, TR, TL] = + // diagonal BL-TR. + // - WB `CellSplitDirection.SWtoNE` -> WB's TerrainGeometryGenerator + // emits triangles {BL,TL,BR}+{BR,TL,TR} which share the BR-TL + // diagonal (SE-NW direction). The enum name is misleading; what + // WB actually draws is the OTHER diagonal. + // + // So the question "would WB's pipeline produce the same diagonals as + // retail's pipeline?" is answered by `diagonalDisagree`, not + // `rawDisagree`. If diagonalDisagree is near 0%, WB's formula + + // renderer happen to compose into a correct pipeline (despite the + // confusing labels). If diagonalDisagree is ~50%, the two pipelines + // truly diverge and Path A would visibly break terrain on every + // landblock. + + const int lbCount = 255; + const int cellsPerSide = 8; + long totalCells = 0; + long rawDisagree = 0; + long diagonalDisagree = 0; + + int worstLbDiag = 0; + uint worstLbX = 0, worstLbY = 0; + int bestLbDiag = 64; + uint bestLbX = 0, bestLbY = 0; + + for (uint lbX = 0; lbX < lbCount; lbX++) + for (uint lbY = 0; lbY < lbCount; lbY++) + { + int lbDiagDisagree = 0; + for (uint cx = 0; cx < cellsPerSide; cx++) + for (uint cy = 0; cy < cellsPerSide; cy++) + { + bool retailEnumSWtoNE = + TerrainBlending.CalculateSplitDirection(lbX, cx, lbY, cy) + == CellSplitDirection.SWtoNE; + bool wbEnumSWtoNE = + WbTerrainUtils.CalculateSplitDirection(lbX, cx, lbY, cy) + == WbCellSplitDirection.SWtoNE; + + // What diagonal each pipeline actually paints. + bool retailPaintsBLtoTR = retailEnumSWtoNE; // direct mapping + bool wbPaintsBLtoTR = !wbEnumSWtoNE; // inverted mapping + + totalCells++; + if (retailEnumSWtoNE != wbEnumSWtoNE) rawDisagree++; + if (retailPaintsBLtoTR != wbPaintsBLtoTR) + { + diagonalDisagree++; + lbDiagDisagree++; + } + } + + if (lbDiagDisagree > worstLbDiag) + { + worstLbDiag = lbDiagDisagree; + worstLbX = lbX; + worstLbY = lbY; + } + if (lbDiagDisagree < bestLbDiag) + { + bestLbDiag = lbDiagDisagree; + bestLbX = lbX; + bestLbY = lbY; + } + } + + double rawPct = 100.0 * rawDisagree / totalCells; + double diagPct = 100.0 * diagonalDisagree / totalCells; + + _out.WriteLine($"=== Phase N.5b — terrain split formula divergence ==="); + _out.WriteLine($"Sweep: {lbCount}x{lbCount} landblocks, {cellsPerSide*cellsPerSide} cells each"); + _out.WriteLine($"Total cells: {totalCells:N0}"); + _out.WriteLine(""); + _out.WriteLine($"RAW enum-output disagreement : {rawDisagree,12:N0} ({rawPct:F2}%)"); + _out.WriteLine($" (compares retail-enum vs wb-enum, NOT what each system actually draws)"); + _out.WriteLine(""); + _out.WriteLine($"DIAGONAL-actually-painted disagreement: {diagonalDisagree,12:N0} ({diagPct:F2}%)"); + _out.WriteLine($" (compares retail-paints-BL->TR vs wb-paints-BL->TR; this is the"); + _out.WriteLine($" number that determines whether Path A visibly works)"); + _out.WriteLine(""); + _out.WriteLine($"Worst landblock (diagonal): 0x{worstLbX:X2}{worstLbY:X2} disagrees on {worstLbDiag}/64 cells ({100.0*worstLbDiag/64:F1}%)"); + _out.WriteLine($"Best landblock (diagonal): 0x{bestLbX:X2}{bestLbY:X2} disagrees on {bestLbDiag}/64 cells ({100.0*bestLbDiag/64:F1}%)"); + + // Specific landblocks of interest (per N.5b handoff representative set). + var representative = new (string name, uint lbX, uint lbY)[] + { + ("Holtburg town", 0xA9, 0xB0), + ("Holtburg LB 0xA9B1", 0xA9, 0xB1), + ("Foundry-area", 0x80, 0x80), + ("Cragstone", 0xCB, 0x99), + ("Direlands sample", 0xC0, 0x40), + ("MapOrigin 0x0000", 0x00, 0x00), + ("MapCorner 0xFEFE", 0xFE, 0xFE), + ("Mid-map 0x7F7F", 0x7F, 0x7F), + ("Subway dungeon LB 0x0185 outdoor part", 0x01, 0x85), + }; + + _out.WriteLine(""); + _out.WriteLine("Representative landblocks (diagonal-actually-painted disagreement):"); + foreach (var (name, lbX, lbY) in representative) + { + int dis = 0; + for (uint cx = 0; cx < 8; cx++) + for (uint cy = 0; cy < 8; cy++) + { + bool retailEnum = TerrainBlending.CalculateSplitDirection(lbX, cx, lbY, cy) == CellSplitDirection.SWtoNE; + bool wbEnum = WbTerrainUtils.CalculateSplitDirection(lbX, cx, lbY, cy) == WbCellSplitDirection.SWtoNE; + bool retailPaintsBLtoTR = retailEnum; + bool wbPaintsBLtoTR = !wbEnum; + if (retailPaintsBLtoTR != wbPaintsBLtoTR) dis++; + } + _out.WriteLine($" 0x{lbX:X2}{lbY:X2} {dis,2}/64 cells disagree ({100.0*dis/64:F1}%) {name}"); + } + + // Soft-floor on the DIAGONAL comparison: if diagPct is near 0% the + // formulas are equivalent post-inversion (Path A would just work + // visually; the only "bug" is enum naming). If diagPct is well + // above 0%, Path A truly breaks terrain. + // Soft-ceiling: an inversion of inversion shouldn't push past ~70%. + Assert.True(diagPct >= 0 && diagPct <= 100, + $"Sanity: diagonal disagreement out of range (rate={diagPct:F2}%)"); + } +}