Animation: Allow NLA strips to be horizontally shuffled #105532

Merged
Nate Rupsis merged 22 commits from nrupsis/blender:NLA-horizontal-shuffle into main 2023-04-03 17:10:50 +02:00
Member

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.

image


Abstracted large conditional branch in recalcData_nla into it's own nlastrip_fix_overlapping method for increased readability. No logical changes were made.


Archived Phabricator Patch

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. ![image](https://cdn.discordapp.com/attachments/993237979705376818/1084987601670385684/NLA_shuffle.gif) --- Abstracted large conditional branch in `recalcData_nla` into it's own `nlastrip_fix_overlapping` method for increased readability. No logical changes were made. --- [Archived Phabricator Patch](https://archive.blender.org/developer/D10102)
Nate Rupsis added the
Module
Animation & Rigging
label 2023-03-07 16:50:00 +01:00
Nate Rupsis added 2 commits 2023-03-07 16:50:03 +01:00
Nate Rupsis added this to the Animation & Rigging project 2023-03-07 16:50:11 +01:00
Nate Rupsis added 1 commit 2023-03-09 17:43:55 +01:00
Nate Rupsis added 7 commits 2023-03-12 05:50:50 +01:00
Nate Rupsis added 1 commit 2023-03-13 23:51:52 +01:00
Nate Rupsis added 1 commit 2023-03-14 00:07:53 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
28adf6418d
adding in some comments
Nate Rupsis changed title from WIP: NLA-horizontal-shuffle to WIP: Animation: Allow NLA strips to be horizontally shuffled 2023-03-14 00:20:37 +01:00
Nate Rupsis changed title from WIP: Animation: Allow NLA strips to be horizontally shuffled to Animation: Allow NLA strips to be horizontally shuffled 2023-03-14 00:54:53 +01:00
Nate Rupsis requested review from Brad Clark 2023-03-14 00:55:03 +01:00
Nate Rupsis requested review from Sybren A. Stüvel 2023-03-14 00:55:17 +01:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105532) when ready.

Thanks for the reviews & the testing!

Oops, wrong PR!

~~Thanks for the reviews & the testing!~~ Oops, wrong PR!
Sybren A. Stüvel requested changes 2023-03-21 13:04:18 +01:00
Sybren A. Stüvel left a comment
Member

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.

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.

Document that this can even remove and free `strip` when it doesn't fit any more.
nrupsis marked this conversation as resolved
@ -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.

Better name would be `nlastrip_validate_transition_start_end`, to show that it's about transitions only.
nrupsis marked this conversation as resolved
@ -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 with strip = strip->next. You'll probably want to replace the loop with a call to LISTBASE_FOREACH_MUTABLE(). That'll determine the next pointer before entering the loop body.

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 with `strip = strip->next`. You'll probably want to replace the loop with a call to `LISTBASE_FOREACH_MUTABLE()`. That'll determine the `next` pointer before entering the loop body.
nrupsis marked this conversation as resolved
@ -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 to if (solo ...), by all means!
</rant>

`<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 to `if (solo ...)`, by all means! `</rant>`
nrupsis marked this conversation as resolved
@ -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.

Nice assertions, really helps to understand the math that follows.
nrupsis marked this conversation as resolved
@ -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 ;-)

I think one set of curlies is enough ;-)
nrupsis marked this conversation as resolved
@ -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 from recalcData_nla(), and that the logic was kept as-is. That way people won't hold you responsible for the code ;-)

Maybe add a note in the PR description (and thus the commit message) that the contents of `nlastrip_fix_overlapping()` were just moved from `recalcData_nla()`, and that the logic was kept as-is. That way people won't hold you responsible for the code ;-)
nrupsis marked this conversation as resolved
@ -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()

Use `LISTBASE_FOREACH()`
nrupsis marked this conversation as resolved
@ -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.

"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.
nrupsis marked this conversation as resolved
Nate Rupsis added 4 commits 2023-03-27 20:04:53 +02:00
Nate Rupsis added 1 commit 2023-03-27 20:06:40 +02:00
Nate Rupsis added 1 commit 2023-03-27 21:02:53 +02:00
Nate Rupsis requested review from Sybren A. Stüvel 2023-03-27 21:03:35 +02:00
Sybren A. Stüvel requested changes 2023-03-28 13:26:24 +02:00
@ -1969,0 +1988,4 @@
nlastrip_validate_transition_start_end(strip);
if (strip == NULL) {

nlastrip_validate_transition_start_end() doesn't change the strip pointer variable, so the if (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 a NlaStrip **strip parameter so that it can modify the strip variable itself, but I'm not really a fan of those.

`nlastrip_validate_transition_start_end()` doesn't change the `strip` pointer variable, so the `if (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 a `NlaStrip **strip` parameter so that it can modify the `strip` variable itself, but I'm not really a fan of those.
nrupsis marked this conversation as resolved
@ -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.

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.
nrupsis marked this conversation as resolved
@ -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?

Why return? Shouldn't the remaining strips be checked too?
nrupsis marked this conversation as resolved
@ -84,0 +209,4 @@
PointerRNA strip_ptr;
for (short iter = 0; iter < 5; iter++) {

Instead of hard-coding 5 (here) and 4 (below), create a constant and use it in both places. That way the relationship between the two values is explicit.

Instead of hard-coding `5` (here) and `4` (below), create a constant and use it in both places. That way the relationship between the two values is explicit.
nrupsis marked this conversation as resolved
@ -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.

Variables should use `snake_case`.
nrupsis marked this conversation as resolved
@ -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 use t->mode == TFM_TRANSLATION. Same for the other is_translating variable.

Do you expect more `TFM_xxx` constants to be added here? If not, just use `t->mode == TFM_TRANSLATION`. Same for the other `is_translating` variable.
nrupsis marked this conversation as resolved
@ -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.

Don't use doxygen for comments inside functions.
nrupsis marked this conversation as resolved
Nate Rupsis reviewed 2023-03-28 17:27:27 +02:00
@ -1969,0 +1988,4 @@
nlastrip_validate_transition_start_end(strip);
if (strip == NULL) {
Author
Member

Would it make more sense to have a <= condition in the for loop, and use the same iter_max variable for both conditional checks? I.e

short iter_max = 4;


 for (short iter = 0; iter <= iter_max; iter++) { 
 ...

 if ((p_exceeded && n_exceeded) || (iter == iter_max)) {
 ...


Would it make more sense to have a `<=` condition in the for loop, and use the same `iter_max` variable for both conditional checks? I.e ``` short iter_max = 4; for (short iter = 0; iter <= iter_max; iter++) { ... if ((p_exceeded && n_exceeded) || (iter == iter_max)) { ... ```
Author
Member

I think the boolean return type is more expressive, and arguably more readable (with an informative comment on the function)

I think the boolean return type is more expressive, and arguably more readable (with an informative comment on the function)
nrupsis marked this conversation as resolved
@ -1969,0 +1992,4 @@
printf(
"NLA strip was in an invalid location and was removed by "
"nlastrip_validate_transition_start_end \n");
return;
Author
Member

oop, nope that was a dumb mistake on my part. Should be a continue.

oop, nope that was a dumb mistake on my part. Should be a continue.
nrupsis marked this conversation as resolved
Nate Rupsis reviewed 2023-03-28 17:28:48 +02:00
Nate Rupsis added 1 commit 2023-03-28 19:37:29 +02:00
Nate Rupsis requested review from Sybren A. Stüvel 2023-03-29 17:54:23 +02:00
Sybren A. Stüvel requested changes 2023-03-30 16:56:32 +02:00
Sybren A. Stüvel left a comment
Member

Almost there!

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 returning true when valid-or-repaired and false when invalid-beyond-repair is better.

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 returning `true` when valid-or-repaired and `false` when invalid-beyond-repair is better.
nrupsis marked this conversation as resolved
@ -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.

Remove space before newline.
nrupsis marked this conversation as resolved
@ -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.

Well, with the `for`-loop as it is now, it's done 6 times.
nrupsis marked this conversation as resolved
@ -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 with iter == iter_max

Replace `iter == 4` with `iter == iter_max`
nrupsis marked this conversation as resolved
Nate Rupsis added 2 commits 2023-03-30 18:00:12 +02:00
Nate Rupsis requested review from Sybren A. Stüvel 2023-03-30 21:28:07 +02:00
Nate Rupsis added 1 commit 2023-03-31 18:51:11 +02:00
Sybren A. Stüvel approved these changes 2023-04-03 17:09:19 +02:00
Sybren A. Stüvel left a comment
Member

👍

:+1:
Nate Rupsis merged commit 8833f5dbf9 into main 2023-04-03 17:10:50 +02:00
Nate Rupsis deleted branch NLA-horizontal-shuffle 2023-04-03 17:10:51 +02:00
Brad Clark approved these changes 2023-04-03 17:27:22 +02:00
Brad Clark left a comment
Member

I approve the result of the changes from art/animation side of things. Also trying out the review button, fingers crossed it works :)

I approve the result of the changes from art/animation side of things. Also trying out the review button, fingers crossed it works :)
Sign in to join this conversation.
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
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
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
No Milestone
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
No description provided.