Differential D12651

Fix T91743: Unify behavior of 'Auto Set Preview Range'
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Sep 27 2021, 5:56 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)

Maniphest Tasks

T91743: "Auto-set Preview Range" don't work

Commits

rB47caeb8c2668: Fix T91743: Unify behavior of 'Auto Set Preview Range'

Summary

This is available in the DopeSheet, GraphEditor, and NLA Editor.

Currently:

Dopesheet advertises to take selection into account...
- ...but doesnt - which might be a mistake in rBe3842d1ca4dd
Graph Editor does not mention selection...
- ...and also does not take it into account
NLA does not mention selection...
- ...but takes it into account

Now:

make them all take selection into account (you can still do a quick 'Select All' prior to get the full range -- better than not being able to set this based on selection)
mention this for all in the tooltip.
also reword to 'Set Preview Range to Selected' since using the term 'Auto' impilies this would change on selection change.

Diff Detail

Repository

rB Blender

Branch

T91743 (branched from master)

Build Status

Buildable 17819
Build 17819: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Sep 27 2021, 5:56 PM

Philipp Oeser (lichtwerk) created this revision.

Harbormaster completed remote builds in B17347: Diff 42501.Sep 27 2021, 5:56 PM

Philipp Oeser (lichtwerk) added a project: Animation & Rigging.Sep 27 2021, 5:57 PM

Brad Clark (RiggingDojo) awarded a token.Sep 30 2021, 9:48 PM

This was discussed last A&R module meeting (notes). The people testing things shared your confusion & welcomed consistency :)

There was one note that could make this patch a bit more complex, and that is to make the NLA look at the keys of the selected strips, and not the entire (potentially held/cycled indefinitely) strip. Do you think that would be something to fix in this patch as well? Or would it be too big of a change and better to do it separately?

3 things(relating to this function, but not the unifying work):

Using the word "auto" makes me think that it'll dynamically update as selection changes. Maybe "Set Selected as Preview Range" if all functions are unified to this functionality?
I don't see any reason for not keeping animation editors in consistency, when it comes to adding this function to the VSE.
Alternatively, a "Loop around Current Frame" function, might be more useful? (Which auto sets a preview range of ex. 5 sec with the playhead in the center(or at the start?) while playing, and removing that preview range again when stopping it).

improve operator description (clarify NLA uses selected strip extends - not keyframes)
reword to 'Set Preview Range to Selected' (to remove the misleading use of the term 'Auto')

Harbormaster completed remote builds in B17819: Diff 43290.Oct 14 2021, 12:22 PM

In D12651#333704, @Sybren A. Stüvel (sybren) wrote:

This was discussed last A&R module meeting (notes). The people testing things shared your confusion & welcomed consistency :)

There was one note that could make this patch a bit more complex, and that is to make the NLA look at the keys of the selected strips, and not the entire (potentially held/cycled indefinitely) strip. Do you think that would be something to fix in this patch as well? Or would it be too big of a change and better to do it separately?

I would indeed keep this change separate (also because I am not sure behavior of keyframes vs. strip extends is desired in all scenarios)

In D12651#334763, @Peter Fog (tintwotin) wrote:

Using the word "auto" makes me think that it'll dynamically update as selection changes. Maybe "Set Selected as Preview Range" if all functions are unified to this functionality?