Page MenuHome

VSE: Limit timeline view height
ClosedPublic

Authored by Richard Antalik (ISS) on Mar 7 2022, 4:40 PM.

Details

Summary

Maximum zoom or view height is now defined by space occupied by strips,
But at least channels 1 to 7 will be always visible. This makes it
overview timeline content by zooming out to maximum extent in Y axis
and panning in X axis.

More channels can be "created" on demand by moving strip to higher
channel. When any strip is removed from channel, operators, should use
flag SPACE_SEQ_CLAMP_SMOOTH to do smooth transition when limiting
height.

Limiting takes into account height of scrubbing and markers area as
well as scrollers. This means that when zoomed out to maximum extent,
no strips are obstructed by fixed UI element.

Fixes T57976

Demo:

Diff Detail

Repository
rB Blender
Branch
transform-edgepan (branched from master)
Build Status
Buildable 20988
Build 20988: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Mar 7 2022, 4:40 PM
Richard Antalik (ISS) created this revision.
  • Forgot view all operator
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 8 2022, 2:10 PM
  • revert last update - applied to wrong diff
  • Make exceptions for clamping, so edge panning can be used
  • Merge remaining changes from D14263
  • Use edge panning method implemented for nodes
  • Use smoothview for delete operator, fix warnings
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 9 2022, 1:02 PM
  • Revert "fix warnings" - it did not help much, Fix incorrect boundbox calculation
  • Add limiting option for edge panning. Perhaps v2d->min and max could be used, but in sequencer content is displayed beyond this boundary, so will have to check if I can do it so it is consistent with current behavior.
  • Fix refaactoring unintended changes
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 10 2022, 6:01 PM
  • Remove hard-coded region dedicated for markers in delete operator. Ref T90964

Diff only relevant changes

  • Remove xxx comment

Remove unrelated changes

Remove unrelated changes manually...

We tested and discussed this quite a bit here at the studio, Hjalti gave some initial feedback too. Francesco, @Sebastian Parborg (zeddb) and I decided that the following should be done for now:

  • The auto-scrolling causes a number of usability issues, we shouldn't do it. It basically enforces the limits too much, and moves away too much from the canvas principle. E.g. after removing a lone strip in the uppermost channel, it should just keep the view, and not move to the next highest strip.
  • The behavior proposed here should be optional, it can be enabled by default. The option can go to the View dropdown, e.g. ViewLimit View to Contents (name is just an idea of course)
  • The edge panning should happen earlier, so the expanding of the view becomes more discoverable. A SEQ_EDGE_PAN_INSIDE_PAD of 3.5-4 should be fine, personally I prefer 3.5.

These things need to get done for this to be ready for a release.

Bugs/issues that can be addressed in bcon3:

  • According to the scrollbar, you cannot scroll all the way to the bottom.
    I think you have to add the bottom offset in the View2D.tot-rect calculations, the cur-rect may still show contents out of tot bounds I think. Or maybe it's more difficult, not sure.
  • The view panning doesn't work when you start dragging within the margin of the edge-panning (SEQ_EDGE_PAN_INSIDE_PAD). If this is increased as mentioned above, this becomes a notable issue. However this is intentional for nodes (4d605ef2f413), and managed via the View2DEdgePanData.enabled option. You could make that behavior optional, e.g. with some UI_view2d_edge_pan_set_xxx() option (not sure about the name).
  • Sometimes when testing I ended up with a channel 8 visible, without being able to scroll it into view. I don't have clear reproduction steps unfortunately.
source/blender/editors/space_sequencer/sequencer_edit.c
1781–1784

This can be committed separately I'd say.

source/blender/editors/space_sequencer/space_sequencer.c
673–675

Any reason this is done this way, and not by getting View2D.hor?

source/blender/makesdna/DNA_space_types.h
681

This flag doesn't seem to be cleared, so needs versioning.

Julian Eisel (Severin) requested changes to this revision.Apr 26 2022, 9:33 PM
This revision now requires changes to proceed.Apr 26 2022, 9:33 PM

Thanks for review, I can do these changes, but following I am not sure if I understand correctly:

  • The auto-scrolling causes a number of usability issues, we shouldn't do it. It basically enforces the limits too much, and moves away too much from the canvas principle. E.g. after removing a lone strip in the uppermost channel, it should just keep the view, and not move to the next highest strip.

Does that mean that limits would not be enforced, but it is possible to scroll only down? If that is the case and user will scroll down but there is still "free space" left, should this be a new limit, so it is not possible to scroll back up where deleted strip was?

My concern is, that without this auto scrolling, scaling and maybe even panning can feel awkward. I had some issues controlling these things so they feel intuitive. But perhaps with above exception to limiting this wouldn't be an issue.

Richard Antalik (ISS) marked 3 inline comments as done.
  • Address inlines and implement suggested changes apart from removing auto scrolling - will work on that soon
source/blender/editors/space_sequencer/space_sequencer.c
673–675

No, this was copy-pasted from elsewhere, not sure from where now.

  • Minor cleanup, If strip is deleted, don't move view automatically, keep current range until it is changed.
  • Fix build warning because UI_view2d_smooth_view does not expect bContext to be const
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Apr 28 2022, 5:31 AM

Tested briefly, seems to behave like discussed now. Two smaller things to look at, but no need for me to re-review once addressed.

source/blender/blenloader/intern/versioning_300.c
818

If we have to keep this flag (see other comment):
Please add a comment that this is clearing a reused flag bit, otherwise it's confusing. Although, shouldn't this be a runtime only flag that is always cleared on file read anyway?

source/blender/makesdna/DNA_space_types.h
683

Is this flag even needed anymore? I see it's still set, but I couldn't get to trigger the smooth view.

This revision is now accepted and ready to land.Apr 28 2022, 3:36 PM
This revision was automatically updated to reflect the committed changes.
Richard Antalik (ISS) marked 2 inline comments as done.