Repository navigation
fix(fps): HOS_FPS_LOG read by the switches' rule, so test_env_on passes on main again - #2575
Merged
Merged
Conversation
…es on main again
host/oot_high_fps.c read its log switch as getenv("HOS_FPS_LOG") != NULL
(NotRexed#2470, kept by NotRexed#2552), the set-means-on form NotRexed#2042 retired:
HOS_FPS_LOG=0 turned the log on. tests/test_env_on.py finds such reads, so
it fails on main since NotRexed#2470: in run 43120809833 (eed4d91) in the Python
source checks' ubuntu-24.04 part 2/2 and windows-2022 part 1/2.
The switch is read with env_on now, as host/nn_vi.c reads the same name:
unset, empty, 0, false, off and no are off.
Assisted-by: Claude Fable 5.2
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This was referenced Nov 12, 2026
DevL0rd
added a commit
to DevL0rd/Hookshot
that referenced
this pull request
Nov 12, 2026
NotRexed
pushed a commit
that referenced
this pull request
Nov 12, 2026
…2613) `tests/test_vram_budget_row.py` fails on Windows with the llvm-mingw setup that CONTRIBUTING.md describes, because there `g++` is clang: ``` FAIL: the fixture does not build: fixture.cpp:96:15: error: unused variable 'kVramMinimumOnly' [-Werror,-Wunused-const-variable] ``` **What changed:** the fixture takes the menu's budget tables, and `kVramMinimumOnly` comes with them. The row's step that reads that constant (`vk_menu.cpp:1906`) does not. The fixture now marks it used with `(void)kVramMinimumOnly;`. GCC, which CI uses, does not warn, so CI was green while a local Windows run was red. **Tested:** Windows 11, llvm-mingw 20261104 ucrt-x86_64 (clang 23.1.5): - `python -I -B tests/test_vram_budget_row.py --cxx g++` failed before the change and passes after it. - Full `tests/run_all.py`: 438 passed, 3 failed before the change. The other two failures are not this one: - `test_env_on`: `HOS_FPS_LOG`, already fixed in #2575; - `nx2/test_nup_paths`: symlinks need Developer Mode on Windows. **Not tested:** GCC/Linux. The change only adds a reference to a constant, so it does not change what the test checks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 12, 2026
hckrman101
pushed a commit
that referenced
this pull request
Nov 12, 2026
**What this is for:** keep the minimap restored after saving and restarting following a Debug teleport (#2588). **What is measured:** Linux / RTX 4090, main `09aac439` versus `4b2388fc`: the live flag, the game's own saved flag, and inspected pictures before teleport, after teleport, and after a fresh process restart. **Not measured:** frame rates, Windows/AMD/Steam Deck, the reporter's save, or the exact Lake Hylia route that originally set the flag. ## Problem This completes @jarynclouatre's approach in #2402, based on @Zarigis's flag diagnosis in #2217. The existing accepted-teleport guard is right; the remaining problem is how the flag is written. Two separate results on main: 1. **The live repair works.** An accepted teleport clears `MapIconDeactivateByAreaActor`, and the minimap appears again. The log says `the area left had hidden the minimap ... shown again`. 2. **That repair is not saved.** System → Save writes a fresh save with the flag still set. Loading that save in a new process hides the minimap again. The existing helper writes only the Bool entry's live byte. The engine also keeps a saved copy and update bookkeeping. The normal Bool setter at `0x806a2b20` takes `(manager, value, hash)` and queues the change through that machinery. The queue consumer updates the live and saved values, or marks a pending saved update; a direct byte write bypasses it. Sources: `host/hos_main.c` on main, the local binding trace, and the save/reload comparison below; no generated game code is included. ## What changed - `host/hos_main.c`: after the same accepted-request check, call the engine's Bool setter for the existing minimap-suppression hash instead of the live-only helper. Preserve the caller's registers and log `restore queued`, matching the deferred operation. - `tests/test_teleport_minimap_flag.py`: keep the existing request/refusal coverage and give the fixture separate live and saved state, an engine-update step and a reload. Also check setter arguments, register restoration, and an unavailable setter binding. `--source` allows the same test to exercise main's implementation. - `readme.md`, the debug tools paragraph: how a save that already lost its minimap is mended. The destination/event path, interface settings, and refusal behavior are unchanged. A destination can still hide its own minimap. The age switch's immediate live update remains unchanged because the destination choice immediately reads the new age; the minimap repair in its shared teleport path uses the engine setter. Sources: `host/hos_main.c:1001-1015,1030-1076` at `4b2388fc`. ## How to recover an affected save with this change Load it, use **Debug → Teleport → Hyrule Field**, wait for gameplay to resume, then **System → Save**. The native save and fresh restart in the fixed runs below retain the minimap. Older save slots are not rewritten; repeat the recovery if loading another affected slot. ## How it was tested The runs below were made on `4b2388fc` over main `09aac439`. The branch has since been rebased onto main `de866017` as `03a63ea3` (with the recovery note moved from the developer notes to the readme's debug tools paragraph); the regression test was run again there with GCC and Clang (passes) and against main's `host/hos_main.c` (fails with both). Arch Linux, kernel 7.2.15-1-omarchy, RTX 4090, NVIDIA 615.78.08; headless, 2048×1152 output, 1280×720 captures, frame generation off. Main `09aac43907a90ae62c8ccd98c77b471d2b94eac2`; fix `4b2388fc77f1f5eb078531ab51171294a0007b28`. Runs on 11 November 2026, local UTC−06:00. Prepared caches were reused and shader precompilation disabled for these runs. Sources: both build logs and the runs' `hookshot_log.txt` / recorded environments. ### Game-free regression Both GCC 16.2.1 and Clang 23.1.3: | Source passed to the same updated fixture | Result | |---|---| | Main `09aac439` | **Fails:** `a native save after teleport kept the minimap hidden`; `the minimap disappeared again after a save and reload` | | Fix `4b2388fc` | **Passes:** `accepted teleport repairs live and saved minimap state; refusals and missing bindings leave it alone` | The fixture is Linux-only, as before; it needs no game files. Source: `tests/test_teleport_minimap_flag.py` and the captured GCC/Clang regression output. Whole `tests/run_all.py`, once with `CXX=g++`: **453 passed, 3 failed, 20 skipped**, including a passing minimap test. The failures are outside the changed implementation: - `nx2/test_depth_sample_types.py` and `nx2/test_unsigned_image_types.py`: their C++20 builds reject the unchanged assertion header's static variable inside a constexpr function. - `tests/test_env_on.py`: unchanged `HOS_FPS_LOG` in `host/oot_high_fps.c:16` still uses set-means-on behavior. (Main's failure at that base; since fixed by #2575.) Those failures were retained, not patched or rerun here; no new full-main baseline was run. Source: the full-suite output, ending with `453 passed, 3 failed, 20 skipped` and `No coredumps found.` ### In the game A normal main run from Hyrule Field through the Lake Hylia debug destination and then Lost Woods kept its minimap. To test an already affected save reproducibly, a copy of a local test save had only the previously reported suppression flag set. **This is controlled reproduction of the persistent-flag case, not reproduction of the original area trigger.** Sources: inspected normal-route captures and the asserted one-byte save preparation. | Run / start | Build | Observed result | Native saved flag | |---|---|---|---| | Normal route / 21:00:43 | main | Minimap visible before and after both debug teleports | 0 | | Affected-state recovery / 21:17:22 | main | Hidden flag 1 → live 0; minimap returns; System → Save still writes hidden | **1** | | Reload and repair / 21:49:25 | fix | Main's native save reloads hidden; accepted teleport changes live and saved values to 0; native Save | **0** | | Fresh restart / 22:01:55 | fix | Continue from the repaired native save; minimap remains visible, live/saved values both 0 | **0** | Sources: per-run logs, read-only flag samples, native saved-file inspection, and the corresponding inspected pictures. All four runs ended normally, with no fatal/crash/device-lost, Xid/cache-operation or NVRM lines and no core dumps. They were shared runs: **no performance claim**. Start → end load averages (1/5/15 minute) were respectively `31.34/14.47/12.45 → 10.10/13.48/12.68`, `10.15/9.55/10.30 → 7.76/9.05/10.09`, `66.19/28.82/22.11 → 6.80/18.64/19.76`, and `7.62/9.02/14.16 → 7.62/8.84/13.88` (recorded run summaries). The fixed run's log contains: ```text cheats: teleport: the area left had hidden the minimap (MapIconDeactivateByAreaActor): restore queued ``` One scripting mistake is retained in the evidence: the first-use map screen was still open, so one teleport request timed out without being taken. After inspecting and closing the menu, the same process completed the repair and native save. Opening and closing the actual full map left the stuck flag at 1; it was not a recovery shortcut in this test. Sources: the request replies, map-page captures, and read-only flag samples. ### Pictures and captions Minimap before teleport, after arrival at Lake Hylia, and after teleporting to Lost Woods.  Main's temporary restoration; loss after loading its native save; fixed restoration; minimap still present after native Save and a fresh restart.  Every frame in both labelled comparisons was inspected. ## Not tested The reporter's exact save/route, a naturally entered map-hiding area, the v2.1.0 binary itself, a 90-fps cap, Windows, AMD, Steam Deck, physical controllers, and Switch age as a runtime action. The save was synthetic only at the initial stuck-flag setup; all subsequent saves in the comparison were made by the game. ## Risks The binding is for the already supported game build, like the adjacent teleport bindings. The change is queued rather than synchronously forcing the live byte; the game owns its application and saved-copy bookkeeping. A missing binding leaves suppression unchanged instead of pretending to repair it. No save file is edited by Hookshot's host code. ## Overlaps Refs #2402 and #2217. The overlap check reported clean merges for its code-file overlaps. I read #2371's host hunk (destination-picture capture ticking) and #2566's host/docs hunks (output/aspect handling); neither modifies this repair block. Other PRs sharing these large files were not functionally reviewed here. Refs #2588 (closed as fixed by #2402; this completes that fix for the saved flag. The exact route that first set the flag remains unconfirmed). Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> --------- Co-authored-by: GPT-6 Astra <noreply@openai.com> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
hckrman101
pushed a commit
that referenced
this pull request
Nov 12, 2026
**What this is for:** stop the texture budget from charging BC1/BC4 images about twice their compressed size, and make the estimate taken before an image is created use whole compressed blocks too, following [hckrman101's review of #2288](#2288 (review)). **What is measured:** game-free runs of the production accounting with GCC and Clang: 4,845 cases plus the original 182 helper cases pass, and 3,040 of them fail on main. One pair of game runs (main and the change, Kokiri Forest, RTX 4090) shows no fault and no difference in behaviour, because the budget is never reached there. **Not measured:** eviction in play, any frame-rate effect, AMD, Windows, or a long session on a card with little VRAM. ## Problem The review's follow-up on BC was: > `texel` is 1 byte for every BC format, but BC1/BC4 are 8 bytes per 4x4 block (0.5 B/texel), so they count 2x until the `x.pad` floor (which only raises, never lowers). `create()` summed the mip chain's texels and charged every BC format one byte a texel. BC1 and BC4 have eight-byte blocks, the other BC formats sixteen-byte blocks, and every level needs its extent rounded up to whole 4x4 blocks, the 1x1 tail included. The allocator floor covers an underestimate but cannot undo an overestimate, and the inflated count goes into `total_bytes`, which the eviction sweep compares with the budget. The review also pointed at the separate estimate taken before creation (`bytes_at`): an ASTC image the GPU samples itself was still treated as at least one byte a texel, four times the size of 8x8 blocks and nine times that of 12x12 blocks. That estimate decides whether the top levels of a very large image are dropped and whether an array is made sparse. Last, the ASTC test mixed decimal MB with binary MiB. Its example is 3,410.0 MiB by the old texel count and 53.8 MiB of blocks; comment and output now agree. ## What changed - `gpu/vk/vk_texture.cpp`: `texture_level_units` is used for native BC and native ASTC in both the estimate before creation and the final count. The helper already rounds each level to whole blocks; no allocation and no further pass over the mip chain is added. - ASTC that is decoded and re-encoded as BC3 counts its own 4x4, 16-byte output blocks, not the source ASTC's block size. ASTC kept as RGBA8 still counts four-byte texels (the image's format decides, not the source's). - Unchanged: the allocator-size floor, a sparse image's count by what is bound, the layer and sample factors, uncompressed and widened images, and all of the eviction policy. - `tests/test_native_astc_texture_count.py`: the helper's cases stay, and the old checks of the source's text are replaced by compiling and running the production estimate and final count: all 16 BC formats, all 14 ASTC block sizes, partial blocks, 1x1 tails, skipped top levels, arrays, volumes and samples, decoded and re-encoded ASTC, widening, sparse images and the allocator floor. - `tests/test_low_vram_texture_mips.py`: its cut-out fixture now brings the real format predicates and `texture_level_units` along, which the changed estimate needs to compile. Every configuration and assertion is kept. - `docs/development.md`, the `RX_VK_VRAM_MB` row: what a texture counts, that the log prints MiB as `MB`, and the default as the code computes it (70% of what the driver allows the process, 45% where that is 8.5 GB or less, at most 6 GB; on an integrated GPU less, by the system's RAM: 1 GB with 8 GB of RAM, 2 with 12, 2.5 with 16, 3.5 with 24, 4 with 32, up to 6 above that). The count changes; the image data, formats, uploads and compression do not. A smaller count is not less VRAM allocated. ## How to try it No new switch. Compare the log's `textures N MB (budget B MB)` and `textures retired ...: budget N` lines in one scene with the same budget settings. Game-free: ```sh python -I -B tests/test_native_astc_texture_count.py --cxx g++ python -I -B tests/test_native_astc_texture_count.py --cxx clang++ python -I -B tests/test_native_astc_texture_count.py --cxx g++ --source /path/to/main/gpu/vk/vk_texture.cpp ``` ## How it was tested Arch Linux, Core i9-13900K, GCC 16.2.1 and Clang 23.1.3. Branch `883c3072`, one commit on main `0ec31ff6` (rebased from `1c08cde1` on `8568959d`: at that base main's own `vk_texture.cpp` did not compile on Linux, `std::min` and `std::max` with mixed integer types in the integrated-GPU budget, which is why this pull request's first AppImage job failed; main has since fixed it). On this head, both compilers: ```text texture accounting: 4845 cases pass; 16 BC formats, 14 ASTC block sizes, decoded/re-encoded ASTC, uncompressed/widened, layers/depth/samples, sparse and allocation floor native ASTC texture count: 182 cases pass (the 1365 x 1365 x 90 ASTC 8x8 array: 3410.0 MiB counted before, 53.8 MiB of blocks) PASS 24 mip-policy configurations ``` The negative checks compile and run rather than fail on a missing string: | Source the test runs against | GCC | Clang | |---|---:|---:| | This branch | 4,845 pass | 4,845 pass | | main (as of `de866017`) | 3,040 fail | 3,040 fail | | The branch with only the old estimate put back | 2,508 fail | 2,508 fail | | The branch with only the old final count put back | 1,140 fail | 1,140 fail | The last two rows were run on the same code change one rebase earlier (a comment and the documentation row changed since); they show that neither half of the fix hides a missing check of the other. The whole `tests/run_all.py` suite was run once on the earlier base (`fce53453`): 447 passed, 3 failed, 20 skipped. The three: the mip-policy fixture, which did not compile without the helpers and is repaired here (above); `nx2/test_depth_sample_types.py`, whose Clang process was killed by SIGKILL (the cause was not established; the machine was heavily loaded) and which passes when run alone; and `test_env_on.py`, main's `HOS_FPS_LOG` failure of that day, since fixed by #2575. **In the game** (RTX 4090, NVIDIA 615.78.08, headless, a save in Kokiri Forest with camera turns, 170 s each, one after the other): main `fce53453` and a build of the same base with this change's `vk_texture.cpp` (the code is the same as on this branch; one comment differs). | Build | Run | Fatal, crash, device lost, driver lines | Textures counted at their highest / budget | Textures retired for the budget | |---|---|---|---|---| | main | 170.6 s, ended by the timer | 0 | 4022 / 6144 MiB | 0 | | this change | 170.6 s, ended by the timer | 0 | 3978 / 6144 MiB | 0 | The pictures of both runs show the forest and the HUD as they should be. Neither run reached its budget, so this pair shows that the change does no harm in an ordinary scene on a large card. It does not show an eviction that comes later, and it is not a timing. ## What to expect in play Reasoned, not measured: with BC1/BC4 no longer counted double, the fixed texture budget is reached later, not earlier. The allocator floor stays, and the rules that react to the driver's own figures still use what the driver reports, so this does not postpone every eviction. A smaller count can also put a BC texture the GPU drew under one of the existing keep-size thresholds; those rules are unchanged. Tiny and partial mip levels that the old formula undercounted now count a whole block, which the allocator floor usually covered already. ## Not tested - Windows, AMD, the Steam Deck, or an Intel run with natively sampled ASTC for this follow-up. - Long play, or a card with 4 to 8 GB where the budget is reached. - A real oversized compressed array that crosses the sparse or the drop-top-levels threshold; the estimate itself is covered by the test. - Frame rate, frame times or stutter. ## Risks Unlike #2288, this changes the count on NVIDIA and AMD too. An undercount would put eviction off too long; the per-format arithmetic in the test, the tiny-level cases and the allocator floor guard against that. Correcting the estimate before creation can keep a large compressed image whole where the old estimate dropped its top levels or made it sparse. That is intended, but how it behaves at those thresholds needs other workloads and hardware. ## Overlaps - #2338 and #2368 add rows next to the `RX_VK_VRAM_MB` row in `docs/development.md`: keep their rows and this description. - #2557 (draft) changes the set-up of `tests/test_low_vram_texture_mips.py` beside this change's two imports: keep both. - #2561 moves `docs/development.md` in its rewrite of the docs: the row's new wording should move with it. - In `vk_texture.cpp` the overlap scan of 2026-11-11 found no conflict with an open pull request. No separate issue: this is the follow-up the review of #2288 asked for. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> Co-authored-by: GPT-6 Astra <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
host/oot_high_fps.creads its log switch withenv_on("HOS_FPS_LOG")instead ofgetenv("HOS_FPS_LOG") != NULL. One line, one include, and the comment that describes the switch.Why
The old form is the set-means-on read that fix: switches that behave as players expect, and dead ones removed #2042 retired: with it
HOS_FPS_LOG=0turned this log on, whilehost/nn_vi.creads the same name withenv_on.tests/test_env_on.pylooks for such reads, so it fails onmainsince fix(fps): keep sound and moving platforms working above a 60 fps cap #2470 brought the line (fix(fps): the sound clock keeps its remainder around 60 fps, and cap 60 runs every phase #2552 kept it). In run 43120809833 onmain(eed4d912) that is the failure ofPython source checks (ubuntu-24.04, part 2/2)and one of the two inPython source checks (windows-2022, part 1/2):How it was tested
tests/test_env_on.pyonmain9df055f2: fails, naminghost/oot_high_fps.c:16. With this change: passes.tests/run_all.py --strictwithtest_env_on,test_menu_env_onandtest_high_fps_fixes: 3 passed (Arch Linux).host/oot_high_fps.ccompiles withgccandclang(-fsyntax-only -Wall -Wextra, with the generated headers of a local build).Not in this change
The same run has two more failures on Windows that this does not touch:
tests/test_psychov35.py(part 1/2) andtests/test_pcache_header_read.py(part 2/2).No issue was opened for this: it is one line and the failing test is the report.
Assisted-by: Claude Fable 5.2