blender
/
blender
136
411
Fork 595
Code Issues 6.3k Pull Requests 771 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 +20 -16
3 changed files with 20 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5150420ecf - Show all commits

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

@ -1974,7 +1974,6 @@ static void nlastrip_validate_transition_start_end(NlaStrip *strip)
void BKE_nla_validate_state(AnimData *adt)
{
NlaStrip *strip = NULL;
NlaTrack *nlt;
/* sanity checks */
@ -1985,10 +1984,17 @@ void BKE_nla_validate_state(AnimData *adt)
/* Adjust blending values for auto-blending,
* and also do an initial pass to find the earliest strip. */
for (nlt = adt->nla_tracks.first; nlt; nlt = nlt->next) {
for (strip = nlt->strips.first; strip; strip = strip->next) {
LISTBASE_FOREACH_MUTABLE (ListBase *, strip, &nlt->strips) {
nlastrip_validate_transition_start_end(strip);
if (strip == NULL) {
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.
"NLA strip was in an invalid location and was removed by "
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.
"nlastrip_validate_transition_start_end \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.
return;
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.
}
/* auto-blending first */
BKE_nlastrip_validate_autoblends(nlt, strip);
BKE_nlastrip_recalculate_blend(strip);

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

@ -181,17 +181,16 @@ static void nlastrip_overlap_reorder(TransDataNla *tdn, NlaStrip *strip)
* strips for overlap instead of all of them. */
static void nlastrip_flag_overlaps(NlaStrip *strip)
{
{
NlaStrip *adj_strip = strip->prev;
if (adj_strip != NULL && !(adj_strip->flag & NLASTRIP_FLAG_SELECT) &&
nlastrip_is_overlap(strip, 0, adj_strip, 0)) {
strip->flag |= NLASTRIP_FLAG_INVALID_LOCATION;
}
adj_strip = strip->next;
if (adj_strip != NULL && !(adj_strip->flag & NLASTRIP_FLAG_SELECT) &&
nlastrip_is_overlap(strip, 0, adj_strip, 0)) {
strip->flag |= NLASTRIP_FLAG_INVALID_LOCATION;
}
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-21 12:54:51 +01:00
Outdated
Review
Copy Link

I think one set of curlies is enough ;-)

I think one set of curlies is enough ;-)
NlaStrip *adj_strip = strip->prev;
if (adj_strip != NULL && !(adj_strip->flag & NLASTRIP_FLAG_SELECT) &&
nlastrip_is_overlap(strip, 0, adj_strip, 0)) {
strip->flag |= NLASTRIP_FLAG_INVALID_LOCATION;
}
adj_strip = strip->next;
if (adj_strip != NULL && !(adj_strip->flag & NLASTRIP_FLAG_SELECT) &&
nlastrip_is_overlap(strip, 0, adj_strip, 0)) {
strip->flag |= NLASTRIP_FLAG_INVALID_LOCATION;
}
}
@ -749,13 +748,12 @@ static void special_aftertrans_update__nla(bContext *C, TransInfo *t)
}
ListBase anim_data = {NULL, NULL};
bAnimListElem *ale;
short filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_FOREDIT | ANIMFILTER_FCURVESONLY);
/* get channels to work on */
ANIM_animdata_filter(&ac, &anim_data, filter, ac.data, ac.datatype);
for (ale = anim_data.first; ale; ale = ale->next) {
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
NlaTrack *nlt = (NlaTrack *)ale->data;
nrupsis marked this conversation as resolved
Sybren A. Stüvel commented 2023-03-21 13:00:31 +01:00
Review
Copy Link

Use LISTBASE_FOREACH()

Use `LISTBASE_FOREACH()`
/* make sure strips are in order again */

2
source/blender/makesdna/DNA_anim_types.h
View File

@ -835,7 +835,7 @@ typedef enum eNlaStrip_Flag {
/** 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. */
* location must be corrected after the transform operator is done. */
nrupsis marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-03-21 13:03:15 +01:00
Outdated
Review
Copy Link

"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.
NLASTRIP_FLAG_INVALID_LOCATION = (1 << 28),
/** NLA strip should ignore frame range and hold settings, and evaluate at global time. */
NLASTRIP_FLAG_NO_TIME_MAP = (1 << 29),