Skip to content

New node: 'Animation Delta Time'#4304

Open
Keavon wants to merge 1 commit into
masterfrom
animation-delta-time
Open

New node: 'Animation Delta Time'#4304
Keavon wants to merge 1 commit into
masterfrom
animation-delta-time

Conversation

@Keavon

@Keavon Keavon commented Jul 2, 2026

Copy link
Copy Markdown
Member

No description provided.

@Keavon Keavon requested a review from TrueDoctor July 2, 2026 08:34

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces animation_delta_time to track the elapsed animation time between frames, adding the necessary traits, context features, and a new animation_delta_time node to the node graph. It also fixes a type validation bug in proto.rs by checking input lengths. Feedback highlights two main issues: first, animation_delta_time is decoupled from frame-based animation time in FrameBased mode, where it should instead be a constant 1. / self.fps during playback and clamped to a maximum limit to prevent physics issues; second, hardcoding animation_delta_time to 0. in export_gif will freeze delta-time-dependent nodes, so it should be set to 1. / animation.fps instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 97 to 104
AnimationMessage::SetTime { time } => {
// Elapsed animation time since the previous frame, in seconds (frozen while paused, so it reads 0 then)
let previous_animation_time = self.timestamp - self.animation_start();
self.timestamp = time;
let current_animation_time = self.timestamp - self.animation_start();
self.animation_delta_time = ((current_animation_time - previous_animation_time) / 1000.).max(0.);
responses.add(AnimationMessage::UpdateTime);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In FrameBased mode, the animation time is determined by self.frame_index / self.fps. However, self.animation_delta_time is always calculated using self.timestamp (real-time based), which completely decouples it from the actual frame-based animation time. Furthermore, when the animation is playing in FrameBased mode, the delta time should be a constant 1. / self.fps rather than fluctuating based on real-time lag, ensuring deterministic playback. Additionally, capping the delta time to a reasonable maximum (e.g., 0.1 seconds) is a standard defensive programming practice to prevent physics or simulation explosions during large jumps or lag spikes.

			AnimationMessage::SetTime { time } => {
				// Elapsed animation time since the previous frame, in seconds (frozen while paused, so it reads 0 then)
				let previous_animation_time = self.timestamp - self.animation_start();
				self.timestamp = time;
				let current_animation_time = self.timestamp - self.animation_start();
				self.animation_delta_time = match self.animation_time_mode {
					AnimationTimeMode::TimeBased => ((current_animation_time - previous_animation_time) / 1000.).clamp(0., 0.1),
					AnimationTimeMode::FrameBased => if self.is_playing() { 1. / self.fps } else { 0. },
				};
				responses.add(AnimationMessage::UpdateTime);
			}

Comment on lines 187 to 191
time: TimingInformation {
time: animation_time.as_secs_f64(),
animation_time,
animation_delta_time: 0.,
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In export_gif, animation_delta_time is hardcoded to 0.. This means any nodes in the graph that rely on delta time (e.g., physics, procedural animations) will completely freeze or fail to update when exported to a GIF. Since GIF frames are rendered at a constant FPS, the delta time should be 1. / animation.fps.

Suggested change
time: TimingInformation {
time: animation_time.as_secs_f64(),
animation_time,
animation_delta_time: 0.,
},
time: TimingInformation {
time: animation_time.as_secs_f64(),
animation_time,
animation_delta_time: 1. / animation.fps,
},

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

3 issues found across 8 files

Confidence score: 3/5

  • In node-graph/graphene-cli/src/export.rs, the GIF export loop hardcodes animation_delta_time to 0., so frame-to-frame timing is lost and exported animations can play with incorrect temporal behavior — compute per-frame delta as 0. for the first frame and 1.0 / animation.fps afterward before merging.
  • In editor/src/messages/animation/animation_message_handler.rs (FrameBased mode), animation_delta_time is derived from wall-clock timestamp differences while animation time advances in fixed 1/fps steps, which can desynchronize simulation timing from frame progression and cause inconsistent playback/render results — align delta-time calculation with the same fixed-step model used by timing_information().
  • In editor/src/messages/animation/animation_message_handler.rs, SetFrameIndex submits a graph render without resetting the new animation_delta_time, so scrubbing to a frame can reuse stale delta values and produce one-frame jumps or incorrect state updates — reset or recompute animation_delta_time on SetFrameIndex before triggering render.
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/export.rs">

<violation number="1" location="node-graph/graphene-cli/src/export.rs:190">
P1: In the GIF export loop, `animation_delta_time` is hardcoded to `0.` for every frame, but it should reflect the time elapsed since the previous frame (`1.0 / animation.fps` for frames after the first, `0.` for the first). As written, the new Animation Delta Time node will always output 0 in GIF exports, defeating its purpose for any animation using delta-time-dependent logic.</violation>
</file>

<file name="editor/src/messages/animation/animation_message_handler.rs">

<violation number="1" location="editor/src/messages/animation/animation_message_handler.rs:37">
P2: The new `animation_delta_time` field is updated in `SetTime` and reset in `RestartAnimation`, but `SetFrameIndex` triggers a graph render (`SubmitActiveGraphRender`) without resetting it. If the user scrubs to a specific frame while the animation is playing, the Animation Delta Time node will read a stale non-zero delta from the last playback frame instead of 0, since manual frame scrubbing involves no elapsed animation time.</violation>

<violation number="2" location="editor/src/messages/animation/animation_message_handler.rs:102">
P2: In `FrameBased` mode, `animation_delta_time` is computed from real-time timestamp differences, but the actual animation time progresses in fixed `1/fps` increments (as shown in `timing_information()`). This decouples delta time from the frame-based animation time, causing non-deterministic playback for physics/simulation nodes. Consider differentiating by mode:

```rust
self.animation_delta_time = match self.animation_time_mode {
    AnimationTimeMode::TimeBased => ((current_animation_time - previous_animation_time) / 1000.).clamp(0., 0.1),
    AnimationTimeMode::FrameBased => if self.is_playing() { 1. / self.fps } else { 0. },
};

Also note the lack of an upper clamp in TimeBased mode — large time jumps (e.g., tab backgrounding, sleep/wake) could cause simulation explosions.


</details>

<sub>Reply with feedback, questions, or to request a fix.<br /><br />[Re-trigger cubic](https://www.cubic.dev/action/re-review/pr/GraphiteEditor/Graphite/4304/ai_pr_review_1782981265769_6f98c5b5-37bf-4953-9483-dcf0fb416d12?returnTo=https%3A%2F%2Fgithub.com%2FGraphiteEditor%2FGraphite%2Fpull%2F4304)</sub>

<!-- cubic:review-post:ai_pr_review_1782981265769_6f98c5b5-37bf-4953-9483-dcf0fb416d12:8d5b04e65b3610c2eed1a0bdc1d35af65ac1085e:abee3c48-4fdb-4653-b31f-3903492e96ab,81921ee6-83d5-4bfb-bb25-9551843cb1c9 -->

time: TimingInformation {
time: animation_time.as_secs_f64(),
animation_time,
animation_delta_time: 0.,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: In the GIF export loop, animation_delta_time is hardcoded to 0. for every frame, but it should reflect the time elapsed since the previous frame (1.0 / animation.fps for frames after the first, 0. for the first). As written, the new Animation Delta Time node will always output 0 in GIF exports, defeating its purpose for any animation using delta-time-dependent logic.

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/export.rs, line 190:

<comment>In the GIF export loop, `animation_delta_time` is hardcoded to `0.` for every frame, but it should reflect the time elapsed since the previous frame (`1.0 / animation.fps` for frames after the first, `0.` for the first). As written, the new Animation Delta Time node will always output 0 in GIF exports, defeating its purpose for any animation using delta-time-dependent logic.</comment>

<file context>
@@ -187,6 +187,7 @@ pub async fn export_gif(
 			time: TimingInformation {
 				time: animation_time.as_secs_f64(),
 				animation_time,
+				animation_delta_time: 0.,
 			},
 			..Default::default()
</file context>
Suggested change
animation_delta_time: 0.,
animation_delta_time: if frame_idx > 0 { 1. / animation.fps } else { 0. },

fps: f64,
animation_time_mode: AnimationTimeMode,
/// Seconds of animation time elapsed between the previous frame and the current one.
animation_delta_time: f64,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: The new animation_delta_time field is updated in SetTime and reset in RestartAnimation, but SetFrameIndex triggers a graph render (SubmitActiveGraphRender) without resetting it. If the user scrubs to a specific frame while the animation is playing, the Animation Delta Time node will read a stale non-zero delta from the last playback frame instead of 0, since manual frame scrubbing involves no elapsed animation time.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/animation/animation_message_handler.rs, line 37:

<comment>The new `animation_delta_time` field is updated in `SetTime` and reset in `RestartAnimation`, but `SetFrameIndex` triggers a graph render (`SubmitActiveGraphRender`) without resetting it. If the user scrubs to a specific frame while the animation is playing, the Animation Delta Time node will read a stale non-zero delta from the last playback frame instead of 0, since manual frame scrubbing involves no elapsed animation time.</comment>

<file context>
@@ -33,6 +33,8 @@ pub struct AnimationMessageHandler {
 	fps: f64,
 	animation_time_mode: AnimationTimeMode,
+	/// Seconds of animation time elapsed between the previous frame and the current one.
+	animation_delta_time: f64,
 }
 impl AnimationMessageHandler {
</file context>

let previous_animation_time = self.timestamp - self.animation_start();
self.timestamp = time;
let current_animation_time = self.timestamp - self.animation_start();
self.animation_delta_time = ((current_animation_time - previous_animation_time) / 1000.).max(0.);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: In FrameBased mode, animation_delta_time is computed from real-time timestamp differences, but the actual animation time progresses in fixed 1/fps increments (as shown in timing_information()). This decouples delta time from the frame-based animation time, causing non-deterministic playback for physics/simulation nodes. Consider differentiating by mode:

self.animation_delta_time = match self.animation_time_mode {
    AnimationTimeMode::TimeBased => ((current_animation_time - previous_animation_time) / 1000.).clamp(0., 0.1),
    AnimationTimeMode::FrameBased => if self.is_playing() { 1. / self.fps } else { 0. },
};

Also note the lack of an upper clamp in TimeBased mode — large time jumps (e.g., tab backgrounding, sleep/wake) could cause simulation explosions.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/animation/animation_message_handler.rs, line 102:

<comment>In `FrameBased` mode, `animation_delta_time` is computed from real-time timestamp differences, but the actual animation time progresses in fixed `1/fps` increments (as shown in `timing_information()`). This decouples delta time from the frame-based animation time, causing non-deterministic playback for physics/simulation nodes. Consider differentiating by mode:

```rust
self.animation_delta_time = match self.animation_time_mode {
    AnimationTimeMode::TimeBased => ((current_animation_time - previous_animation_time) / 1000.).clamp(0., 0.1),
    AnimationTimeMode::FrameBased => if self.is_playing() { 1. / self.fps } else { 0. },
};

Also note the lack of an upper clamp in TimeBased mode — large time jumps (e.g., tab backgrounding, sleep/wake) could cause simulation explosions.

@@ -89,7 +95,11 @@ impl MessageHandler for AnimationMessageHandler { + let previous_animation_time = self.timestamp - self.animation_start(); self.timestamp = time; + let current_animation_time = self.timestamp - self.animation_start(); + self.animation_delta_time = ((current_animation_time - previous_animation_time) / 1000.).max(0.); responses.add(AnimationMessage::UpdateTime); } ```
Suggested change
self.animation_delta_time = ((current_animation_time - previous_animation_time) / 1000.).max(0.);
self.animation_delta_time = match self.animation_time_mode {
AnimationTimeMode::TimeBased => ((current_animation_time - previous_animation_time) / 1000.).clamp(0., 0.1),
AnimationTimeMode::FrameBased => if self.is_playing() { 1. / self.fps } else { 0. },
};

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