blender
/
blender
136
411
Fork 595
Code Issues 6.3k Pull Requests 770 Projects 19 Wiki Activity

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
Conversation 3 Commits 22 Files Changed 4 +9 -9
2 changed files with 9 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 2de0b856c3 - Show all commits

14
source/blender/blenkernel/intern/nla.c
View File

@ -1953,13 +1953,13 @@ static void BKE_nlastrip_validate_autoblends(NlaTrack *nlt, NlaStrip *nls)
}
/* Ensure every transition's start/end properly set.
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-21 12:43:29 +01:00
Outdated
Review
Copy Link

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.
* Strip will be removed / freed if it doesn't fit.
* Return value indicates if passed strip was freed or not. */
* Strip will be removed / freed if it doesn't fit (invalid).
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-21 12:42:56 +01:00
Outdated
Review
Copy Link

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.
* Return value indicates if passed strip is valid/fixed or invalid/removed. */
static bool nlastrip_validate_transition_start_end(NlaStrip *strip)
nrupsis marked this conversation as resolved
Sybren A. Stüvel commented 2023-03-30 16:47:40 +02:00
Review
Copy Link

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.
👍 1
{
if (!(strip->type & NLASTRIP_TYPE_TRANSITION)) {
return false;
return true;
}
if (strip->prev) {
strip->start = strip->prev->end;
@ -1969,9 +1969,9 @@ static bool nlastrip_validate_transition_start_end(NlaStrip *strip)
}
if (strip->start >= strip->end || strip->prev == NULL || strip->next == NULL) {
BKE_nlastrip_free(strip, true);
return true;
return false;
}
return false;
return true;
}
void BKE_nla_validate_state(AnimData *adt)
@ -1988,10 +1988,10 @@ void BKE_nla_validate_state(AnimData *adt)
for (nlt = adt->nla_tracks.first; nlt; nlt = nlt->next) {
LISTBASE_FOREACH_MUTABLE (NlaStrip *, strip, &nlt->strips) {
if (nlastrip_validate_transition_start_end(strip)) {
if (!nlastrip_validate_transition_start_end(strip)) {
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-28 13:12:58 +02:00
Outdated
Review
Copy Link

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.
Nate Rupsis commented 2023-03-28 17:17:28 +02:00
Outdated
Review
Copy Link

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)) { ... ```
Nate Rupsis commented 2023-03-28 17:24:45 +02:00
Outdated
Review
Copy Link

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)
printf(
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-21 12:44:59 +01:00
Outdated
Review
Copy Link

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.
"While moving NLA strips, a transition strip could no longer be applied to the new "
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-28 13:20:18 +02:00
Outdated
Review
Copy Link

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.
"positions and was removed. \n");
"positions and was removed.\n");
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-30 12:06:26 +02:00
Outdated
Review
Copy Link

Remove space before newline.

Remove space before newline.
continue;
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-28 13:20:36 +02:00
Outdated
Review
Copy Link

Why return? Shouldn't the remaining strips be checked too?

Why return? Shouldn't the remaining strips be checked too?
Nate Rupsis commented 2023-03-28 17:23:33 +02:00
Outdated
Review
Copy Link

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.
}

4
source/blender/editors/transform/transform_convert_nla.c
View File

@ -204,7 +204,7 @@ static void nlastrip_fix_overlapping(TransInfo *t, TransDataNla *tdn, NlaStrip *
*
* this is done as a iterative procedure (done 5 times max for now)
nrupsis marked this conversation as resolved
Sybren A. Stüvel commented 2023-03-30 16:52:09 +02:00
Review
Copy Link

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.
*/
short iter_max = 5;
short iter_max = 4;
NlaStrip *prev = BKE_nlastrip_prev_in_track(strip, true);
NlaStrip *next = BKE_nlastrip_next_in_track(strip, true);
@ -214,7 +214,7 @@ static void nlastrip_fix_overlapping(TransInfo *t, TransDataNla *tdn, NlaStrip *
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)) {
if ((p_exceeded && n_exceeded) || (iter == iter_max)) {
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-30 16:52:26 +02:00
Outdated
Review
Copy Link

Replace iter == 4 with iter == iter_max

Replace `iter == 4` with `iter == iter_max`
/* both endpoints exceeded (or iteration ping-pong'd meaning that we need a
* compromise)
* - Simply crop strip to fit within the bounds of the strips bounding it