acdream/docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md
Erik 0d85fe1f10 plan(O): Phase O — DatPath Unification — filed + active (pre-empts M1.5)
Phase O extracts the WB pieces we actually use (mesh pipeline, texture
decode, GL state, scenery, terrain blending, EnvCell/portal decode —
roughly 3-5K LOC) into src/AcDream.Core/Rendering/Wb/, swaps their
dat dependency from DefaultDatReaderWriter to our DatCollection, and
drops the WorldBuilder.Shared + Chorizite.OpenGLSDLBackend project
references. WB stays in references/ as a read reference, not as a
project dependency. MIT attribution in NOTICE.md.

Tagline: ONE thing touches the DATs.

Discipline: verbatim copy first, no "improvements" while extracting.
Refactors land in follow-up phases. Out of scope: re-porting from
retail decomp; perf optimization; API cleanup.

User direction 2026-05-21: pre-empts M1.5. M1.5 paused at its
2026-05-20 baseline; A6/A7 don't touch dat infrastructure so no
rework needed when it resumes.

Files:
- docs/superpowers/specs/2026-05-21-phase-o-dat-path-unification-design.md (new, full spec)
- docs/plans/2026-04-11-roadmap.md (Phase O block inserted before M1.5; M1.5 marked PAUSED)
- CLAUDE.md (Currently-working-toward line updated; M1.5 block marked paused)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-21 13:59:33 +02:00

237 lines
17 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Phase O — DatPath Unification — Design Spec
**Filed:** 2026-05-21
**Status:** DRAFT (not yet scheduled)
**Owner:** TBD
**Estimated effort:** 1-2 focused weeks of work; one ship-window, no slicing across multiple weeks.
> **Tagline**: ONE thing touches the DATs. Today we have two readers in
> process (acdream's `DatCollection` + WorldBuilder's
> `DefaultDatReaderWriter`) reading the same files independently. Phase O
> collapses that to one.
---
## 1. Problem statement
As of Phase N.4 ship (2026-05-08), WorldBuilder is integrated as our
rendering + dat-handling base. Concretely:
- `WbMeshAdapter.cs:79` constructs **`_wbDats = new DefaultDatReaderWriter(datDir)`** — WB's own dat reader.
- Our **`DatCollection`** is constructed independently at startup for the rest of the client (network, physics, animation, clothing, audio, UI, etc.).
**Both readers open the same four files** (`client_portal.dat`,
`client_cell_1.dat`, `client_highres.dat`, `client_local_English.dat`)
with independent file handles and independent in-memory index caches.
**Costs**:
- **~50-100 MB duplicated index cache memory** (per `WbMeshAdapter.cs:27` comment).
- **Double seek cost** when the same dat block is read by both pipelines (mesh path via WB; surface metadata side-table via our `DatCollection`).
- **Cross-check awkwardness** — `WbMeshAdapter.cs:224-229` has explicit "if WE find the cell but WB doesn't" diagnostic code, born from the divergence.
- **Architectural smell** — a third-party feedback signal (AC community comment, 2026-05-21) flagged WB's dat-touching as "built for tools, not runtime." The double-reader is the most concrete manifestation.
**WB is MIT-licensed**, so the path to fixing this is not to "wrap" WB but to **extract its load-bearing code into our repo** and route it through our `DatCollection`. This is the long-term clean architecture: one reader, one cache, one source of dat truth.
---
## 2. Decisions log
| # | Decision | Why |
|---|---|---|
| O-D1 | **Extract WB code verbatim into our repo.** No re-port from retail decomp. No "improvements" while extracting. | CLAUDE.md is explicit: "Re-porting from retail decomp when WB already has a tested port is how subtle bugs (scenery edge-vertex, triangle-Z) keep slipping in." Verbatim copy preserves all the corner-case fixes WB already has. |
| O-D2 | **Make extracted code consume `DatCollection`**, not `IDatReaderWriter`. | Single source of dat truth. The whole point of the phase. |
| O-D3 | **Drop the `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend` project references** at the end of the phase. | If we still reference WB after extraction, we haven't actually finished the work. |
| O-D4 | **Keep WB in `references/` for reading/comparison**. Don't delete the vendored directory. | We'll still want to grep WB during ports of NEW pieces (e.g., minimap renderer if/when we add it). |
| O-D5 | **MIT attribution per WB convention**. Add `NOTICE.md` entry crediting WorldBuilder for the extracted code. | License compliance. |
| O-D6 | **One ship-window, not sliced**. Either the whole extraction lands and `references/WorldBuilder` is dropped, or we roll back the entire phase. | Half-extracted state (some WB code in our repo, some still referenced) is worse than either endpoint. |
---
## 3. Architecture overview
### Today (Phase N.4A.5 state)
```
┌─────────────────────────┐ ┌──────────────────────────────────┐
│ acdream subsystems │ │ WorldBuilder (referenced project)│
│ - Network │ │ ┌────────────────────────────┐ │
│ - Physics │ ───→ │ │ DefaultDatReaderWriter │ │
│ - Animation │ │ │ (opens 4 .dat files) │ │
│ - Clothing/Audio/UI │ │ └────────────┬───────────────┘ │
│ - Surface metadata │ │ │ │
└──────────┬──────────────┘ │ ┌────────────▼───────────────┐ │
│ │ │ ObjectMeshManager │ │
│ DatCollection │ │ TextureHelpers │ │
│ (opens same 4 files) │ │ SceneryHelpers │ │
│ │ │ LandSurfaceManager │ │
▼ │ │ OpenGLGraphicsDevice │ │
┌──────────────┐ │ └────────────────────────────┘ │
│ Dat files │ └──────────────────────────────────┘
│ (same files, │ ▲
│ two readers)│──────────────┘
└──────────────┘
```
### Phase O target
```
┌─────────────────────────────────────────────────────────────────┐
│ acdream subsystems │
│ - Network / Physics / Animation / Clothing / Audio / UI │
│ - Mesh pipeline (extracted from WB.ObjectMeshManager) │
│ - Texture decode (extracted from WB.TextureHelpers) │
│ - Scenery (extracted from WB.SceneryHelpers etc.) │
│ - Terrain blend (extracted from WB.LandSurfaceManager) │
│ - GL state (extracted from WB.OpenGLGraphicsDevice) │
└──────────────────────────┬──────────────────────────────────────┘
│ DatCollection (the ONLY reader)
┌──────────────┐
│ Dat files │
└──────────────┘
```
### What stays acdream-original (unchanged by this phase)
- `TerrainModernRenderer` (Phase N.5b — already ours, uses `LandblockMesh.Build` with retail's `FSplitNESW`)
- Network, physics, animation, movement, UI, audio, chat, streaming controller, plugin API
- Our `DatCollection` itself (becomes the **only** dat reader)
---
## 4. Component changes (the actual extract list)
### 4.1 Mesh pipeline (highest-value extraction)
| WB source | New acdream home | Adaptation |
|---|---|---|
| `Chorizite.OpenGLSDLBackend.Lib.ObjectMeshManager` | `src/AcDream.Core/Rendering/Wb/ObjectMeshManager.cs` | Replace `IDatReaderWriter` field with `DatCollection`. Constructor takes our `DatCollection`. All `_dats.Get<T>()` calls preserved. |
| `Chorizite.OpenGLSDLBackend.Lib.ObjectRenderBatch` + supporting types (`TextureKey`, etc.) | Same directory | Verbatim copy. No interface changes. |
| `Chorizite.OpenGLSDLBackend.Lib.SceneryHelpers` | `src/AcDream.Core/Rendering/Wb/SceneryHelpers.cs` | Verbatim. |
| `Chorizite.OpenGLSDLBackend.Lib.SceneryRenderManager` | Same | Verbatim + DatCollection swap. |
### 4.2 Texture pipeline
| WB source | New acdream home | Adaptation |
|---|---|---|
| `Chorizite.OpenGLSDLBackend.Lib.TextureHelpers` | `src/AcDream.Core/Rendering/Wb/TextureHelpers.cs` | Verbatim copy. Pure functions — no dat dependency. |
| `Chorizite.OpenGLSDLBackend.Lib.LandSurfaceManager` | `src/AcDream.Core/Rendering/Wb/LandSurfaceManager.cs` | Verbatim + DatCollection swap. |
### 4.3 GL infrastructure
| WB source | New acdream home | Adaptation |
|---|---|---|
| `Chorizite.OpenGLSDLBackend.OpenGLGraphicsDevice` | `src/AcDream.App/Rendering/Wb/OpenGLGraphicsDevice.cs` | Verbatim. No dat dependency. |
| `Chorizite.OpenGLSDLBackend.Frustum` + `VisibilityManager` | Same | Verbatim. |
### 4.4 EnvCell decode
| WB source | New acdream home | Adaptation |
|---|---|---|
| `Chorizite.OpenGLSDLBackend.Lib.EnvCellRenderManager` (if used at runtime) | `src/AcDream.Core/Rendering/Wb/EnvCellRenderManager.cs` | Verbatim + DatCollection swap. |
| `Chorizite.OpenGLSDLBackend.Lib.PortalRenderManager` | Same | Verbatim. |
### 4.5 What we DROP
- `_wbDats = new DefaultDatReaderWriter(datDir)` in `WbMeshAdapter.cs:79` — replaced with the existing `_dats: DatCollection` field.
- Cross-check code in `WbMeshAdapter.cs:224-229` (`if WE find the cell but WB doesn't`) — no longer relevant after unification.
- The two project references in `AcDream.App.csproj` and `AcDream.Core.csproj` to `WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend`.
---
## 5. Task breakdown
| Task | Description | Estimated effort |
|---|---|---|
| O-T1 | Audit WB call graph — produce a closure of every WB type/file we transitively use. Generate a "must extract" list with line counts. | 0.5d |
| O-T2 | Create `src/AcDream.Core/Rendering/Wb/` directory structure. Add `NOTICE.md` entry with MIT attribution to WB. | 0.25d |
| O-T3 | **Extract texture / GL infrastructure** (`TextureHelpers`, `OpenGLGraphicsDevice`, `Frustum`, `VisibilityManager`) — verbatim copy, no dat dependency means low risk. Build green. | 1d |
| O-T4 | **Extract mesh pipeline** (`ObjectMeshManager`, `ObjectRenderBatch`, `TextureKey`) — verbatim copy. Replace `IDatReaderWriter` with `DatCollection`. Build green. Existing tests green. | 2d |
| O-T5 | **Extract scenery + terrain pipelines** (`SceneryHelpers`, `SceneryRenderManager`, `LandSurfaceManager`) — verbatim + DatCollection swap. Build green. Visual verification on outdoor terrain. | 1.5d |
| O-T6 | **Extract EnvCell + portal renderers** if used at runtime (`EnvCellRenderManager`, `PortalRenderManager`) — verbatim + DatCollection swap. Visual verification on indoor scenes. | 1d |
| O-T7 | **Drop project references** from `AcDream.App.csproj` and `AcDream.Core.csproj`. Drop `_wbDats` and the cross-check code from `WbMeshAdapter`. Build green. | 0.5d |
| O-T8 | **Verification gate** — visual side-by-side with main against retail in three scenes: Holtburg town (outdoor + scenery), inn interior (EnvCell), and a dungeon (portals). User confirms "looks identical." | 1d (incl. user time) |
| O-T9 | **Ship** — single merge to main with one descriptive commit per task (T3..T7), then a "drop WB references" final commit. Update CLAUDE.md to remove the "WB is referenced as projects" language and replace with "extracted into src/AcDream.Core/Rendering/Wb/." Update `docs/architecture/worldbuilder-inventory.md`. | 0.5d |
| **Total** | | **~7-8 working days** + visual verification cycles |
---
## 6. Acceptance criteria
- [ ] `dotnet build` green across the solution.
- [ ] `dotnet test` green; no test count regression (currently ~1147).
- [ ] **Zero references to `WorldBuilder.*` or `Chorizite.OpenGLSDLBackend.*` namespaces** in `AcDream.App.csproj` and `AcDream.Core.csproj` (`PackageReference` for `Chorizite.DatReaderWriter` stays — that's the NuGet DatReaderWriter lib, not WB).
- [ ] **Zero `using WorldBuilder.*`** in `src/AcDream.*` (the extracted code lives in our namespace now).
- [ ] **`DefaultDatReaderWriter` referenced in exactly zero places** in our source. `DatCollection` is the only dat reader.
- [ ] Resident memory measured at `streaming: radius=4` + 50 entities visible: **>50 MB reduction** vs. pre-Phase-O main (validates the duplication is actually gone).
- [ ] **Visual side-by-side with main**: Holtburg town, an inn interior, a dungeon — all render identically. User confirms.
- [ ] `NOTICE.md` includes WB attribution per its MIT license.
- [ ] `references/WorldBuilder/` directory remains in the repo for read-reference, but is not referenced by any csproj.
- [ ] `CLAUDE.md` updated: WB is now a *read reference*, not a *project dependency*. The "WB integration cribs" section is rewritten to point at our extracted code.
- [ ] `docs/architecture/worldbuilder-inventory.md` updated to reflect the new ownership.
---
## 7. Risks + mitigations
| Risk | Likelihood | Severity | Mitigation |
|---|---|---|---|
| Re-introduce bugs WB had already fixed (edge-vertex, triangle-Z) | Medium | High | **Discipline: verbatim copy first.** No "improvements." A separate post-Phase-O ticket can refactor. |
| WB has implicit assumptions about `DefaultDatReaderWriter` semantics our `DatCollection` doesn't match (caching, thread safety) | Medium | High | Task O-T1 must audit the actual call surface used. If `DatCollection` diverges, add a thin adapter layer rather than monkey-patch WB code. |
| Hidden transitive WB dependencies (some helper class we didn't notice we use) | Low | Medium | Task O-T1 closure pass should catch this. If it slips, build will fail loudly. |
| Visual regression we don't catch in side-by-side | Low | Medium | Take screenshots of three reference scenes BEFORE starting; compare AFTER. Don't trust eyeballs alone. |
| User upstream-tracks WB for some reason | Low | Low | We keep `references/WorldBuilder/` in-tree. Re-syncing diffs is a manual port if needed (same as today). |
| Phase O is scoped too tightly and a piece of WB we use is missed | Medium | Medium | T1's audit is the safety. If we miss one, build fails before T7's reference-drop. |
---
## 8. Out of scope (explicitly)
- **Re-porting from retail decomp** anywhere in the extracted code. We copy WB verbatim. If a retail-faithfulness audit is needed later, file a separate phase.
- **Performance optimization** of extracted code. Even if WB's `ObjectMeshManager` has a 30% improvement waiting in it, this phase ships it as-is.
- **API cleanup** of the extracted code. If the constructor has 8 parameters and it's ugly, ugly it stays. Refactor in a follow-up.
- **Refactoring `WbMeshAdapter` itself.** Phase O drops the second reader; the adapter shape stays.
- **`tools/InspectCoatTex` or other tools that use the NuGet `Chorizite.DatReaderWriter`**. Those keep working — they use the NuGet `DatReaderWriter`, not the WB project reference.
---
## 9. Open questions
1. **Does WB's `ObjectMeshManager` thread-model match `LandblockStreamer`'s worker thread?**
- Phase O-T1 must answer. If WB assumes single-threaded mesh-pipeline access and we call into it from the worker, we have a real bug today that gets exposed louder after extraction.
- Fallback: if WB's code is not thread-safe, route the mesh-prepare call through a dispatcher to the render thread (extra latency, but correct).
2. **Should we extract WB's `TerrainRenderManager`** even though we already wrote `TerrainModernRenderer`?
- Today: we use OUR terrain renderer (Phase N.5b). WB's `TerrainRenderManager` is *not* in our render path.
- But: `LandSurfaceManager` (terrain texture blending) IS WB code we use, and it might pull on `TerrainRenderManager` types.
- Recommendation: extract `LandSurfaceManager` only; leave `TerrainRenderManager` in `references/` since we don't use it.
3. **What's the right namespace for extracted code?**
- Option A: `AcDream.Core.Rendering.Wb.*` — preserves "this came from WB" context.
- Option B: `AcDream.Core.Rendering.Meshes.*`, `AcDream.Core.Rendering.Textures.*` — clean namespaces, drops the WB association.
- Recommendation: **Option A** for now (keeps the audit trail), with a follow-up phase to fold into Option B once dust settles.
4. **Do we extract the `Chorizite.DatReaderWriter` NuGet package code itself?**
- **No.** That's a separate, well-maintained library on NuGet. We keep depending on it via `PackageReference`. The phase is specifically about the WB project references (`WorldBuilder.Shared` + `Chorizite.OpenGLSDLBackend`) — the *application* code, not the *protocol* library.
---
## 10. Naming + filing
- **Phase name**: O (next free letter — A-N + R are in use).
- **Phase tagline**: "ONE thing touches the DATs."
- **Roadmap entry**: add to `docs/plans/2026-04-11-roadmap.md` under "Phases ahead" with cross-reference to this spec.
- **Issue tracking**: file as a Phase, not an issue (multi-commit, multi-day, infrastructural).
---
## 11. After Phase O ships
Things that become unblocked or cheaper:
- **Single-cache memory pressure budget.** Easier to size streaming radii, MSAA, anisotropic budget, etc.
- **Audit a single dat-touching code path** instead of two when investigating bugs like #37 (the coat-stub palette overlay).
- **Future N.6+ rendering work** doesn't have to ask "is this WB's concern or ours?" — it's ours.
- **AC community-friendly architecture** — the "WB is for tools" criticism is addressed at the structural level, not just papered over.