fix(builder): find damaged textures still in the .nup's zstd frame #2391

Merged

SimonKimDev merged 1 commit from fix/wrapped-damaged-textures into main

2026-11-11 21:32:54 +00:00

SimonKimDev commented

2026-11-11 16:59:57 +00:00

(Migrated from github.com)

A .zs file whose frame states a content size equal to its own file size is decompressed at its start; all zeros, or no decompression, now count as damaged.
.hookshot_romfs_checked carries a check version, so folders already checked are checked once more.

Closes #2361

Builders before #456 left three damaged .bntx.zs files wrapped in the .nup's own zstd frame. They start as zstd, so the damaged-file check passed them and the Water Temple, Great Fairy Fountain and Ganon's Stage kept their invisible walls and floors. - A .zs file whose frame states a content size equal to its own file size is decompressed at its start; all zeros, or no decompression, now count as damaged. - `.hookshot_romfs_checked` carries a check version, so folders already checked are checked once more. Closes #2361

hckrman101 (Migrated from github.com) reviewed

2026-11-11 17:35:26 +00:00

hckrman101 (Migrated from github.com) left a comment

Nice, targeted fix, and the approach is sound. Checked at c474bd0.

Common 23.3 GB dump: no false positives that I can see. still_in_nup_frame only does work when the frame header states a content size equal to the file's own size, and a real game .zs is compressed, so its content is larger than the file; those return False from the 18-byte head alone. A dictionary-made file that happened to match the size would raise ZstdError and be listed as damaged, but that needs the content size to equal the compressed size, which a compressed file does not hit in practice. The three known zero-head files are still caught by the old check.

Performance: no extra open or read (18 bytes instead of 4 from the same read), a header parse in Python per file, and at most a 16-byte decompress on size matches. Negligible next to the 100,000 file opens.

Overlap: #2253 and #2254 are merged and don't touch these lines. #2096 (open, approved) does: it changes the same .hookshot_romfs_checked stamp lines in stand_in_damaged_files ("resolved-v1\n" + stamp, versus your CHECK + stamp) and the same two assertions in tests/test_make_game_cache.py. GitHub already shows this PR as conflicting. Whichever lands second has to merge both versions into one stamp, and the order matters: if #2096 lands first, folders already stamped resolved-v1 must still be re-scanned for these wrapped files, so the combined stamp needs a version that older stamps don't match. Worth settling with #2096 before merging. Also, once #2096 is in, a wrapped texture with no replacement now stops the build, which is the right behaviour here too.

CI: the red jobs are tests/test_vcore_video_acquire.py and tests/test_vcore_fs_open.py (undeclared env_on in a fixture) plus an appimage timeout. None are in files this PR touches, and nx2/test_nup_stand_in.py, test_nup_zstd_heads.py and test_make_game_cache.py pass on ubuntu and windows. The title check was still pending.

Nit: RUN_ALL_MODULES = ["zstandard"] in test_nup_stand_in.py is right, thanks for declaring it.

Not approving yet only because of the stamp conflict with #2096; the code itself looks good.

Nice, targeted fix, and the approach is sound. Checked at c474bd0. Common 23.3 GB dump: no false positives that I can see. `still_in_nup_frame` only does work when the frame header states a content size equal to the file's own size, and a real game .zs is compressed, so its content is larger than the file; those return False from the 18-byte head alone. A dictionary-made file that happened to match the size would raise ZstdError and be listed as damaged, but that needs the content size to equal the compressed size, which a compressed file does not hit in practice. The three known zero-head files are still caught by the old check. Performance: no extra open or read (18 bytes instead of 4 from the same read), a header parse in Python per file, and at most a 16-byte decompress on size matches. Negligible next to the 100,000 file opens. Overlap: #2253 and #2254 are merged and don't touch these lines. #2096 (open, approved) does: it changes the same `.hookshot_romfs_checked` stamp lines in `stand_in_damaged_files` (`"resolved-v1\n" + stamp`, versus your `CHECK + stamp`) and the same two assertions in `tests/test_make_game_cache.py`. GitHub already shows this PR as conflicting. Whichever lands second has to merge both versions into one stamp, and the order matters: if #2096 lands first, folders already stamped `resolved-v1` must still be re-scanned for these wrapped files, so the combined stamp needs a version that older stamps don't match. Worth settling with #2096 before merging. Also, once #2096 is in, a wrapped texture with no replacement now stops the build, which is the right behaviour here too. CI: the red jobs are `tests/test_vcore_video_acquire.py` and `tests/test_vcore_fs_open.py` (undeclared `env_on` in a fixture) plus an appimage timeout. None are in files this PR touches, and nx2/test_nup_stand_in.py, test_nup_zstd_heads.py and test_make_game_cache.py pass on ubuntu and windows. The title check was still pending. Nit: `RUN_ALL_MODULES = ["zstandard"]` in test_nup_stand_in.py is right, thanks for declaring it. Not approving yet only because of the stamp conflict with #2096; the code itself looks good.

SimonKimDev commented

2026-11-11 18:01:16 +00:00

(Migrated from github.com)

Thanks. Rebased onto main. I'll wait for #2096: once it lands I'll rebase onto it and make the combined stamp resolved-v2, so folders already stamped resolved-v1 are checked again for the wrapped files.

Thanks. Rebased onto main. I'll wait for #2096: once it lands I'll rebase onto it and make the combined stamp `resolved-v2`, so folders already stamped `resolved-v1` are checked again for the wrapped files.

Arkex-ai commented

2026-11-11 18:04:44 +00:00

(Migrated from github.com)

Thanks for coordinating with #2096. resolved-v2 after it lands makes sense: it must invalidate both the legacy stamp and resolved-v1, while only marking a check successful after every detected file has a stand-in. I'm finishing the current-main integration that writes the persistent damaged-file report before an unresolved failure. Please preserve that ordering, the unresolved heads list for retries, and the strict UTF-8/legacy stamp reader when combining the paths. Your proposed merge order works for me.

Thanks for coordinating with #2096. `resolved-v2` after it lands makes sense: it must invalidate both the legacy stamp and `resolved-v1`, while only marking a check successful after every detected file has a stand-in. I'm finishing the current-main integration that writes the persistent damaged-file report before an unresolved failure. Please preserve that ordering, the unresolved heads list for retries, and the strict UTF-8/legacy stamp reader when combining the paths. Your proposed merge order works for me.

👍 1

SimonKimDev commented

2026-11-11 20:04:08 +00:00

(Migrated from github.com)

Rebased onto #2096: its marker is now resolved-v2, with #2096's report ordering, heads list and stamp reader kept as is. A resolved-v1 marker gets one re-check.

Rebased onto #2096: its marker is now `resolved-v2`, with #2096's report ordering, heads list and stamp reader kept as is. A `resolved-v1` marker gets one re-check.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 21:22:23 +00:00

Joey-Magnus (Migrated from github.com) left a comment

The blocker is resolved: #2096 is merged and this is rebased onto it. resolved-v2 (nx2/make_game.py:463) re-checks folders stamped v1, and a test covers that case. still_in_nup_frame only decompresses when the frame's content size equals the file size, so real .zs files are skipped without the cost. CI is green.

The blocker is resolved: #2096 is merged and this is rebased onto it. `resolved-v2` (`nx2/make_game.py:463`) re-checks folders stamped v1, and a test covers that case. `still_in_nup_frame` only decompresses when the frame's content size equals the file size, so real `.zs` files are skipped without the cost. CI is green.