Repository navigation
Conversation
|
Rebased onto main |
d7a29e8 to
9f915c0
Compare
Joey-Magnus
left a comment
There was a problem hiding this comment.
Nice, self-contained addition. Some specifics I checked:
- The file is new (
host/oot_state_tap.c).nx2/build.pycompiles everyhost/*.c, so nothing else needs wiring. - The hook in
hos_oot_lift_playerruns under the existingcheats_lock. - While
HOS_OOT_STATE_FILEis unset, the cost is onegetenvon 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) matchesoot_gameplay_viewinhost/hos_main.c. - The switches are documented in
docs/development.md. fopenon a UTF-8 path is fine on Windows because of the exe's UTF-8 manifest;tests/test_utf8_paths.pypasses.
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.cstate_entry: the probe loop runs up tosizeiterations, wheresizecomes from guest memory.state_put_changescaps it at1 << 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 998already report map, event, loading and age. You offered to move the fields intostatus. A maintainer should decide whether to keep two channels. I'm fine with this file as it is, because a file stream suitsHOS_INPUT_LIVEscripts. - Not tested on Windows or AMD, as the description says. The code has nothing platform-specific beyond
InterlockedCompareExchange64, whichhost/posixprovides.
|
Hi, can you check if this is integrated with the control bridge? |
Joey-Magnus
left a comment
There was a problem hiding this 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.
hckrman101
left a comment
There was a problem hiding this 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.calready hasstatusandsubscribe, anddocs/control.mdcalls 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 suitHOS_INPUT_LIVEscripts) or fold the fields intostatus/subscribeis a call for @NotRexed. I'm not approving over that question. state_entryprobe cap. The loop inhost/oot_state_tap.cstill runs up to asizeread from guest memory with no upper bound.state_put_changescaps it at1 << 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.
9f915c0 to
d847c0c
Compare
|
@hckrman101 @Joey-Magnus thanks both.
|
…^22 slots state_entry probed up to `size` slots, and size is read from guest memory: a torn or garbage read while a save loads could make one line walk billions of slots on a guest worker. state_put_changes already refuses a table that reports more than 1 << 22 slots; state_entry now does too (review of NotRexed#2292). tests/test_oot_state_tap.py sets such a size on a table whose first slot matches the flag asked for and expects no entry; with the limit itself the flag is still found. It fails without the change, with gcc and clang. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> Assisted-by: Claude Fable 5.2 Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
hckrman101
left a comment
There was a problem hiding this comment.
Re-review at d847c0c.
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.
|
@hckrman101 @Joey-Magnus the one-channel question, answered in code: I moved the fields into the bridge in a57f35f: 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 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 @NotRexed if you would rather keep the file stream for |
…TE_FILE) A script that plays the game through HOS_INPUT_LIVE sees only pictures and the log, so it cannot tell where Link is or whether a cutscene holds him before its next press. With HOS_OOT_STATE_FILE=<file> the update of Link writes a line of JSON every HOS_OOT_STATE_EVERY presents (6): the map, Link's position and facing, the camera's yaw, whether an event plays, loading, age, life, rupees, the A button's action and the music, read from the same singletons, GameDataMgr tables and actor fields the debug tools use. HOS_OOT_STATE_FLAGS adds flags by name and HOS_OOT_STATE_CHANGES=1 every Bool, Int, Enum and UInt flag that changed. Nothing in the game is written; without the switch nothing is read. tests/test_oot_state_tap.py runs the whole file against a guest memory of its own and reads the lines back. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…^22 slots state_entry probed up to `size` slots, and size is read from guest memory: a torn or garbage read while a save loads could make one line walk billions of slots on a guest worker. state_put_changes already refuses a table that reports more than 1 << 22 slots; state_entry now does too (review of NotRexed#2292). tests/test_oot_state_tap.py sets such a size on a table whose first slot matches the flag asked for and expects no entry; with the limit itself the flag is still found. It fails without the change, with gcc and clang. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> Assisted-by: Claude Fable 5.2 Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Move read-only Link snapshots into status.game and an opt-in oot subscription. Sample guest memory only in the existing locked Link update, hand copied snapshots to the bridge, and keep watch lists and change baselines per connection. Drop the state-file switches and preserve the bounded table readers. Carry the file sampler checks over to real socket/pipe fixtures, preserve existing bridge/client coverage, and add focused regression mutations. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Keep a changed flag in the same hash slot across the synthetic resize, so a missing fresh-baseline reset cannot pass merely because every key rehashed. Exercise the bridge existing 199-byte accepted and 200-byte rejected JSON ID boundaries without changing their implementation. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Record the exact field comparison, the watched-flag transition, the interrupted first analysis, and the HUD-only candidate capture without claiming rendering or frame-rate parity. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com>
|
Rebased the actual pushed head |
045fc13 to
723758f
Compare
… EINVAL
On Windows hookshot_ctl.py waited for a busy pipe only when the error
carried winerror 231. Python's open() goes through the C runtime, which
has no errno for ERROR_PIPE_BUSY: it raises "[Errno 22] Invalid
argument" with no winerror, so the wait never happened. With one client
it did not show. tests/test_oot_state_tap.py connects four clients in a
row, and the Windows checks failed there with exactly that error when
the bridge's next pipe instance was not up yet.
The client now waits, up to its timeout, for either form. A pipe that
does not exist (ENOENT) is still reported at once.
Tested: Linux, Python 3.12: three cases in tests/test_hookshot_ctl.py
drive the Windows branch with open() stood in: busy in both forms then
connected, a missing pipe not waited for, a pipe that stays busy fails
at the deadline. With main's client the first case fails ("no game
listens ... Invalid argument"), the other two pass.
tests/test_hookshot_ctl.py, tests/test_oot_state_tap.py and
tests/test_control.py pass as before.
Not tested: on Windows itself (the project's Windows job does that; it
is where the failure was seen).
Assisted-by: Claude Fable 5.2
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Joey-Magnus
left a comment
There was a problem hiding this comment.
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 withchanges:truescans up to four GameData tables while holdingstate_lockon the game's worker, so astatuscall waits for that scan. Fine for a debug channel.status.game.event,loadingandagecan now benull. Before, they were always booleans or a string.docs/control.mdsays so, but scripts that test== falsewill notice.- #2694 calls
G.game->oot_state(&event, &loading, &child), which this PR replaces. Whichever lands second needs to adapt. #2561 also movesdocs/control.md.
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.calready offersstatusand subscriptions, anddocs/control.mddescribes it as the connection for tools.What changed
host/oot_state_tap.chost/control.h,host/control.cHosControlGamecallbacks copy the latest snapshot, replace/cancel a watch and consume a pending sample.status.gamecarries the core fields; the explicitly requestedootevent carries samples with optional flags/changes. Disconnect, replacement and unsubscribe clear the old watch.host/hos_main.cscripts/hookshot_ctl.pywatch --events oot --every N --flags NAME,0xHASH --changesexposes the stream.docs/control.md,docs/development.mddocs/tested-configurations.mdThe new core fields are
t, sampleframe,map,pos,facing,camera,life,max_life,rupees,promptandmusic, alongside the bridge'sevent,loadingandage. Unknown/unreadable values arenull. Position is in metres, yaw in degrees, life in quarter hearts. The top-levelmapremains the last known map;game.mapis the sampled value.flagsaccepts up to 64 names or explicit hashes. Enum values retain the#<hash>representation.changes:truescans Bool/Int/Enum/UInt values: the first sample establishes a baseline, at most 32 changes are returned, andmorecounts the remainder. Both lookup and scanning reject tables above 2^22 slots; d847c0c'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
stateevents and the defaultpresent,statesubscription are unchanged. Sampling is off withHOS_CONTROLunset: 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_CONTROLas described indocs/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 --changesEquivalent subscription:
{"id":11,"cmd":"subscribe","events":["oot"],"every":6,"flags":["PlayerStatus.Life","0x31ab5580"],"changes":true}Events have
event:"oot", the subscription'ssub, and a nestedgamesnapshot. Usegame.frame, not the envelope's delivery frame, to date it. Samples stop wherever Link is not updated, including the title and pauses;statusretains 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 a5b955f. File binary d847c0c; socket binary a57f35f. Later commits d06e495 and 045fc13 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.pywith gcc and clang, warnings as errors, through the no-core wrapper: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:The failure is
tests/test_bake_mimo.py:223:AssertionError: the unreadable .zs was not reported. That test andnx2/bake_mimo.pyare byte-identical to a5b955f; 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.pypasses: twice on this head (theg++and theclang++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.[hos host], so it stopped before starting the socket game. Kept, not counted as a comparison.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:
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
mainhas moved on since (ef81aee when this was pushed); the branch merges with it without a conflict, and nothing was rerun on the merge.Risks
changes:truescans 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.status.gamethrough 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:
docs/tested-configurations.md. Keep both its historical compiler-worker row and these run rows; no runtime hunk conflicts.docs/control.mdanddocs/development.md. Its replacement control documentation isdocs/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 local pre-push checks pass (commit identity, trailers, no game file, a test changed, the title). The range-diff against d847c0c 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 noreply@openai.com