Skip to content

Add Frame Number node#4197

Open
benstockil wants to merge 4 commits into
GraphiteEditor:masterfrom
benstockil:feat/frame-number-node
Open

Add Frame Number node#4197
benstockil wants to merge 4 commits into
GraphiteEditor:masterfrom
benstockil:feat/frame-number-node

Conversation

@benstockil
Copy link
Copy Markdown

Add Frame Number node to get the current frame index, complementing the existing Animation Time node.

New context traits ExtractFrameNumber and InjectFrameNumber have been added to support this.

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 a new frame_number context feature to track and retrieve the current frame number in animations. This includes updating TimingInformation, adding extraction and injection traits, introducing a frame_number node, and updating the render cache key to include the frame number. A critical issue was identified in node-graph/libraries/core-types/src/context.rs where inserting FRAME_NUMBER in the middle of the ContextFeatures bitflags shifts subsequent flags, breaking backward compatibility for serialized documents. It is recommended to append FRAME_NUMBER to the end of the bitflags 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 thread node-graph/libraries/core-types/src/context.rs Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@TrueDoctor
Copy link
Copy Markdown
Member

I think the frame number should be derived from the animation time instead. But you can build an exact frame number node which reads the animation time and the frame rate

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

1 issue found across 9 files

Confidence score: 4/5

  • This PR appears safe to merge from a runtime perspective; the reported issue is process/policy-related (PR title format) rather than a concrete code regression.
  • The most severe finding (6/10, high confidence) is that the PR title for a new node does not match the required New node: 'Node Name' convention, which can impact review/release hygiene or automation expectations.
  • Given the issue is non-functional and limited in scope, risk to end users is low, but it is still worth correcting for consistency.
  • Pay close attention to node-graph/graphene-cli/src/export.rs - ensure the associated new-node change is tracked with the required PR title format.
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">
P2: Custom agent: **PR title enforcement**

PR title for a new node does not follow the required dedicated format. For new nodes, the rule mandates `New node: 'Node Name'` (single quotes, Title Case). The current title "Add Frame Number node" should be updated to `New node: 'Frame Number'`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@@ -187,6 +187,7 @@ pub async fn export_gif(
time: TimingInformation {
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: Custom agent: PR title enforcement

PR title for a new node does not follow the required dedicated format. For new nodes, the rule mandates New node: 'Node Name' (single quotes, Title Case). The current title "Add Frame Number node" should be updated to New node: 'Frame Number'.

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>PR title for a new node does not follow the required dedicated format. For new nodes, the rule mandates `New node: 'Node Name'` (single quotes, Title Case). The current title "Add Frame Number node" should be updated to `New node: 'Frame Number'`.</comment>

<file context>
@@ -187,6 +187,7 @@ pub async fn export_gif(
 			time: TimingInformation {
 				time: animation_time.as_secs_f64(),
 				animation_time,
+				frame_number: frame_idx as u64,
 			},
 			..Default::default()
</file context>

@benstockil
Copy link
Copy Markdown
Author

benstockil commented Jun 4, 2026

I think the frame number should be derived from the animation time instead.

@TrueDoctor Is this true for both animation modes, or just TimeBased? If acceptable I'd prefer to maintain some way to read the exact frame counter for FrameBased animations, i.e.:

// in AnimationMessageHandler::timing_information:

let frame_number = match self.animation_time_mode {
	AnimationTimeMode::TimeBased => self.fps * (animation_time_millis / 1000.0),
	AnimationTimeMode::FrameBased => self.frame_index,
};

@TrueDoctor
Copy link
Copy Markdown
Member

When in frame based animation mode, the timing will always be an increment of 1/fps. If we actually needed to pass the frame number through the context, we would most likely store this as a tuple with the animation time instead of as its own field / extract traits since they semantically express the same thing. Keeping them separate would introduce a different dependency flow analysis / different cache behavior. And we try to keep the context as light weight as possible so adding and extra field is not ideal if we can avoid 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