release: v2.3.0 "Datum II" — PPU-accuracy capstone + true multi-viewport tool windows - #347
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughRustyNES v2.3.0 adds native detached debugger windows, narrows emulator lock scope during rendering, optimizes PPU helpers, enforces exact 141/141 AccuracyCoin validation, and updates release, licensing, provenance, and project documentation. Changesv2.3.0 release
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant DetachedManager
participant DebuggerOverlay
participant Gfx
App->>DetachedManager: Reconcile detached panels and route events
DetachedManager->>DebuggerOverlay: Build panel UI
DebuggerOverlay-->>DetachedManager: Return prepared frame
DetachedManager->>Gfx: Acquire surface and present frame
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Cuts v2.3.0 “Datum II” and aligns the repo with the release’s stated goals: true native multi-window tool panels, frame-pacing improvements, and tightened accuracy/provenance/packaging gates.
Changes:
- Introduces/rewires “detachable” debugger/tools windows to support real OS windows on native targets (plus supporting UI/plumbing changes).
- Tightens the AccuracyCoin gate by asserting zero failing tests (141/141) in CI, and updates release-facing version/license metadata to GPL-3.0-or-later.
- Updates documentation, release notes, packaging, and provenance headers to reflect the new licensing/provenance posture and corrected terminology.
Reviewed changes
Copilot reviewed 129 out of 133 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| to-dos/plans/v2.0.2-harbor-plan.md | Corrects TriCNES characterization wording in historical plan notes. |
| SUPPORT.md | Updates “current release” and license FAQ text (needs v2.3.0 consistency fix). |
| ROADMAP.md | Updates license statement in roadmap overview. |
| mkdocs.yml | Adds provenance/licensing docs to MkDocs navigation. |
| LICENSE-MIT | Removes MIT license file as part of relicense. |
| docs/scheduler.md | Clarifies performance targets as aspirational and points to measured numbers. |
| docs/SALVAGE_MANIFEST.md | Adjusts manifest text around removed/vendored items. |
| docs/hd-pack-zelda-troubleshooting.md | Updates reference path wording to avoid in-tree ref-proj paths. |
| docs/DOCUMENTATION_INDEX.md | Adds a provenance/licensing index section and links. |
| docs/adr/0006-vrc7-audio-landed.md | Updates reference path wording. |
| deny.toml | Updates license allowlist commentary and includes GPL-3.0-or-later. |
| crates/rustynes-test-harness/tests/accuracycoin.rs | Adds a strict “zero failing tests” AccuracyCoin regression guard. |
| crates/rustynes-test-harness/src/bin/pgo_trainer.rs | Adds SPDX/provenance header for a derived PGO trainer harness. |
| crates/rustynes-ppu/src/palette_gen.rs | Adds SPDX/provenance header for palette generation provenance notes. |
| crates/rustynes-mappers/src/sachen_8259.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/mmc3_clones.rs | Adds SPDX/provenance header and clarifies derived portions. |
| crates/rustynes-mappers/src/m513_sachen_9602.rs | Adds SPDX/provenance header and clarifies derived portions. |
| crates/rustynes-mappers/src/m268_bmc_coolboy.rs | Adds SPDX/provenance header and clarifies derived portions. |
| crates/rustynes-mappers/src/m250_nitra250.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m246_fong_shen_bang246.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m244_cne_decathlon.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m241_bxrom241.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m240_cne_multicart.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m232_camerica_bf9096.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m185_cnrom185.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m180_nichibutsu180.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m156_daou156.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m132_txc_22211.rs | Clarifies GeraNES cross-reference wording in decode comment. |
| crates/rustynes-mappers/src/m113_ave_nina006.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m107_magic_dragon107.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m096_bandai96.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m093_sunsoft3r.rs | Replaces quoted source line/path with independent-behavior wording. |
| crates/rustynes-mappers/src/m089_sunsoft2.rs | Replaces quoted source behavior with independent-behavior wording. |
| crates/rustynes-mappers/src/m079_ave_nina03_06.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m069_sunsoft_fme7.rs | Adds SPDX/provenance header for derived/cross-referenced detail. |
| crates/rustynes-mappers/src/m041_caltron41.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m039_subor39.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m038_bitcorp38.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/m035_jy_asic.rs | Adds SPDX/provenance header and removes in-tree ref-proj path mention. |
| crates/rustynes-mappers/src/m024_vrc6.rs | Clarifies wording around “ref-proj field” to avoid in-tree implication. |
| crates/rustynes-mappers/src/m016_bandai_fcg.rs | Adds SPDX/provenance header and clarifies derivation statement. |
| crates/rustynes-mappers/src/lib.rs | Adds SPDX/provenance header and clarifies derivation in mapper dispatch notes. |
| crates/rustynes-mappers/src/kaiser.rs | Adds SPDX/provenance header and clarifies derived implementation statement. |
| crates/rustynes-mappers/src/jaleco_discrete.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/homebrew_boards.rs | Rewords reference-emulator citation to “cross-referenced, not copied”. |
| crates/rustynes-mappers/src/fds.rs | Adds SPDX/provenance header and clarifies derived table statement. |
| crates/rustynes-libretro/rustynes_libretro.info | Updates libretro core metadata for GPL + v2.3.0 version. |
| crates/rustynes-gfx-shaders/src/crt_stack.rs | Adds SPDX/provenance header for shader reimplementation provenance. |
| crates/rustynes-frontend/src/ntsc_bisqwit.rs | Adds SPDX/provenance header and updates commentary to reflect derivation. |
| crates/rustynes-frontend/src/lib.rs | Adds native-only detached module export. |
| crates/rustynes-frontend/src/icons.rs | Updates icon documentation and adds detach/reattach glyph constants. |
| crates/rustynes-frontend/src/emu_thread.rs | Adds lock-free has_rom() accessor for frame pacing. |
| crates/rustynes-frontend/src/debugger/trace_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/tastudio_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/source_map.rs | Adds SPDX/provenance header and clarifies derivation statement. |
| crates/rustynes-frontend/src/debugger/script_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/rom_info_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/replay_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/ppu_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/perf_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/nsf_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/memory_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/memory_compare_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/mapper_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/input_rebind_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/input_miniatures_panel.rs | Routes panel through shared detachable window helper; updates reference wording. |
| crates/rustynes-frontend/src/debugger/header_editor.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/hd_pixel_panel.rs | Updates reference wording to avoid in-tree ref-proj paths. |
| crates/rustynes-frontend/src/debugger/game_db_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/event_panel.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/debugger/doc_panel.rs | Routes panel through shared detachable window helper; updates license string (needs accuracy/features consistency fix). |
| crates/rustynes-frontend/src/debugger/cpu_panel.rs | Routes panel through shared detachable window helper; updates signature. |
| crates/rustynes-frontend/src/debugger/audio_mixer.rs | Routes panel through shared detachable window helper. |
| crates/rustynes-frontend/src/crt.rs | Updates reference wording to avoid in-tree ref-proj paths. |
| crates/rustynes-frontend/src/cli.rs | Updates CLI “License …” text to GPL-3.0-or-later. |
| crates/rustynes-core/src/movie_interop.rs | Updates reference wording to avoid in-tree ref-proj paths. |
| crates/rustynes-cheevos/Cargo.toml | Updates crate license metadata to GPL-3.0-or-later. |
| crates/rustynes-apu/src/opll.rs | Adds SPDX/provenance header for emu2413-derived implementation. |
| crates/rustynes-apu/src/blip.rs | Adds SPDX/provenance header and clarifies derivation statement. |
| CONTRIBUTING.md | Updates contribution licensing language for GPL-3.0-or-later. |
| CHANGELOG-FULL.md | Corrects TriCNES characterization wording in historical changelog entry. |
| Cargo.toml | Bumps workspace version to 2.3.0 and updates workspace license metadata. |
| android/app/src/main/res/values/strings.xml | Updates Android About text for GPL + 141/141 + mapper count. |
| android/app/src/main/res/values-es/strings.xml | Updates Spanish Android About text for GPL + 141/141 + mapper count. |
| .gitignore | Adds/expands reference-firewall documentation for ref-proj ignore. |
| .github/workflows/release.yml | Updates release packaging to include LICENSE (not MIT/Apache files). |
| .github/release-notes/v2.0.2.md | Corrects TriCNES characterization wording in published historical notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ression guard) The AccuracyCoin gate asserted only `ram_pct >= MIN_PASS_RATE` where `MIN_PASS_RATE = 0.60` — a coarse honesty floor that cannot catch a single-test regression. During the v2.3.0 investigation of the suspected Rad Racer hybrid-address glitch, neutralizing the delayed-`CopyV` model (`COPY_V_DELAY = 0`) dropped exactly the "Hybrid Addresses" test under the `PPU Misc.` suite — 141/141 → 140/141 (99.29%) — which still cleared the 60% floor silently. So the project's standing "141/141" claim had no enforcing gate: a hybrid-address (or any single-test) regression would pass CI unnoticed. Add a strict guard: after the floor check, assert the failing-test list is empty (the shipped headless build has held a full 141/141 with zero failures since v2.0.3, when the promoted 2-cycle-ALE / delayed-`CopyV` model closed "ALE + Read" $0491 and "Hybrid Addresses" $0492 — ADR 0030). This is CI-runnable and in-repo (the AccuracyCoin ROM is MIT), so it is the authoritative regression guard for the hybrid-address behavior — the two hybrid tests being the usual canaries for a `COPY_V_DELAY` / octal-latch regression. The test is the headless, deterministic RAM-direct battery, so 141/141 holds byte-identically across platforms (the desktop / run-ahead variance lives in the separate `accuracycoin_runahead` path, untouched). An intentional, reviewed accuracy change that moves the count updates this assertion in the same commit (docs-as-spec). Verified: passes at the shipped default (141/141, 0 fails); would now fail on the delay=0 probe (140/141) the old floor missed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…STATUS) Bring the top-level docs current for the v2.3.0 line and fold in the readability pass carried on this branch: - README.md — Current Release + Roadmap de-staled to the v2.2.6 -> v2.3.0 line (v2.3.0 the PPU-accuracy capstone; the honest "detach embeds" scope; no monetized store launch); Highlights trimmed of per-version tags; Features roughly halved; Acknowledgments corrected for the GPL relicense (Mesen2 the primary derivation source, puNES/FCEUX/Nestopia subsystem derivation, higan/ares/GeraNES oracles); ADR range 0001-0036; the [Unreleased] doc-firewall work folded into [2.2.9]. 1106 -> ~874 lines. - CHANGELOG.md — the bloated entries condensed to the file's own "few tight highlights" intent (v2.2.3 410->60, v2.2.2, v2.1.5, v2.1.6, v2.2.9), a duplicate v2.1.9/v2.1.10 mislabel under v2.1.6 removed, and a new [Unreleased] "v2.3.0 Datum II (in progress)" block recording the accuracy verification + the exact 141/141 gate + the provenance finalization + this doc refresh. - VERSION-PLAN.md — was stuck at "current: v2.0.4"; the post-1.0 version table now runs through v2.2.9 + v2.3.0, the header/engine-lineage note are current, the relicense is noted (GPL-3.0-or-later, not a SemVer break), and the forward path reflects the withdrawn monetized launch (permanently OSS/income-free, ADR 0035). - docs/STATUS.md — the stale "fast_dotloop recommended for promotion" line now records its v2.2.3 promotion to default. Documentation only; markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ct, oracle-derived v2.2.6 disclosed that the PPU octal-latch / hybrid-address timing had been calibrated to TriCNES and flagged a suspected Rad Racer artifact for a v2.3.0 rework. v2.3.0 investigated it (systematic-debugging: reproduce before fixing) and found the artifact does NOT reproduce in the shipped build: - AccuracyCoin (the authoritative in-repo oracle; Mesen2 fails these tests, per ADR 0030) holds 141/141 with the delayed-CopyV model; an A/B probe disabling it (COPY_V_DELAY = 0) drops exactly the "Hybrid Addresses" test to 140/141. - Rad Racer's road/horizon renders cleanly at the shipped default; the delay=0 A/B is more distorted on the same game state. The artifact lived in the pre-v2.0.3 "+1 coarse-X" approximation, superseded by the promoted 2-cycle-ALE / delayed-CopyV model. The behavior is therefore documentation/oracle-derived (matches the NESdev delayed-CopyV timing + the MIT AccuracyCoin ROM), with TriCNES (MIT, vendored) the original cross-reference. - NOTICE + docs/originality-and-provenance.md §4: reworded from "TriCNES-calibrated, being reworked in v2.3.0" to "verified correct, oracle/doc-derived". - docs/adr/0030: appended a dated v2.3.0 Update section recording the evidence and the no-code-change disposition (not rewritten in place). Pairs with the exact-141/141 AccuracyCoin gate (already committed) that locks it. Documentation only; markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ehavior framing
Correct the in-source provenance record for GeraNES (GPL-3.0-only). A number of
comments, written while a local reference-emulator tree (`ref-proj/`) was on
disk, cited GeraNES *source* files, functions, and in two cases quoted a line of
GeraNES C++ (`data &= readPrg(addr)` in `Mapper093.h`). Those citations both
dangled (the tree has been removed, and the paths never existed in the published
crate) and, more importantly, overstated the relationship: they read as source
derivation, contradicting `NOTICE`'s standing "GeraNES = oracle, no code derived"
disclosure. This commit reconciles the comments to what actually happened and
documents the licensing verification behind that.
What GeraNES was, honestly: for a handful of files its source was consulted as a
cross-reference to confirm publicly-documented hardware behavior and public
conventions — not pure black-box observation. But no GeraNES code was copied. The
matching operations are mechanically dictated by the documented hardware:
* m089 (Sunsoft-2) / m093 (Sunsoft-3R): the bus-conflict `written & rom_byte`
mask and the register bit-field extractions are exactly the nesdev-documented
layouts (`INES_Mapper_089` `[CPPP MCCC]`, `INES_Mapper_093` `[.PPP ...E]`,
both marked "BUS CONFLICTS"). Given the documented register, `(v >> 4) & 0x07`
is the single correct expression of a bits-4-6 field in any language.
* m132 (TXC): the `(addr & 0x0103) == 0x0100` read-decode is a documented
address-decode fact.
* shader_pass::parse_pragma_parameters: implements the *public* RetroArch
`#pragma parameter` wire format, re-attributed accordingly; the parser is
independent Rust (let-else, strip_prefix, filter_map(parse::<f32>)) with no
C++ structure surviving.
Verified two-sided at the maintainer's direction: the GeraNES source
(gracioni/GeraNES `Mapper089.h`/`Mapper093.h`) was compared against RustyNES's
implementations and against the nesdev wiki. Only the hardware-forced operations
coincide; every creative/arbitrary choice (identifiers, decomposition, idiom,
comment wording) is independent. That convergence-on-documented-fact — not
copying — is the merger doctrine, so no GPL-3.0-only code is incorporated and the
project's GPL-3.0-or-later grant is unaffected.
Changes:
* 22 discrete-logic mapper headers: drop the stale
`reference (GeraNES/src/GeraNES/Mappers/Mapper0NN.h)` path, keep the
"cross-checked against the GeraNES reference emulator and the nesdev wiki"
oracle framing -> "reference emulator (cross-referenced, not copied)".
* m089/m093: remove the quoted C++ line and file/function citations; cite the
nesdev bus-conflict layout as the source of truth.
* m132: attribute the read-decode mask to a nesdev/GeraNES cross-check, drop
the "GeraNES decodes ..." source-implying phrasing.
* shader_pass.rs (module doc + ShaderParam + parse_pragma_parameters): attribute
to RetroArch's `#pragma parameter` convention; drop `parseShaderParameters` /
`ShaderPass::Parameter` / `ShaderWindowUI.inl` citations.
* settings_panel / hd_pixel_panel / input_miniatures_panel / ui_shell / icons /
app: drop `.inl` source paths, `FontAwesomeIcons.h`, and the internal-identifier
citations (`replayInteractionLocked`, `replayRecordingActive`,
`netplayRomChangeRestricted`); reword to describe the observable behavior of
the GeraNES reference emulator.
* NOTICE + docs/originality-and-provenance.md: refine the GeraNES entry to
disclose source consultation honestly (no code copied, or-later unaffected)
and record the two-sided verification and merger-doctrine rationale.
No code behavior changes (comments + attribution docs only); the deterministic
core is byte-identical, so AccuracyCoin holds 141/141 by construction.
`RUSTDOCFLAGS="-D warnings" cargo doc` green on rustynes-mappers and
rustynes-frontend.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…of TriCNES TriCNES is not a transistor-level emulator, and describing it as one overstated the authority of the oracle RustyNES's octal-latch / hybrid-address model is pinned to. Verified against the upstream project: it is written in C#, the author describes it as "a NES emulator written in C# for playing back TASes" with "a focus on test-driven accuracy", and its accuracy comes from a detailed sub-cycle CPU/PPU/APU/DMA state machine — not from simulating transistors. The distinction is not pedantic. "Transistor level" properly denotes die-derived simulations that model the actual silicon (Visual2C02 / phantom2c02), and the repository already cites those correctly under that term in docs/tooling/oracle-tooling-setup.md and the accuracycoin-remediation plan. Using the same phrase for a high-level C# emulator inflated its standing precisely where the provenance record must be exact, since ADR 0030 rests on TriCNES being the definitive oracle for the "ALE + Read" / "Hybrid Addresses" tests. It remains the correct oracle for those tests — Mesen2 demonstrably fails them — but on the strength of its sub-cycle model, which is what the text now says. Reworded to "cycle-accurate C# emulator (a detailed sub-cycle CPU/PPU/APU/DMA state machine)" in NOTICE and README.md, and to "at the pin/sub-cycle level" where the phrase described the AD/A-bus + octal-latch modelling. Historical documents (ADR 0030, the v2.0.2 Harbor plan, CHANGELOG-FULL, the v2.0.2 release note) keep their original narrative and carry a dated correction note rather than being silently rewritten. The v2.2.5 release note's "transistor-literal octal-latch fetch" — RustyNES describing its OWN port in the same inflated terms — becomes "pin-level octal-latch fetch". The published GitHub releases for v2.0.2 and v2.2.5 were edited to match, so the user-facing copies do not continue to assert the claim. No behavior change: the only source touched is a comment block in crates/rustynes-ppu/src/ppu.rs, which lands with the PPU performance change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot a gate AGENTS.md instructed contributors to "Target <= 2 ms/frame headless", and docs/scheduler.md repeated it as a performance target, but that number was written during the design phase — before the cycle-accurate core existed — and specified for 2018-era Skylake hardware. The implemented core has never met it and is not expected to: it measures ~3.9 ms (nes_run_frame_nestest_fast) and ~2.5 ms (flowing_palette) on a 2020 desktop, which docs/performance.md has recorded as knowingly accepted for the master-clock design since v2.0.1. Leaving an unmet aspiration phrased as a live target is actively harmful here, because the only remaining ways to reach it are to compute less. The two dominant costs are work the accuracy model requires: `cpu_clock` is APU BLEP synthesis plus the non-linear mixer (whose ceiling was measured at <=1.9% in the v2.2.3 P4 campaign, with both textbook optimizations already implemented), and `Ppu::tick` is the per-dot lockstep loop that is the architecture. The obvious micro-levers were already measured and rejected with published numbers — `emit_pixel` bounds-check elision made the shipped default *slower*, as did the SIMD blitter. A contributor reading "target <= 2 ms" and optimizing toward it would be trading away the accuracy that is the project's entire purpose. Both files now state the measured-and-accepted figures, note that the gate which actually runs in CI is the relative same-runner regression check rather than this table, and point at the rejection write-ups. The core at ~3.8 ms is ~23% of the 16.639 ms NTSC budget; the real-world multiplier on frame cost is run-ahead (which runs the core 1-3x per displayed frame), not the per-frame core cost. Documentation only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-5.1%)
Two byte-identical changes to the per-dot PPU helpers, adopted under the project's
>3% same-runner bar after a measure-first campaign (v2.3.0 P1 in docs/performance.md).
The campaign began by discarding its own baseline. The first profile ran on a
contended machine and criterion reported 39% outliers; re-measured quiet it
reported 2%, which is the only reason the subsequent 3-5% deltas mean anything. A
noisy machine invalidates the baseline long before it invalidates the conclusion.
Excluding ~17% of samples belonging to criterion's own harness (rayon plumbing,
libm exp, its sorts), frame self-time split PPU ~53% / CPU+bus ~39%. Two per-dot
helpers sat outside every previously-examined lever: `tick_sprite_eval_per_dot`
(4.45%) and `tick_oam_bus` (3.22%). `perf annotate` — the same instrument that
redirected the v2.2.3 P4 investigation away from its stated premise — showed the
cost was not the state machines at all:
* In `tick_sprite_eval_per_dot`, the two hottest instructions in the entire body
were its own `push %rax` (5.35%) and `ret` (5.41%). That is pure call
overhead. The fast dot path invokes it once per dot, 89,342 times per frame,
and LLVM had declined to inline it.
* `tick_oam_bus` derived `sprite_height` (a PpuCtrl test) and the y-test
reference `scan` BEFORE its `cycle == 0` early-out, computing and discarding
both on that dot.
The fixes follow directly: `#[inline]` on the former, and hoisting the early-out
above the two derivations in the latter. Both are byte-identical by construction —
an inlining hint changes no behavior, and neither hoisted value is observable on
the path that returns.
Measured (same runner, criterion, 12 s / 10 s measurement windows):
nes_run_frame_nestest_fast 3.8987 ms -> 3.7830 ms -5.13%
(95% CI -5.60..-4.60, p = 0.00)
nes_run_frame_flowing_palette_fast 2.7314 ms -> 2.6354 ms -3.51%
(95% CI -3.93..-3.10, p = 0.00)
Both workloads clear the >3% bar; the second exists to prove the win is not
specific to nestest's mostly-text rendering profile.
Byte-identity verified rather than asserted: AccuracyCoin 141/141 through the
exact-count gate (not merely the 60% floor), the nestest golden CPU log 0-diff,
and rustynes-ppu 91/91 unit tests. The no_std thumbv7em-none-eabihf cross-compile
and workspace clippy -D warnings are clean.
This does NOT move the core toward the aspirational <=2 ms — it lands at ~3.78 ms.
The remaining bulk is accuracy-required work, as docs/performance.md §Targets now
states explicitly.
The comment block above the octal-latch model also drops the inaccurate
"transistor-level" description of TriCNES, matching the provenance correction in
558b143 (this was the one source file that correction touched).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n vocabulary The core-info file carried the SPDX identifier "GPL-3.0-or-later", which is correct for Cargo metadata but is not the notation libretro's ecosystem uses. Its .info files carry a short human-readable license string, and RetroArch surfaces that string verbatim in the Core Downloader and core-information views. Checked against the cores that share RustyNES's license rather than guessing at the convention: mesen (GPLv3-or-later, and RustyNES's primary derivation source), melonDS, and bsnes all declare exactly "GPLv3". The ecosystem does have a "+" suffix for or-later grants — MAME uses "GPLv2+" — but it is the exception, and every GPLv3-or-later core sampled writes the bare "GPLv3". The crate's Cargo metadata is untouched and still resolves to GPL-3.0-or-later via license.workspace = true, which is the right form there. This changes only the display string libretro reads. Note for the upstream sync: the copy RetroArch actually ships is the one in libretro-super, which is still pending in the open buildbot PRs. That copy needs the same value when those merge or the recipe is re-synced; this commit only fixes the in-repo source of truth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…k decoupling
Completes the v2.2.9 "Studio II" detach affordance, which shipped with an honest
caveat: on a single-viewport egui_winit integration, show_viewport_immediate
merely EMBEDDED a "detached" panel inside the main window, so the Windows-10
"every tool window is trapped in the main window" report was not actually
resolved. Tool panels now open as real OS windows, and the render path that made
that possible also fixes a long-standing frame-pacing defect.
Multi-viewport, without unsafe
------------------------------
egui's native multi-viewport path was rejected on inspection. An *immediate*
viewport is rendered by a re-entrant callback that must create a winit window on
the spot, which needs &ActiveEventLoop — a reference valid only during event
dispatch, which is why eframe erases its lifetime into a 'static thread-local
behind unsafe. A *deferred* viewport avoids that but its callback is
Fn(&Context) + Send + Sync + 'static, so it cannot borrow the &mut Nes a debugger
panel needs.
The new crates/rustynes-frontend/src/detached.rs takes a third route: each
detached panel owns a real winit window plus its own egui Context / State /
Renderer / wgpu Surface, and is rendered on its OWN RedrawRequested, in its own
stack frame, where the panel's borrows are freshly re-acquired. Nothing is stashed
across frames, so no unsafe and no lifetime erasure are required. Windows share
the main Instance/Adapter/Device/Queue (retained on Gfx for this purpose) because
an egui_wgpu::Renderer's resources must live on the device its pass targets.
A panel is routed to its window by a thread-local render target: the detached pass
re-runs the SAME chip_panels/tool_panels dispatch with DETACH_TARGET set, so
detachable_window paints only the matching id and everything else early-returns.
An RAII guard restores the target even on panic. App reconciles the open window
set against the debugger's detached-panel set each iteration, creating and closing
real windows, and routes window events by WindowId.
Every tool panel is now detachable. The nine that predated the shared helper and
rolled their own egui::Window — CPU, Cartridge Info / Header, Lua Script,
BasicBot, Input bindings, TAStudio, Settings, Netplay, RetroAchievements — were
converted; BasicBot's bespoke embed-viewport implementation was removed in favour
of the shared path. Detach/reattach uses real Font Awesome glyphs (EXPAND /
COMPRESS): the previous U+29C9 is in neither the icon font nor the default font,
so it silently rendered as nothing.
Detached windows adopt the main window's theme, UI zoom, and locale and paint into
a themed central-panel frame; each opens at the size its docked window actually
had, captured at the moment Detach is clicked rather than from a static table.
Emulator-lock decoupling (the frame-pacing fix)
----------------------------------------------
Profiling the stutter found a root cause predating this work. The overlay-visible
render branch held the emulator mutex, in its own words, "until after the present
call" — and inside Gfx::render_with_overlay the BLOCKING Surface::get_current_texture
runs BEFORE the overlay closure. So every frame with the overlay or any
nes-reading tool panel open, the winit thread owned the mutex across a swapchain
wait (up to a full display refresh), the entire egui build, the encode, the submit
and the present, while the emulation thread sat parked on emu.lock() in
drive_wallclock/drive_one, unable to produce. Each detached window would have
added another such acquisition.
DebuggerOverlay::render_shell is split into run_shell_ui (needs &mut Nes; the
caller holds the lock) and paint_shell (pure GPU work, lock released), carried by
PreparedShell. The overlay-visible branch now scopes the guard to phase 1 only.
The framebuffer already came from self.present_staging, so nes_for_render was the
sole reason to hold the lock at all. render_shell remains as a wrapper for the
debugger-hidden path, which already rendered unlocked. DetachedManager is split
the same way (render_ui / present).
pace_frames answered "is a ROM loaded?" by taking the emulator mutex on EVERY
about_to_wait iteration — a tight spin in the wall-clock regime — which could
block the UI thread for a full produce (~4 ms) each time. It now reads
EmuControl::has_rom, the lock-free atomic App itself maintains, falling back to
the locked read only when no emulation thread exists so the emu-thread-off build
is unchanged.
Detached windows repaint on a per-panel tier rather than free-running: Live
(60 Hz) for continuously-changing state (CPU, PPU, OAM, APU, memory, event, trace,
watch, performance, audio mixer, input display), Throttled (~10 Hz) for
slowly-changing status, and OnInteraction-only for static panels (Cheats, ROM Info,
Game DB, Settings, documentation), which repaint solely from egui's repaint flag.
reconcile_detached gained an allocation-free fast path; it had been allocating a
Vec every about_to_wait iteration.
Scope and verification
----------------------
Frontend-only — no file under rustynes-{cpu,ppu,apu,mappers,core} is touched, so
the deterministic core is byte-identical and AccuracyCoin holds 141/141 by
construction. All detach code is #[cfg(not(target_arch = "wasm32"))]; wasm is
single-canvas and keeps every panel docked, unchanged.
Gates: cargo fmt; clippy -D warnings on default, scripting, scripting+hd-pack,
retroachievements, and both wasm flavours; RUSTDOCFLAGS=-D warnings cargo doc; and
a clean --release build. The p99 improvement was confirmed interactively by the
maintainer, docked and detached.
Known limitation: egui cannot place a control in a native window's title bar (that
bar belongs to the window manager), so Detach is a small top-row control rather
than sitting beside the close button. Reattach is available from the detached
window's own button and from its OS close button.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ort tool windows
Cuts v2.3.0, closing the v2.2.6 -> v2.3.0 NESdev-remediation line. Both remaining
forum-reported accuracy concerns were investigated under reproduce-before-fixing
discipline and found ALREADY CORRECT in the shipped build, so the release's
substance landed elsewhere: tool panels that open as real OS windows, and a
frame-pacing defect that had been degrading every session with a debugger panel
open.
Version bump 2.2.9 -> 2.3.0 across the single source of truth and its derivatives:
the workspace Cargo.toml (+ Cargo.lock), the libretro core-info display_version,
and the README version badge + CITATION block. Verified consistent before tagging,
per the release checklist.
CHANGELOG.md's [Unreleased] becomes [2.3.0] - 2026-08-05 - "Datum II", written to
the file's "few tight highlights" intent with Keep-a-Changelog sections: Added
(multi-viewport detach), Fixed (the emulator-lock frame-pacing defect and the
per-iteration pace_frames lock), Changed (the -5.13% / -3.51% PPU optimization,
per-panel refresh tiers, the exact-141/141 AccuracyCoin gate, the <=2 ms
relabelling, the libretro GPLv3 string), Verified-no-change-required (SMB left
edge and the Rad Racer hybrid-address render), and Documentation (hybrid-address
provenance finalized, the TriCNES correction, the GeraNES comment rewording).
README.md's Current Release and Roadmap are rewritten: the v2.2.6 -> v2.3.0 line
now reads COMPLETE rather than in-progress, and the headline leads with the real
OS-window detach plus the frame-pacing fix rather than the accuracy capstone that
turned out to need no code change.
docs/STATUS.md leads with v2.3.0 and, in the same pass, corrects a leftover
overclaim about the PREVIOUS release: it had described v2.2.9 as letting tool
windows "detach into real OS windows (fixing the Windows-10 trapped-window
report)", which is precisely what v2.2.9 could not do — its affordance embedded
the panel. The entry now says "embedded", with the fix attributed to v2.3.0.
AGENTS.md's current-release block, its v2.2.9 embed caveat, its "never claim a
version later than N is released" guard, and the line-status are all advanced;
VERSION-PLAN.md's current marker and release table gain the v2.3.0 row.
.github/release-notes/v2.3.0.md is authored full-width so release-auto.yml can use
it as the published body verbatim (the narrow hard-wrapping that affected v2.2.5
through v2.2.9 was corrected earlier in this line).
Pre-release gate, all green on this tree at 2.3.0:
cargo fmt --all --check clean
cargo clippy --workspace --all-targets -D warnings clean
cargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-features
clean (no_std)
RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps clean
markdownlint (pinned pre-commit hook) clean
cargo deny check licenses ok
cargo audit (675 crate dependencies) 0 vulnerabilities
cargo test --workspace 1923 passed / 0 failed / 13 ignored
cargo test --workspace --features test-roms 2227 passed / 0 failed / 20 ignored
AccuracyCoin holds 141/141 (100.00%) and nestest is 0-diff — now enforced as an
exact count rather than the previous 60% floor, which was too loose to catch a
single-test regression.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot's review of #347 flagged two user-facing strings still asserting pre-v2.0.3 state; a sweep for the same shapes found three more. All five are current-state claims (not historical narrative), so they are corrected here rather than left to contradict the release being published on merge. SUPPORT.md "current release is v2.2.9 Studio II" -> v2.3.0 "Datum II" debugger/doc_panel.rs (About) AccuracyCoin 98.58% (139/141) -> 100% (141/141); 150 mapper families -> 172 debugger/doc_panel.rs (devtools) AccuracyCoin (98.58%, 139/141) -> (100%, 141/141) cli.rs (--help Accuracy block) AccuracyCoin 98.58% (139/141) -> 100% (141/141) av_record.rs (module doc) "stays 139/141 (the two newest upstream PPU tests are known gaps)" -> "stays 141/141" — those two tests, "ALE + Read" and "Hybrid Addresses", have passed since v2.0.3 promoted the 2-cycle-ALE model Historical references to 139/141 in CHANGELOG.md, VERSION-PLAN.md, AGENTS.md and docs/STATUS.md are deliberately left alone: they describe what was true at the v2.0.1 re-sync and are correct as history. Text only — no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4f1a334 to
306410c
Compare
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/release-notes/v2.0.2.md:
- Line 24: Update the TriCNES provenance reference in the release note to use
the current path crates/rustynes-test-harness/golden/tricnes/tricnes-full-src/
instead of ref-proj/TriCNES/Emulator.cs, or explicitly mark the old path as
historical if retaining it. Keep the existing emulator and commit attribution
unchanged.
In @.github/release-notes/v2.2.5.md:
- Around line 9-12: Update the v2.2.5 release-note provenance paragraphs around
the claims that no GPL-licensed emulator source was incorporated so they are
clearly labeled as superseded historical statements and link to the corrective
disclosure in CHANGELOG.md or NOTICE. Do not present those license or provenance
conclusions as current facts, while preserving the historical context of the
release note.
In `@AGENTS.md`:
- Line 201: Synchronize the stale release guidance in AGENTS.md: update the
current-release declaration near the top to v2.3.0 “Datum II,” and revise the
forward-plan text so v2.3.0 is described as shipped and not a future store
launch. Mark obsolete v2.2.9/current-release and store-launch statements as
historical or replace them with the established v2.3.0 guidance, preserving the
repository’s release-policy wording.
In `@CHANGELOG.md`:
- Around line 419-421: Update the v2.2.3 summary in the changelog to remove the
inaccurate claim of zero shipped-default behavior change; state that there was
no regression or precisely identify only the unchanged surface, while keeping
the documented PPU-path, mapper, audio-calibration, and snapshot changes in the
same entry consistent.
In `@crates/rustynes-frontend/src/app.rs`:
- Around line 8762-8776: The phase-1/phase-2 lock-scope invariant lacks
regression protection. Add a debug-only re-entrancy or lock-held check tied to
EmuHandle::lock and paint_shell/overlay, asserting that paint_shell and the
overlay/extra closure do not acquire the emulator lock after phase 1; preserve
the existing snapshot-based native extra path and avoid affecting release
builds.
- Line 4115: Update the documentation comment adjacent to the replay-interaction
lockout to state that the independent implementation was compared against the
GeraNES reference emulator as a black-box oracle, rather than saying it mirrors
or derives from GeraNES. Preserve the provenance attribution while making clear
no porting or derivation is claimed.
- Around line 3998-4003: Move the detached.request_redraw_tick() call from
on_emu_frame into post_produce_housekeeping so every native frame-production
path updates detached-panel throttle ticks, including builds without emu-thread.
Preserve the existing non-wasm conditional and remove the original call to avoid
duplicate redraw ticks.
- Around line 8007-8023: Update the detached-window creation failure branch in
the reconcile loop around DetachedManager::create to reattach the failed panel
through the existing DebuggerOverlay reattachment path. Remove the panel id from
DebuggerOverlay::detached_panels or otherwise invoke its established reattach
operation after logging the error, so the panel becomes docked again and
subsequent event-loop iterations do not retry creation.
In `@crates/rustynes-frontend/src/debugger/cpu_panel.rs`:
- Line 100: Add an adjacent explanatory comment to the local
#[allow(clippy::too_many_arguments)] attribute for CpuPanelState::show,
documenting why its current argument list must remain; do not alter the method
signature or introduce a context struct unless choosing that alternative
instead.
In `@crates/rustynes-frontend/src/debugger/mod.rs`:
- Around line 396-436: Update the detached-panel lifecycle around
detachable_window and render_detached_body to detect when a detached target
produces no content because its show flag or eligibility condition was cleared.
Track whether the detached target rendered, reset that state before dispatch,
and remove the target from detached_panels when dispatch paints nothing so
App::reconcile_detached closes the orphaned window. Preserve normal rendering
and reattach behavior for targets that do produce content.
- Around line 1694-1697: In DebuggerOverlay::render_detached_body, stop calling
crate::ui_shell::apply_theme unconditionally on every repaint. Add a
per-detached-target theme cache, such as detached_theme keyed by the target’s
static panel identifier, and apply the theme only when that cached value
differs; do not reuse self.last_theme because it belongs to the main window.
Optionally remove the target entry in App::handle_detached_window_event when the
detached window closes.
In `@crates/rustynes-frontend/src/detached.rs`:
- Around line 291-306: Update render_ui to inspect output.viewport_output for
the zero repaint_delay request, and call request_redraw on the corresponding
detached window when egui requests another frame. Preserve the existing frame
preparation and platform-output handling while ensuring OnInteraction
repaint-driven animations re-arm redraws.
- Around line 329-337: Apply the prepared egui texture delta, including both set
and free entries, before the w.surface.get_current_texture call in the detached
render flow. Preserve the existing Lost/Outdated reconfiguration behavior, and
request a redraw after reconfiguring so the window retries rendering without
waiting for another event; ensure the later texture-update loops do not apply
the same delta twice.
In `@crates/rustynes-frontend/src/emu_thread.rs`:
- Around line 303-315: Update the doc comment for EmuThread::has_rom to describe
the operation as a single Acquire load, removing the contradictory
“relaxed-acquire” wording; leave the implementation and synchronization behavior
unchanged.
In `@crates/rustynes-frontend/src/gfx.rs`:
- Around line 796-816: In the detached surface configuration flow, validate the
capabilities returned by surface.get_capabilities(&self.adapter) before
selecting format or alpha_mode. If both caps.formats and caps.alpha_modes are
empty, return GfxError::Surface(...) so DetachedManager::create can log the
failure; otherwise preserve the existing sRGB selection and configuration
behavior.
In `@crates/rustynes-ppu/src/ppu.rs`:
- Around line 4314-4318: Update the inline rationale for
tick_sprite_eval_per_dot in crates/rustynes-ppu/src/ppu.rs lines 4314-4318 to
say it runs once per eligible rendered dot, or use the measured eligible-dot
count instead of 89,342. Update the corresponding profiling record in
docs/performance.md lines 587-590 with the same call-count wording so both
performance records match the implementation.
In `@crates/rustynes-test-harness/tests/accuracycoin.rs`:
- Around line 336-357: Update the AccuracyCoin assertions near the existing
failing.is_empty() guard to explicitly count result categories: require zero
Fail plus Unknown results, exactly 141 Pass plus PassWithCode results, and
reject or otherwise explicitly handle skipped tests. Do not rely on the existing
60% assigned-test threshold or failing list alone; preserve the regression
message context while enforcing complete execution of all 141 tests.
In `@docs/originality-and-provenance.md`:
- Around line 139-167: Revise the definitive licensing conclusions in the
GeraNES discussion to present them as a provisional assessment subject to human
and expert review. Retain the source-consultation evidence, independent
implementation details, and provenance disclosure, but replace claims that no
GPL code was incorporated and that the GPL-3.0-or-later grant stands with
appropriately qualified uncertainty.
In `@docs/scheduler.md`:
- Around line 157-163: Correct the Frame cost metric in the scheduler design
aspirations to match the workload conditions documented in the performance
Targets section. Remove the “no rendering” label from the ~3.9 ms nestest value,
and either identify it as headless core only where supported or distinguish the
rendering-enabled nestest and rendering-disabled flowing_palette measurements.
In `@NOTICE`:
- Around line 61-73: Replace the GeraNES provenance statements at NOTICE (lines
61-73), .github/release-notes/v2.3.0.md (line 50), and CHANGELOG.md (lines
102-105) with one consistent, qualified statement: separately describe source
consultation and avoid definitive conclusions about copying or copyrightability,
explicitly note that the legal classification is uncertain and requires human
and expert review, and preserve the documented-behavior/no-incorporated-code
context.
In `@README.md`:
- Line 88: Update the mapper-family count in SUPPORT.md to match the
authoritative 172 mapper families stated in the README.md Highlights table,
preserving the surrounding support description and terminology.
- Around line 409-413: Update the README description of native panel detaching
near the TAStudio and Audio Mixer list to state that the Detach button opens
panels as separate OS windows, matching the current-release behavior and
existing documentation.
In `@SUPPORT.md`:
- Line 97: Update the roadmap link in the release description to target the
authoritative current roadmap at to-dos/ROADMAP.md instead of the stale root
ROADMAP.md, while preserving the surrounding v2.3.0 release text.
In `@VERSION-PLAN.md`:
- Line 97: Correct the AccuracyCoin timeline in VERSION-PLAN.md: do not claim
141/141 throughout v2.x, and state that the exact 141/141 result applies from
v2.0.3 onward while documenting the v2.0.2 139/141 exception. Keep the wording
consistent with the authoritative entries in CHANGELOG-FULL.md.
- Around line 79-82: Update VERSION-PLAN.md by removing the duplicate v2.3.0
“Datum II” in-progress row and revising the “Forward path” paragraph to describe
work after the shipped v2.3.0 release. Keep the existing current v2.3.0 row and
preserve the documented open-source, income-free policy while removing
references that identify v2.2.6 → v2.3.0 as the active line.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8cdda054-0944-424d-a608-8cbe37047b20
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (68)
.github/release-notes/v2.0.2.md.github/release-notes/v2.2.5.md.github/release-notes/v2.3.0.mdAGENTS.mdCHANGELOG-FULL.mdCHANGELOG.mdCargo.tomlNOTICEREADME.mdSUPPORT.mdVERSION-PLAN.mdcrates/rustynes-frontend/src/app.rscrates/rustynes-frontend/src/av_record.rscrates/rustynes-frontend/src/cli.rscrates/rustynes-frontend/src/debugger/basic_bot_panel.rscrates/rustynes-frontend/src/debugger/cheevos_panel.rscrates/rustynes-frontend/src/debugger/cpu_panel.rscrates/rustynes-frontend/src/debugger/doc_panel.rscrates/rustynes-frontend/src/debugger/hd_pixel_panel.rscrates/rustynes-frontend/src/debugger/header_editor.rscrates/rustynes-frontend/src/debugger/input_miniatures_panel.rscrates/rustynes-frontend/src/debugger/input_rebind_panel.rscrates/rustynes-frontend/src/debugger/mod.rscrates/rustynes-frontend/src/debugger/netplay_panel.rscrates/rustynes-frontend/src/debugger/script_panel.rscrates/rustynes-frontend/src/debugger/settings_panel.rscrates/rustynes-frontend/src/debugger/tastudio_panel.rscrates/rustynes-frontend/src/detached.rscrates/rustynes-frontend/src/emu_thread.rscrates/rustynes-frontend/src/gfx.rscrates/rustynes-frontend/src/icons.rscrates/rustynes-frontend/src/lib.rscrates/rustynes-frontend/src/shader_pass.rscrates/rustynes-frontend/src/ui_shell.rscrates/rustynes-libretro/rustynes_libretro.infocrates/rustynes-mappers/src/homebrew_boards.rscrates/rustynes-mappers/src/jaleco_discrete.rscrates/rustynes-mappers/src/m038_bitcorp38.rscrates/rustynes-mappers/src/m039_subor39.rscrates/rustynes-mappers/src/m041_caltron41.rscrates/rustynes-mappers/src/m079_ave_nina03_06.rscrates/rustynes-mappers/src/m089_sunsoft2.rscrates/rustynes-mappers/src/m093_sunsoft3r.rscrates/rustynes-mappers/src/m096_bandai96.rscrates/rustynes-mappers/src/m107_magic_dragon107.rscrates/rustynes-mappers/src/m113_ave_nina006.rscrates/rustynes-mappers/src/m132_txc_22211.rscrates/rustynes-mappers/src/m156_daou156.rscrates/rustynes-mappers/src/m180_nichibutsu180.rscrates/rustynes-mappers/src/m185_cnrom185.rscrates/rustynes-mappers/src/m232_camerica_bf9096.rscrates/rustynes-mappers/src/m240_cne_multicart.rscrates/rustynes-mappers/src/m241_bxrom241.rscrates/rustynes-mappers/src/m244_cne_decathlon.rscrates/rustynes-mappers/src/m246_fong_shen_bang246.rscrates/rustynes-mappers/src/m250_nitra250.rscrates/rustynes-mappers/src/multicart_discrete.rscrates/rustynes-mappers/src/ntdec.rscrates/rustynes-mappers/src/sachen_8259.rscrates/rustynes-mappers/src/sachen_discrete.rscrates/rustynes-ppu/src/ppu.rscrates/rustynes-test-harness/tests/accuracycoin.rsdocs/STATUS.mddocs/adr/0030-accuracycoin-ale-read-hybrid-addresses-octal-latch.mddocs/originality-and-provenance.mddocs/performance.mddocs/scheduler.mdto-dos/plans/v2.0.2-harbor-plan.md
CodeRabbit and the Antigravity reviewer each found real regressions in the
v2.3.0 detached-window code. All three are fixed here.
1. Texture-delta discard on a lost swapchain (CRITICAL)
-------------------------------------------------------
`DetachedManager::present` acquired the swapchain image BEFORE applying egui's
texture delta, and returned early when the surface reported Lost/Outdated. That
delta is one-shot: egui hands over each created or updated texture exactly once
and then forgets it. Dropping a frame's `set` list therefore meant the font atlas
(or any newly created image) never reached the renderer, and every subsequent
frame drew with a texture id the renderer had never seen — the window stayed
blank or garbled permanently, long after the transient surface error cleared. A
window resize or a monitor change is enough to trigger it.
Texture uploads now happen before anything that can bail out, so a skipped
present is merely a dropped frame. The `free` list moved into a shared
`free_textures` helper invoked on every exit path, so a bail-out neither leaks
GPU memory nor frees a texture the next frame still references.
2. Failed window creation retried forever (BLOCKING)
----------------------------------------------------
If `DetachedManager::create` failed — OS window limits, surface creation refusal
— the panel id stayed in the debugger's detached set while `has_panel(id)` stayed
false. `reconcile_detached` runs on every `about_to_wait` iteration, so it
retried the failing create thousands of times a second, writing an `eprintln!`
line each time. The panel is now reattached on failure: it degrades to the
pre-v2.3.0 in-window behaviour (still fully usable), reports once, and cannot
spin.
3. Orphaned OS window when a panel's `show_*` flag is cleared (MAJOR)
---------------------------------------------------------------------
Every `detachable_window` call site is `if self.show_x { ... }`, so clearing the
flag while the panel is detached means the function is never reached — neither
branch runs, nothing removes the id from `detached_panels`, and the OS window
lingers: titled, painted only with the clear colour, and with no Reattach button.
This is reachable without touching the window at all. `clear_tas_editor` fires on
every ROM load, ROM close, and power cycle; hardcore mode suppresses the Memory
and Memory Compare panels the same way.
`detachable_window` now records that it painted the current detached target, and
`render_detached_body` reattaches any target that produced no content, letting
the next `reconcile_detached` close the window.
Also addressed from the same review: `apply_theme` ran on every frame for every
detached window, rebuilding a full `Visuals` and calling `set_visuals` each time,
while the main window deliberately gates that behind a change check. Each
detached window has its own `egui::Context`, so the main window's `last_theme`
cache cannot speak for it; a per-window `detached_themes` map mirrors that cache
keyed by panel id. `set_zoom_factor` and `set_locale` stay unconditional — both
are documented no-ops when unchanged.
Gates: fmt; clippy -D warnings on default, retroachievements, and wasm. Frontend
only, so the deterministic core is untouched and AccuracyCoin holds 141/141 by
construction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nce, docs) Works through the rest of the CodeRabbit review on top of the three defects already fixed in 8983b26. Four more are functional; the rest are provenance honesty and documentation drift, several of which contradicted the release being published. Correctness ----------- * Detached windows froze under an `emu-thread`-off build. `request_redraw_tick()` was called only from `on_emu_frame`, which is compiled under that feature; the synchronous production paths run `post_produce_housekeeping` instead, so `Live`/`Throttled` tiers never advanced there. Moved to the one point both regimes share. * `create_detached_surface` could panic the whole emulator while merely opening a tool window. The retained adapter was selected with `compatible_surface` pointing at the MAIN window's surface, so it is not guaranteed to present to a new one; when it cannot, wgpu reports empty capability vectors and `caps .formats[0]` / `caps.alpha_modes[0]` panic. Both now return `GfxError::Surface`, which `DetachedManager::create` already degrades to "panel stays docked". * Detached windows discarded egui's repaint request. `run_ui` reports, per viewport, how long egui will wait before it wants another frame; a zero delay means "now". That was dropped, and `OnInteraction` is the one refresh tier that never re-arms from the frame clock — so a hover or active-widget colour transition would stop partway and only resume on the next input. Now honoured. * The AccuracyCoin exact-count guard proved less than it appeared. `failing .is_empty()` only covers Fail/Unknown, so a battery that *under-executed* — an early bail, a skipped suite, a decoder that stopped assigning cells — produced an empty failing list and passed. That is the same "absent looks like success" shape the guard exists to prevent. It now also asserts exactly `EXPECTED_PASS_COUNT == 141` passes, so under-execution fails as loudly as regression. Verified: still green at 141. * Added a debug-only guard for the v2.3.0 lock-scope invariant. The phase-1 / phase-2 split is load-bearing for frame pacing but was enforced by nothing but care — a future panel re-locking inside the paint path would compile, pass tests, and silently restore the stall. `EmuHandle::lock` now trips a `debug_assert` when reached from the GPU phase, armed by an RAII guard around the overlay closure. Zero cost in release; not compiled on wasm. Provenance honesty ------------------ The GeraNES assessment was written as a conclusion — "no GeraNES code is copied; GPL-3.0-or-later stands". That is precisely the self-certification this project's own guardrails forbid, and the reason the rule exists is on the record: the original provenance failure was caught by an outside NESdev reviewer, not by the project's tooling. `NOTICE`, `docs/originality-and-provenance.md`, the v2.3.0 release note and `CHANGELOG.md` now carry one consistent, qualified statement: the evidence and the two-sided review are retained, but the result is the maintainers' assessment offered for expert review, with the residual uncertainties named — whether a coincidence is "the single correct expression of a documented fact" is a legal judgement, and the review covered the files the in-source comments pointed at rather than proving absence elsewhere. The v2.2.5 release note still asserted "No GPL-licensed emulator source is incorporated into RustyNES" — withdrawn by the v2.2.9 relicense. It now opens with a superseded banner naming the correction, with the original text retained unedited beneath it: the remedy for a bad provenance claim is to correct it in the open, never to quietly rewrite it. Also reworded the "Mirrors the GeraNES ... lockout" comment, which could be read as a derived-from claim, into an explicit behaviour-parity note. Documentation drift ------------------- * `SUPPORT.md` advertised **51 mapper families** (the count is 172) and pointed users at the root `ROADMAP.md`, a pre-1.0 snapshot; three links now target `to-dos/ROADMAP.md`. * `README.md` still described Detach as popping the panel out *within* the main window — the exact limitation this release removes. * `AGENTS.md` contradicted itself: v2.2.9 as current in one place, v2.3.0 as a future store launch in another. * `VERSION-PLAN.md` listed v2.3.0 twice (current *and* in progress) and claimed 141/141 "throughout the v2.x line"; v2.0.1-v2.0.2 shipped an honest 139/141 until the v2.0.3 promotion. * The v2.2.3 CHANGELOG entry claimed "zero shipped-default behavior change" while the same entry documents a default PPU-path promotion, mapper fixes, an audio calibration and a schema bump. Restated as "no regression" with the intentional changes named. * `docs/scheduler.md` labelled the 3.9 ms figure "no rendering"; nestest renders. * The v2.0.2 release note pointed at the removed `ref-proj/` clone. * The `#[allow(clippy::too_many_arguments)]` on `cpu_panel::show` gained the justification the project's guidelines require. * Corrected two of my own overstatements: `tick_sprite_eval_per_dot` runs once per *eligible* dot (up to 61,440/frame), not on all 89,342 — idle lines and rendering-disabled paths bypass it; and an "Acquire" load was described as "relaxed-acquire" in a file where ordering is load-bearing. Gates: fmt; clippy --workspace -D warnings; wasm clippy; markdownlint; AccuracyCoin re-run green at exactly 141/141. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review closeout — all 25 CodeRabbit findings + the Antigravity blocker addressedThanks — this review caught four real defects that would have shipped, one of them permanent-corruption class. Everything below is fixed in Functional defects (fixed)
Provenance honesty (fixed)
Documentation drift (fixed)All of it, and the mapper-count one was worse than reported: Two of my own overstatements corrected in the same pass: Declined, with reasons
Gates: fmt; |
Antigravity review (Gemini via Ultra)Error: timeout waiting for response Automated first-pass review by |
Addresses the review on #348. Seven findings were valid and are fixed; three were false positives and are declined with evidence in the threads. RELEASE METADATA (CodeRabbit, major -- the substantive one). The PR was titled `release: v2.3.1` while every metadata surface still said unreleased/in-progress, and Cargo.toml still read 2.3.0 -- so release-auto.yml, which derives the tag from the workspace version, would never have produced a v2.3.1 tag. Checked against how v2.3.0 was cut (PR #347 bumped Cargo.toml in the same PR alongside CHANGELOG/README/STATUS) and matched that ceremony: workspace version 2.3.0 -> 2.3.1 (inherited by all 17 crates, Cargo.lock refreshed), the CHANGELOG [Unreleased] block cut to [2.3.1] - 2026-08-06 - "Plumb Line", VERSION-PLAN header + release table + forward path updated, docs/STATUS.md current-release banner rewritten, the plan doc marked COMPLETE, and .github/release-notes/ v2.3.1.md written for release-auto to consume. SCRIPT + CODE FIXES. * bench_relative_check.sh interpolated ${MAX_REGRESSION_PCT} into inline Python source to derive the CV threshold, so a non-numeric BENCH_MAX_REGRESSION_PCT would break parsing or execute as code. Replaced with awk, which treats the value as data; verified that garbage input now yields the 3.33 fallback rather than executing. * frame_probe accepted `--frames 0`, which produced an empty sample set and then reported a 0.00% CV ("host: QUIET"), a 0 ms median and an infinite realtime multiplier -- a confident-looking measurement of nothing, which is precisely what this probe exists to prevent. Both count flags now reject missing/non-numeric values (and --frames rejects zero) with exit code 2. Verified all four cases. * frame_breakdown.sh claimed the script "asserts" that debuginfo does not perturb the measurement by comparing against a stock build. It never builds a stock probe, so it asserts nothing. Reworded to state the figure is context, and to say how a reader can check the claim themselves. * The BOLT probe hard-coded an llvm-bolt-16..21 window, so any other version reported have_bolt=false -- indistinguishable from "not installed", the exact failure this probe exists to eliminate. Now enumerates /usr/bin/llvm-bolt-*, /usr/local/bin/llvm-bolt-* and /usr/lib/llvm-*/bin/llvm-bolt, with the install candidate list widened too. * find_bolt returned success even when its `ln -sf` failed, echoing a directory for a symlink that does not exist. Now propagates the failure and re-checks executability. DOC FIXES. Removed a paragraph duplicated verbatim in the plan doc; relabelled the campaign ranking table as a PRE-CAMPAIGN recommendation (its "promote to first" column is the prediction, and the outcome table above it records that all ten were rejected); corrected 61,440 B/frame to 61,440 u16 entries (122,880 B), index_framebuffer being Box<[u16]>. Verified: workspace clippy clean at -D warnings, cargo fmt clean, YAML and shellcheck clean, cargo check --workspace green on the bumped version.
Cuts v2.3.0 "Datum II", closing the v2.2.6 → v2.3.0 NESdev-remediation line.
Both remaining forum-reported accuracy concerns were investigated under reproduce-before-fixing discipline and turned out to be already correct — so the substance of this release landed elsewhere: tool panels that open as real OS windows, and a frame-pacing defect that had been quietly degrading every session with a debugger panel open.
AccuracyCoin 141/141 (100.00%), nestest 0-diff — now enforced as an exact count rather than a 60% floor.
Tool windows are now real OS windows
v2.2.9's detach affordance only embedded the panel (a single-viewport
egui_winitintegration can't makeshow_viewport_immediateproduce a window), so the Windows-10 "trapped window" report was never actually resolved. It is now. The newcrates/rustynes-frontend/src/detached.rsgives each detached panel a real winit window with its own eguiContext/State/Rendererand wgpuSurface, sharing the main device — with nounsafe, unlike eframe's immediate-viewport path (which erases an&ActiveEventLooplifetime into a'staticthread-local). Each window renders on its ownRedrawRequested, where the panel's&mut Nesborrow is freshly re-acquired; a thread-local render target makes the shared dispatch paint exactly one panel per window.All nine panels that predated the shared helper (CPU, Cartridge Info, Lua Script, BasicBot, Input bindings, TAStudio, Settings, Netplay, RetroAchievements) were converted, so every tool window is detachable. They inherit the main window's theme/zoom/locale and open at the size their docked window actually had.
The frame-pacing fix (stutter / high p99)
Profiling found a root cause predating this work. The overlay-visible render branch held the emulator mutex — in its own comment's words — "until after the present call", and inside
render_with_overlaythe blockingget_current_textureruns before the egui pass. So every frame with the overlay or anynes-reading panel open, the winit thread owned the lock across a swapchain wait, the whole egui build, the encode and the present, while the emulation thread sat parked onemu.lock()unable to produce.render_shellis split intorun_shell_ui(locked) andpaint_shell(unlocked GPU work).pace_framesalso stopped taking the mutex on everyabout_to_waititeration, reading the lock-freeEmuControl::has_romatomic instead.Maintainer-confirmed: p99 substantially improved, docked and detached.
PPU: −5.13% frame cost, byte-identical
A measure-first campaign (
v2.3.0 P1) that began by discarding its own baseline — the first profile ran contended at 39% criterion outliers; re-measured quiet it reported 2%.perf annotateshowedtick_sprite_eval_per_dot's ownpush/retwere the two hottest instructions in its body (pure call overhead across 89,342 calls/frame), and thattick_oam_busderived values it discarded before its dot-0 early-out.nes_run_frame_nestest_fastnes_run_frame_flowing_palette_fastBoth clear the project's >3% bar on both workloads.
Honesty / provenance
≤ 2 msframe-cost figure is relabelled a design-phase aspiration, not a gate — it predates the cycle-accurate core. The core is ~3.8 ms (~23% of the NTSC budget) and that's knowingly accepted; the levers were already measured and rejected (emit_pixelelision and the SIMD blitter were both slower). Recorded so nobody optimizes toward it by trading away accuracy.Visual2C02/phantom2c02), which the repo cites correctly. Fixed in source,NOTICE, README, ADR 0030, and the published v2.0.2 / v2.2.5 release notes.docs/STATUS.mdalso corrects a leftover overclaim that credited v2.2.9 with real OS-window detach.Pre-release gate — all green
Notes for review
backup/pre-reword-bcc9c769.mainwill triggerrelease-auto.ymlto tag and publish v2.3.0 with desktop binaries oncemainCI is green on the full Windows/macOS matrix (PRs run Linux-only).🤖 Generated with Claude Code
Summary by CodeRabbit