Replace AnimationCurve curve value inputs during preprocessing - #4290
Gavin-Niederman wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| @@ -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)?; | |||
There was a problem hiding this comment.
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> ...
_ => {}
}There was a problem hiding this comment.
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?
acd08a7 to
5ddaa8b
Compare
| [package] | ||
| name = "graphene-animation" | ||
| version = "0.1.0" | ||
| edition = "2024" |
There was a problem hiding this comment.
use *.workspace = true like in other crates for version etc. (copy from other crates to keep order)
| if matches!(&node.implementation, DocumentNodeImplementation::ProtoNode(identifier) if *identifier == platform_application_io::resource::IDENTIFIER) { | ||
| continue; | ||
| } | ||
| let is_resource_node = identifier == Some(&platform_application_io::resource::IDENTIFIER); |
There was a problem hiding this comment.
Not sure if this is needed.
The function should never run on those nodes, right?
| @@ -0,0 +1,267 @@ | |||
| //! Animation Curves | |||
There was a problem hiding this comment.
I was not that involved in the discussion about this but it seems like this fits the usecase.
personally I would changes some names but I assume the naming is common for similar keyframe based animation systems (like blender)?
This branch has not been deployed
2 participants
This PR adds a
replace_animation_curve_inputsmethod to the preprocessor that replaces anyTaggedValue::AnimationCurve(curve)value inputs in the network neweval_curvenodes. It also refactors out some of the logic inreplace_resource_inputsintovisit_*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