Skip to content

Replace AnimationCurve curve value inputs during preprocessing#4290

Draft
Gavin-Niederman wants to merge 8 commits into
GraphiteEditor:masterfrom
Gavin-Niederman:add/animation-curve-preprocessing
Draft

Replace AnimationCurve curve value inputs during preprocessing#4290
Gavin-Niederman wants to merge 8 commits into
GraphiteEditor:masterfrom
Gavin-Niederman:add/animation-curve-preprocessing

Conversation

@Gavin-Niederman

Copy link
Copy Markdown

This PR adds a replace_animation_curve_inputs method to the preprocessor that replaces any TaggedValue::AnimationCurve(curve) value inputs in the network new eval_curve nodes. It also refactors out some of the logic in replace_resource_inputs into visit_* functions to reduce code duplication.

In the future, this will be used in the expose dialog to allow exposing inputs to the timeline.

This PR is a follow-up to #4164. Until that PR is merged, this one will include its commits.

Demo

untitled.mp4

@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 the graphene-animation crate, which implements animation primitives like Keyframe, InterpolationBehavior, and AnimationCurve for the Graphene node system. It integrates these curves into the node registry, adds an eval_curve node, and updates the preprocessor to replace AnimationCurve inputs with eval_curve nodes. The feedback highlights two critical issues: first, the preprocessor's node visitor skips preprocessing inputs on nested Network nodes due to an early continue; second, evaluating bezier curves with non-finite handle coordinates can lead to panics or infinite loops, requiring sanitization of the handles.

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 thread node-graph/preprocessor/src/lib.rs
Comment thread node-graph/libraries/animation/src/lib.rs
@@ -23,6 +23,7 @@ impl Preprocessor {
pub fn preprocess(&self, network: &mut NodeNetwork, resolve_resource: &dyn Fn(ResourceId) -> Option<ResourceHash>) -> Result<(), PreprocessorError> {
self.insert_inject_scopes(network);
self.replace_resource_inputs(network, resolve_resource)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should consolidate both input replace passes into one.

Having one replace_inputs_with_producer_nodes (or similar) that has a match for the node input types for node in the network.

match input {
  <resource> ...
  <animation_curve> ...
  _ => {}
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe we should consolidate both input replace passes into one.

Would you also want to remove the visitor abstractions in this case since there will only be one function that uses it?

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