Skip to content

feat(gui): egui editor with live step grid + clickable toggles - #3

Merged
gustavokch merged 2 commits into
mainfrom
stage3-egui-editor
Jun 14, 2026
Merged

feat(gui): egui editor with live step grid + clickable toggles#3
gustavokch merged 2 commits into
mainfrom
stage3-egui-editor

Conversation

@gustavokch

Copy link
Copy Markdown
Owner

Stage 3 — GUI port

Adds a custom nih_plug_egui editor restoring the Stage-1 openFrameworks UX (DS-UI.png): a 16-step grid with a live playhead and click-to-toggle cells, plus sliders for the latent vector, per-step pitches, timing, and tuning. The original training UI (GENERATE / Train / Make Dataset / Open Corpus / Epochs / Loss) is dropped — no runtime training, regeneration is reactive on latent change.

What changed

  • src/shared.rs (new) — lock-free SharedState { steps: AtomicU16, current_step: AtomicUsize } bridging the audio thread and the editor (Relaxed; no cross-variable invariant).
  • src/lib.rsmaybe_regen writes the decoded pattern into shared.steps (now the playback source of truth, replacing the old self.steps array); process reads on/off from the mask, publishes the playhead, clears it when stopped; editor() wired up.
  • src/editor.rs (new) — create_egui_editor; ParamSlider widgets + a custom painter-drawn grid (fill = on, red outline = playhead, click = shared.toggle(i)); repaints each frame so the playhead animates.
  • src/params.rs — persisted EguiState (600×640).
  • Cargo — pin both nih_plug and nih_plug_egui to the same rev f36931f7; leaving the top-level nih_plug unpinned pulled a second newer copy and broke the Params derive.

Trade-offs

  • Step toggles are runtime overrides, not params: not preset-saved, not automatable, and a latent move regenerates over them (intended generative semantics). Promoting to 16 bool params is a possible later pass.

Verification

  • cargo test — 16 pass (existing sequencer/decoder + new SharedState bit tests)
  • cargo build --release + cargo xtask bundle deepsteps-plugin --release — clean
  • clap-validator validate — 18 passed / 0 failed / 3 skipped (unchanged)
  • Manual GUI render (Carla) — not yet done, pending reviewer check

Design doc: docs/plans/2026-06-14-stage3-egui-editor-design.md

Adds a custom nih_plug_egui editor restoring the Stage-1 UX: a 16-step grid
with a live playhead and click-to-toggle cells, plus sliders for the latent
vector, per-step pitches, timing, and tuning. Training UI from the original is
dropped (no runtime training; regeneration is reactive on latent change).

- src/shared.rs: lock-free SharedState (AtomicU16 step mask + AtomicUsize
  playhead) bridging audio thread and editor.
- src/lib.rs: maybe_regen writes the decoded pattern into shared.steps (now
  the playback source of truth); process reads on/off from the mask, publishes
  the playhead, and clears it when stopped; editor() wired up.
- src/params.rs: persisted EguiState (600x640).
- Pin nih_plug + nih_plug_egui to the same rev f36931f7; an unpinned top-level
  nih_plug pulled a second copy and broke the Params derive.

cargo test 16 pass; clap-validator 18 passed/0 failed/3 skipped.
@gustavokch
gustavokch merged commit c42a1ff into main Jun 14, 2026
1 check failed
@gustavokch

Copy link
Copy Markdown
Owner Author

Code Review — PR #3 (feat: egui editor with live step grid)

Reviewed post-merge at the current main state. Overall: clean, well-documented work. The lock-free audio↔GUI bridge is correct and the process loop ordering is sound. No correctness bugs that hang notes or crash. Findings below are polish + stale-scaffolding, not blockers.

Overview

Adds a nih_plug_egui editor: 16-step grid with live playhead + click-to-toggle, sliders for latent/timing/tuning/pitches. Step on/off mask moves from self.steps into a lock-free Arc<SharedState> (AtomicU16 mask + AtomicUsize playhead), shared between the audio thread and editor. nih_plug/nih_plug_egui pinned to the same rev.

What's good

  • SharedState design is correct: Relaxed is justified (no cross-variable invariant; toggle is an atomic RMW), well-commented, and unit-tested (mask_roundtrip_and_toggle, pack_matches_bits).
  • Process loop drains pending NoteOffs before stepping, so a same-block NoteOff can't precede its NoteOn — and clamps the NoteOn offset into [0, nframes-1]. Good.
  • Cargo rev-pin rationale (avoiding a second nih_plug copy breaking the Params derive) documented in the PR.

Issues

Medium — stale scaffolding in src/params.rs now masks real dead code.

  • Module doc (lines 4-6): "The struct is not wired into a plugin yet (Task 13/14); dead_code is therefore expected until then." It is wired now (lib.rs builds editor::create(self.params.clone(), …)).
  • #![allow(dead_code)] (line 6) is therefore no longer scoped to scaffolding — it will silently hide genuinely dead params/fields from here on. Drop it (or narrow to specific items) and let the compiler talk.
  • _ParamsArc marker (lines 165-168) and the "silence the unused-Arc import" comment are obsolete — Arc<EguiState> is a live field. Remove.

Low/Medium — editor.rs repaints unconditionally.
ctx.request_repaint() runs every frame regardless of transport state, so the editor renders at full framerate (GPU/CPU spin) even when the transport is stopped and the playhead isn't moving. Gate it: only request a repaint while shared.current() != NO_STEP (transport playing), otherwise let egui idle.

Low — stale/misleading latent-range comment (params.rs:129-135).
Comment says the Stage-1 sliders were -10..10/default 0 and "Task 13 will rescale as needed." Verified the shipped reference_vectors.json latents are all in 0.028..0.981, so the chosen 0.0..1.0/default 0.5 range is correct for this decoder — no rescale is pending. Reword so a future reader doesn't think there's an open normalization task.

Low — SharedState::get/toggle are pub with no bound guard.
1 << i on the u16 mask is UB-shift / debug panic for i >= 16. All internal callers pass 0..16, but since these are public add a debug_assert!(i < 16) or document the invariant.

Low — playhead is last-write-wins within a block (lib.rs:210).
If a process block spans multiple steps, set_current(idx) only leaves the last step visible; intermediate steps are never shown. Fine at typical block sizes (≤1 step/block at 16 steps/bar), but worth a comment, or publish the playhead based on wall-clock position rather than the step loop.

Cosmetic — doc drift.

  • PR body says editor is 600×640; code is 600×520 (params.rs EDITOR_HEIGHT, per commit c42a1ff). The params.rs:12 "matching the Stage-1 openFrameworks canvas" comment no longer matches either.
  • editor.rs:73 comment says "4 columns" while the grid is num_columns(8) (4 label+slider pairs). Reword to "4 step columns".

Test coverage

shared.rs and decoder.rs covered; the egui editor is untested (expected — GUI). The PR notes manual Carla render is not yet done — that's the real gap. A headless render/snapshot check or at minimum a manual Carla pass should confirm the grid + playhead actually draw before this is called done.

gustavokch added a commit that referenced this pull request Jun 14, 2026
gustavokch added a commit that referenced this pull request Jun 14, 2026
gustavokch added a commit that referenced this pull request Jun 14, 2026
… note

Loaded the VST3 in carla-single on a live X display: grid, panels and param
values render correctly (screenshot in docs/img/). Logs a new observation — the
Carla host window opens far larger than the 600x520 EguiState, egui content
top-left, rest black; the VST3 wrapper isn't constraining the host window to the
editor size. Interactive click/playhead and a headless snapshot test remain open.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant