Anim: Discuss edge cases and add regression tests for NLA strip evaluation #109212
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#109212
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "pamadini/blender:nla-adjacent-strips-evaluate-later"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request has a wider discussion of some NLA edge cases and adds regression tests. The core change that was agreed upon has now been rebased on the latest main branch and moved to #113487.
This changes the behavior of adjacent NLA strips for Blender 4.0, as discussed in the 2023-06-08 Animation & Rigging module meeting.
I'd like to write some regression tests before requesting review.
Optionally, the first part of the
nlastrips_ctime_get_strip
function could be streamlined with a larger refactoring.First thanks for exploring the NLA :)
Second I don't think having inconsistent behavior if the clips are on the same track or split should happen, if it changes it so that the next clips start frame is more important to show than the first clips last frame, that should be how it is no matter where the clip is..touching or above/below...the next clip is the next clip.
Third, I don't know how this changes how the HOLD and HOLD forward behavior works with the clips on the same track.
If you can fix it so that it is consistent when the clips are on different tracks and can you please test how this impacts/changes how with two clips with Blending together would work.
Looking testing this in other tools when not touching or blending the last frame of the clip is shown, when they touch or blend, the NEXT clips start frame becomes the winner and is shown.... and I wonder if this would help blending between clips work better in Blender.
Thank you @BClark! I had a more complete write-up made before I saw your comment, I originally wanted to update the pull request description but I'll just include the write-up in its original form below, since you didn't have a chance to see it before, and I didn't see you comment either.
Let me know if this answers some or all of your questions. The behavior with clips on a different track is not changed in the pull request based on the assumptions I made. My thinking is still that no change is necessary there, but we can discuss further and I don't feel strongly either way, as it covers a non-default use case that takes a few steps to trigger.
Summary
The goal of this task is to identify and implement the desired behavior for non-linear animation (NLA) strips when evaluated at the boundary of their frame range in Blender 4.0 and beyond.
This investigation is motivated by an agreed inconsistency in Blender 3.x and earlier, when two strips are adjacent on the same track. The last frame of the first strip is evaluated, instead of the first frame of the second strip. This is not so much the result of deliberate design, as much as the result of how the current code iterates through strips.
While it is unambiguous that the first frame of a strip should be evaluated when encountered, there are alternatives about what to do when the time is exactly on the last frame of a strip, and no other strip is present at the same time.
Assumptions
Unless otherwise specified, throughout the discussion it is assumed that the first and last frame of an NLA strip are not extrapolated beyond the boundary, despite the default in Blender being to hold the values of the first and last frames. This means that, when the NLA track is evaluated at a time with no strip, the track has no effect on the result.
The problem space of defining frame ranges in non-linear animation tracks is slightly more nuanced than non-linear video editing software, because frame numbers can be fractional. I’d note that in every case I found in practice, the boundaries are integers, but this can’t be relied upon.
When expressing frame ranges in natural language, it is my experience that the first and last frames are assumed to be included. For example:
However, when game developers deploy non-linear animation, it may be convenient to reason "mathematically" and never evaluate the last frame. It seems to be the case both in Unity’s Timeline and Unreal’s Sequencer that a clip extending from frames 1 to 4 would evaluate at frames 1, 2, 3, and up to 3.999…, but go back to no evaluation at frame 4.
Inside a strip, animation curves are not necessarily continuous. For example, when constant (stepped) tangents are used on keys at frames 3 and 4, the value at frame 3.999… may be different from the value at frame 4.
Current Behavior and Alternatives
At present, Blender evaluates strips on their first and last frames, both included. My opinion is that this makes sense, thinking of the wider target audience that it has compared to just game development. For example, consider this workflow:
The strip extends from frame 1 to 4, and the key on frame 4 is evaluated.
The strip is still evaluated at frame 4, and back to default at frame 5.
It might be confusing if the last frame was always excluded, as a key would be there and visible in most editors, but it would have no effect. This could also lead to workarounds such as extending the last frame of the strip to just a little over the last frame, such as frame 4.00...01. The exact value to choose would be ambiguous.
Using the last key only when "Extrapolation" is not "Nothing" would mean that the strip would effectively “lose” its last frame when the extrapolation is changed. It would require moving the boundary to keep the last pose. On the other hand, it would be consistent with how game engines work, and address differently the edge case on different tracks that is discussed below.
A new extrapolation mode "Nothing, but include the last frame" would require too much micro-management, and more variables would have to be considered when debugging an NLA sequence.
Adjacent Clips on The Same Track (Changed)
When a strip is manually repeated, a new copy or instance is placed adjacent to the previous one. For example, a cycle from 1001 to 1033 would be followed by the same strip running from 1033 to 1065. In this case, regardless of the extrapolation, frame 1033 should be the first frame of the second strip, which is the only change made by this pull request.
This new behavior would be the same as when the strip is extended and set to repeat (loop) internally. The last frame of the underlying action is only evaluated if the last iteration of the loop falls exactly at the end boundary of the strip. This is the same frame that is held afterwards.
As a side note, Unity has a flag to control looping, and when enabled, the last frame would always be excluded from evaluation. Setting "Post-extrapolate" to "Hold" would hold the first frame of the underlying animation at the end, instead of the last frame.
Clips on Different Tracks (Kept as Now)
If the two strips in the example above have "Extrapolation" set to "Nothing" and one of them is moved to a different track, the value at frame 1033 would be combined based on the strip blending modes. With the default of "Replace", the result would be different based on which clip is above the other.
In game engines, always excluding the last frame has the effect that the result at the boundary does not depend on the order of the tracks, or whether the same track is used for both clips. However, the context in which game developers operate may be different, and what may seem mathematically “cleaner” to reason about for a developer may be more confusing for a target audience that is accustomed to considering the key at the last frame to be significant.
Moreover, with "Hold" being the default extrapolation, the result for all frames, not just 1033, is already different when the clips are moved to a different track. Thus, there may be simply not be enough motivation to change the current behavior for a non-default edge case.
Test File
The following test file has an action running from frames 1 to 4, where the X Location of the cube is the same as the frame number, with constant tangents. This action is referenced by different strips in the NLA editor.
With the change in the pull request, the value at frame 4 will be "1", like it is at frame 22, instead of "4", like it is at frame 13.
075cdbc1cb
toc60c9acfc0
I still don't have a good sense for how this is going to feel/change but I hope I can look at it when I have some more rest next week.
Thank you for your interest and looking at the NLA code :)
I have 15 or so issues in my document that are still open and some are very tiny and some are whole new features involving the NLA.
I really like that you've added new tests for the NLA. How would you feel about moving these into a separate PR, so that they cover the current behaviour of Blender? That way this PR can remain focused on the change in behaviour, keeping the two goals nicely separated. Approving & landing the new tests will be considerably easier than discussing the change in behaviour, so let's keep the latter as small as possible.
Thank you, Sybren, for taking a look!
My availability will be limited for the time being, and between the two goals I'd rather focus on making the behavior consistent. However, the tests are already nicely separated in their own commit, and even if they have not landed yet, the core team is very welcome to take the commit and use the code as a template to cover the current behavior, or use it for testing other parts of the nonlinear animation editor.
Having separate commits is good. The thing is mostly that the test now sits after the change in functionality and thus will need adjustment if they're to be committed to
main
first.WIP: Animation: At the boundary frame between non-muted NLA strips, evaluate the first frame of the later strip instead of the last frame of the earlier.to WIP: Discuss edge cases and add regression tests for NLA strip evaluationExactly, I was suggesting the team could use the code "as a template" if they wanted to make the adjustments you mention, or if they wanted to add tests to other pull requests that they were working on. This didn't happen in the meantime, since other pull requests touching the same file landed without tests, which is fine as the current convention for that file is that tests are not required.
In fact, I believe you are fundamentally correct, in that adding regression tests and multi-track behavior to the discussion in the same pull request has caused some scope bleed. Thus, I moved the fundamental change to #113487 to keep it separate and not overload reviewers with too much information. I may pick the tests up again and ask for review sometime later, but for now I'd like to save reviewer bandwidth for the change that actually matters the most to the users.
This is actually the opposite of what I intended. One of the big issues with the NLA, which makes working on it so cumbersome and risky, is the lack of unit tests. I wasn't trying to point out scope creep, but rather trying to motivate you to take the tests that you have in this PR, and adjust them so that they work on current
main
. Then a separate PR can alter Blender's behaviour, which would impact both the main code and the tests (which is a good thing).c60c9acfc0
to5eda2f8766
WIP: Discuss edge cases and add regression tests for NLA strip evaluationto Discuss edge cases and add regression tests for NLA strip evaluationI've taken the tests and adjusted them so that they work on current
main
. Who should I ask for review?You can add @nrupsis and me as reviewers.
@ -0,0 +1,120 @@
# SPDX-FileCopyrightText: 2023 Blender Authors
#
# SPDX-License-Identifier: GPL-2.0-or-later
Just double checking we're using GPL-2.0
I'm seeing a lot of test files with
SPDX-License-Identifier: Apache-2.0
Both seem to be in use, happy to switch if it's needed once you've determined which license works best for new files here.
Blender is "GPLv2.0 or later". Other code could have been submitted with a different (but compatible) license. When in doubt, use GPLv2+.
@ -0,0 +27,4 @@
def setUpClass(cls):
bpy.ops.wm.read_factory_settings()
cls.test_object = bpy.data.objects["Cube"]
Might be worth creating / tearing down an object as well, that way we're not reliant on the default Blender scene which could (probably won't) change.
@ -0,0 +47,4 @@
strip.extrapolation = "NOTHING"
return strip
def assertFrameValue(self, frame: float, value: float):
Would change the parameter name to "expected_value". Gives a clearer indication what the method does.
@ -0,0 +117,4 @@
if __name__ == "__main__":
# Drop all arguments before "--", or everything if the delimiter is absent. Keep the executable path.
unittest.main(argv=sys.argv[:1] + (sys.argv[sys.argv.index("--") + 1:] if "--" in sys.argv else []))
(Unrelated to this PR. ) This seems like a pretty consistent pattern across the python tests.
@dr.sybren would it make sense for this to be abstracted into a Util of some sort?
Maybe at some point, but not in this PR ;-)
5eda2f8766
to8db7f4f041
Awesome to see some python tests for the NLA!
@pamadini for some reason anonymous access to https://projects.blender.org/pamadini/blender is blocked. Was this something you configured? To integrate well with other systems, please make sure the project is publicly accessible.
Huh, his fork is loading fine for me...
It loads fine when I'm logged in. When in a private window, though, I get a "404 not found" error.
Oh okay, I'm experiencing that same behavior.
It's fixed now! I noticed the same behavior, I thought it was expected but it turns out I've just flipped a profile setting at some point.
8db7f4f041
to6eea97b236
I rebased this on the latest
main
and I've re-requested review to be able to land the updated version.Make sure to prefix the PR title with "Anim:"
https://wiki.blender.org/wiki/Style_Guide/Commit_Messages
Discuss edge cases and add regression tests for NLA strip evaluationto Anim: Discuss edge cases and add regression tests for NLA strip evaluationDone, and the commit message already started with "Animation".
The documentation seems to suggest that the final merge should be done by the reviewer? I still don't seem to have permission, even though I now have an up-to-date review and I've rebased the code.
About 2 months back we decided on the "Anim" prefix. probably should document it somewhere 😬
I can merge this in if you want :)
6eea97b236
toe4b2a428a0
Fixed!
Yes, thanks, I don't seem to have the required permissions to perform the merge after the review.
For the record: this is how Gitea works. You either have commit access (and can commit whatever), or you don't. Having an accepted PR doesn't give you the right to land that PR, so unless you're part of the regular dev team, it'll be the reviewer who lands the PR. This is fine, you'll still be registered as author of the commit.