Integrate "color" into "explore" GUI - #172
Merged
Merged
Conversation
Phase 4, commit 1 of 4 (GUI unification roadmap §5). Pure module reorg with no behavior change — sets up the home for the live color editor in later commits. - Move ExploreApp/explore from src/core/user_interface.rs into a new src/core/interactive/ module (mod.rs + app.rs); rename ExploreApp to FractalApp. - Delete src/core/user_interface.rs; swap the module decl in core/mod.rs. - Retarget the dispatch sites in cli/explore.rs and newtons_method.rs from user_interface::explore to interactive::explore. All CI gates green; pixel-hash regression tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 4, commit 2 of 4 (GUI unification roadmap §5). Adds the plumbing for live color editing; inert until the editor panel is wired in the next commit (nothing sets the dirty flag yet besides R-reset). - RenderingPipeline::recolorize_only — skips compute/histogram (steps a/b), rebuilds CDFs (identically, from the retained histograms) + LUTs + background, then re-walks the existing field. Two unit tests: byte- identical to a full render when the palette is unchanged, and matches a full re-render after a keyframe edit. - PixelGrid gains: a separate Arc<Mutex<ColorPalette>> as the editor's source of truth (synced into the fractal at the start of each render / recolorize), a color_dirty flag, a recolorize() background task, a last_sampling_level so recolorize walks the same populated field cells, and initial_color_palette so reset() (R key) restores the initial colors. - update() spawns a recolorize when color_dirty is set and no full render is launched this tick (view changes take priority, since they regenerate the field a recolorize would re-walk). Design note: the editor's palette lives in its own lightweight mutex rather than being edited through the pipeline mutex directly (as the roadmap text suggests). The background render holds the pipeline mutex for the entire compute, so locking it on the UI thread would freeze the editor during long renders. The separate mutex keeps the UI responsive; the fractal's embedded palette stays authoritative for serialization, refreshed each render. All CI gates green; pixel-hash regression tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 4, final commit (GUI unification roadmap §5). Adds the right-side color editor and wires edits to the preview live. Combines the roadmap's planned "editor widget", "live-sync", and "key remap" commits: the pure edit helpers are public API the key handlers call, so splitting them across a commit boundary would have required temporary #[allow(dead_code)]. - New src/core/interactive/editor.rs: show_palette_editor mutates a ColorPalette in place and returns whether anything changed. Background swatch, per-color-map tabs (suppressed when there is a single map; "Root N" otherwise, switching resets selection), vertical keyframe color cells with `+` insert buttons and segment-fraction drag values, a read-only gradient bar (paint_gradient_bar lifted from the demo editor), and an inline color picker bound to the selected keyframe. - Pure edit helpers with unit tests: set_segment_fraction (proportional rescale + MIN_FRACTION clamp + position rebuild), insert_midpoint (interpolated color at the segment midpoint), delete_keyframe (anchors protected). - app.rs: FractalApp gains EditorState; layout is now Panel::right (editor) + CentralPanel (preview), both BLACK / no separator per §4.1, panel resizable via size_range per §4.3. On any palette change the preview is marked color-dirty and repainted. - Key remaps (§6.3): Esc clears the keyframe selection (no longer quits); Delete removes the selected keyframe; R resets view AND palette; Q / Ctrl+C quit. All CI gates green; pixel-hash regression tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update both status tables and annotate the Phase 4 section with the two intentional deviations (separate palette mutex; merged commits). Phase 5 is now the next step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR delivers Phase 4 of the GUI unification roadmap by shipping a live color-palette editor integrated into the interactive fractal explorer. The editor enables fast re-coloring of cached fractal fields without recomputation, with color-dirty task scheduling, keyframe manipulation (insert, delete, adjust fractions), and right-side panel UI integration. ChangesPhase 4: Interactive Color Editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the interactive “explore” GUI by moving it under a unified core::interactive module and adding a live color-palette editor panel. It also introduces a fast “recolorize only” render path so color edits can update the preview without recomputing the fractal field.
Changes:
- Replace
user_interface::explorewithinteractive::exploreand add the newsrc/core/interactive/module. - Add a live palette editor UI (keyframes/background) and wire it into the interactive app, including key handling for selection/delete/reset.
- Add a color-only pipeline path (
RenderingPipeline::recolorize_only) andPixelGridplumbing (color_dirty, palette mutex,last_sampling_level) to enable fast color updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/fractals/newtons_method.rs | Switch Newton explorer entrypoint to core::interactive::explore. |
| src/core/render_window.rs | Add palette state + dirty flag + recolorize task support in PixelGrid. |
| src/core/render_pipeline.rs | Add recolorize_only fast-path plus unit tests for equivalence vs full render. |
| src/core/mod.rs | Export interactive module and remove user_interface from core. |
| src/core/interactive/mod.rs | New interactive module root; re-export explore. |
| src/core/interactive/editor.rs | New palette editor widget + keyframe editing helpers + tests. |
| src/core/interactive/app.rs | Wire editor panel into explorer app; add keybindings; route color edits to recolorize. |
| src/core/image_utils.rs | Clarify intent of color_palette_mut for interactive sync. |
| src/cli/explore.rs | Switch CLI explorer entrypoint to core::interactive::explore. |
| docs/gui-unification-roadmap.md | Update roadmap to mark Phase 4 shipped and document deviations. |
Comments suppressed due to low confidence (1)
src/core/interactive/app.rs:261
Ris handled withkey_down, so holding the key will repeatedly callreset()every frame. That can continuously re-trigger color-dirty and/or full renders unnecessarily; this should be edge-triggered like other one-shot actions (e.g. Space).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review follow-ups: - set_segment_fraction: fix a broken invariant. The old code clamped each rescaled segment up to MIN_FRACTION and then divided everything by the total, which could push a just-floored segment back below MIN_FRACTION. Rebuild so the fractions sum to exactly 1.0 by construction — give each other segment a MIN_FRACTION floor, then distribute only the surplus in proportion to the old widths. No normalization pass, so the floor holds. Add a regression test with highly unequal segments. - recolorize_only: add the same defensive debug-asserts render() has (field outer dim and out.size vs the fractal spec) to catch stale-buffer call sites (e.g. after a resize) early. - PixelGrid::update: clear color_dirty when a full render is launched. The full render already clones the current palette into the fractal, so a pending color edit is satisfied; clearing avoids a redundant recolorize firing the instant that render completes. - render_window: drop the stale "wired in the next commit" notes and #[allow(dead_code)] on palette() / mark_color_dirty() — both are used. - app: edge-trigger the R reset (key_pressed, not key_down) so holding the key resets once instead of re-cloning the palette and re-marking dirty every frame. reset() forces a full render via the regulator, so the view still refreshes correctly. All CI gates green; pixel-hash regression tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/interactive/mod.rs (1)
7-10: ⚡ Quick winAdd item docs for the exported module surface.
These new public items are undocumented right now. Please add
///comments toapp,editor, and theexplorere-export so this module stays guideline-compliant.As per coding guidelines, "Every
pubitem (structs, enums, traits, functions, type aliases, and theirpubfields/methods) must have a///doc comment".🤖 Prompt for 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. In `@src/core/interactive/mod.rs` around lines 7 - 10, Add missing doc comments for the public module surface: add a short `///` doc comment above `pub mod app;` describing the app submodule's purpose, another `///` comment above `pub mod editor;` describing the editor submodule, and a `///` comment above the `pub use app::explore;` re-export explaining what `explore` provides to users; ensure each comment is a concise sentence following crate docs style so all `pub` items (`app`, `editor`, `explore`) have `///` documentation.
🤖 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 `@src/core/render_window.rs`:
- Around line 100-109: render_to_file() currently serializes only
self.image_specification(), so exported snapshots don't include live edits in
the editor-held palette (palette: Arc<Mutex<ColorPalette>>); update
render_to_file() (or the serialization path it calls) to include the current
palette by reading the Arc<Mutex<ColorPalette>> lock and embedding that
ColorPalette into the serialized fractal params (or call a helper that syncs the
locked palette into the struct used by image_specification() before
serializing). Ensure reset still restores initial_color_palette as before and
that image_specification() uses the synced palette so PNG + JSON remain
consistent.
---
Nitpick comments:
In `@src/core/interactive/mod.rs`:
- Around line 7-10: Add missing doc comments for the public module surface: add
a short `///` doc comment above `pub mod app;` describing the app submodule's
purpose, another `///` comment above `pub mod editor;` describing the editor
submodule, and a `///` comment above the `pub use app::explore;` re-export
explaining what `explore` provides to users; ensure each comment is a concise
sentence following crate docs style so all `pub` items (`app`, `editor`,
`explore`) have `///` documentation.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: df0c18e6-5853-4370-96b0-1f3d47c9803b
📒 Files selected for processing (10)
docs/gui-unification-roadmap.mdsrc/cli/explore.rssrc/core/image_utils.rssrc/core/interactive/app.rssrc/core/interactive/editor.rssrc/core/interactive/mod.rssrc/core/mod.rssrc/core/render_pipeline.rssrc/core/render_window.rssrc/fractals/newtons_method.rs
Comment on lines
+100
to
+109
| // Editor's source-of-truth color palette, held in its own lightweight | ||
| // mutex so the UI thread can read/mutate it every frame without ever | ||
| // locking the (long-held) pipeline mutex. Synced into the fractal at the | ||
| // start of each render / recolorize. See the Phase-4 roadmap deviation | ||
| // note: this avoids freezing the editor during a long render. | ||
| palette: Arc<Mutex<ColorPalette>>, | ||
|
|
||
| // The palette the fractal was constructed with; restored by `reset` so | ||
| // `R` returns to the initial colors as well as the initial view. | ||
| initial_color_palette: ColorPalette, |
There was a problem hiding this comment.
Snapshot exports no longer capture live palette edits.
palette is now the editor's source of truth, but render_to_file() still serializes only self.image_specification(). After any keyframe/background change, the saved PNG reflects the edited colors while the sibling JSON cannot reproduce them. Please export the current palette together with the current viewport/render params, or sync the shared palette back into the serialized fractal params before writing snapshots.
Also applies to: 156-159
🤖 Prompt for 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.
In `@src/core/render_window.rs` around lines 100 - 109, render_to_file() currently
serializes only self.image_specification(), so exported snapshots don't include
live edits in the editor-held palette (palette: Arc<Mutex<ColorPalette>>);
update render_to_file() (or the serialization path it calls) to include the
current palette by reading the Arc<Mutex<ColorPalette>> lock and embedding that
ColorPalette into the serialized fractal params (or call a helper that syncs the
locked palette into the struct used by image_specification() before
serializing). Ensure reset still restores initial_color_palette as before and
that image_specification() uses the synced palette so PNG + JSON remain
consistent.
Owner
Author
There was a problem hiding this comment.
That will be the topic of the next PR, so I'm going to leave that as a follow-up.
There was a problem hiding this comment.
@MatthewPeterKelly — understood, makes sense to keep that as a focused follow-up. Would you like me to open a GitHub issue to track this? I can file it with a summary of the problem (exported PNG reflects live palette edits but the sibling JSON doesn't include the current palette state) so it doesn't get lost between PRs.
(ᵔ ᴥ ᵔ)
🧠 Learnings used
Learnt from: MatthewPeterKelly
Repo: MatthewPeterKelly/fractal-renderer PR: 154
File: examples/render-newton-cosh-minus-one/params.json:17-27
Timestamp: 2026-01-21T23:44:22.592Z
Learning: In Newton method fractals for this repository, a single color map in FullColorSpec can be intentionally used to color the fractal based on convergence speed rather than which specific root/attractor each point converges to, producing a different style of visualization.
Learnt from: MatthewPeterKelly
Repo: MatthewPeterKelly/fractal-renderer PR: 154
File: examples/render-newton-cosh-minus-one/main.rs:4-7
Timestamp: 2026-01-09T23:01:01.712Z
Learning: In the fractal-renderer repo, the cargo alias rex is defined in .cargo/config.toml as a cargo alias expanding to 'run --release --example'. Use 'cargo rex' to run Rust example binaries (in release mode) across all Rust source files (i.e., for any example under the repository).
Owner
Author
|
Code LGTM from a quick human review pass. Some minor improvements to be had in the GUI support code, but we'll tune those up in follow-up PRs to polish the GUI from a usability standpoint. Rendering pipeline details will be polished in the next PR along with the implementation of the correct "save" implementation. |
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.
Upgrade the "explore" GUI to include the full interactive support for editing the color map live. Here is what the new GUI looks like:
I could use a bit of polish, but it works!.
Keyframes can be inserted between existing frames with the "+" button; click a keyframe color to pull up the HSV color editor. Adjust the relative spacing with the spin boxes between each keyframe. Explore mode interaction and dynamic rendering quality still work. Color updates are super fast because they reuse a cached scalar field, rather than recomputing the fractal.