Page MenuHome

NLA: Always Show All Strips
ClosedPublic

Authored by Wayde Moss (GuiltyGhost) on May 2 2020, 2:25 AM.

Details

Summary

This goes hand in hand with D7601: NLA: Remain in tweak mode on new strip selection which has a comparison video.

Problem
Currently, it's difficult to use the NLA system for users who are only interested in using it as animation layers. Entering tweak mode hides strips which are not evaluated. If the user wanted to edit a different strip, they must exit tweak mode, look for the strip, select it then re-enter tweak mode.

Solution
All strips are always shown. The user can now see the next strip they want to start editing.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D7600-nla_always_show_all_strips (branched from master)
Build Status
Buildable 10129
Build 10129: arc lint + arc unit

Event Timeline

Wayde Moss (GuiltyGhost) requested review of this revision.May 2 2020, 2:25 AM
Wayde Moss (GuiltyGhost) created this revision.
Wayde Moss (GuiltyGhost) retitled this revision from NLA: Always Show All Strips and Sync Length Fix to NLA: Always Show All Strips.May 2 2020, 2:30 AM
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)
  • -when tweak mode on, upper strips draw as disabled and strips that use the same action are also properly tagged as such
Sybren A. Stüvel (sybren) requested changes to this revision.Aug 14 2020, 12:06 PM

From a user perspective I think this is a nice change. I just think the code needs a small adjustment, nothing major.

source/blender/editors/space_nla/nla_draw.c
439–441

This condition is too complex, and on top of that the result is used in another negation. It's better to transform it into a separate function. That can then check various sub-conditions, maybe have some comments to explain what's going on, and use early returns to short-cut the evaluation. You can then call the function where it's needed, instead of declaring the boolean at the top of the function but using it in the middle.

As a rule of thumb, try to avoid double negations. The existing code is also double-negating, using if (non_solo == 0), in other words "if not non-solo". If that were on my plate to review now I'd also not accept it ;-)

The condition also produces a compiler warning:

blender/source/blender/editors/space_nla/nla_draw.c: In function ‘nla_draw_strip’:
blender/source/blender/editors/space_nla/nla_draw.c:424:55: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
  424 |                               (adt->act_track == nlt) &&(adt->actstrip != strip));
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
This revision now requires changes to proceed.Aug 14 2020, 12:06 PM
Wayde Moss (GuiltyGhost) updated this revision to Diff 27750.EditedAug 15 2020, 10:48 AM
  • simplified condition and extracted into its own function.

    Some redundant checks were removed. There's no point in checking for tweak mode since the disabled flag is never arbitrarily set by a user. There's no point in checking for whether the track is the tweaked track either. Of the disabled tracks and strips, only the tweaked strip is enabled.

Thank you. I appreciate the feedback.

The updated UI looks so much better, thanks Wayde!

This revision is now accepted and ready to land.Sep 16 2020, 2:43 PM
This revision was automatically updated to reflect the committed changes.