From 859cf5ec02d579684354ec044f4a4c87a2157d87 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 5 Jul 2026 22:18:30 +0200 Subject: [PATCH] refactor(pipeline): MP1b review - unify DatCollectionAdapter + TOC/log test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarially-verified review findings 7 and 8: (7) The DatCollection->IDatReaderWriter adapter existed as THREE near-identical copies (App-internal original, Bake's copy, Content. Tests' copy) — a structure where adapter drift is exactly what the live-vs-pak equivalence suite cannot detect (both sides would only drift together if they shared one implementation). Now ONE public AcDream.Content.DatCollectionAdapter next to IDatReaderWriter (GL-free home established in MP1a), carrying App's FULL behavior including the [dat-miss] TryGet tripwire log (which now also covers the bake tool and the equivalence suite) and the caching/locking. All three copies deleted; WbMeshAdapter (App), BakeRunner (Bake), and PakEquivalenceTests (Content.Tests) resolve the shared class. Iteration properties return the REAL dat iterations — the App copy's hardcoded 0 was a stub nothing read; the unification intentionally keeps truth (noted in the doc comment). Verified post-move: no Silk.NET anywhere in Content / Bake / Content.Tests / Bake.Tests resolved dependency graphs. (8) Two test gaps closed in PakRoundTripTests: (a) direct on-disk TOC sortedness — blobs added in DESCENDING key order, then the raw file bytes parsed (not through the reader) and every TOC entry asserted strictly ascending; (b) corrupt-blob logging — five repeated reads through both public paths (TryReadObjectMeshData + ContainsKey) with stderr captured, asserting exactly ONE [pak-corrupt] line for the victim key. Full suite: 4120 tests, 0 failures (Content.Tests 56, Bake.Tests 1, plus the pre-existing 4 skips). --- src/AcDream.Bake/BakeDatCollectionAdapter.cs | 147 ------------------ src/AcDream.Bake/BakeRunner.cs | 6 +- .../DatCollectionAdapter.cs | 104 ++++++------- src/AcDream.Content/IDatReaderWriter.cs | 7 +- .../ContentTestDatCollectionAdapter.cs | 145 ----------------- .../PakEquivalenceTests.cs | 4 +- .../PakRoundTripTests.cs | 73 +++++++++ 7 files changed, 135 insertions(+), 351 deletions(-) delete mode 100644 src/AcDream.Bake/BakeDatCollectionAdapter.cs rename src/{AcDream.App/Rendering/Wb => AcDream.Content}/DatCollectionAdapter.cs (66%) delete mode 100644 tests/AcDream.Content.Tests/ContentTestDatCollectionAdapter.cs diff --git a/src/AcDream.Bake/BakeDatCollectionAdapter.cs b/src/AcDream.Bake/BakeDatCollectionAdapter.cs deleted file mode 100644 index a3843b9c..00000000 --- a/src/AcDream.Bake/BakeDatCollectionAdapter.cs +++ /dev/null @@ -1,147 +0,0 @@ -using System; -using System.Collections.Concurrent; -using System.Collections.Generic; -using System.Collections.ObjectModel; -using System.Diagnostics.CodeAnalysis; -using AcDream.Content; -using DatReaderWriter; -using DatReaderWriter.Enums; -using DatReaderWriter.Lib.IO; - -namespace AcDream.Bake; - -/// -/// Adapts acdream's to -/// for MeshExtractor's constructor. A separate copy from -/// AcDream.App.Rendering.Wb.DatCollectionAdapter (that one is `internal` to -/// AcDream.App and, more importantly, AcDream.App carries the Silk.NET/GL -/// dependency chain the bake tool must never reference — see -/// AcDream.Bake.csproj's "NO Silk.NET" comment). This is plain dat-access -/// glue, not an AC-specific algorithm, so a small duplicated adapter is the -/// right call rather than inventing a shared-but-App-rooted package. -/// -internal sealed class BakeDatCollectionAdapter : IDatReaderWriter { - private readonly DatCollection _dats; - private readonly DatDatabaseWrapper _portal; - private readonly DatDatabaseWrapper _cell; - private readonly DatDatabaseWrapper _highRes; - private readonly DatDatabaseWrapper _language; - private readonly ReadOnlyDictionary _cellRegions; - - public BakeDatCollectionAdapter(DatCollection dats) { - ArgumentNullException.ThrowIfNull(dats); - _dats = dats; - _portal = new DatDatabaseWrapper(dats.Portal); - _cell = new DatDatabaseWrapper(dats.Cell); - _highRes = new DatDatabaseWrapper(dats.HighRes); - _language = new DatDatabaseWrapper(dats.Local); - - var regions = new Dictionary { [0u] = _cell }; - _cellRegions = new ReadOnlyDictionary(regions); - } - - public string SourceDirectory => _dats.Options.DatDirectory ?? string.Empty; - - public IDatDatabase Portal => _portal; - public ReadOnlyDictionary CellRegions => _cellRegions; - public IDatDatabase HighRes => _highRes; - public IDatDatabase Language => _language; - - public ReadOnlyDictionary RegionFileMap => - new ReadOnlyDictionary(new Dictionary()); - - public int PortalIteration => _portal.Iteration; - public int CellIteration => _cell.Iteration; - public int HighResIteration => _highRes.Iteration; - public int LanguageIteration => _language.Iteration; - - public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) { - return _dats.Cell.TryGetFileBytes(fileId, ref bytes, out bytesRead); - } - - /// Mirrors DefaultDatReaderWriter.ResolveId ordering: HighRes -> Portal -> Language -> Cell. - public IEnumerable ResolveId(uint id) { - var results = new List(); - - void CheckDb(DatDatabaseWrapper wrapper) { - var rawDb = wrapper.RawDatabase; - if (rawDb.Tree.TryGetFile(id, out _)) { - var type = rawDb.TypeFromId(id); - if (type != DBObjType.Unknown) { - results.Add(new IDatReaderWriter.IdResolution(wrapper, type)); - } - } - } - - CheckDb(_highRes); - CheckDb(_portal); - CheckDb(_language); - CheckDb(_cell); - - return results; - } - - public bool TrySave(T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(BakeDatCollectionAdapter)} is read-only."); - - public bool TrySave(uint regionId, T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(BakeDatCollectionAdapter)} is read-only."); - - public void Dispose() { - // The underlying DatCollection is owned by the caller. - } -} - -/// Wraps a as . -internal sealed class DatDatabaseWrapper : IDatDatabase { - private readonly DatDatabase _db; - private readonly ConcurrentDictionary<(Type, uint), IDBObj> _cache = new(); - private readonly object _lock = new(); - - public DatDatabaseWrapper(DatDatabase db) { - ArgumentNullException.ThrowIfNull(db); - _db = db; - } - - internal DatDatabase RawDatabase => _db; - - public DatDatabase Db => _db; - public int Iteration => _db.Iteration?.CurrentIteration ?? 0; - - public IEnumerable GetAllIdsOfType() where T : IDBObj => _db.GetAllIdsOfType(); - - public bool TryGet(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj { - if (_cache.TryGetValue((typeof(T), fileId), out var cached)) { - value = (T)cached; - return true; - } - - lock (_lock) { - if (_db.TryGet(fileId, out value)) { - _cache.TryAdd((typeof(T), fileId), value); - return true; - } - } - - return false; - } - - public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) { - lock (_lock) { - return _db.TryGetFileBytes(fileId, out value); - } - } - - public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) { - lock (_lock) { - return _db.TryGetFileBytes(fileId, ref bytes, out bytesRead); - } - } - - public bool TrySave(T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(DatDatabaseWrapper)} is read-only."); - - public void Dispose() { - // The underlying DatDatabase is owned by DatCollection. - } -} diff --git a/src/AcDream.Bake/BakeRunner.cs b/src/AcDream.Bake/BakeRunner.cs index 18db6c2b..9e5676d3 100644 --- a/src/AcDream.Bake/BakeRunner.cs +++ b/src/AcDream.Bake/BakeRunner.cs @@ -53,7 +53,11 @@ public static class BakeRunner { var sw = Stopwatch.StartNew(); using var dats = new DatCollection(options.DatDir, DatAccessType.Read); - using var datReaderWriter = new BakeDatCollectionAdapter(dats); + // The unified AcDream.Content.DatCollectionAdapter (MP1b review + // finding 7): same instance class the client and the equivalence + // suite use — adapter drift between bake and runtime is impossible + // by construction. + using var datReaderWriter = new DatCollectionAdapter(dats); var extractorLogger = new ConsoleErrorLogger(nameof(MeshExtractor)); // Thread-safe side-stage sink for particle-preload meshes MeshExtractor diff --git a/src/AcDream.App/Rendering/Wb/DatCollectionAdapter.cs b/src/AcDream.Content/DatCollectionAdapter.cs similarity index 66% rename from src/AcDream.App/Rendering/Wb/DatCollectionAdapter.cs rename to src/AcDream.Content/DatCollectionAdapter.cs index e2598750..6c8b3aa5 100644 --- a/src/AcDream.App/Rendering/Wb/DatCollectionAdapter.cs +++ b/src/AcDream.Content/DatCollectionAdapter.cs @@ -1,4 +1,3 @@ -using AcDream.Content; using DatReaderWriter; using DatReaderWriter.Enums; using DatReaderWriter.Lib.IO; @@ -8,19 +7,32 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics.CodeAnalysis; -namespace AcDream.App.Rendering.Wb; +namespace AcDream.Content; /// -/// Adapts acdream's to WB's interface. +/// THE → adapter, +/// shared by all three consumers: ObjectMeshManager (AcDream.App, via +/// WbMeshAdapter), acdream-bake (AcDream.Bake.BakeRunner), and the pak +/// equivalence suite (AcDream.Content.Tests). MP1b's review found the +/// original App-internal copy had silently forked into three near-identical +/// versions — drift between them is exactly what the live-vs-pak equivalence +/// suite CANNOT detect (both sides would drift together only if they share +/// one implementation), so unification here is load-bearing, not cosmetic. +/// GL-free (DatReaderWriter only), hence Content is the right home +/// (established in MP1a alongside ). /// -/// O-D7 fallback path: taken because ObjectMeshManager has 26 _dats.X call sites (threshold is 20), -/// making a full refactor to DatCollection larger than spec permits in a single task. -/// This adapter lets ObjectMeshManager stay byte-identical to the WB original while -/// routing all DAT I/O through our single DatCollection. The adapter is dropped in T7 -/// when the WorldBuilder project reference is removed entirely. +/// History: O-D7 originally introduced this adapter (App-internal) +/// because ObjectMeshManager had 26 _dats.X call sites, letting it +/// stay byte-identical to the WB original while routing all DAT I/O through +/// the single DatCollection. +/// +/// Iteration properties return the REAL dat iterations (the App copy +/// hardcoded 0 — a stub nothing read; the bake tool stamps iterations into +/// the pak header from DatCollection directly, and any future consumer of +/// these properties should get truth, so the unification intentionally +/// keeps the real values). /// -internal sealed class DatCollectionAdapter : IDatReaderWriter -{ +public sealed class DatCollectionAdapter : IDatReaderWriter { private readonly DatCollection _dats; private readonly DatDatabaseWrapper _portal; private readonly DatDatabaseWrapper _cell; @@ -28,8 +40,7 @@ internal sealed class DatCollectionAdapter : IDatReaderWriter private readonly DatDatabaseWrapper _language; private readonly ReadOnlyDictionary _cellRegions; - public DatCollectionAdapter(DatCollection dats) - { + public DatCollectionAdapter(DatCollection dats) { ArgumentNullException.ThrowIfNull(dats); _dats = dats; _portal = new DatDatabaseWrapper(dats.Portal); @@ -51,18 +62,16 @@ internal sealed class DatCollectionAdapter : IDatReaderWriter public IDatDatabase HighRes => _highRes; public IDatDatabase Language => _language; - // RegionFileMap is used by some WB internals but not by ObjectMeshManager. + // RegionFileMap is used by some WB internals but not by any acdream consumer. public ReadOnlyDictionary RegionFileMap => new ReadOnlyDictionary(new Dictionary()); - // Iteration properties — not used by ObjectMeshManager, so delegate to 0. - public int PortalIteration => 0; - public int CellIteration => 0; - public int HighResIteration => 0; - public int LanguageIteration => 0; + public int PortalIteration => _portal.Iteration; + public int CellIteration => _cell.Iteration; + public int HighResIteration => _highRes.Iteration; + public int LanguageIteration => _language.Iteration; - public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) - { + public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) { // Route to cell db (the only region we expose) return _dats.Cell.TryGetFileBytes(fileId, ref bytes, out bytesRead); } @@ -72,15 +81,12 @@ internal sealed class DatCollectionAdapter : IDatReaderWriter /// Mirrors DefaultDatReaderWriter.ResolveId — checks each underlying DatDatabase /// via DatDatabase.TypeFromId (which reads the type range tables). /// - public IEnumerable ResolveId(uint id) - { + public IEnumerable ResolveId(uint id) { var results = new List(); - void CheckDb(DatDatabaseWrapper wrapper) - { + void CheckDb(DatDatabaseWrapper wrapper) { var rawDb = wrapper.RawDatabase; - if (rawDb.Tree.TryGetFile(id, out _)) - { + if (rawDb.Tree.TryGetFile(id, out _)) { var type = rawDb.TypeFromId(id); if (type != DBObjType.Unknown) results.Add(new IDatReaderWriter.IdResolution(wrapper, type)); @@ -102,8 +108,7 @@ internal sealed class DatCollectionAdapter : IDatReaderWriter public bool TrySave(uint regionId, T obj, int iteration = 0) where T : IDBObj => throw new NotSupportedException("DatCollectionAdapter is read-only."); - public void Dispose() - { + public void Dispose() { // The underlying DatCollection is owned by the caller — do not dispose it here. // Individual wrapper objects hold no unmanaged resources. } @@ -111,17 +116,16 @@ internal sealed class DatCollectionAdapter : IDatReaderWriter /// /// Wraps a as . -/// Mirrors WorldBuilder.Shared.Services.DefaultDatDatabase but lives in our namespace -/// so the WorldBuilder project reference can be dropped in T7. +/// Mirrors WorldBuilder.Shared.Services.DefaultDatDatabase (taken into our +/// tree in Phase O; moved here from AcDream.App in the MP1b adapter +/// unification). /// -internal sealed class DatDatabaseWrapper : IDatDatabase -{ +public sealed class DatDatabaseWrapper : IDatDatabase { private readonly DatDatabase _db; private readonly ConcurrentDictionary<(Type, uint), IDBObj> _cache = new(); private readonly object _lock = new(); - public DatDatabaseWrapper(DatDatabase db) - { + public DatDatabaseWrapper(DatDatabase db) { ArgumentNullException.ThrowIfNull(db); _db = db; } @@ -135,18 +139,14 @@ internal sealed class DatDatabaseWrapper : IDatDatabase public IEnumerable GetAllIdsOfType() where T : IDBObj => _db.GetAllIdsOfType(); - public bool TryGet(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj - { - if (_cache.TryGetValue((typeof(T), fileId), out var cached)) - { + public bool TryGet(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj { + if (_cache.TryGetValue((typeof(T), fileId), out var cached)) { value = (T)cached; return true; } - lock (_lock) - { - if (_db.TryGet(fileId, out value)) - { + lock (_lock) { + if (_db.TryGet(fileId, out value)) { _cache.TryAdd((typeof(T), fileId), value); return true; } @@ -155,8 +155,9 @@ internal sealed class DatDatabaseWrapper : IDatDatabase // a miss for an id whose BTree entry EXISTS is always an anomaly — // either Unpack returned false or the lookup flickered transiently. // Legit not-found probes (e.g. Portal→HighRes fallback) stay silent. - if (_db.Tree.TryGetFile(fileId, out _)) - { + // Kept through the MP1b unification — the tripwire now covers the + // bake tool and the equivalence suite too, not just the client. + if (_db.Tree.TryGetFile(fileId, out _)) { Console.WriteLine( $"[dat-miss] {typeof(T).Name} 0x{fileId:X8} entry EXISTS but TryGet failed " + $"(thread={Environment.CurrentManagedThreadId})"); @@ -166,18 +167,14 @@ internal sealed class DatDatabaseWrapper : IDatDatabase return false; } - public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) - { - lock (_lock) - { + public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) { + lock (_lock) { return _db.TryGetFileBytes(fileId, out value); } } - public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) - { - lock (_lock) - { + public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) { + lock (_lock) { return _db.TryGetFileBytes(fileId, ref bytes, out bytesRead); } } @@ -185,8 +182,7 @@ internal sealed class DatDatabaseWrapper : IDatDatabase public bool TrySave(T obj, int iteration = 0) where T : IDBObj => throw new NotSupportedException("DatDatabaseWrapper is read-only."); - public void Dispose() - { + public void Dispose() { // The underlying DatDatabase is owned by DatCollection — do not dispose here. } } diff --git a/src/AcDream.Content/IDatReaderWriter.cs b/src/AcDream.Content/IDatReaderWriter.cs index 164bb42f..2ab4afda 100644 --- a/src/AcDream.Content/IDatReaderWriter.cs +++ b/src/AcDream.Content/IDatReaderWriter.cs @@ -10,9 +10,10 @@ using System.Diagnostics.CodeAnalysis; // // MP1a (2026-07-05, Task 4): moved to AcDream.Content, namespace only — // MeshExtractor's constructor takes IDatReaderWriter and must be GL-free. -// Consumers: MeshExtractor (this assembly) reads through it; -// DatCollectionAdapter (the concrete DatCollection-backed implementation) -// and ObjectMeshManager live in AcDream.App and implement/hold it from there. +// MP1b review (finding 7): the concrete DatCollection-backed implementation +// (DatCollectionAdapter, this assembly) now lives HERE too — one shared +// adapter for ObjectMeshManager (App), acdream-bake, and the equivalence +// suite, so the three consumers can never drift apart. namespace AcDream.Content; diff --git a/tests/AcDream.Content.Tests/ContentTestDatCollectionAdapter.cs b/tests/AcDream.Content.Tests/ContentTestDatCollectionAdapter.cs deleted file mode 100644 index 2a04f58e..00000000 --- a/tests/AcDream.Content.Tests/ContentTestDatCollectionAdapter.cs +++ /dev/null @@ -1,145 +0,0 @@ -using System; -using System.Collections.Concurrent; -using System.Collections.Generic; -using System.Collections.ObjectModel; -using System.Diagnostics.CodeAnalysis; -using AcDream.Content; -using DatReaderWriter; -using DatReaderWriter.Enums; -using DatReaderWriter.Lib.IO; - -namespace AcDream.Content.Tests; - -/// -/// Adapts to for -/// MeshExtractor's constructor, scoped to the equivalence test suite. A -/// third copy alongside AcDream.App.Rendering.Wb.DatCollectionAdapter -/// (internal to AcDream.App) and AcDream.Bake.BakeDatCollectionAdapter -/// (internal to AcDream.Bake) — tests live in the project matching the -/// layer under test (structure rule 6), so Content.Tests cannot reference -/// AcDream.Bake just to borrow its copy. Plain dat-access glue, not an -/// AC-specific algorithm. -/// -internal sealed class ContentTestDatCollectionAdapter : IDatReaderWriter { - private readonly DatCollection _dats; - private readonly TestDatDatabaseWrapper _portal; - private readonly TestDatDatabaseWrapper _cell; - private readonly TestDatDatabaseWrapper _highRes; - private readonly TestDatDatabaseWrapper _language; - private readonly ReadOnlyDictionary _cellRegions; - - public ContentTestDatCollectionAdapter(DatCollection dats) { - ArgumentNullException.ThrowIfNull(dats); - _dats = dats; - _portal = new TestDatDatabaseWrapper(dats.Portal); - _cell = new TestDatDatabaseWrapper(dats.Cell); - _highRes = new TestDatDatabaseWrapper(dats.HighRes); - _language = new TestDatDatabaseWrapper(dats.Local); - - var regions = new Dictionary { [0u] = _cell }; - _cellRegions = new ReadOnlyDictionary(regions); - } - - public string SourceDirectory => _dats.Options.DatDirectory ?? string.Empty; - - public IDatDatabase Portal => _portal; - public ReadOnlyDictionary CellRegions => _cellRegions; - public IDatDatabase HighRes => _highRes; - public IDatDatabase Language => _language; - - public ReadOnlyDictionary RegionFileMap => - new ReadOnlyDictionary(new Dictionary()); - - public int PortalIteration => _portal.Iteration; - public int CellIteration => _cell.Iteration; - public int HighResIteration => _highRes.Iteration; - public int LanguageIteration => _language.Iteration; - - public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) { - return _dats.Cell.TryGetFileBytes(fileId, ref bytes, out bytesRead); - } - - public IEnumerable ResolveId(uint id) { - var results = new List(); - - void CheckDb(TestDatDatabaseWrapper wrapper) { - var rawDb = wrapper.RawDatabase; - if (rawDb.Tree.TryGetFile(id, out _)) { - var type = rawDb.TypeFromId(id); - if (type != DBObjType.Unknown) { - results.Add(new IDatReaderWriter.IdResolution(wrapper, type)); - } - } - } - - CheckDb(_highRes); - CheckDb(_portal); - CheckDb(_language); - CheckDb(_cell); - - return results; - } - - public bool TrySave(T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(ContentTestDatCollectionAdapter)} is read-only."); - - public bool TrySave(uint regionId, T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(ContentTestDatCollectionAdapter)} is read-only."); - - public void Dispose() { - // The underlying DatCollection is owned by the caller. - } -} - -internal sealed class TestDatDatabaseWrapper : IDatDatabase { - private readonly DatDatabase _db; - private readonly ConcurrentDictionary<(Type, uint), IDBObj> _cache = new(); - private readonly object _lock = new(); - - public TestDatDatabaseWrapper(DatDatabase db) { - ArgumentNullException.ThrowIfNull(db); - _db = db; - } - - internal DatDatabase RawDatabase => _db; - - public DatDatabase Db => _db; - public int Iteration => _db.Iteration?.CurrentIteration ?? 0; - - public IEnumerable GetAllIdsOfType() where T : IDBObj => _db.GetAllIdsOfType(); - - public bool TryGet(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj { - if (_cache.TryGetValue((typeof(T), fileId), out var cached)) { - value = (T)cached; - return true; - } - - lock (_lock) { - if (_db.TryGet(fileId, out value)) { - _cache.TryAdd((typeof(T), fileId), value); - return true; - } - } - - return false; - } - - public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) { - lock (_lock) { - return _db.TryGetFileBytes(fileId, out value); - } - } - - public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) { - lock (_lock) { - return _db.TryGetFileBytes(fileId, ref bytes, out bytesRead); - } - } - - public bool TrySave(T obj, int iteration = 0) where T : IDBObj => - throw new NotSupportedException($"{nameof(TestDatDatabaseWrapper)} is read-only."); - - public void Dispose() { - // The underlying DatDatabase is owned by DatCollection. - } -} diff --git a/tests/AcDream.Content.Tests/PakEquivalenceTests.cs b/tests/AcDream.Content.Tests/PakEquivalenceTests.cs index 707b4adb..e42567f3 100644 --- a/tests/AcDream.Content.Tests/PakEquivalenceTests.cs +++ b/tests/AcDream.Content.Tests/PakEquivalenceTests.cs @@ -42,7 +42,9 @@ public sealed class PakEquivalenceTests { if (datDir is null) return; // dats absent (CI) — skip, matching suite convention using var dats = new DatCollection(datDir, DatAccessType.Read); - using var datReaderWriter = new ContentTestDatCollectionAdapter(dats); + // The unified AcDream.Content.DatCollectionAdapter — same class the + // client and the bake tool use (MP1b review finding 7). + using var datReaderWriter = new DatCollectionAdapter(dats); var logger = new TestConsoleLogger(); var sideStaged = new List(); diff --git a/tests/AcDream.Content.Tests/PakRoundTripTests.cs b/tests/AcDream.Content.Tests/PakRoundTripTests.cs index 20f6db43..48ec18fc 100644 --- a/tests/AcDream.Content.Tests/PakRoundTripTests.cs +++ b/tests/AcDream.Content.Tests/PakRoundTripTests.cs @@ -383,6 +383,79 @@ public class PakRoundTripTests : IDisposable { Assert.False(File.Exists(path)); } + // ---- on-disk TOC sortedness (review finding 8) --------------------------- + + [Fact] + public void OnDiskToc_IsSortedAscendingByKey_RegardlessOfAddOrder() { + var path = NewTempPakPath(); + // Add blobs in DESCENDING key order — the on-disk TOC must still come + // out ascending (the reader's binary-search precondition, asserted + // here against the raw bytes, not through the reader). + var blobs = MakeBlobSet(12).OrderByDescending(b => PakKey.Compose(b.Type, b.FileId)).ToArray(); + WritePak(path, blobs); + + var fileBytes = File.ReadAllBytes(path); + var header = PakHeader.ReadFrom((ReadOnlySpan)fileBytes); + Assert.Equal((uint)blobs.Length, header.TocCount); + + ulong previousKey = 0; + for (uint i = 0; i < header.TocCount; i++) { + int pos = checked((int)((long)header.TocOffset + i * PakTocEntry.Size)); + var entry = PakTocEntry.ReadFrom(fileBytes.AsSpan(pos, PakTocEntry.Size)); + Assert.True(entry.Key > previousKey || i == 0, + $"TOC entry {i} key 0x{entry.Key:X16} is not strictly greater than its predecessor 0x{previousKey:X16}"); + previousKey = entry.Key; + } + } + + // ---- corruption logged once (review finding 8) ---------------------------- + + [Fact] + public void CorruptBlob_RepeatedReads_LogExactlyOnce() { + var path = NewTempPakPath(); + var blobs = MakeBlobSet(2); + WritePak(path, blobs); + + // Flip a byte in the first blob (plain CRC corruption). + using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) { + fs.Position = PakHeader.Size + 4; + int b = fs.ReadByte(); + fs.Position = PakHeader.Size + 4; + fs.WriteByte((byte)(b ^ 0xFF)); + } + + var victimKey = PakKey.Compose(blobs[0].Type, blobs[0].FileId); + var originalError = Console.Error; + var capture = new StringWriter(); + try { + Console.SetError(capture); + using var reader = new PakReader(path); + // Hammer the corrupt entry through BOTH public paths, repeatedly. + Assert.False(reader.TryReadObjectMeshData(victimKey, out _)); + Assert.False(reader.TryReadObjectMeshData(victimKey, out _)); + Assert.False(reader.ContainsKey(victimKey)); + Assert.False(reader.ContainsKey(victimKey)); + Assert.False(reader.TryReadObjectMeshData(victimKey, out _)); + } + finally { + Console.SetError(originalError); + } + + string logged = capture.ToString(); + int occurrences = CountOccurrences(logged, $"0x{victimKey:X16}"); + Assert.True(occurrences == 1, + $"expected exactly ONE [pak-corrupt] line for key 0x{victimKey:X16} across 5 reads, got {occurrences}:\n{logged}"); + } + + private static int CountOccurrences(string haystack, string needle) { + int count = 0, index = 0; + while ((index = haystack.IndexOf(needle, index, StringComparison.Ordinal)) >= 0) { + count++; + index += needle.Length; + } + return count; + } + // ---- helpers ------------------------------------------------------------- /// Locates the on-disk file position of the TOC entry for by raw parsing.