Skip to content

Inherit preceding keyframe interpolation when adding a keyframe - #3438

Open
Silvertree2010 wants to merge 1 commit into
storytold:mainfrom
Silvertree2010:fix/h2916-adding-a-keyframe-inside-a-hold
Open

Silvertree2010 wants to merge 1 commit into
storytold:mainfrom
Silvertree2010:fix/h2916-adding-a-keyframe-inside-a-hold

Conversation

@Silvertree2010

@Silvertree2010 Silvertree2010 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Found while testing crates/project/src/keyframe.rs:238 (Param::add_keyframe / set_at_force) (no existing issue): Adding a keyframe inside a Hold segment turns the rest of the segment into a linear ramp.

Cause

Param::set_at_force always created new keyframes with Interpolation::Linear. Adding a keyframe inside a Hold segment therefore turned the part after the new keyframe into a linear ramp, which changed the curve.

Fix

When set_at_force inserts a keyframe after an existing one, it now copies that preceding keyframe's interpolation, as set_at already does. This covers add_keyframe and put_keyframe. A regression test is included.

Verified

  • Regression test keyframe::tests::add_keyframe_inside_hold_keeps_curve: fails on main without the fix and passes with it (cargo test -p filmcraft-project add_keyframe_inside_hold_keeps_curve).
  • cargo xtask ci passes except hevc_probe::register_answers_the_hevc_probe_in_the_background, menus::tests::hovering_another_title_switches_the_open_menu, which also fail on main without this change.

Not changed

A keyframe inserted before the first keyframe still uses the default Linear interpolation. set_at is unchanged.

🤖 Generated with AI by ApolloOffice, see the Co-Authored-By trailers for the models

Summary by CodeRabbit

  • Bug Fixes
    • Keyframes inserted within a segment now preserve the preceding keyframe’s interpolation, maintaining the expected behavior of Hold segments.

Found while testing crates/project/src/keyframe.rs:238 (Param::add_keyframe / set_at_force)

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 10d626c and 558fdeb.

Files selected for processing: 1
  • crates/project/src/keyframe.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

set_at_force now assigns an inserted keyframe the preceding keyframe’s interpolation. A regression test checks that inserting within a Hold segment preserves the held value later in the segment.

Changes: Keyframe Interpolation

Layer / File(s) Summary
Insertion behavior and regression test
crates/project/src/keyframe.rs
set_at_force copies the preceding keyframe’s interpolation when one exists. A regression test checks that inserting at tick 50 in a Hold segment keeps the value at tick 75 at 0.0.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: echelon

Merge Risk: Minimal · up to 558fd

The intended Hold interpolation behavior is covered, with no established merge-blocking issue.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title states the main change: when a keyframe is added, it inherits the interpolation of the preceding keyframe. The raw summary confirms that set_at_force now copies the preceding keyframe's inte…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title states the main change: when a keyframe is added, it inherits the interpolation of the preceding keyframe. The raw summary confirms that set_at_force now copies the preceding keyframe's interpolation, and the PR objectives describe the same fix for inserting inside a Hold segment. The title is specific and concise, and it does not use vague terms.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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