Skip to content

feat: add optional animate mode for deep Mandelbrot zoom#14

Open
dustycyanide wants to merge 2 commits intoPottierLoic:mainfrom
dustycyanide:feat/animate-mode
Open

feat: add optional animate mode for deep Mandelbrot zoom#14
dustycyanide wants to merge 2 commits intoPottierLoic:mainfrom
dustycyanide:feat/animate-mode

Conversation

@dustycyanide
Copy link
Copy Markdown

Summary

  • add a new optional --animate mode that keeps the original fractouille default behavior unchanged while enabling continuous Mandelbrot zoom animation on demand
  • implement runtime animation controls in the app loop (adaptive iterations, scale ceiling reset, deep-point cycling) with live pause/resume via p
  • document old vs new run commands and expose animation tuning flags (--animate-fps, --animate-zoom-factor, --animate-scale-ceiling, --animate-no-cycle)

Prompt

Please implement an optional terminal animation mode for Fractouille that preserves existing default behavior (fractouille stays manual), and enables a new deep Mandelbrot zoom experience only when requested by CLI flag. The mode should continuously zoom, increase iteration depth as zoom deepens, recover/reset gracefully when floating-point depth is exhausted, and optionally cycle curated deep points to keep visuals interesting. Include user-facing controls/help text and documentation for comparing old/manual mode and new animation mode.

AI Attribution

This change was written by AI using openai/gpt-5.3-codex in OpenCode.

Verification

  • cargo build
  • fractouille --help

@PottierLoic
Copy link
Copy Markdown
Owner

Hey, thanks for the PR, the animation idea is cool and I like the direction overall.

That said, I'm not convinced the extra CLI arguments are the right approach here. Animation feels more like a runtime feature than something that should be configured at startup.

I'd rather keep it simple:

  • keep the p keybind to toggle animation
  • add a command like animate on|off in command mode
  • move all tuning options to in-app commands instead of CLI flags
    (for example :animate_fps 30 or :animate_speed 0.693 etc.)

This would make it possible to tweak the animation parameters at runtime, it would be more natural than restarting the app with different flags.

I'm also not a big fan of hardcoding zoom coordinates. I'd prefer to simply let the user decide to use the :reset command instead of automatically jump to predefined points when a threshold is reached.
More generally, I'm not sure the automatic threshold reset is necessary.

@dustycyanide
Copy link
Copy Markdown
Author

thanks for the review! Just did another pass to move the animation control into the runtime commands and separate the concerns by module.

  • Moved animation behavior out of App into src/animation/mod.rs with a focused AnimationState (timing + zoom step only).
  • removed animation feature logic from src/app.rs.
  • Kept main thin (by removing all animation CLI flags and startup animation setup
  • Switched control to runtime commands in command mode:
    • animate <on|off>
    • animate_fps <value>
    • animate_speed <value>
  • Removed hard coded deep zoom coordinate cycling and threshold reset behavior entirely
  • Updated docs to match runtime-control model:
    • removed --animate* usage examples from README.md
    • documented animation commands in COMMANDS.md.

Validation:

  • cargo test passes
  • Manual checks for animate on/off, animate_fps, and animate_speed behave as expected

@dustycyanide dustycyanide requested a review from PottierLoic March 6, 2026 14:41
@PottierLoic
Copy link
Copy Markdown
Owner

Hey, thanks for the changes !
Sorry for the delay, I have some personal things to deal with at the moment.
I'll review everything ASAP.

Copy link
Copy Markdown
Owner

@PottierLoic PottierLoic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the updates and sorry for the wait, I was quite busy on some personal things recently.

The feature is much cleaner now and I think we just need to clean a few things before merging it.

The main thing is the removal of the FPS system entirely. Fractal rendering involves heavy computations, and as you probably noticed, the time needed to compute a frame is increasing as we zoom deeper into it. This computation duration will quickly exceed any target FPS interval, making the FPS control meaningless in practice.

The 60fps poll loop in handle_input was already in place, we don't need a second timing layer on top of it.

As a consequence, tick() can be removed too. The zoom can be applied directly in handle_input.

if self.animation.enabled {
    f.scale *= self.animation.speed;
    self.fractal_view.need_render = true;
}

The rest of the comments are mainly to mark where fps related parts are so we don't miss any, once this is cleaned up across all files, this is good to go.

Comment on lines +41 to +48
pub fn set_fps(&mut self, fps: f64) -> Result<(), String> {
if fps <= 0.0 {
return Err("Animation FPS must be positive".to_string());
}
self.fps = fps;
self.last_tick = None;
Ok(())
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alongside with the fps removal, this should go.

Comment on lines +24 to +30
pub fn tick_interval(&self) -> Duration {
if self.enabled {
Duration::from_secs_f64(1.0 / self.fps.max(0.1))
} else {
Duration::from_secs_f64(1.0 / 60.0)
}
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alongside with the fps removal, this should go.

#[derive(Debug, Clone)]
pub struct AnimationState {
pub enabled: bool,
pub fps: f64,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field and all fps related features should be removed. Please read the global review for the reasons.

pub enabled: bool,
pub fps: f64,
pub speed: f64,
last_tick: Option<Instant>,
Copy link
Copy Markdown
Owner

@PottierLoic PottierLoic Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed. (edit: on fps and last_tick are concerned, I missclicked)

RecordJulia(u32, u32, f64, f64, f64, f64),
CycleSpeed(f64),
Animate(bool),
AnimateFps(f64),
Copy link
Copy Markdown
Owner

@PottierLoic PottierLoic Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. (edit: line 21 is no concerned obviously, I missclicked)

Comment on lines +314 to +327
pub fn parse_animate_fps(parts: &[&str]) -> Command {
if parts.len() != 2 {
return Command::Unknown(format!(
"Usage: animate_fps <value>. Got {} arguments",
parts.len() - 1
));
}

match parts[1].parse::<f64>() {
Ok(v) if v > 0.0 => Command::AnimateFps(v),
Ok(_) => Command::Unknown("Animation FPS must be positive".to_string()),
Err(_) => Command::Unknown(format!("Invalid animation FPS: {}", parts[1])),
}
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment on lines -10 to +8
let timeout = Duration::from_secs_f32(1.0 / 60.0);
let timeout = self.animation.tick_interval();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go back to it's original state as we are removing tick_interval.

- `animate <on|off>`
Enables or disables continuous zoom animation.

- `animate_fps <value>`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment on lines +58 to +76
pub fn tick(&mut self, fractal: &mut Fractal) -> bool {
if !self.enabled {
return false;
}

let now = Instant::now();
let interval = self.tick_interval();

if let Some(last_tick) = self.last_tick {
if now.duration_since(last_tick) < interval {
return false;
}
}

self.last_tick = Some(now);
fractal.scale *= self.speed;
true
}
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we are removing the fps, tick is no longer needed, we will do the zoom in input.rs

Comment on lines +80 to +82
if self.animation.tick(&mut self.fractal) {
self.fractal_view.need_render = true;
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zoom can be done here directly now.

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.

2 participants