Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds a significant new feature: exporting animations as GIF files from the command line. The implementation is well-structured, introducing AnimationParams to handle animation settings and a dedicated export_gif function for the rendering loop. The changes to the CLI arguments are also correctly implemented. I have a few suggestions to improve robustness and clarity, particularly around input validation for animation parameters.
| pub fn new(fps: f64, frames: Option<u32>, duration: Option<f64>) -> Self { | ||
| let frames = match (frames, duration) { | ||
| // Duration takes precedence if both provided | ||
| (_, Some(dur)) => (dur * fps).round() as u32, | ||
| (Some(f), None) => f, | ||
| // Default to 1 frame if neither provided | ||
| (None, None) => 1, | ||
| }; | ||
| Self { fps, frames } | ||
| } |
There was a problem hiding this comment.
The fps argument can be provided by the user and could be zero or negative, which would lead to a panic or incorrect calculations in frame_delay_centiseconds. It's better to validate this input and return a Result to handle the error gracefully. The call site in main.rs will also need to be updated to handle this Result.
pub fn new(fps: f64, frames: Option<u32>, duration: Option<f64>) -> Result<Self, String> {
if fps <= 0.0 {
return Err("FPS must be a positive number.".to_string());
}
let frames = match (frames, duration) {
// Duration takes precedence if both provided
(_, Some(dur)) => (dur * fps).round() as u32,
(Some(f), None) => f,
// Default to 1 frame if neither provided
(None, None) => 1,
};
Ok(Self { fps, frames })
}| pub fn frame_delay_centiseconds(&self) -> u16 { | ||
| ((100.0 / self.fps).round() as u16).max(1) | ||
| } |
There was a problem hiding this comment.
While as casts from float to integer are saturating in recent Rust editions, this behavior can be subtle. Using clamp makes the intention of handling both lower and upper bounds explicit, improving code clarity and robustness.
pub fn frame_delay_centiseconds(&self) -> u16 {
let delay = (100.0 / self.fps).round();
// Clamp to the valid range for GIF frame delay (1 to u16::MAX centiseconds).
delay.clamp(1.0, u16::MAX as f64) as u16
}| export::export_document(&executor, wgpu_executor_ref, output, file_type, scale, (width, height), transparent).await?; | ||
| // Perform export based on file type | ||
| if file_type == export::FileType::Gif { | ||
| let animation = export::AnimationParams::new(fps, frames, duration); |
There was a problem hiding this comment.
To handle the Result now returned by AnimationParams::new (as suggested in another comment), you can use the ? operator to propagate any errors.
| let animation = export::AnimationParams::new(fps, frames, duration); | |
| let animation = export::AnimationParams::new(fps, frames, duration)?; |
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/graphene-cli/src/main.rs">
<violation number="1" location="node-graph/graphene-cli/src/main.rs:80">
P2: The `fps` parameter accepts zero or negative values, which will cause a panic at runtime when rendering GIF frames. `Duration::from_secs_f64(NaN)` (from `0/0`) or `Duration::from_secs_f64(-x)` both panic. Consider adding a `value_parser` constraint or validating `fps > 0` before use.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| /// Frames per second for GIF animation (default: 30) | ||
| #[clap(long, default_value = "30")] | ||
| fps: f64, |
There was a problem hiding this comment.
P2: The fps parameter accepts zero or negative values, which will cause a panic at runtime when rendering GIF frames. Duration::from_secs_f64(NaN) (from 0/0) or Duration::from_secs_f64(-x) both panic. Consider adding a value_parser constraint or validating fps > 0 before use.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graphene-cli/src/main.rs, line 80:
<comment>The `fps` parameter accepts zero or negative values, which will cause a panic at runtime when rendering GIF frames. `Duration::from_secs_f64(NaN)` (from `0/0`) or `Duration::from_secs_f64(-x)` both panic. Consider adding a `value_parser` constraint or validating `fps > 0` before use.</comment>
<file context>
@@ -74,6 +74,18 @@ enum Command {
+
+ /// Frames per second for GIF animation (default: 30)
+ #[clap(long, default_value = "30")]
+ fps: f64,
+
+ /// Total number of frames for GIF animation
</file context>
No description provided.