feat(hd-pack): Settings UI pack selector, config persistence, CLI docs, ADR 0010 (v1.3.0)#67
Merged
Merged
Conversation
…s, ADR 0010 (v1.3.0) - rustysnes-frontend: EmuCore gains HD texture pack management (available_hd_packs/hd_pack_name/set_hd_pack), wired so load_rom/close_rom clear a previously active pack (it's keyed to the ROM it was discovered under) and power_cycle re-enables tagging on the freshly reconstructed Ppu. - Settings -> Video gains a pack ComboBox (dynamic, unlike the fixed-choice present-mode/theme radios); VideoConfig gains hd_pack_name: Option<String> (additive, default None). The configured pack is re-selected automatically after loading a ROM, both via the CLI argument and File -> Open ROM. - cli.rs's "features"/"config" help topics updated to describe the actual current state instead of the stale "reserved, not wired" placeholder. - New docs/adr/0010: the four load-bearing HD-pack design decisions (hashing scheme, the write-only off-by-default TileTag hook, the core/frontend split, the versioned TOML manifest) plus honest trade-offs. - docs/ppu.md and docs/frontend.md updated to describe the current state: everything through the Settings selector now exists; only wiring the compositor into the live wgpu present path remains outstanding. Verified via a real headless (xvfb-run) launch against both a real generated pack and a missing one -- both paths run without panicking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the frontend-side HD texture pack management and Settings UI integration for the hd-pack feature, as described in the newly added ADR 0010. It introduces configuration persistence, automatic pack re-selection on ROM load, and an interactive pack selector in the Settings menu, while keeping the core emulator pack-agnostic. The review feedback highlights opportunities to optimize performance by using .as_deref() instead of cloning hd_pack_name (an Option<String>) in app.rs and ui_shell.rs, which avoids unnecessary string allocations during the UI rendering loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR advances the hd-pack feature by adding frontend-side HD texture pack selection and persistence, updating user-facing CLI/docs, and formalizing the design in ADR 0010—while keeping the actual wgpu present-path compositing wiring explicitly out of scope.
Changes:
- Add HD texture pack management APIs to the frontend
EmuCore, plus UI actions/plumbing to select/clear packs from Settings → Video. - Persist the selected pack name in
config.video.hd_pack_nameand auto-reselect it on ROM load paths. - Update docs/CLI help to reflect current implementation status and add ADR 0010 documenting key design decisions and tradeoffs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/ppu.md | Updates HD pack status and “not yet done” section to reflect UI + config persistence now landed. |
| docs/frontend.md | Documents frontend pack management + Settings selector, and clarifies remaining present-path wiring gap. |
| docs/adr/0010-hd-texture-pack-system.md | Adds ADR capturing hashing, TileTag hook, core/frontend boundary, and manifest/versioning decisions. |
| crates/rustysnes-frontend/src/ui_shell.rs | Adds Settings → Video ComboBox for pack selection and dispatches MenuAction::SetHdPack. |
| crates/rustysnes-frontend/src/emu.rs | Adds available_hd_packs, hd_pack_name, and set_hd_pack, plus lifecycle handling across load/close/power-cycle. |
| crates/rustysnes-frontend/src/config.rs | Adds VideoConfig.hd_pack_name: Option<String> (default None) for persistence. |
| crates/rustysnes-frontend/src/cli.rs | Updates feature/config help text to describe the real hd-pack state and directory layout. |
| crates/rustysnes-frontend/src/app.rs | Plumbs ShellInfo pack info and handles MenuAction::SetHdPack; reselects configured pack after ROM load. |
… comment power_cycle is compiled unconditionally, but its doc referenced [Self::hd_pack_name] via intra-doc-link syntax -- that method only exists under the hd-pack feature, so `cargo doc --workspace --no-deps` (default features) failed with rustdoc::broken_intra_doc_links. Switched to a plain code span, matching this crate's existing convention for referencing feature-gated items from unconditionally-compiled doc comments. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- EmuCore::load_rom no longer clears the active HD texture pack (or the cached ROM hash) on a failed load -- facade::EmuCore::load_rom leaves the previously-running System completely untouched on error, so the previous pack must stay active too. Regression test added. - Cache the loaded ROM's SHA-256 at load time instead of recomputing it on every available_hd_packs()/set_hd_pack() call -- available_hd_packs runs once per frame while Settings is open, and re-hashing a multi-megabyte ROM that often (including while the emu lock is held) was a real, avoidable cost. Regression test added. - Use .as_deref() instead of .clone() for two Option<String> reads in app.rs's ROM-load auto-select paths (no closure re-borrow issue there). ui_shell.rs's ComboBox keeps its .clone() with an explanatory comment -- the suggested .as_deref() version doesn't actually compile there (E0502), since the closure mutates cfg.video.hd_pack_name while a borrowed `current` would still be alive across that write; verified by trying it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
8 tasks
doublegate
added a commit
that referenced
this pull request
Jul 11, 2026
- CHANGELOG: fixed "all four feature PRs" to "all three" -- only three PR numbers (#66, #67, #68) were ever listed, an arithmetic mistake caught by both bots independently. - CHANGELOG: fixed an identifier (hd_pack_tagging_toggle_does_not_alter_ framebuffer_output) that got line-wrapped mid-word inside its own inline code span, which broke the rendered code span and made the test name unsearchable/uncopyable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 11, 2026
* chore(release): v1.3.0 "Palimpsest" Version bump 1.2.0 -> 1.3.0 across the workspace and all 12 crate manifests, CHANGELOG closeout (the HD texture pack feature: hashing + TileTag hook, frontend loader + compositor, Settings UI + config + ADR 0010, and the final live-present-path integration -- PRs #66-#68), and doc/README/ ROADMAP/VERSION-PLAN sync. Full regression gate green: cargo test --workspace (455 tests, 44 suites), cargo test -p rustysnes-test-harness --features test-roms --release (28 tests, 17 suites, zero regressions), cargo clippy --workspace --all-targets -- -D warnings across default/hd-pack/full/emu-thread,hd-pack/debug-hooks/ scripting/cheats/netplay/retroachievements, cargo fmt --all --check, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps, the no_std gate, cargo build -p rustysnes-libretro, and real trunk build --release runs for both wasm32 frontends (wasm-winit default, wasm-canvas). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(release): address Gemini/Copilot review findings on PR #69 - CHANGELOG: fixed "all four feature PRs" to "all three" -- only three PR numbers (#66, #67, #68) were ever listed, an arithmetic mistake caught by both bots independently. - CHANGELOG: fixed an identifier (hd_pack_tagging_toggle_does_not_alter_ framebuffer_output) that got line-wrapped mid-word inside its own inline code span, which broke the rendered code span and made the test name unsearchable/uncopyable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EmuCore(frontend wrapper) gains HD texture pack management:available_hd_packs(),hd_pack_name(),set_hd_pack(Option<&str>).load_rom/close_romclear a previously active pack (it's keyed to the ROM it was discovered under);power_cyclere-enables tagging on the freshly reconstructedPpusince that reconstruction resets the tagging flag to itsfalsedefault.ComboBoxpopulated from the current ROM's discovered packs, dispatchingMenuAction::SetHdPackon selection.VideoConfiggainshd_pack_name: Option<String>(additive, defaultNone). The configured pack is re-selected automatically after loading a ROM (both the CLI-argument path and File → Open ROM).cli.rs's "features"/"config" help topics updated to describe the actual current state instead of the stale "reserved, not wired" placeholder.docs/adr/0010-hd-texture-pack-system.md: the four load-bearing design decisions (hashing scheme, the write-only off-by-defaultTileTaghook, the core/frontend split, the versioned TOML manifest) plus honest trade-offs.docs/ppu.mdanddocs/frontend.mdupdated to describe the current state accurately.Wiring the compositor into the live wgpu present path (the actual on-screen texture swap) is intentionally out of scope here — tracked for the next PR ("Final integration").
Test plan
cargo test -p rustysnes-frontend --features hd-pack(87 tests) and without the feature (63 tests)cargo clippy --all-targets -- -D warningsacross default /hd-pack/fullcargo fmt --all --checkcargo test --workspace(455 tests, no regressions)cargo build -p rustysnes-core --target thumbv7em-none-eabihf --no-default-features(no_std gate)cargo check -p rustysnes-frontend --target wasm32-unknown-unknown --no-default-features --features wasm-winit,hd-packRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --features hd-packxvfb-run) launch against both a real generated pack and a missing one — both paths run without panicking🤖 Generated with Claude Code