Prototype: Interactive color rendering#159
Closed
MatthewPeterKelly wants to merge 4 commits into
Closed
Conversation
Nice improvements. Lets tweak some more: -- arrow selectionand clicking on the points now works great -- adding keyframes works -- color editing is still not quite right - it is laggy and hard to select. It isn't clear where to click to grab a handle. -- The image is too small - you're dropping the resolution, but there is a better way - change the downsample stride, which keeps the resolution the same but renders a subset of the pixels. (1) make the color UI wider (2) Use a fixed offset for each keyframe, rather than dynamically adjusting. For example, each keyframe is given an offset of 1.1*outer_circle_width. This way, each of the handles "has its own horizontal track". (3) Replace the 2D color picker with three HSV bars and three RGB bars (two columns). At the end of each bar it should show the value (0-255), both both types. If the user interacts with RGB, then it sets HSV. If the user interacts with HSV, then it sets RGB. Also allow the user to directly type in the RGB or HSV values into the box. (4) At the end of each "keyframe slider row", show the query value (0,1). Allow the user to manually type in a value (5) Have two explicit modes: color interaction OR render. If color interaction, then the color editor should be highly responsive, rapidly updating all elements and showing color previews. Then, when the user presses enter, it should temporarily lock the gui while rendering happens. When the GUI is locked, it should show some subtle visual indicator. (6) spacebar saves the parameters
📝 WalkthroughWalkthroughA new interactive color map editor feature is added to the CLI, allowing users to edit fractal color map keyframes through a dedicated UI. The implementation includes command dispatch, UI state management with real-time rendering preview, keyboard/mouse event handling, and persistent parameter saving. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Dispatcher
participant Editor as Color Map Editor
participant UI as Editor UI
participant Renderer as Fractal Renderer
participant Storage as File System
User->>CLI: Run color-map-editor command
CLI->>Storage: Load params.json
Storage-->>CLI: FractalParams
CLI->>Editor: edit_color_map(params, path)
Editor->>UI: Launch editor window
loop User Interaction
User->>UI: Adjust keyframes/HSV/RGB sliders
UI->>Renderer: Request preview render
Renderer-->>UI: Rendered preview pixels
UI->>UI: Update display
end
User->>UI: Press Enter or close window
UI-->>Editor: Return edited keyframes
Editor->>Editor: Update in-memory params
Editor->>Storage: Persist updated params (JSON)
Storage-->>Editor: Write complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/render-mandelbrot/params.json`:
- Around line 61-68: Add a trailing newline at the end of params.json and reduce
the example's histogram_sample_count to a much lower default to improve initial
render speed (e.g., 10_000–100_000); update the JSON value for the
"histogram_sample_count" key under the top-level settings and keep the rest of
the render_options ("downsample_stride", "subpixel_antialiasing") unchanged.
In `@src/cli/color_map_editor.rs`:
- Around line 5-11: The import block in src/cli/color_map_editor.rs is
misformatted for rustfmt; adjust the use statement formatting (the block that
references crate::core::color_map_editor_ui and
crate::fractals::{common::FractalParams, quadratic_map::QuadraticMap}) by
running cargo fmt to automatically reformat the imports so they match rustfmt
expectations and satisfy the CI pipeline.
- Around line 18-40: Extract the duplicated logic in the
FractalParams::Mandelbrot and FractalParams::Julia match arms into a small
helper that accepts the inner params and a constructor for the enclosing
FractalParams variant; inside the helper create the renderer with
QuadraticMap::new(*inner.clone()), clone saved_params and params_path, then call
color_map_editor_ui::edit with a closure that updates
saved_params.color_map.keyframes, wraps into the provided constructor (e.g.,
make_full(updated) -> FractalParams), and calls write_params(&path, &full);
replace both match arms to call this helper with the appropriate inner and a
closure that builds FractalParams::Mandelbrot(Box::new(...)) or
FractalParams::Julia(Box::new(...)).
- Around line 52-55: The current write_params function uses expect() which will
panic on serialization or file-write failure; change write_params to return a
Result<(), Box<dyn std::error::Error>> (or a suitable error type) instead of
panicking, replace serde_json::to_string_pretty(...).expect(...) and
std::fs::write(...).expect(...) with ?-based error propagation, and update
callers (e.g., the save callback) to handle the Result by logging the error via
eprintln! or a logger and returning gracefully instead of crashing; references:
function write_params and type FractalParams.
- Around line 42-48: The match arm in color_map_editor should not call panic!()
and should report the specific FractalParams variant passed; modify the
unsupported branch to construct and return a Result::Err (or propagate a custom
error type) containing a message that includes the actual variant via
format!("{:?}", params) or by matching/extracting the variant name instead of
using type_name::<FractalParams>(), e.g., return Err(...) from the function (or
call std::process::exit(1) if the CLI must terminate) so callers can handle the
error gracefully; update the color_map_editor signature to return a Result if
needed and ensure callers propagate or handle the returned error.
In `@src/core/color_map_editor_ui.rs`:
- Around line 826-841: Replace the direct unwraps on the mutex locks inside
spawn_render with match handling for poisoned locks: when calling buffer.lock()
and renderer.lock(), match on the Result and recover from poisoning by using
poisoned.into_inner() so the background thread won't panic on a poisoned mutex;
keep the same call to renderer.lock().render_to_buffer(&mut buf) (i.e., call
Renderable::render_to_buffer on the recovered guard), and preserve the
busy.store(false, Ordering::Release) and ready.store(true, Ordering::Release)
semantics.
- Around line 1-14: This file has formatting violations; run `cargo fmt --all`
to auto-fix spacing, line breaks and alignment across the file (the file
contains imports and symbols like Pixels, SurfaceTexture, AtomicBool, Instant,
EventLoop and WindowBuilder which you can use to locate the module), then re-run
CI to verify; if any editor or rustfmt config is forcing different style, ensure
rustfmt is installed and consistent with the repo config before committing the
formatted changes.
- Around line 737-744: Clippy flags the long inline type for the local `bars`
slice; define short type aliases to simplify it: add `type GradientFn = Box<dyn
Fn(f32) -> [u8;3]>; type ThumbFn = Box<dyn Fn() -> [u8;3]>; type BarSpec =
(SlotId, char, GradientFn, ThumbFn, f32);` and then change the declaration `let
bars: &[(SlotId, char, Box<dyn Fn(f32)->[u8;3]>, Box<dyn Fn()->[u8;3]>, f32)] =
&[ ... ];` to `let bars: &[BarSpec] = &[ ... ];` so the code in
`color_map_editor_ui.rs` that builds `bars` (the closures for H,S,V,R,G,B) keeps
the same behavior but uses the alias to satisfy Clippy.
- Around line 650-668: Clippy warns that text_box has too many parameters;
refactor by introducing a TextBoxParams struct (fields: x: u32, y: u32, w: u32,
h: u32, active: bool, cursor: bool) and change the method signature to
text_box(&mut self, text: &str, params: TextBoxParams); update the body to read
values from params (e.g., params.x, params.y, etc.), and update all callers to
construct and pass TextBoxParams (optionally derive Copy/Clone or implement
Default if helpful) so the function no longer exceeds the 7-parameter threshold.
- Around line 105-114: The rgb_to_hsv function uses exact float equality (max ==
r / g) to pick the hue branch which can mis-select when channels are nearly
equal; replace those equality checks with epsilon comparisons (e.g., (max -
r).abs() < EPS) using a small constant EPS (like 1e-6) and test (max - g).abs()
< EPS for the green branch, otherwise treat blue as the fallback, keeping the
existing d < EPS early-return for grayscale; update the branch conditions
referencing variables max, r, g, b and keep the hue formulas unchanged.
- Around line 966-969: The code currently calls fractal_pixels.render().ok() and
editor_pixels.render().ok(), silently dropping errors; replace these .ok()
usages with explicit error handling (e.g., if let Err(e) =
fractal_pixels.render() { log the error } and same for editor_pixels.render())
so render failures are surfaced; update the branches where
fractal_pixels.render() and editor_pixels.render() are invoked (the block using
draw_editor(..., &state) and the fractal drawing branch) to log or propagate the
error instead of ignoring it.
- Around line 723-727: Replace the map_or(false, ...) pattern with
is_some_and(...) to satisfy the Clippy lint: change the q_active assignment that
currently uses state.active_input.as_ref().map_or(false, |inp| inp.field ==
FieldId::Query(i)) to use state.active_input.as_ref().is_some_and(|inp|
inp.field == FieldId::Query(i)); update the corresponding display computation
that relies on q_active accordingly, and apply the exact same replacement for
the other occurrence of map_or(false, ...) elsewhere in this file (the second
usage that checks FieldId::Query) so both sites use is_some_and.
- Around line 558-561: Remove the unused method selected_rgb from the impl block
(the fn selected_rgb(&self) -> Option<[u8; 3]>) to eliminate the dead-code
warning; locate the method that maps self.selected to self.keyframes[s].rgb_raw
and delete that entire function definition, leaving the rest of the impl (fields
like selected and keyframes) unchanged.
- Around line 998-1012: Hold the renderer mutex once instead of locking twice:
obtain a single guard via renderer.lock().unwrap(), call
guard.set_keyframes(state.keyframes.clone()) and
guard.set_speed_optimization_level(level, &speed_cache) while the guard is held,
then drop the guard (end the scope) before calling
spawn_render(renderer.clone(), ...). Update the code around set_keyframes,
set_speed_optimization_level, and spawn_render so the lock is consolidated and
released prior to spawn_render to avoid redundant locks and potential deadlocks;
keep the existing uses of state.keyframes_dirty, state.mode, and
state.render_start unchanged.
- Line 809: Remove the dead-code artifact: either use the ORANGE constant where
the rendering indicator color is set (replace the pulsing color usage with
ORANGE so the visual uses the static ORANGE constant), or if you want to keep
the pulsing behavior delete the ORANGE constant declaration and remove the
orphaned let _ = ORANGE; line; ensure references to ORANGE are eliminated if you
choose deletion so no unused-constant warnings remain.
- Around line 525-546: The add_keyframe function can panic when
self.keyframes.len() < 2 due to computing self.keyframes.len()-1 and indexing
keyframes[ins] / keyframes[ins-1]; add an early guard at the start of
add_keyframe that returns immediately if self.keyframes.len() < 2 (or otherwise
ensures at least two keyframes exist) to avoid underflow and out-of-bounds
access, keeping the rest of the logic (ins, lo, hi calculation, insertion,
set_selected, keyframes_dirty) unchanged.
- Around line 186-210: The EditorState::new initializer unconditionally sets
selected: Some(0) which is invalid when keyframes is empty; compute a selected
value before moving keyframes (e.g., let selected = if keyframes.is_empty() {
None } else { Some(0) }) and use that variable in the struct init for the
selected field so selected is None for empty keyframes and Some(0) otherwise,
leaving all other fields (hsv, drag, keyframes_dirty, etc.) unchanged.
- Around line 329-333: The code uses Option::map_or(true, ...) on `best` inside
the `wins` calculation (variables: best, wins, sel, self.selected, i, d2, pi,
pd2); refactor to use Option::is_none_or(...) for clarity by replacing the
`best.map_or(true, |(pi, pd2)| { ... })` call with `best.is_none_or(|(pi, pd2)|
{ ... })`, but only do this if the project is compiled with Rust 1.82.0 or newer
because `Option::is_none_or` was stabilized in 1.82.0; if you cannot guarantee
MSRV ≥ 1.82.0, keep `map_or` to maintain compatibility.
In `@src/core/mod.rs`:
- Around line 2-3: Reorder the public module declarations in src/core/mod.rs so
they are alphabetically sorted: move the declaration for pub mod color_map;
above pub mod color_map_editor_ui; (i.e., ensure color_map appears before
color_map_editor_ui) and then run cargo fmt to confirm formatting passes.
In `@src/fractals/quadratic_map.rs`:
- Line 7: The import line for color_map (color_map::{ColorMap, ColorMapEditable,
ColorMapKeyFrame, ColorMapLookUpTable, ColorMapper}) is not formatted for
rustfmt; reformat it into a proper multi-line use block (each item on its own
line or otherwise matching rustfmt style) and then run cargo fmt to apply the
canonical formatting so the file src/fractals/quadratic_map.rs passes CI.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bbdaec8c-bbd2-4759-9073-69b22e005725
📒 Files selected for processing (9)
examples/render-mandelbrot/params.jsonsrc/cli/args.rssrc/cli/color_map_editor.rssrc/cli/mod.rssrc/core/color_map.rssrc/core/color_map_editor_ui.rssrc/core/mod.rssrc/fractals/quadratic_map.rssrc/main.rs
Comment on lines
+61
to
+68
| "histogram_sample_count": 1000000 | ||
| }, | ||
| "render_options": { | ||
| "downsample_stride": 1, | ||
| "subpixel_antialiasing": 0 | ||
| "subpixel_antialiasing": 2 | ||
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding trailing newline and review high sample count.
The file lacks a trailing newline. Additionally, histogram_sample_count of 1,000,000 is quite high for an example file and may result in slow initial renders for users trying the example.
Add trailing newline
}
-}
+}
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "histogram_sample_count": 1000000 | |
| }, | |
| "render_options": { | |
| "downsample_stride": 1, | |
| "subpixel_antialiasing": 0 | |
| "subpixel_antialiasing": 2 | |
| } | |
| } | |
| } | |
| } | |
| "histogram_sample_count": 1000000 | |
| }, | |
| "render_options": { | |
| "downsample_stride": 1, | |
| "subpixel_antialiasing": 2 | |
| } | |
| } | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/render-mandelbrot/params.json` around lines 61 - 68, Add a trailing
newline at the end of params.json and reduce the example's
histogram_sample_count to a much lower default to improve initial render speed
(e.g., 10_000–100_000); update the JSON value for the "histogram_sample_count"
key under the top-level settings and keep the rest of the render_options
("downsample_stride", "subpixel_antialiasing") unchanged.
Comment on lines
+5
to
+11
| use crate::{ | ||
| core::color_map_editor_ui, | ||
| fractals::{ | ||
| common::FractalParams, | ||
| quadratic_map::QuadraticMap, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Fix import formatting to satisfy cargo fmt.
The pipeline failure indicates the import block formatting differs from rustfmt expectations.
Run cargo fmt to auto-fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/color_map_editor.rs` around lines 5 - 11, The import block in
src/cli/color_map_editor.rs is misformatted for rustfmt; adjust the use
statement formatting (the block that references crate::core::color_map_editor_ui
and crate::fractals::{common::FractalParams, quadratic_map::QuadraticMap}) by
running cargo fmt to automatically reformat the imports so they match rustfmt
expectations and satisfy the CI pipeline.
Comment on lines
+18
to
+40
| FractalParams::Mandelbrot(inner) => { | ||
| let renderer = QuadraticMap::new(*inner.clone()); | ||
| let saved_params = inner.clone(); | ||
| let path = params_path.clone(); | ||
| color_map_editor_ui::edit(renderer, move |keyframes| { | ||
| let mut updated = *saved_params.clone(); | ||
| updated.color_map.keyframes = keyframes.to_vec(); | ||
| let full = FractalParams::Mandelbrot(Box::new(updated)); | ||
| write_params(&path, &full); | ||
| }) | ||
| } | ||
|
|
||
| FractalParams::Julia(inner) => { | ||
| let renderer = QuadraticMap::new(*inner.clone()); | ||
| let saved_params = inner.clone(); | ||
| let path = params_path.clone(); | ||
| color_map_editor_ui::edit(renderer, move |keyframes| { | ||
| let mut updated = *saved_params.clone(); | ||
| updated.color_map.keyframes = keyframes.to_vec(); | ||
| let full = FractalParams::Julia(Box::new(updated)); | ||
| write_params(&path, &full); | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the duplicated logic for Mandelbrot and Julia.
Both match arms follow the identical pattern: create renderer, clone params, call edit, update keyframes, wrap in variant, write. This duplication could be reduced with a helper, though it's acceptable for a prototype.
Since both MandelbrotParams and JuliaParams share the QuadraticMapParams trait and have identical color_map field access, a macro or generic helper could reduce duplication if more variants are added later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/color_map_editor.rs` around lines 18 - 40, Extract the duplicated
logic in the FractalParams::Mandelbrot and FractalParams::Julia match arms into
a small helper that accepts the inner params and a constructor for the enclosing
FractalParams variant; inside the helper create the renderer with
QuadraticMap::new(*inner.clone()), clone saved_params and params_path, then call
color_map_editor_ui::edit with a closure that updates
saved_params.color_map.keyframes, wraps into the provided constructor (e.g.,
make_full(updated) -> FractalParams), and calls write_params(&path, &full);
replace both match arms to call this helper with the appropriate inner and a
closure that builds FractalParams::Mandelbrot(Box::new(...)) or
FractalParams::Julia(Box::new(...)).
Comment on lines
+42
to
+48
| _ => { | ||
| eprintln!( | ||
| "ERROR: color_map_editor is not yet supported for `{}`.", | ||
| type_name::<FractalParams>() | ||
| ); | ||
| panic!(); | ||
| } |
There was a problem hiding this comment.
Return an error instead of panicking for unsupported variants.
Using panic!() for unsupported fractal types is harsh and doesn't allow graceful error handling. Also, type_name::<FractalParams>() always prints the enum type name, not the specific variant that was passed.
Proposed fix using a custom error or early return
_ => {
- eprintln!(
- "ERROR: color_map_editor is not yet supported for `{}`.",
- type_name::<FractalParams>()
- );
- panic!();
+ eprintln!("ERROR: color_map_editor is not yet supported for this fractal type.");
+ // Return Ok to avoid crashing; the user has been notified.
+ Ok(())
}Alternatively, if you want the CLI to exit with a non-zero status, consider returning a proper error type or using std::process::exit(1) after the error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/color_map_editor.rs` around lines 42 - 48, The match arm in
color_map_editor should not call panic!() and should report the specific
FractalParams variant passed; modify the unsupported branch to construct and
return a Result::Err (or propagate a custom error type) containing a message
that includes the actual variant via format!("{:?}", params) or by
matching/extracting the variant name instead of using
type_name::<FractalParams>(), e.g., return Err(...) from the function (or call
std::process::exit(1) if the CLI must terminate) so callers can handle the error
gracefully; update the color_map_editor signature to return a Result if needed
and ensure callers propagate or handle the returned error.
Comment on lines
+52
to
+55
| fn write_params(path: &str, params: &FractalParams) { | ||
| let json = serde_json::to_string_pretty(params).expect("Failed to serialize params"); | ||
| std::fs::write(path, json).expect("Failed to write params file"); | ||
| eprintln!("Saved color map to {path}"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
expect() calls will panic if serialization or file write fails.
For a save operation, panicking on failure could be unexpected UX. Consider logging the error and returning gracefully, or propagating the error up.
Propagate errors or handle gracefully
-fn write_params(path: &str, params: &FractalParams) {
- let json = serde_json::to_string_pretty(params).expect("Failed to serialize params");
- std::fs::write(path, json).expect("Failed to write params file");
+fn write_params(path: &str, params: &FractalParams) -> Result<(), Box<dyn std::error::Error>> {
+ let json = serde_json::to_string_pretty(params)?;
+ std::fs::write(path, json)?;
eprintln!("Saved color map to {path}");
+ Ok(())
}Then handle the result in the callback (or log and continue).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn write_params(path: &str, params: &FractalParams) { | |
| let json = serde_json::to_string_pretty(params).expect("Failed to serialize params"); | |
| std::fs::write(path, json).expect("Failed to write params file"); | |
| eprintln!("Saved color map to {path}"); | |
| fn write_params(path: &str, params: &FractalParams) -> Result<(), Box<dyn std::error::Error>> { | |
| let json = serde_json::to_string_pretty(params)?; | |
| std::fs::write(path, json)?; | |
| eprintln!("Saved color map to {path}"); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/color_map_editor.rs` around lines 52 - 55, The current write_params
function uses expect() which will panic on serialization or file-write failure;
change write_params to return a Result<(), Box<dyn std::error::Error>> (or a
suitable error type) instead of panicking, replace
serde_json::to_string_pretty(...).expect(...) and
std::fs::write(...).expect(...) with ?-based error propagation, and update
callers (e.g., the save callback) to handle the Result by logging the error via
eprintln! or a logger and returning gracefully instead of crashing; references:
function write_params and type FractalParams.
Comment on lines
+826
to
+841
| /// Attempt to start a background render; returns `false` if busy (no-op). | ||
| fn spawn_render<F: Renderable + Send + 'static>( | ||
| renderer: Arc<Mutex<F>>, | ||
| buffer: Arc<Mutex<Vec<Vec<image::Rgb<u8>>>>>, | ||
| busy: Arc<AtomicBool>, | ||
| ready: Arc<AtomicBool>, | ||
| ) -> bool { | ||
| if busy.swap(true, Ordering::Acquire) { return false; } | ||
| std::thread::spawn(move || { | ||
| let mut buf = buffer.lock().unwrap(); | ||
| renderer.lock().unwrap().render_to_buffer(&mut buf); | ||
| busy.store(false, Ordering::Release); | ||
| ready.store(true, Ordering::Release); | ||
| }); | ||
| true | ||
| } |
There was a problem hiding this comment.
Lock unwraps could panic if a thread panics while holding the lock.
Lines 835-836 use .unwrap() on Mutex::lock(). If the render thread panics while holding either lock, subsequent calls will panic on the poisoned mutex. For a prototype this is acceptable, but production code should handle poisoning:
🛡️ More robust approach
let mut buf = match buffer.lock() {
Ok(guard) => guard,
Err(poisoned) => poisoned.into_inner(), // recover from panic
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/color_map_editor_ui.rs` around lines 826 - 841, Replace the direct
unwraps on the mutex locks inside spawn_render with match handling for poisoned
locks: when calling buffer.lock() and renderer.lock(), match on the Result and
recover from poisoning by using poisoned.into_inner() so the background thread
won't panic on a poisoned mutex; keep the same call to
renderer.lock().render_to_buffer(&mut buf) (i.e., call
Renderable::render_to_buffer on the recovered guard), and preserve the
busy.store(false, Ordering::Release) and ready.store(true, Ordering::Release)
semantics.
Comment on lines
+966
to
+969
| fractal_pixels.render().ok(); | ||
| } else if wid == editor_wid { | ||
| draw_editor(editor_pixels.frame_mut(), &state); | ||
| editor_pixels.render().ok(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent error dropping may hide rendering failures.
The .ok() calls on lines 966, 969 (and 948, 951) silently discard errors. While acceptable for a prototype, logging these would help diagnose issues:
♻️ Example with logging
if let Err(e) = fractal_pixels.render() {
eprintln!("Fractal render error: {e}");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/color_map_editor_ui.rs` around lines 966 - 969, The code currently
calls fractal_pixels.render().ok() and editor_pixels.render().ok(), silently
dropping errors; replace these .ok() usages with explicit error handling (e.g.,
if let Err(e) = fractal_pixels.render() { log the error } and same for
editor_pixels.render()) so render failures are surfaced; update the branches
where fractal_pixels.render() and editor_pixels.render() are invoked (the block
using draw_editor(..., &state) and the fractal drawing branch) to log or
propagate the error instead of ignoring it.
Comment on lines
+998
to
+1012
| if state.keyframes_dirty { | ||
| // Update the renderer's color map. | ||
| renderer.lock().unwrap().set_keyframes(state.keyframes.clone()); | ||
|
|
||
| // Choose quality based on mode. | ||
| let level = if state.mode == Mode::Rendering { FULL_LEVEL } else { FAST_LEVEL }; | ||
| renderer.lock().unwrap().set_speed_optimization_level(level, &speed_cache); | ||
|
|
||
| if spawn_render(renderer.clone(), display_buffer.clone(), | ||
| render_busy.clone(), render_ready.clone()) { | ||
| state.keyframes_dirty = false; | ||
| if state.mode == Mode::Rendering { | ||
| state.render_start = Some(Instant::now()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate lock acquisitions to avoid redundant locking.
Lines 1000 and 1004 each acquire the renderer lock separately. Since both operations occur in the same code path, holding the lock once is more efficient:
♻️ Suggested consolidation
if state.keyframes_dirty {
- // Update the renderer's color map.
- renderer.lock().unwrap().set_keyframes(state.keyframes.clone());
-
- // Choose quality based on mode.
- let level = if state.mode == Mode::Rendering { FULL_LEVEL } else { FAST_LEVEL };
- renderer.lock().unwrap().set_speed_optimization_level(level, &speed_cache);
+ {
+ let mut r = renderer.lock().unwrap();
+ r.set_keyframes(state.keyframes.clone());
+ let level = if state.mode == Mode::Rendering { FULL_LEVEL } else { FAST_LEVEL };
+ r.set_speed_optimization_level(level, &speed_cache);
+ }
if spawn_render(renderer.clone(), display_buffer.clone(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if state.keyframes_dirty { | |
| // Update the renderer's color map. | |
| renderer.lock().unwrap().set_keyframes(state.keyframes.clone()); | |
| // Choose quality based on mode. | |
| let level = if state.mode == Mode::Rendering { FULL_LEVEL } else { FAST_LEVEL }; | |
| renderer.lock().unwrap().set_speed_optimization_level(level, &speed_cache); | |
| if spawn_render(renderer.clone(), display_buffer.clone(), | |
| render_busy.clone(), render_ready.clone()) { | |
| state.keyframes_dirty = false; | |
| if state.mode == Mode::Rendering { | |
| state.render_start = Some(Instant::now()); | |
| } | |
| } | |
| if state.keyframes_dirty { | |
| { | |
| let mut r = renderer.lock().unwrap(); | |
| r.set_keyframes(state.keyframes.clone()); | |
| let level = if state.mode == Mode::Rendering { FULL_LEVEL } else { FAST_LEVEL }; | |
| r.set_speed_optimization_level(level, &speed_cache); | |
| } | |
| if spawn_render(renderer.clone(), display_buffer.clone(), | |
| render_busy.clone(), render_ready.clone()) { | |
| state.keyframes_dirty = false; | |
| if state.mode == Mode::Rendering { | |
| state.render_start = Some(Instant::now()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/color_map_editor_ui.rs` around lines 998 - 1012, Hold the renderer
mutex once instead of locking twice: obtain a single guard via
renderer.lock().unwrap(), call guard.set_keyframes(state.keyframes.clone()) and
guard.set_speed_optimization_level(level, &speed_cache) while the guard is held,
then drop the guard (end the scope) before calling
spawn_render(renderer.clone(), ...). Update the code around set_keyframes,
set_speed_optimization_level, and spawn_render so the lock is consolidated and
released prior to spawn_render to avoid redundant locks and potential deadlocks;
keep the existing uses of state.keyframes_dirty, state.mode, and
state.render_start unchanged.
Comment on lines
+2
to
3
| pub mod color_map_editor_ui; | ||
| pub mod color_map; |
There was a problem hiding this comment.
Fix module ordering to satisfy cargo fmt.
The pipeline failure indicates color_map_editor_ui is not in alphabetical order. It should come after color_map.
Proposed fix
pub mod chaos_game;
-pub mod color_map_editor_ui;
pub mod color_map;
+pub mod color_map_editor_ui;
pub mod controller;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/mod.rs` around lines 2 - 3, Reorder the public module declarations
in src/core/mod.rs so they are alphabetically sorted: move the declaration for
pub mod color_map; above pub mod color_map_editor_ui; (i.e., ensure color_map
appears before color_map_editor_ui) and then run cargo fmt to confirm formatting
passes.
| use crate::{ | ||
| core::{ | ||
| color_map::{ColorMap, ColorMapKeyFrame, ColorMapLookUpTable, ColorMapper}, | ||
| color_map::{ColorMap, ColorMapEditable, ColorMapKeyFrame, ColorMapLookUpTable, ColorMapper}, |
There was a problem hiding this comment.
Fix import formatting to satisfy cargo fmt.
The pipeline failure indicates this import block needs rustfmt multi-line formatting.
Run cargo fmt to auto-fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fractals/quadratic_map.rs` at line 7, The import line for color_map
(color_map::{ColorMap, ColorMapEditable, ColorMapKeyFrame, ColorMapLookUpTable,
ColorMapper}) is not formatted for rustfmt; reformat it into a proper multi-line
use block (each item on its own line or otherwise matching rustfmt style) and
then run cargo fmt to apply the canonical formatting so the file
src/fractals/quadratic_map.rs passes CI.
Owner
Author
|
Neat proof of concept, but code quality is poor. I'm going to close this, and then we'll use this as inspiration to work toward. |
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
1 participant
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: This PR is a draft / prototype demonstrating a working (but totally hacked together / vibe coded) implementation for an interactive color map GUI built on top of the core rendering library.
To run it, use:
Here is what it looks like:
This prototype was written almost entirely by Claude. I'm not planning to merge it directly, Instead I'm going to pull out smaller PRs and review and land those.
Child PRs:
Summary
This PR implements an interactive color map editor GUI for fractal visualization. It adds ~1,100 lines of new UI code plus supporting infrastructure, allowing users to edit color keyframes in real-time while previewing changes on a fractal render.
Architecture & Design Patterns
Thread Model: The implementation uses
Arc<Mutex<T>>with background rendering on a separate thread. The editor runs an event loop with:AtomicBool) for coordination between render thread and UIOrdering::Acquirefor thread-safe state synchronizationUI Rendering: Custom software rasterization via a
Canvas<'a>helper—all drawing is direct pixel manipulation rather than GPU calls. This includes a hardcoded bitmap font (5×7 glyphs), making it deterministic and self-contained.Performance Observations
Concerning Patterns:
Excessive Cloning (🔴 Major issue for a performance-focused developer):
edit_color_map()clones parameters multiple times:inner.clone()(creatingsaved_params), then again in the closure withsaved_params.clone()before every keyframe updaterenderer.lock().unwrap().set_keyframes(state.keyframes.clone())Arc Contention:
Arc<Mutex<QuadraticMap<T>>>with.lock().unwrap()called in hot paths (every frame when rendering).unwrap()will panic if a poisoned mutex occurs (unidiomatic for production)Closure Capture with
move: The save callback capturessaved_params.clone()andpath.clone()separately for Mandelbrot vs Julia branches—repetitive and allocates strings unnecessarilyPositive Perf Choices:
ControlFlow::Wait(no busy-spinning)Canvasdrawing avoids graphics library overheadRust Idioms & Code Quality
Idiomatic Issues:
Error Handling:
The unsupported variant case uses
panic!()instead of returning an error. Better:Err(Box::new(...))Unwrap Chaining: Multiple
.unwrap()calls without context:.build(&event_loop).unwrap().lock().unwrap()serde_json::to_string_pretty(...).expect(...)These should distinguish between "should never fail" (use
expect()with explanation) vs "can fail" (returnResult)Redundant Cloning: In
edit_color_map():Only one clone needed—either clone
inneronce or restructure to avoid itClosure Design: The
save_fncallback receives&[ColorMapKeyFrame](good—borrowed slice), but callers dosave_fn(&state.keyframes)rather thansave_fn(&state.keyframes[..])(minor—both work, but explicit slice syntax is clearer)Positive Idioms:
Arc<Mutex<T>>with proper bounds (T: Send + 'static)Renderable + ColorMapEditablemakes code flexibleOption::map_or()for optional selection checksCode Maintainability
Strengths:
color_map_editor_ui.rsMode::RendervsMode::ColorEdit)Weaknesses:
[u8; 4]) could be extracted to named constantsEditorStatestruct likely tracks many fields—consider documenting invariantsMissing Features/Considerations
expect()inwrite_params())—should returnResultup the chainpanic!())Recommendations for Your Learning
&FractalParamsorArc<FractalParams>to avoid repeated cloneslock().unwrap()calls; considerMutex<Arc<...>>if the inner type is expensive to copypanic!()and.unwrap()with proper error chains (.map_err(),?operator)Clonebounds if cloning is unavoidable, making it explicit at the signature level