MagicBeanDev/Hookshot
Archived
Watch
6
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

feat(control): stream OoT state through the control bridge #2292

Open
mooki000 wants to merge 5 commits from mooki000/play-harness into main
pull from: mooki000/play-harness
merge into: MagicBeanDev:main
Conversation 11 Commits 5 Files changed 12 +1511 -70
mooki000 commented 2026-11-11 13:46:21 +00:00 (Migrated from github.com)

What this is for: a script steering Link needs his position, camera direction and current game state before its next input. This moves the proposed state-file stream into the control bridge already merged in #926, so tools use one protocol rather than a second output channel. Work is tracked here; #2335 was closed to keep it in this pull request. What is measured: on Arch Linux/RTX 4090, gcc and clang pass the socket sampler and bridge checks; each rejects 38 compiled behavioral regressions. A copied-save comparison found nine standing-state fields identical over file and socket, and a watched Bool transition also appeared in changed. Not measured: Windows, AMD/Intel GPUs, rendering parity, frame-rate cost, long play sessions or real controllers.

Problem

  • host/control.c already offers status and subscriptions, and docs/control.md describes it as the connection for tools.
  • The first version of this PR wrote a separate file. It duplicated map/event/loading/age reporting and made consumers combine two channels.
  • Reading the remaining fields directly from the socket thread would be the wrong move: the actor and manager pointers belong to the game's update. The reads must stay there, with copied results crossing to the bridge.

What changed

Files Change
host/oot_state_tap.c Retains the bounded, read-only guest readers in Link's update. Publishes a copied core snapshot and per-connection samples instead of opening/writing/flushing a file. Each subscription has its own watch list and changed-flag baseline.
host/control.h, host/control.c HosControlGame callbacks copy the latest snapshot, replace/cancel a watch and consume a pending sample. status.game carries the core fields; the explicitly requested oot event carries samples with optional flags/changes. Disconnect, replacement and unsubscribe clear the old watch.
host/hos_main.c Keeps sampling under the existing debug-tools lock. Removes the superseded three-flag snapshot callback; the existing last-known-map cache remains.
scripts/hookshot_ctl.py Plain and JSON status show the snapshot. watch --events oot --every N --flags NAME,0xHASH --changes exposes the stream.
docs/control.md, docs/development.md Document the fields, units, cadence, nulls, stale snapshots, limits and migration to the bridge. The four state-file switches are removed.
Tests Carry the old sampler assertions into a bounded fake guest connected to the real socket/pipe bridge; preserve the bridge/client tests and add subscription lifecycle, payload-boundary and focused regression checks.
docs/tested-configurations.md Records the completed comparison, the interrupted first attempt, and the visual limitation rather than claiming rendering parity.

The new core fields are t, sample frame, map, pos, facing, camera, life, max_life, rupees, prompt and music, alongside the bridge's event, loading and age. Unknown/unreadable values are null. Position is in metres, yaw in degrees, life in quarter hearts. The top-level map remains the last known map; game.map is the sampled value.

flags accepts up to 64 names or explicit hashes. Enum values retain the #<hash> representation. changes:true scans Bool/Int/Enum/UInt values: the first sample establishes a baseline, at most 32 changes are returned, and more counts the remainder. Both lookup and scanning reject tables above 2^22 slots; d847c0cf's cap is retained. A pending sample is not overwritten before the bridge takes it.

Protocol 1 is retained for these additive fields and the opt-in event. Existing draw-count state events and the default present,state subscription are unchanged. Sampling is off with HOS_CONTROL unset: one early return, no guest reads or allocation. No arbitrary-memory opt-in is needed for these named, read-only values.

How to try it

Start the game with HOS_CONTROL as described in docs/control.md, then:

python -I scripts/hookshot_ctl.py --json status
python -I scripts/hookshot_ctl.py --json watch --events oot --every 6 \
    --flags PlayerStatus.Life,0x31ab5580 --changes

Equivalent subscription:

{"id":11,"cmd":"subscribe","events":["oot"],"every":6,"flags":["PlayerStatus.Life","0x31ab5580"],"changes":true}

Events have event:"oot", the subscription's sub, and a nested game snapshot. Use game.frame, not the envelope's delivery frame, to date it. Samples stop wherever Link is not updated, including the title and pauses; status retains the last snapshot. Without a fresh sample, do not assume Link is still in control. The default cadence stays 60 presents; a steering client explicitly requests 6 here.

How it was tested

Environment: 2026-11-11 UTC; Arch Linux, kernel 7.2.15-1-omarchy; Core i9-13900K; RTX 4090, NVIDIA 615.78.08; gcc 16.2.1 and clang 23.1.3. Source base a5b955fb. File binary d847c0cf; socket binary a57f35fe. Later commits d06e4956 and 045fc13d change only the test fixture and evidence documentation, not runtime code. Both binaries used the same matching lift and cached hot objects through the build gate.

Game-free

  • test_oot_state_tap.py with gcc and clang, warnings as errors, through the no-core wrapper:

    PASS: OoT socket sampler, preserved hash/constants/lock wiring, both table caps and no file switches

    Coverage retains the old values, typed name/hash lookup and collision, MurmurHash3 reference and main constants, both table limits, escaped strings, nulls and hook-lock wiring. It adds independent subscriptions, 6/12/default-60 cadence, pending samples, replacement/unsubscribe/disconnect, moved/resized tables, 32 changes plus overflow, and the largest escaped watch payload. The resize case changes a key that stays in its slot, so rehashing alone cannot make a broken reset pass.

  • 38 compiled behavioral mutations fail with each compiler: missing fields, cadence, independent baselines, null/range and finite-value guards, both caps, hashing/collisions, change accounting, lifecycle, retained samples, buffer size and argument validation. Two additional source negatives per compiler check missing hook wiring and the old checkout: 80 expected negative failures total, not 80 compiled cases.

  • test_control.py: 29 PASS groups with gcc and 29 with clang, including the existing transport/security/input checks.

  • test_hookshot_ctl.py: 14 tests, OK. The same tests against the old client fail in the new status/watch checks (2 failures, 1 error).

  • Whole suite through hs-idle.sh, using the dependency Python:

    437 passed, 1 failed, 20 skipped in 235 s (linux, 32 at once)

    The failure is tests/test_bake_mimo.py:223: AssertionError: the unreadable .zs was not reported. That test and nx2/bake_mimo.py are byte-identical to a5b955fb; this PR does not change either. The base was not rerun, so this is an unchanged-source failure, not a separately reproduced baseline result. The sampler, bridge and client tests pass in the suite. The no-core check reported no coredumps. Rerun on its own afterwards, test_bake_mimo.py passes: twice on this head (the g++ and the clang++ run of the four checks) and once on the base's tree, so it failed once in the 32-at-once suite run under load and not since.

In the game

Each game loaded a fresh copy of the same harness save. No directional input was sent. The completed pair used one ordinary 197.8 s GPU hold, headless, 1920x1080 output with 1280x720 captures and RX_VK_PRECOMPILE=0. The machine was shared; the wall times below are run lengths, not performance measurements.

Run Build Wall time Load average before → after (1 minute) Result
First attempt, file only d847c0cf 121.1 s 333.81 → 222.50 State captured and game exited cleanly. My analysis parser expected the version at column zero rather than after [hos host], so it stopped before starting the socket game. Kept, not counted as a comparison.
Completed pair, file d847c0cf 69.4 s 84.04 → 131.22 Reference standing snapshot; Link visible in the inspected capture.
Completed pair, socket a57f35fe 128.3 s 131.22 → 148.89 All nine compared fields equal; watched Bool 0→1, with the same hash/value in changed.

The comparison script asserted exact equality of map, position, facing, camera, life, maximum life, rupees, prompt and music, before the flag-changing debug action in the throwaway run. It then checked the watched name and its independently computed hash in the socket event stream:

PASS: 9 standing fields identical; watched Bool and changed hash proven; hold 197.8s

All three games exited 0. Each had 0 fatal, crash, device-lost, kernel-driver-fault and NVRM lines.

Visual limit: I inspected the captures. The file run shows Link at the saved position. The socket run's last capture shows the hearts and controls on black, not Link; its log still reports shader jobs queued and draws left out as not ready. Shader caches were not matched or warmed for an image comparison. The run proves the state values and flag reporting, not rendering parity or the cause of the black picture.

Not tested

  • Windows/named-pipe execution here, AMD, Intel/Steam Deck, real controllers, and long sessions.
  • In-game pause/resume and map-transition sampling; game-free update gaps and stale snapshots are covered.
  • Rendering parity, frame-rate or sampling-overhead claims. These were shared-load, non-exclusive runs with unmatched shader-cache state.
  • Later upstream runtime changes: the measured pair is deliberately pinned to a5b955fb. main has moved on since (ef81aee7 when this was pushed); the branch merges with it without a conflict, and nothing was rerun on the merge.

Risks

  • These are samples from Link's update, not an atomic snapshot of every game thread. The debug lock serializes Link's workers and the debug tools; range checks and nulls cover individual unreadable values. The bridge never follows guest pointers on its own thread.
  • changes:true scans four tables at the requested cadence. The 32-entry output cap does not make that scan free; it stays opt-in. Intermediate values between samples are not a history of every write.
  • A retained status.game through a pause/load is not proof of current control. Clients must use fresh sample frames and known-false event/loading values before steering.

Overlaps

The overlap scan found 43 open PRs touching these files; the actual conflicts were read:

  • #2414: only simultaneous row insertion at the top of docs/tested-configurations.md. Keep both its historical compiler-worker row and these run rows; no runtime hunk conflicts.
  • #2561: modify/delete conflicts for docs/control.md and docs/development.md. Its replacement control documentation is docs/dev/automation.md. Move the new protocol text, development reference and their source-reading test checks with that reorganization; do not drop their coverage. The host/client changes merge automatically.
  • The same-code-file PRs, including #1025, #1543 and #2048, merge cleanly in the scan. That is a merge result, not joint runtime testing.

The local pre-push checks pass (commit identity, trailers, no game file, a test changed, the title). The range-diff against d847c0cf keeps its two existing commits and adds the bridge migration, strengthened fixture and run evidence.

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]

**What this is for:** a script steering Link needs his position, camera direction and current game state before its next input. This moves the proposed state-file stream into the control bridge already merged in #926, so tools use one protocol rather than a second output channel. Work is tracked here; #2335 was closed to keep it in this pull request. **What is measured:** on Arch Linux/RTX 4090, gcc and clang pass the socket sampler and bridge checks; each rejects 38 compiled behavioral regressions. A copied-save comparison found nine standing-state fields identical over file and socket, and a watched Bool transition also appeared in `changed`. **Not measured:** Windows, AMD/Intel GPUs, rendering parity, frame-rate cost, long play sessions or real controllers. ## Problem - `host/control.c` already offers `status` and subscriptions, and `docs/control.md` describes it as the connection for tools. - The first version of this PR wrote a separate file. It duplicated map/event/loading/age reporting and made consumers combine two channels. - Reading the remaining fields directly from the socket thread would be the wrong move: the actor and manager pointers belong to the game's update. The reads must stay there, with copied results crossing to the bridge. ## What changed | Files | Change | |---|---| | `host/oot_state_tap.c` | Retains the bounded, read-only guest readers in Link's update. Publishes a copied core snapshot and per-connection samples instead of opening/writing/flushing a file. Each subscription has its own watch list and changed-flag baseline. | | `host/control.h`, `host/control.c` | `HosControlGame` callbacks copy the latest snapshot, replace/cancel a watch and consume a pending sample. `status.game` carries the core fields; the explicitly requested `oot` event carries samples with optional flags/changes. Disconnect, replacement and unsubscribe clear the old watch. | | `host/hos_main.c` | Keeps sampling under the existing debug-tools lock. Removes the superseded three-flag snapshot callback; the existing last-known-map cache remains. | | `scripts/hookshot_ctl.py` | Plain and JSON status show the snapshot. `watch --events oot --every N --flags NAME,0xHASH --changes` exposes the stream. | | `docs/control.md`, `docs/development.md` | Document the fields, units, cadence, nulls, stale snapshots, limits and migration to the bridge. The four state-file switches are removed. | | Tests | Carry the old sampler assertions into a bounded fake guest connected to the real socket/pipe bridge; preserve the bridge/client tests and add subscription lifecycle, payload-boundary and focused regression checks. | | `docs/tested-configurations.md` | Records the completed comparison, the interrupted first attempt, and the visual limitation rather than claiming rendering parity. | The new core fields are `t`, sample `frame`, `map`, `pos`, `facing`, `camera`, `life`, `max_life`, `rupees`, `prompt` and `music`, alongside the bridge's `event`, `loading` and `age`. Unknown/unreadable values are `null`. Position is in metres, yaw in degrees, life in quarter hearts. The top-level `map` remains the last known map; `game.map` is the sampled value. `flags` accepts up to 64 names or explicit hashes. Enum values retain the `#<hash>` representation. `changes:true` scans Bool/Int/Enum/UInt values: the first sample establishes a baseline, at most 32 changes are returned, and `more` counts the remainder. Both lookup and scanning reject tables above 2^22 slots; d847c0cf's cap is retained. A pending sample is not overwritten before the bridge takes it. Protocol **1** is retained for these additive fields and the opt-in event. Existing draw-count `state` events and the default `present,state` subscription are unchanged. Sampling is off with `HOS_CONTROL` unset: one early return, no guest reads or allocation. No arbitrary-memory opt-in is needed for these named, read-only values. ## How to try it Start the game with `HOS_CONTROL` as described in `docs/control.md`, then: ```sh python -I scripts/hookshot_ctl.py --json status python -I scripts/hookshot_ctl.py --json watch --events oot --every 6 \ --flags PlayerStatus.Life,0x31ab5580 --changes ``` Equivalent subscription: ```json {"id":11,"cmd":"subscribe","events":["oot"],"every":6,"flags":["PlayerStatus.Life","0x31ab5580"],"changes":true} ``` Events have `event:"oot"`, the subscription's `sub`, and a nested `game` snapshot. Use **`game.frame`**, not the envelope's delivery frame, to date it. Samples stop wherever Link is not updated, including the title and pauses; `status` retains the last snapshot. Without a fresh sample, do not assume Link is still in control. The default cadence stays 60 presents; a steering client explicitly requests 6 here. ## How it was tested Environment: 2026-11-11 UTC; Arch Linux, kernel 7.2.15-1-omarchy; Core i9-13900K; RTX 4090, NVIDIA 615.78.08; gcc 16.2.1 and clang 23.1.3. Source base **a5b955fb**. File binary **d847c0cf**; socket binary **a57f35fe**. Later commits d06e4956 and 045fc13d change only the test fixture and evidence documentation, not runtime code. Both binaries used the same matching lift and cached hot objects through the build gate. ### Game-free - `test_oot_state_tap.py` with gcc and clang, warnings as errors, through the no-core wrapper: ```text PASS: OoT socket sampler, preserved hash/constants/lock wiring, both table caps and no file switches ``` Coverage retains the old values, typed name/hash lookup and collision, MurmurHash3 reference and main constants, both table limits, escaped strings, nulls and hook-lock wiring. It adds independent subscriptions, 6/12/default-60 cadence, pending samples, replacement/unsubscribe/disconnect, moved/resized tables, 32 changes plus overflow, and the largest escaped watch payload. The resize case changes a key that stays in its slot, so rehashing alone cannot make a broken reset pass. - **38 compiled behavioral mutations fail with each compiler**: missing fields, cadence, independent baselines, null/range and finite-value guards, both caps, hashing/collisions, change accounting, lifecycle, retained samples, buffer size and argument validation. Two additional source negatives per compiler check missing hook wiring and the old checkout: **80 expected negative failures total**, not 80 compiled cases. - `test_control.py`: **29 PASS groups with gcc and 29 with clang**, including the existing transport/security/input checks. - `test_hookshot_ctl.py`: **14 tests, OK**. The same tests against the old client fail in the new status/watch checks (2 failures, 1 error). - Whole suite through `hs-idle.sh`, using the dependency Python: ```text 437 passed, 1 failed, 20 skipped in 235 s (linux, 32 at once) ``` The failure is `tests/test_bake_mimo.py:223`: `AssertionError: the unreadable .zs was not reported`. That test and `nx2/bake_mimo.py` are byte-identical to a5b955fb; this PR does not change either. The base was not rerun, so this is an unchanged-source failure, not a separately reproduced baseline result. The sampler, bridge and client tests pass in the suite. The no-core check reported no coredumps. Rerun on its own afterwards, `test_bake_mimo.py` passes: twice on this head (the `g++` and the `clang++` run of the four checks) and once on the base's tree, so it failed once in the 32-at-once suite run under load and not since. ### In the game Each game loaded a fresh copy of the same harness save. No directional input was sent. The completed pair used one ordinary **197.8 s** GPU hold, headless, 1920x1080 output with 1280x720 captures and `RX_VK_PRECOMPILE=0`. The machine was shared; the wall times below are run lengths, **not performance measurements**. | Run | Build | Wall time | Load average before → after (1 minute) | Result | |---|---|---:|---|---| | First attempt, file only | d847c0cf | 121.1 s | 333.81 → 222.50 | State captured and game exited cleanly. My analysis parser expected the version at column zero rather than after `[hos host]`, so it stopped before starting the socket game. Kept, not counted as a comparison. | | Completed pair, file | d847c0cf | 69.4 s | 84.04 → 131.22 | Reference standing snapshot; Link visible in the inspected capture. | | Completed pair, socket | a57f35fe | 128.3 s | 131.22 → 148.89 | All nine compared fields equal; watched Bool 0→1, with the same hash/value in `changed`. | The comparison script asserted exact equality of **map, position, facing, camera, life, maximum life, rupees, prompt and music**, before the flag-changing debug action in the throwaway run. It then checked the watched name and its independently computed hash in the socket event stream: ```text PASS: 9 standing fields identical; watched Bool and changed hash proven; hold 197.8s ``` All three games exited 0. Each had 0 fatal, crash, device-lost, kernel-driver-fault and NVRM lines. **Visual limit:** I inspected the captures. The file run shows Link at the saved position. The socket run's last capture shows the hearts and controls on black, not Link; its log still reports shader jobs queued and draws left out as not ready. Shader caches were not matched or warmed for an image comparison. The run proves the state values and flag reporting, **not rendering parity or the cause of the black picture**. ## Not tested - Windows/named-pipe execution here, AMD, Intel/Steam Deck, real controllers, and long sessions. - In-game pause/resume and map-transition sampling; game-free update gaps and stale snapshots are covered. - Rendering parity, frame-rate or sampling-overhead claims. These were shared-load, non-exclusive runs with unmatched shader-cache state. - Later upstream runtime changes: the measured pair is deliberately pinned to a5b955fb. `main` has moved on since (ef81aee7 when this was pushed); the branch merges with it without a conflict, and nothing was rerun on the merge. ## Risks - These are samples from Link's update, not an atomic snapshot of every game thread. The debug lock serializes Link's workers and the debug tools; range checks and nulls cover individual unreadable values. The bridge never follows guest pointers on its own thread. - `changes:true` scans four tables at the requested cadence. The 32-entry output cap does not make that scan free; it stays opt-in. Intermediate values between samples are not a history of every write. - A retained `status.game` through a pause/load is not proof of current control. Clients must use fresh sample frames and known-false event/loading values before steering. ## Overlaps The overlap scan found **43 open PRs** touching these files; the actual conflicts were read: - **#2414:** only simultaneous row insertion at the top of `docs/tested-configurations.md`. Keep both its historical compiler-worker row and these run rows; no runtime hunk conflicts. - **#2561:** modify/delete conflicts for `docs/control.md` and `docs/development.md`. Its replacement control documentation is `docs/dev/automation.md`. Move the new protocol text, development reference and their source-reading test checks with that reorganization; do not drop their coverage. The host/client changes merge automatically. - The same-code-file PRs, including #1025, #1543 and #2048, merge cleanly in the scan. That is a merge result, not joint runtime testing. The local pre-push checks pass (commit identity, trailers, no game file, a test changed, the title). The range-diff against d847c0cf keeps its two existing commits and adds the bridge migration, strengthened fixture and run evidence. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <[email protected]>
mooki000 commented 2026-11-11 17:08:14 +00:00 (Migrated from github.com)

Rebased onto main b29967bf so the workflows run here: they had waited for an approval on the old head, and main's game-free tests are green again since #2380. The patch is unchanged (git range-diff: the commit is equal), and its test passes on the new base.

Rebased onto main `b29967bf` so the workflows run here: they had waited for an approval on the old head, and main's game-free tests are green again since #2380. The patch is unchanged (`git range-diff`: the commit is equal), and its test passes on the new base.
Joey-Magnus (Migrated from github.com) approved these changes 2026-11-11 20:07:51 +00:00
Joey-Magnus (Migrated from github.com) left a comment

Nice, self-contained addition. Some specifics I checked:

  • The file is new (host/oot_state_tap.c). nx2/build.py compiles every host/*.c, so nothing else needs wiring.
  • The hook in hos_oot_lift_player runs under the existing cheats_lock.
  • While HOS_OOT_STATE_FILE is unset, the cost is one getenv on the first call and then an early return.
  • Every guest read goes through hos_guest_range, and nothing in the game is written.
  • The camera path (0x84ca6bc0 → manager +32/+40 → camera +120) matches oot_gameplay_view in host/hos_main.c.
  • The switches are documented in docs/development.md.
  • fopen on a UTF-8 path is fine on Windows because of the exe's UTF-8 manifest; tests/test_utf8_paths.py passes.

tests/test_oot_state_tap.py passes here (gcc, -Werror). The red Windows check is tests/test_native_astc_texture_count.py, which main fixed in #2397. The appimage failure was Launchpad timing out. Neither comes from this PR.

Minor points, none of them blocking:

  • minor host/oot_state_tap.c state_entry: the probe loop runs up to size iterations, where size comes from guest memory. state_put_changes caps it at 1 << 22, but this function doesn't. A torn or garbage read while a save loads could make one line walk billions of slots on a guest worker. Using the same cap here (if (size > (1u << 22)) return 0;) would keep a bad read from stalling the game.
  • question Fields that read the same thing: #926's control socket status (now on main) and #2048's !cheat 998 already report map, event, loading and age. You offered to move the fields into status. A maintainer should decide whether to keep two channels. I'm fine with this file as it is, because a file stream suits HOS_INPUT_LIVE scripts.
  • Not tested on Windows or AMD, as the description says. The code has nothing platform-specific beyond InterlockedCompareExchange64, which host/posix provides.
Nice, self-contained addition. Some specifics I checked: - The file is new (`host/oot_state_tap.c`). `nx2/build.py` compiles every `host/*.c`, so nothing else needs wiring. - The hook in `hos_oot_lift_player` runs under the existing `cheats_lock`. - While `HOS_OOT_STATE_FILE` is unset, the cost is one `getenv` on the first call and then an early return. - Every guest read goes through `hos_guest_range`, and nothing in the game is written. - The camera path (`0x84ca6bc0` → manager +32/+40 → camera +120) matches `oot_gameplay_view` in `host/hos_main.c`. - The switches are documented in `docs/development.md`. - `fopen` on a UTF-8 path is fine on Windows because of the exe's UTF-8 manifest; `tests/test_utf8_paths.py` passes. `tests/test_oot_state_tap.py` passes here (gcc, `-Werror`). The red Windows check is `tests/test_native_astc_texture_count.py`, which main fixed in #2397. The `appimage` failure was Launchpad timing out. Neither comes from this PR. Minor points, none of them blocking: - **minor** `host/oot_state_tap.c` `state_entry`: the probe loop runs up to `size` iterations, where `size` comes from guest memory. `state_put_changes` caps it at `1 << 22`, but this function doesn't. A torn or garbage read while a save loads could make one line walk billions of slots on a guest worker. Using the same cap here (`if (size > (1u << 22)) return 0;`) would keep a bad read from stalling the game. - **question** Fields that read the same thing: #926's control socket `status` (now on main) and #2048's `!cheat 998` already report map, event, loading and age. You offered to move the fields into `status`. A maintainer should decide whether to keep two channels. I'm fine with this file as it is, because a file stream suits `HOS_INPUT_LIVE` scripts. - Not tested on Windows or AMD, as the description says. The code has nothing platform-specific beyond `InterlockedCompareExchange64`, which `host/posix` provides.
hckrman101 commented 2026-11-11 20:32:17 +00:00 (Migrated from github.com)

Hi, can you check if this is integrated with the control bridge?

Hi, can you check if this is integrated with the control bridge?
Joey-Magnus (Migrated from github.com) reviewed 2026-11-11 21:25:16 +00:00
Joey-Magnus (Migrated from github.com) left a comment

The code is clean (opt-in, read-only, every guest read checked, and the test passes).

To answer the earlier question: it isn't integrated with the control bridge. host/control.c already has status (event, loading, age) and subscribe (map, state), and docs/control.md describes the bridge as the one connection for tools. Since you offered, moving these fields into status/subscribe seems better than a second channel.

One fix either way: state_entry (host/oot_state_tap.c:107) walks up to size slots read from guest memory with no upper limit. The sibling function caps it at 1u<<22, and this one should too.

The code is clean (opt-in, read-only, every guest read checked, and the test passes). To answer the earlier question: it isn't integrated with the control bridge. `host/control.c` already has `status` (event, loading, age) and `subscribe` (map, state), and `docs/control.md` describes the bridge as the one connection for tools. Since you offered, moving these fields into `status`/`subscribe` seems better than a second channel. One fix either way: `state_entry` (`host/oot_state_tap.c:107`) walks up to `size` slots read from guest memory with no upper limit. The sibling function caps it at `1u<<22`, and this one should too.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 22:18:54 +00:00
hckrman101 (Migrated from github.com) left a comment

I read the code and the discussion. I agree with Joey-Magnus's review: the tap is opt-in, read-only, every guest read is range-checked, and the test is self-contained and declared (RUN_ALL_ARGS). I have nothing to add on correctness.

Two things are still open:

  • Control bridge (my earlier question). Joey-Magnus confirmed it is not integrated: host/control.c already has status and subscribe, and docs/control.md calls the bridge the one connection for tools. @mooki000 offered in the description to move the fields there, but hasn't answered since. Whether to keep a second channel (a file stream, which does suit HOS_INPUT_LIVE scripts) or fold the fields into status/subscribe is a call for @NotRexed. I'm not approving over that question.
  • state_entry probe cap. The loop in host/oot_state_tap.c still runs up to a size read from guest memory with no upper bound. state_put_changes caps it at 1 << 22; this one should too (Joey-Magnus's point, still unfixed at this head). Small change, no new test needed beyond the existing one.

CI: the Windows source-check failure is test_native_astc_texture_count.py, which main fixed in #2397 (this branch is based just before it), and appimage is a Launchpad timeout. Neither touches this PR. Once the cap is in and the channel question is settled, this is fine to merge.

I read the code and the discussion. I agree with Joey-Magnus's review: the tap is opt-in, read-only, every guest read is range-checked, and the test is self-contained and declared (`RUN_ALL_ARGS`). I have nothing to add on correctness. Two things are still open: - **Control bridge (my earlier question).** Joey-Magnus confirmed it is not integrated: `host/control.c` already has `status` and `subscribe`, and `docs/control.md` calls the bridge the one connection for tools. @mooki000 offered in the description to move the fields there, but hasn't answered since. Whether to keep a second channel (a file stream, which does suit `HOS_INPUT_LIVE` scripts) or fold the fields into `status`/`subscribe` is a call for @NotRexed. I'm not approving over that question. - **`state_entry` probe cap.** The loop in `host/oot_state_tap.c` still runs up to a `size` read from guest memory with no upper bound. `state_put_changes` caps it at `1 << 22`; this one should too (Joey-Magnus's point, still unfixed at this head). Small change, no new test needed beyond the existing one. CI: the Windows source-check failure is `test_native_astc_texture_count.py`, which main fixed in #2397 (this branch is based just before it), and `appimage` is a Launchpad timeout. Neither touches this PR. Once the cap is in and the channel question is settled, this is fine to merge.
mooki000 commented 2026-11-11 22:31:06 +00:00 (Migrated from github.com)

@hckrman101 @Joey-Magnus thanks both.

  • The cap is in (d847c0cf): state_entry returns no entry for a table that reports more than 1 << 22 slots, the same limit as state_put_changes. The test's fixture sets such a size on a table whose first slot matches the flag asked for and expects no entry; it fails without the line, with gcc and clang. Rebased onto a5b955fb, so the Windows job has #2397's fix.
  • Control bridge: not integrated at this head, as Joey-Magnus found. I'll do what I offered: move the fields into status/subscribe and drop the file switches, so the bridge stays the one connection for tools (unless @NotRexed wants the file kept). I'll push that here once a run in the game shows the same values over the socket as the file gives.
@hckrman101 @Joey-Magnus thanks both. - **The cap is in** (`d847c0cf`): `state_entry` returns no entry for a table that reports more than `1 << 22` slots, the same limit as `state_put_changes`. The test's fixture sets such a size on a table whose first slot matches the flag asked for and expects no entry; it fails without the line, with gcc and clang. Rebased onto `a5b955fb`, so the Windows job has #2397's fix. - **Control bridge:** not integrated at this head, as Joey-Magnus found. I'll do what I offered: move the fields into `status`/`subscribe` and drop the file switches, so the bridge stays the one connection for tools (unless @NotRexed wants the file kept). I'll push that here once a run in the game shows the same values over the socket as the file gives.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 22:40:55 +00:00
hckrman101 (Migrated from github.com) left a comment

Re-review at d847c0cf.

Fixed: the state_entry probe cap (Joey-Magnus's point). It now returns no entry for a table that reports more than 1 << 22 slots, the same limit as state_put_changes. I read the new check and the added test: the fixture accepts exactly 1 << 22 and rejects (1 << 22) + 1 even when the first slot matches, so it covers the case. Nothing else changed in the code; the rebase onto current main also picks up #2397's Windows fix.

Still open: the one-channel question. mooki000 confirmed the tap is not integrated with the control bridge and offered to move the fields into status/subscribe and drop the file switches, unless @NotRexed wants the file stream kept (it does suit HOS_INPUT_LIVE scripts). That is @NotRexed's call, so I'm not approving over it. If the fields move, this will need another look, since the file, its docs and the test would change a lot.

CI: at the time of writing most jobs are still running on the new head (builder package, appimage and actionlint are green). I found no problems in the code itself.

Re-review at d847c0cf. **Fixed:** the `state_entry` probe cap (Joey-Magnus's point). It now returns no entry for a table that reports more than `1 << 22` slots, the same limit as `state_put_changes`. I read the new check and the added test: the fixture accepts exactly `1 << 22` and rejects `(1 << 22) + 1` even when the first slot matches, so it covers the case. Nothing else changed in the code; the rebase onto current main also picks up #2397's Windows fix. **Still open:** the one-channel question. mooki000 confirmed the tap is not integrated with the control bridge and offered to move the fields into `status`/`subscribe` and drop the file switches, unless @NotRexed wants the file stream kept (it does suit `HOS_INPUT_LIVE` scripts). That is @NotRexed's call, so I'm not approving over it. If the fields move, this will need another look, since the file, its docs and the test would change a lot. CI: at the time of writing most jobs are still running on the new head (builder package, appimage and actionlint are green). I found no problems in the code itself.
mooki000 commented 2026-11-12 00:07:41 +00:00 (Migrated from github.com)

@hckrman101 @Joey-Magnus the one-channel question, answered in code:

I moved the fields into the bridge in a57f35fe: status.game is the snapshot; subscribe with events:["oot"], every, flags and changes streams it. The four file switches and file writing are gone. Reads stay in Link's locked update; the socket thread receives copies. The lookup cap from d847c0cf remains, and d06e4956 strengthens the resize-baseline check.

On Linux/RTX 4090, the same copied save gave identical map, position, facing, camera, life/max life, rupees, prompt and music over file versus socket. A watched Bool changed 0→1 and the same hash appeared in changed. Both games exited cleanly; no driver faults. gcc/clang checks pass, with 38 behavioral regressions rejected by each.

Windows and AMD are not tested. Nor is rendering parity: the socket capture showed HUD on black with shaders still queued. The full suite was 437 passed, 1 failed, 20 skipped; the failure was test_bake_mimo.py's unreadable-.zs assertion, a test and a script this change does not touch, and rerun on its own it passes here and on the base's tree. Details and all three runs are in the updated description.

@NotRexed if you would rather keep the file stream for HOS_INPUT_LIVE scripts, say so: the first two commits are still the file version as reviewed (d847c0cf), and the move is the three commits above them, so it can come out again or the file can stay beside the bridge.

@hckrman101 @Joey-Magnus the one-channel question, answered in code: I moved the fields into the bridge in a57f35fe: `status.game` is the snapshot; `subscribe` with `events:["oot"]`, `every`, `flags` and `changes` streams it. The four file switches and file writing are gone. Reads stay in Link's locked update; the socket thread receives copies. The lookup cap from d847c0cf remains, and d06e4956 strengthens the resize-baseline check. On Linux/RTX 4090, the same copied save gave identical map, position, facing, camera, life/max life, rupees, prompt and music over file versus socket. A watched Bool changed 0→1 and the same hash appeared in `changed`. Both games exited cleanly; no driver faults. gcc/clang checks pass, with 38 behavioral regressions rejected by each. Windows and AMD are not tested. Nor is rendering parity: the socket capture showed HUD on black with shaders still queued. The full suite was 437 passed, 1 failed, 20 skipped; the failure was `test_bake_mimo.py`'s unreadable-`.zs` assertion, a test and a script this change does not touch, and rerun on its own it passes here and on the base's tree. Details and all three runs are in the updated description. @NotRexed if you would rather keep the file stream for `HOS_INPUT_LIVE` scripts, say so: the first two commits are still the file version as reviewed (d847c0cf), and the move is the three commits above them, so it can come out again or the file can stay beside the bridge.
github-sync commented 2026-11-12 06:23:01 +00:00
Owner

@mooki000 commented on GitHub · 2026-11-12 06:07 UTC · original

Rebased the actual pushed head 045fc13d onto current main 87991bc8 as 723758ff. The only conflict was additive test-evidence rows; both contributors' records remain verbatim, with no runtime change, dropped commit or altered author/message/trailer. All 27 selected checks passed with GCC and Clang on the first rebase; the sampler, bridge and client controls failed against main's missing feature. Its suite was 470 passed / 1 inherited background-input fixture warning / 20 skipped. The final rebase adds only upstream #2671 and changes no PR file blob; the three feature checks, final-main negatives, #2671's offline check and Windows-target syntax all ran again. The final PR gate passes. The suite was not repeated, and no new gameplay, native Windows or performance evidence is claimed. Sources: the #2292 evidence and final-handoff ledger above.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 06:07 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2292#issuecomment-6816118106) Rebased the actual pushed head `045fc13d` onto current main `87991bc8` as `723758ff`. The only conflict was additive test-evidence rows; both contributors' records remain verbatim, with no runtime change, dropped commit or altered author/message/trailer. All 27 selected checks passed with GCC and Clang on the first rebase; the sampler, bridge and client controls failed against main's missing feature. Its suite was 470 passed / 1 inherited background-input fixture warning / 20 skipped. The final rebase adds only upstream #2671 and changes no PR file blob; the three feature checks, final-main negatives, #2671's offline check and Windows-target syntax all ran again. The final PR gate passes. The suite was not repeated, and no new gameplay, native Windows or performance evidence is claimed. Sources: the #2292 evidence and final-handoff ledger above. <!-- github-sync:c:6816118106 -->
github-sync commented 2026-11-12 14:42:14 +00:00
Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:32 UTC · original

Both open points from the earlier round are settled. The state_entry cap is in, and the fields now go through status.game and an oot subscription instead of a second file channel, as hckrman101 and I asked. Guest reads still happen only in Link's update under cheats_lock. Only copied JSON crosses state_lock. Watch names are restricted to [A-Za-z0-9_.] or 0x plus 8 hex digits before they go into JSON. Every path that drops a connection clears its watch. I merged it into current main: test_oot_state_tap, test_control, test_hookshot_ctl, test_utf8_paths and test_env_on pass.

Minor points:

  • host/oot_state_tap.c:351-371: a subscription with changes:true scans up to four GameData tables while holding state_lock on the game's worker, so a status call waits for that scan. Fine for a debug channel.
  • status.game.event, loading and age can now be null. Before, they were always booleans or a string. docs/control.md says so, but scripts that test == false will notice.
  • #2694 calls G.game->oot_state(&event, &loading, &child), which this PR replaces. Whichever lands second needs to adapt. #2561 also moves docs/control.md.
> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:32 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2292#pullrequestreview-5876207768) Both open points from the earlier round are settled. The `state_entry` cap is in, and the fields now go through `status.game` and an `oot` subscription instead of a second file channel, as hckrman101 and I asked. Guest reads still happen only in Link's update under `cheats_lock`. Only copied JSON crosses `state_lock`. Watch names are restricted to `[A-Za-z0-9_.]` or `0x` plus 8 hex digits before they go into JSON. Every path that drops a connection clears its watch. I merged it into current main: `test_oot_state_tap`, `test_control`, `test_hookshot_ctl`, `test_utf8_paths` and `test_env_on` pass. Minor points: - `host/oot_state_tap.c:351-371`: a subscription with `changes:true` scans up to four GameData tables while holding `state_lock` on the game's worker, so a `status` call waits for that scan. Fine for a debug channel. - `status.game.event`, `loading` and `age` can now be `null`. Before, they were always booleans or a string. `docs/control.md` says so, but scripts that test `== false` will notice. - #2694 calls `G.game->oot_state(&event, &loading, &child)`, which this PR replaces. Whichever lands second needs to adapt. #2561 also moves `docs/control.md`. <!-- github-sync:r:5876207768 -->
github-sync commented 2026-11-12 19:01:28 +00:00
Owner

@mooki000 commented on GitHub · 2026-11-12 18:54 UTC · original

Rebased 45310b24 onto 86a9cda2; new head f2970916.
The merged age command keeps main’s typed oot_state; our JSON copy is now oot_sample. No wire-protocol change.
test_control.py, test_hookshot_ctl.py and test_oot_state_tap.py pass with GCC and Clang (6 invocations).
Whole source check: 497 passed, 3 failed, 20 skipped. Each failure also occurs on untouched pinned main; this is not an all-green result.
Failures: the one-variant cache fixture’s missing ZSTD declarations, the missing Skip opening README row, and this isolated environment’s .NET reference pack. The first two are fixed on main since; this head merges cleanly with it.
Built this head from source; one windowless Linux/RTX 4090 run used a copied save.
status showed child then adult; age adult --arrive 60 succeeded. The same watch delivered 2 checked samples before and 2 after the reload (49 events received overall).
The run exited normally with no fatal, crash, device-loss or kernel-fault line. No performance claim.
A post-switch screenshot was captured and inspected; it is not a rendering-parity comparison.
The earlier approval predates this rebase. A full run of the project's checks in my fork, Windows included, ran on this head: the control and client tests pass on both systems, and the only failing tests were the ones failing on main at that time (since fixed there by #2861 and #2883). It also found one real Windows problem, fixed in the last commit: the client now waits for a busy pipe that open() reports as EINVAL.
Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 18:54 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2292#issuecomment-6832956079) Rebased `45310b24` onto `86a9cda2`; new head `f2970916`. The merged age command keeps main’s typed `oot_state`; our JSON copy is now `oot_sample`. No wire-protocol change. `test_control.py`, `test_hookshot_ctl.py` and `test_oot_state_tap.py` pass with GCC and Clang (6 invocations). Whole source check: 497 passed, 3 failed, 20 skipped. Each failure also occurs on untouched pinned main; this is not an all-green result. Failures: the one-variant cache fixture’s missing ZSTD declarations, the missing Skip opening README row, and this isolated environment’s .NET reference pack. The first two are fixed on main since; this head merges cleanly with it. Built this head from source; one windowless Linux/RTX 4090 run used a copied save. `status` showed child then adult; `age adult --arrive 60` succeeded. The same watch delivered 2 checked samples before and 2 after the reload (49 events received overall). The run exited normally with no fatal, crash, device-loss or kernel-fault line. No performance claim. A post-switch screenshot was captured and inspected; it is not a rendering-parity comparison. The earlier approval predates this rebase. A full run of the project's checks in my fork, Windows included, ran on this head: the control and client tests pass on both systems, and the only failing tests were the ones failing on main at that time (since fixed there by #2861 and #2883). It also found one real Windows problem, fixed in the last commit: the client now waits for a busy pipe that `open()` reports as EINVAL. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <[email protected]> <!-- github-sync:c:6832956079 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Joey-Magnus
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2292
No description provided.