Animation: Allow NLA strips to be horizontally shuffled #105532
Merged
Nate Rupsis
merged 22 commits from 2023-04-03 17:10:50 +02:00
nrupsis/blender:NLA-horizontal-shuffle
into main
Reviewers
Request review
No reviewers
Labels
Clear labels
This issue affects/is about backward or forward compatibility
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Apply labels
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
This issue affects/is about backward or forward compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE & Viewport
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
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
Virtual Reality
Interest
Vulkan
Interest: Wayland
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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 Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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 & Viewport
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
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
Virtual Reality
Interest
Vulkan
Interest: Wayland
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105532
Reference in New Issue
There is no content yet.
Delete Branch "nrupsis/blender:NLA-horizontal-shuffle"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
Allows NLA strips to horizontally translated over each other. If a strip is dropped when translating, it'll cause the strip to shuffle into place.
Abstracted large conditional branch in
recalcData_nla
into it's ownnlastrip_fix_overlapping
method for increased readability. No logical changes were made.Archived Phabricator Patch
WIP: NLA-horizontal-shuffleto WIP: Animation: Allow NLA strips to be horizontally shuffledWIP: Animation: Allow NLA strips to be horizontally shuffledto Animation: Allow NLA strips to be horizontally shuffled@blender-bot package
Package build started. Download here when ready.
Thanks for the reviews & the testing!Oops, wrong PR!
Impressive patch, that was a lot of pointer juggling.
A few notes, most notably a potential for accessing freed memory.
I can confirm the shuffling works with plain strips, haven't tested on a combination with transform strips yet.
@ -1952,6 +1952,25 @@ static void BKE_nlastrip_validate_autoblends(NlaTrack *nlt, NlaStrip *nls)
}
}
/* Ensure every transition's start/end properly set. */
Document that this can even remove and free
strip
when it doesn't fit any more.@ -1953,2 +1953,4 @@
}
/* Ensure every transition's start/end properly set. */
static void nlastrip_validate_start_end(NlaStrip *strip)
Better name would be
nlastrip_validate_transition_start_end
, to show that it's about transitions only.@ -1969,2 +1989,4 @@
nlastrip_validate_start_end(strip);
/* auto-blending first */
BKE_nlastrip_validate_autoblends(nlt, strip);
This can access memory after freeing it, crashing Blender. Better to let the caller know somehow whether
nlastrip_validate_start_end()
freed the strip.A similar issue occurs in the
for
-loop withstrip = strip->next
. You'll probably want to replace the loop with a call toLISTBASE_FOREACH_MUTABLE()
. That'll determine thenext
pointer before entering the loop body.@ -486,3 +486,3 @@
/* draw 'inside' of strip itself */
if (non_solo == 0 && is_nlastrip_enabled(adt, nlt, strip)) {
if (non_solo == 0 && is_nlastrip_enabled(adt, nlt, strip) &&
<rant about existing code>
I really don't like negated booleans.
if (non_solo == 0 ...)
just makes my head spin. As a followup patch, if you want to change that toif (solo ...)
, by all means!</rant>
@ -58,0 +133,4 @@
const float offset_left = transdata_get_time_shuffle_offset_side(trans_datas, true);
const float offset_right = transdata_get_time_shuffle_offset_side(trans_datas, false);
BLI_assert(offset_left <= 0);
BLI_assert(offset_right >= 0);
Nice assertions, really helps to understand the math that follows.
@ -84,0 +181,4 @@
* strips for overlap instead of all of them. */
static void nlastrip_flag_overlaps(NlaStrip *strip)
{
{
I think one set of curlies is enough ;-)
@ -84,0 +198,4 @@
/** Check the Transformation data for the given Strip, and fix any overlap. Then
* apply the Transformation.
*/
static void nlastrip_fix_overlapping(TransInfo *t, TransDataNla *tdn, NlaStrip *strip)
Maybe add a note in the PR description (and thus the commit message) that the contents of
nlastrip_fix_overlapping()
were just moved fromrecalcData_nla()
, and that the logic was kept as-is. That way people won't hold you responsible for the code ;-)@ -536,3 +757,2 @@
/* Perform after-transform validation. */
ED_nla_postop_refresh(&ac);
for (ale = anim_data.first; ale; ale = ale->next) {
Use
LISTBASE_FOREACH()
@ -835,0 +835,4 @@
/** When transforming strips, this flag is set when the strip is placed in an invalid location
* such as overlapping another strip or moved to a locked track. In such cases, the strip's
* location must be fixed. */
"fixed" can mean 'corrected' as well as 'prevented from moving' (as in 'to fix a shelf to the wall'). Probably better to replace it with "corrected after the transform operator is done" or something along those lines.
@ -1969,0 +1988,4 @@
nlastrip_validate_transition_start_end(strip);
if (strip == NULL) {
nlastrip_validate_transition_start_end()
doesn't change thestrip
pointer variable, so theif (strip == NULL)
check doesn't check for 'if the strip was freed'.How about making it so that
nlastrip_validate_transition_start_end()
returns a boolean that indicates whether the strip was freed or not. Alternatively you could give it aNlaStrip **strip
parameter so that it can modify thestrip
variable itself, but I'm not really a fan of those.@ -1969,0 +1990,4 @@
if (strip == NULL) {
printf(
"NLA strip was in an invalid location and was removed by "
Such a warning is a good idea. It can be more specific, though, because we know it's an transition strip, so do mention that. Maybe also turn the message more positive, like "While moving NLA strips, a transition strip could no longer be applied to the new positions and was removed."
It would be better to have this as a warning in the UI though, and not just printed on the terminal. That would require some more work that's not necessarily related to this PR, though.
@ -1969,0 +1992,4 @@
printf(
"NLA strip was in an invalid location and was removed by "
"nlastrip_validate_transition_start_end \n");
return;
Why return? Shouldn't the remaining strips be checked too?
@ -84,0 +209,4 @@
PointerRNA strip_ptr;
for (short iter = 0; iter < 5; iter++) {
Instead of hard-coding
5
(here) and4
(below), create a constant and use it in both places. That way the relationship between the two values is explicit.@ -84,0 +210,4 @@
PointerRNA strip_ptr;
for (short iter = 0; iter < 5; iter++) {
const bool pExceeded = (prev != NULL) && (tdn->h1[0] < prev->end);
Variables should use
snake_case
.@ -282,6 +475,8 @@ static void recalcData_nla(TransInfo *t)
{
SpaceNla *snla = (SpaceNla *)t->area->spacedata.first;
const bool is_translating = ELEM(t->mode, TFM_TRANSLATION);
Do you expect more
TFM_xxx
constants to be added here? If not, just uset->mode == TFM_TRANSLATION
. Same for the otheris_translating
variable.@ -502,0 +649,4 @@
/** horizontally translate (shuffle) the transformed strip to a non-overlapping state. */
static void nlastrip_shuffle_transformed(TransDataContainer *tc, TransDataNla *first_trans_data)
{
/** Element: (IDGroupedTransData*) */
Don't use doxygen for comments inside functions.
@ -1969,0 +1988,4 @@
nlastrip_validate_transition_start_end(strip);
if (strip == NULL) {
Would it make more sense to have a
<=
condition in the for loop, and use the sameiter_max
variable for both conditional checks? I.eI think the boolean return type is more expressive, and arguably more readable (with an informative comment on the function)
@ -1969,0 +1992,4 @@
printf(
"NLA strip was in an invalid location and was removed by "
"nlastrip_validate_transition_start_end \n");
return;
oop, nope that was a dumb mistake on my part. Should be a continue.
Almost there!
@ -1955,0 +1955,4 @@
/* Ensure every transition's start/end properly set.
* Strip will be removed / freed if it doesn't fit.
* Return value indicates if passed strip was freed or not. */
static bool nlastrip_validate_transition_start_end(NlaStrip *strip)
I feel the return value now contradicts the name of the function, as
true
is returned when the strip could not be validated. I think returningtrue
when valid-or-repaired andfalse
when invalid-beyond-repair is better.@ -1969,0 +1991,4 @@
if (nlastrip_validate_transition_start_end(strip)) {
printf(
"While moving NLA strips, a transition strip could no longer be applied to the new "
"positions and was removed. \n");
Remove space before newline.
@ -84,0 +202,4 @@
/* firstly, check if the proposed transform locations would overlap with any neighboring
* strips (barring transitions) which are absolute barriers since they are not being moved
*
* this is done as a iterative procedure (done 5 times max for now)
Well, with the
for
-loop as it is now, it's done 6 times.@ -84,0 +214,4 @@
const bool p_exceeded = (prev != NULL) && (tdn->h1[0] < prev->end);
const bool n_exceeded = (next != NULL) && (tdn->h2[0] > next->start);
if ((p_exceeded && n_exceeded) || (iter == 4)) {
Replace
iter == 4
withiter == iter_max
👍
I approve the result of the changes from art/animation side of things. Also trying out the review button, fingers crossed it works :)
Reviewers