Skip to content

add: animation curves support#4164

Draft
Gavin-Niederman wants to merge 2 commits into
GraphiteEditor:masterfrom
Gavin-Niederman:add/animation-curves
Draft

add: animation curves support#4164
Gavin-Niederman wants to merge 2 commits into
GraphiteEditor:masterfrom
Gavin-Niederman:add/animation-curves

Conversation

@Gavin-Niederman
Copy link
Copy Markdown

A work-in-progress PR adding support for f-curves and a timeline NodeInput variant.

TODO:

  • AnimationCurve (f-curves implementation based on Blender's system)
  • Timeline NodeInput variant.

In the future, the timeline inputs can be hooked up to the timeline curve editor.

Copy link
Copy Markdown
Contributor

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

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 an AnimationCurve implementation, featuring keyframe-based interpolation (Linear, Constant, and Bezier) and integration into the node graph system via new nodes and registry updates. Review feedback suggests refining the evaluation logic to clamp out-of-bounds values to the nearest keyframe for better consistency and optimizing keyframe insertion using binary search to improve performance and data validation.

Comment on lines +78 to +83
if index == 0 {
return 0.0;
} else if index == self.keyframes.len() {
// unwrap is safe because of the non-empty guard at the top
return self.keyframes.last().unwrap().knot.y;
}
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.

medium

The evaluation logic for time values before the first keyframe returns 0.0, while evaluation after the last keyframe returns the last keyframe's value. For animation curves, it is more consistent to clamp to the first keyframe's value when evaluated before the start of the curve, ensuring that properties (like scale or opacity) don't unexpectedly jump to zero.

Suggested change
if index == 0 {
return 0.0;
} else if index == self.keyframes.len() {
// unwrap is safe because of the non-empty guard at the top
return self.keyframes.last().unwrap().knot.y;
}
if index == 0 {
return self.keyframes[0].knot.y;
} else if index == self.keyframes.len() {
// unwrap is safe because of the non-empty guard at the top
return self.keyframes.last().unwrap().knot.y;
}

Comment on lines +120 to +123
pub fn push_keyframe(&mut self, keyframe: Keyframe) {
self.keyframes.push(keyframe);
self.keyframes.sort_by(|lhs, rhs| lhs.knot.x.partial_cmp(&rhs.knot.x).unwrap_or(std::cmp::Ordering::Equal));
}
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.

medium

Sorting the entire keyframes vector after every push is inefficient (O(N log N)). Since the vector is already sorted, you can use binary_search_by to find the correct insertion point and use insert or replace the existing keyframe, which is O(N). Additionally, validating that knot.x is finite prevents issues with sorting and searching logic caused by NaN values.

Suggested change
pub fn push_keyframe(&mut self, keyframe: Keyframe) {
self.keyframes.push(keyframe);
self.keyframes.sort_by(|lhs, rhs| lhs.knot.x.partial_cmp(&rhs.knot.x).unwrap_or(std::cmp::Ordering::Equal));
}
pub fn push_keyframe(&mut self, keyframe: Keyframe) {
if !keyframe.knot.x.is_finite() {
return;
}
let index = self.keyframes.binary_search_by(|kf| kf.knot.x.partial_cmp(&keyframe.knot.x).unwrap_or(std::cmp::Ordering::Equal));
match index {
Ok(idx) => self.keyframes[idx] = keyframe,
Err(idx) => self.keyframes.insert(idx, keyframe),
}
}

knot: DVec2::new(1.0, 10.0),
interp_behavior: InterpolationBehavior::Constant,
});
assert_eq!(single_kf.evaluate(0.0), 0.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.

medium

This test case should be updated to reflect the suggested clamping behavior for out-of-bounds evaluation (returning the first keyframe's value instead of 0.0).

Suggested change
assert_eq!(single_kf.evaluate(0.0), 0.0);
assert_eq!(single_kf.evaluate(0.0), 10.0);

@Keavon Keavon force-pushed the add/animation-curves branch from 0105b40 to 3ddcda6 Compare May 23, 2026 05:44
@Keavon
Copy link
Copy Markdown
Member

Keavon commented May 23, 2026

Note: I rebased your branch.

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