Implement the bounding box (xform) gizmo in the seq preview window
ClosedPublic
Actions

Authored by Sebastian Parborg (zeddb) on Oct 1 2021, 4:18 PM.

Details

Reviewers

Richard Antalik (ISS)
Campbell Barton (campbellbarton)

Commits

rB482806c81678: VSE: Implement the bounding box (xform) tool in the seq preview window

Summary

Make the "xform" gizmo available for strip transformations in the sequencer preview window.

Because of the amount of hacks needed to make the gizmo work nicely with multiple strips at the same time, it was decided to only show the translate gizmo when multiple strips are selected.

Diff Detail

Repository: rB Blender

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Oct 1 2021, 4:18 PM

Sebastian Parborg (zeddb) created this revision.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)Oct 1 2021, 4:45 PM

Forgot to add Campbell.
@Campbell Barton (campbellbarton) does the 2d gizmo drawing changes look good?
I removed the modal calculations in the drawing code as it seems to me that the result there was always overwritten by the "pre draw" callback functions either way.

Overall this works quite well.

One thing that doesn't quite work is when image origin is not in center. I think it should be possible to set gizmo to respect this, just not sure if this would mean moving gizmo center point. Ideally center point could be moved, so with transform origin mode this could be moved interactively in future, but if this is not possible currently it would be enough to just fit cage properly to image shape

Another thing that would be very nice to resolve it mouse cursor drawing when image is rotated 90 degrees - it is parallel with cage line, which is not nice. Seems, that it is not possible to rotate cursor by arbitrary amount so best would be probably to use WM_CURSOR_NSEW_SCROLL for cage lines too.

source/blender/editors/transform/transform_orientations.c
613	This will cause crash when active strip is sound and you call transform operator

If it is out of the question of getting the multi tool to work on the selection, I don't know if it is allowed to force change a tool change(in the tool bar), but in a case like the video, when selecting more strips then the multi tool could be disabled and the active tool could be changed to the selection tool?

(Minor thing, but a spacer could be inserted between the Multi tool and the Sample tool)

Looks good, accepting as I don't think extra iterations from me are needed.

Code comments should clarify what the intended long-term behavior is multiple strips - if this is meant to be a TODO,
or an acceptable limitation that could be lifted if shear is ever supported.

source/blender/editors/transform/transform_convert_sequencer_image.c
68	verticies - spelling.
202–204	Clang format wraps poorly, assigning a variable reads better.
source/blender/editors/transform/transform_gizmo_2d.c
245	Naming is spesific (object often refers to Blender objects, could be `has_select` as used elsewhere).
568	nececary - spelling.
664	diffent - spelling.
667	refered - spelling.
source/blender/editors/transform/transform_mode_resize.c
223	ortogonal - spelling.

This revision is now accepted and ready to land.Oct 6 2021, 10:05 AM

Sebastian Parborg (zeddb) marked 8 inline comments as done.Oct 6 2021, 7:09 PM

Updated patch with the latest feedback.

In D12729#334201, @Peter Fog (tintwotin) wrote:

If it is out of the question of getting the multi tool to work on the selection, I don't know if it is allowed to force change a tool change(in the tool bar), but in a case like the video, when selecting more strips then the multi tool could be disabled and the active tool could be changed to the selection tool?

We don't do automatic tool switching in these cases.
It would be very annoying having to reselect the bounding box tool is you by mistake select multiple strips.

(Minor thing, but a spacer could be inserted between the Multi tool and the Sample tool)

Done. :)

Sebastian Parborg (zeddb) updated this revision to Diff 42986.Oct 6 2021, 7:33 PM

Richard Antalik (ISS) accepted this revision.Oct 7 2021, 1:28 AM

Campbell Barton (campbellbarton) added inline comments.Oct 7 2021, 6:45 AM

source/blender/editors/transform/transform_gizmo_2d.c
258–274	This should be moved into a generic function as we have for `ED_uvedit_minmax_multi`, we'll want this to support bounding-box pivot, view to selection, etc.

Sebastian Parborg (zeddb) added inline comments.Oct 7 2021, 3:55 PM

source/blender/editors/transform/transform_gizmo_2d.c
258–274	Hmm, this code contains specific logic for the bounding box gizmo. (Bounding box coordinates are not take transform rotation into account if we are operating on a single strip) I would think it would be better to add the generic function to get the bounding box coordinates when we add support for changing pivot points and so on? I can of course do it now, but I feel it would be a bit weird to implement the function but have it be unused because the gizmo needs some special logic that won't be in the stand alone function.

Campbell Barton (campbellbarton) accepted this revision.Oct 8 2021, 4:49 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/editors/transform/transform_gizmo_2d.c
258–274	In that case leave as is. Although the case where `apply_rotation == false` looks like it could be generalized since as far as I can see this is a regular axis aligned bound-box.

Jose Carlos (chepecarlos) added a subscriber: Jose Carlos (chepecarlos).Oct 8 2021, 5:20 AM

Jose Carlos (chepecarlos) awarded a token.Oct 8 2021, 5:25 AM

Sebastian Parborg (zeddb) added inline comments.Oct 8 2021, 11:24 AM

source/blender/editors/transform/transform_gizmo_2d.c
258–274	My concern is that this would break if you call the theoretical `ED_seq_minmax_multi` with `apply_rotation == false` As then none of the strips would be rotated and thus the bounding box min/max coordinates wouldn't make any sense. Perhaps it is not that horrible, but I feel bad creating general usage functions where you have caveats like this.

Closed by commit rB482806c81678: VSE: Implement the bounding box (xform) tool in the seq preview window (authored by Sebastian Parborg (zeddb)). · Explain WhyOct 8 2021, 12:15 PM

This revision was automatically updated to reflect the committed changes.

Sebastian Parborg (zeddb) added a commit: rB482806c81678: VSE: Implement the bounding box (xform) tool in the seq preview window.

Campbell Barton (campbellbarton) added inline comments.Oct 10 2021, 2:13 AM

source/blender/editors/transform/transform_gizmo_2d.c
258–274	My mistake, I should have said `apply_rotation == true`, The point being, we have this function for objects, so I don't see why we couldn't have an equivalent function for the sequence preview.