docs(movie): disclose that a movie records two ports, under the claim that says four - #421
Conversation
… that says four
Found in a sweep of the project's remaining TODO markers. One of the two
real ones -- `movie_interop.rs`'s "carry P3/P4 once `FrameInput` grows
beyond two ports" -- turns out to describe a live, silent gap in the
NATIVE recording path, not just the `.fm2` importer it sits in.
`FrameInput` models ports 0 and 1. `MovieRecorder::capture` therefore
reads `nes.buttons(0)` and `nes.buttons(1)` and nothing else, while the
core carries four: `EmuCore::latch` calls `set_buttons(2)` and
`set_buttons(3)` whenever the Four Score adapter is active. So recording
a four-player session captures half of what drove it, and replaying that
movie is a different run.
Nothing said so. Worse, the Replay panel's topology grid printed
"Four Score (P1..P4)" -- true of the console, false of the movie -- which
is an over-claim at exactly the moment a user decides to hit Record. The
failure is silent at record time: nothing about a `.rnm` says which ports
it could not hold, and the divergence only appears on playback.
Disclosed at all three levels rather than fixed, and the distinction is
deliberate. Widening `FrameInput` is a `.rnm` format EPOCH change (ADR
0028), not an additive one, so it is a release-boundary decision rather
than a remediation. The `.fm2` importer already takes this exact
position for this exact reason -- it keeps pads 1 and 2, drops 3 and 4,
and preserves the `fourscore` flag "so the caller is not silently
misled". The native path simply never got the same treatment.
* `MovieRecorder::capture` says what it records and what it drops.
* The Replay panel prints the caveat directly UNDER the topology row
that makes the claim, not in a separate place a reader may not
reach.
* `docs/creator-tools.md` records it as a documented limit with the
reason it is one.
`rustynes-core` changes, but comment-only: the diff contains no
non-comment line, so AccuracyCoin and nestest are untouched by
construction and that is checkable from the diff rather than asserted.
`no_std` cross-compile, workspace clippy, `full`, wasm32 and rustdoc all
clean.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Antigravity review (Gemini via Ultra)This PR adds documentation and a frontend UI warning to clarify that movie recording only captures inputs for the first two ports, silently dropping inputs for ports 3 and 4 when the Four Score adapter is active. Blocking issuesNone found. SuggestionsNone. This is a trivial and clear documentation/UI change. NitpicksNone. Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
This PR updates documentation and in-UI messaging to accurately disclose that the native .rnm movie format records only two controller ports (P1/P2), even when the Four Score adapter is enabled, preventing users from being silently misled during four-player recording.
Changes:
- Documented the
.rnmlimitation (P1/P2 only) indocs/creator-tools.md, including why widening is a format-epoch change (ADR 0028) and referencing the existing.fm2importer precedent. - Added a Replay panel warning shown when Four Score is active, placed directly under the “Four Score (P1..P4)” topology row.
- Expanded
MovieRecorder::capturerustdoc to explicitly state that only ports 0/1 are captured and why this is disclosed rather than “fixed” in-place.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/creator-tools.md | Adds explicit user-facing documentation that .rnm movies record only two ports and why this is a format constraint. |
| crates/rustynes-frontend/src/debugger/replay_panel.rs | Surfaces a Four Score recording caveat directly in the Replay UI where the four-port topology is displayed. |
| crates/rustynes-core/src/movie.rs | Documents at the capture point that FrameInput/recording stores only ports 0/1 and links the limitation to .rnm epoching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Found while sweeping the project's remaining
TODOmarkers. There are only two real ones in our source (the third hit is in awasm-bindgenbuild artifact), and one of them describes a live, silent gap in the native recording path rather than the importer it sits in.What happens today
FrameInputmodels ports 0 and 1. SoMovieRecorder::capturereadsnes.buttons(0)andnes.buttons(1)and nothing else — while the core carries four:EmuCore::latchcallsset_buttons(2)andset_buttons(3)whenever the Four Score adapter is active.Recording a four-player session therefore captures half of what drove it, and replaying that movie is a different run.
Nothing said so, and the Replay panel actively said the opposite: its topology grid prints "Four Score (P1..P4)" — true of the console, false of the movie — at exactly the moment a user decides to press Record. The failure is silent at record time; nothing about a
.rnmsays which ports it could not hold, and the divergence only appears on playback.Disclosed, not fixed — and why that is the right call here
Widening
FrameInputis a.rnmformat epoch change (ADR 0028), not an additive one. That is a release-boundary decision, not a remediation-pass one.The precedent is already in the tree and points the same way: the
.fm2importer keeps pads 1 and 2, drops 3 and 4, and preserves thefourscoreflag "so the caller is not silently misled." The native path simply never got the same treatment.Three levels, each where the reader actually is:
MovieRecorder::capturedocs/creator-tools.mdPutting the panel line under the claim rather than elsewhere is the point — a caveat in a place the reader does not reach is the shape of defect this release keeps finding.
Verification
rustynes-corechanges, but comment-only:So AccuracyCoin and nestest are untouched by construction, and that is checkable from the diff rather than asserted.
no_stdcross-compile, workspace clippy,full, wasm32 and rustdoc all clean; core suite 183, frontend 534.Related, and left alone
rustynes verifyalready catches this after the fact — its attestation folds in the video the run produced, not only the input, so a divergent four-player replay fails rather than passing quietly. That is the backstop; this PR is the thing that tells the user before they rely on it.