HarbourMasters / HarbourMasters/Shipwright
why the dinner room is broken
- Dominant language
- C
- Stars
- 5.4k
- Forks
- 837
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 72
Description
tl;dr -
* this has been broken since `8.0.0`
* the fix in https://github.com/HarbourMasters/Shipwright/pull/4136 didn't work because it moved the file without changing it (the binary references the wrong paths still)
* it currently wouldn't work even if the paths were fixed because `oot.o2r` wins over `soh.o2r` so the empty extracted file is loaded
Claude investigation
## Summary
The **Syotes** test scene (Room 120) no longer renders its intended geometry. Depending on version you get either the *wrong* geometry (leftover/garbage from the previous scene) or an *empty* room. Root cause is an **archive path collision**: the hand-authored custom room shipped in `soh.o2r` and an **empty ROM-extracted room stub** in `oot.o2r` both occupy the exact same resource path, and load order makes the empty stub win.
## Symptoms by release
| Version | Behavior |
|---|---|
| 7.1.1 (Sulu Bravo) | ✅ loads correctly |
| 8.0.0 – 8.0.5 | ❌ loads the **wrong** geometry |
| 8.0.6 (MacReady Golf) | ❌ loads **nothing** |
| latest `develop` | ❌ loads the **wrong** geometry |
Bisected across release tags to `8.0.0` as the first bad release (`7.1.1` good). The two related PRs are **#3191** ("[OTR Archive] Move shared scenes out of nonmq/mq folders") and **#4136** ("Fix syotes scene path to use shared").
## Root cause
Two files resolve to the **identical archive path** `scenes/shared/syotes_scene/syotes_room_0`:
| Archive | Size | What it is |
|---|---|---|
| `soh.o2r` (custom/port assets) | 211 B | hand-authored room with real mesh; the *intended* room |
| `oot.o2r` (ROM-extracted) | 68 B | an **empty** room — header + zero commands |
The 68 B extracted room is genuinely empty:
```
0x00 00000000 4d4f524f("MORO") 00000000 efbeadde <- 0x40-byte resource header
0x40 00 00 00 00 <- command list: zero commands
```
Compare `syotes2_room_0`, which extracts correctly (a real `0x16` mesh command pointing at `scenes/shared/syotes2_scene/syotes2_room_0DL_0046B8`, which exists). Syotes2 works precisely because it has no custom binary and nothing to collide with.
### Why the empty stub wins
`ArchiveManager::AddArchive` maps every file hash to its archive with an unconditional overwrite:
```cpp
mFileToArchive[hash] = archive; // last archive added for a given path wins
```
- `soh.o2r` is added **first** — `OTRGlobals.cpp:303` → `Context::InitResourceManager({ portArchivePath }, ...)` (eager, `Context.cpp:243`).
- `oot.o2r` is added **last** — `OTRGlobals::Initialize()` → `AddArchive(ootPath)` (`OTRGlobals.cpp:796`).
So `LoadFile("scenes/shared/syotes_scene/syotes_room_0")` returns `oot.o2r`'s empty stub. An empty room draws no mesh, so the renderer executes whatever display-list/segment state was left over → "wrong stuff." The custom 211 B room in `soh.o2r` is shadowed and never loads.
### Secondary bug in the custom binary
Even if the custom room *did* win, its two internal DL references are stale:
```
scenes/nonmq/syotes_scene/syotes_room_0DL_0031C80 <- wrong folder (nonmq) AND name doesn't exist (archive has 0031C8)
scenes/nonmq/syotes_scene/syotes_room_0DL_00BF70 <- name exists, but under shared, not nonmq
```
#4136 relocated the custom binary's *file* to the `shared` folder but never rewrote the `nonmq` path strings baked *inside* it. (Currently moot, since the binary is shadowed.)
## Where the DLs come from
The real geometry DLs (`syotes_room_0DL_*`, `Vtx_*`) are extracted by ZAPD's `SyotesRoomFix()` (`ZRoom.cpp:337`), driven by `` in `assets/xml/GC_{N,}MQ_D/scenes/test_levels/syotes.xml`. Room 120 is a headerless early-dev room, so the hack parses a `RoomShapeCullable` at offset 0 and `poly.DeclareReferences()` emits the DLs — while the room resource body comes out empty. In other words, the same `` element that provides the DLs also produces the empty stub.
## Why there is no asset-only (XML) fix
- The scene's room list references the room by `res->GetName()` (`SetRoomList.cpp` → `RomFile::GetBodySourceCode`, iterates `Room` resources).
- The archive leaf path is also built from `res->GetName()` (`OTRExporter Main.cpp:325`, `fName = scenes/shared//`). `OutName` is **not** used for the leaf.
So the extracted stub's path and the string the scene looks up are the *same value*. Renaming the `` moves both together (orphaning the custom room); removing it empties the scene's room list. There is no XML attribute that decouples reference-name from output-path here.
## Proposed fixes (in order of preference)
1. **ZAPD:** in `SyotesRoomFix`, keep the DL declaration + room-list entry but **don't emit the empty room resource body**, so nothing is extracted at `scenes/shared/syotes_scene/syotes_room_0` and the custom `soh.o2r` room wins. Then also repoint the custom binary's internal DL refs from `scenes/nonmq/...` to the real `scenes/shared/syotes_scene/syotes_room_0DL_{0031C8,00BF70}`.
2. **soh archive precedence:** make `soh.o2r` (custom/port assets) win duplicate paths over `oot.o2r` (add it last, or a "custom overrides ROM" rule). This is arguably the correct semantics for shipped custom assets, but it changes global resource resolution and needs testing.
3. **ZAPD (proper):** make `SyotesRoomFix` emit a real room from the ROM data, retiring the custom binary entirely.
## Repro / verification notes
- Inspect an `oot.o2r` (it's a zip): `scenes/shared/syotes_scene/syotes_room_0` is 68 bytes and empty; the same path in `soh.o2r` is 211 bytes with `nonmq` DL strings.
- Load order confirmed in `OTRGlobals.cpp` (soh first via `InitResourceManager`, oot last via `Initialize`) and `ArchiveManager::AddArchive` (`mFileToArchive[hash] = archive`).
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with ArchiveManager::AddArchive and the archive setup in OTRGlobals.cpp and Context.cpp, then inspect ZRoom.cpp:337 and the SyotesRoomFix XML entries. Reproduce the collision by comparing the Syotes room in oot.o2r and soh.o2r. Done means the intended Syotes geometry loads reliably and the stale internal display-list references no longer point at nonexistent paths.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- game-dev, tooling
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100