VSE: Improve retiming UI #109044
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#109044
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "iss/blender:retiming-gizmos2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently retiming is quite awkward, when you need to retime multiple
strips strips in sync. It is possible to use meta strips, but this is
still not great. This is resolved by implementing selection.
General changes:
Gizmos are removed, since they are designed to operate only on active
strip and don't support selection.
Transform operator code is implemented for retiming data, which allows
more sophisticated manipulation.
Instead of drawing marker-like symbols, keyframes are drawn to
represent retiming data. Retiming handles are now called keys. To have
consistent names, DNA structures have been renamed.
Retiming data is drawn on strip as overlay.
UI changes:
Retiming tool is removed. To edit retiming data, press Ctrl + R, select
a key and move it. When retiming is edited, retiming menu and
context menu shows more relevant features, like making transitions.
Strip and retiming key selection can not be combined. It is possible to
use box select operator to select keys, if any key is selected.
Otherwise strips are selected.
Adding retiming keys is possible with I shortcut or from menu.
Retiming keys are always drawn at strip left and right boundary. These
keys do not really exist until they are selected. This is to simplify
retiming of strips that are resized. These keys are called "fake keys"
in code.
API changes:
Functions, properties and types related to retiming handles are renamed
to retiming keys:
retiming_handle_add() -> retiming_key_add()
retiming_handle_move() -> retiming_key_move()
retiming_handle_remove() -> retiming_key_remove()
retiming_handles -> retiming_keys
RetimingHandle -> RetimingKey
Retiming editing "mode" is activated by setting
Sequence.show_retiming_keys
.da7772a43a
to10c9e86941
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot build
@blender-bot package
Package build started. Download here when ready.
Comments from the VSE chat(which have now been deleted, due to time restrain):
This patch does not draw "needles" anymore. But in any case, I would rather not use top of strips for drawing retiming stuff.
In the latest build, selecting the Retime tool and then left-clicking on a strip will switch the tool to Tweak.
Took me a long time to realize I had to right-click on the strip to insert a speed point, since the VSE in all other situations only have one generic right-click menu.
Maybe use the current right-click behavior on left-click as well?
Trim numbers will collide with speed values:
Maybe placing the speed values and keys vertically centered in strips would be a better solution?
Texts on thumbnails will need an outline or a background box to ensure they're readable, no matter the background.
Left-clicking on a key, should immediately select it, currently it needs two clicks.
Consider color coding the yellow line, depending on value is less than 100%, exactly 100%, or over 100%(see #3 in image above)
It isn't possible to jump between the speed keyframes using the jump-to-keyframe operator.
(Displaying keyframes like this could be considered for all keyable strip values)
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
WIP: VSE: Improve retiming UIto VSE: Improve retiming UI@blender-bot package
Package build started. Download here when ready.
@Sergey Hi, can you review this patch? Mainly following parts:
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
I do not think we should be adding a dedicated list to do selection book-keeping.
It helps in cases when you need to traverse all selection, but it introduces extra requirement for ensuring things are in sync between the selection list and actual data. it also makes it less efficient to do queries like "is this retiming key selected?". I also don't think the traversal over strips and keys is something that users will contribute user-measurable time difference.
I would not pre-emptively add data management complexity.
That being said. if you have some demo .blend file which shoes that simpler bit in a key's bitfield causes unacceptable performance, we'd better first look why is it exactly so. And if that is not solvable only then go the route of a more complicated data management.
I have used managed selection, mainly because I believe, that it is good solution. I wanted to do this for strips as well. I believe this is good, because you have 1 managed system which is rather transparent instead of number of different caches or performance specific optimizations or features that were not implemented, because of this limitation.
I will test performance traversing strips, and if it is acceptable, will change selection implementaton. I am rather unhappy with it, since I can't easily reference retiming key pointer, since they are in array which gets reallocated rather often.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
Tried the latest. As mentioned, is the first thing a user would to is to left-click on the strip, and it'll change tool, and you do not notice, since the mouse cursor stays the same. Since the tool is automatically changing, there should be some mouse cursor change(even if it is a place holder), so you can see if the retime tool is active or not.
Selecting in the bottom of the strip is problematic, not only bc trimming number collide with the re-timing values(as mentioned), but also bc the bottom of the sequencer area is packed with the lowest channel(which you can't get out of that area), scrollbar, view last action and markers(GIF):
I know this wip, but switching on the markers not only reveals a brutal black bar, but also that the keys are drawn on top of the bar and are still selectable(GIF).
Also, when slowing a strip, it'll extend across the next strip(overlapping), instead of relocating, as expected.
The missing click-drag interaction of the keys, often feels like they're broken. Click-drag of keys is working in ex. the Graph Editor.
As the middle area of a strip should be considered the content area, and as such, the re-timing elements could be centered vertically in the strips. The font could be more dominant than the trim numbers and the strip info, to set re-timing apart from the rest, and indicate that this is content related.
I guess the overlap was implemented only in previous operators, will fix for transform. I know about some UI elements that should be hidden, will fix this as well.
The different cursor probably won't be implemented. Not sure if thinking about accidental actions is great way to do design. But sure if there is any way how to emphasize context, that would be nice.
These block retiming keys only if you don't move the view. Don't think that is a problem.
What do you mean by this?
Perhaps that is true, the issue with middle position is that with small strips you would have same problem anyway. That said, it doesn't look great with this patch either.
If you compare with the Blade tool, the retiming tool is breaking the convention. You select the Blade tool, the mouse cursor changes(so you can see you're in tool mode), and then you left-click on the strip and something happens. When you select the retiming tool, the mouse cursor doesn't change(so you don't know that you're in retiming tool mode), and when you left-click on the strip nothing happens... well, actually something is happening: you deselected the tool. This is both inconsistent and also not a very good/intuitive UX.
Also, having tool related operators exposed in the menu seems very unconventional to me. Normally, users would look in the sidebar or tool header for options relating to the tool. And having the operators exposed when the tool is not active, makes it confusing if this is actually a tool, and should be a tool? IMO, should the options and the re-time values and keys only be visible when the tool is active. If at some point there will be other keys on strips, for ex. volume, the strips will be a cluttered mess.
But they do block the view, and the hot zones are intersecting, isn't that visible in the gif? Ex. you can't see the strip, but the keys belonging to the strip are drawn on top of the marker's black bar, and you can still select them. When a black bar is drawn like that, the expectation would be that it separating every thing in the front from everything in the back. But with click through and something drawn below and something above, it's really confusing.
When you change re-timing keys, first you need to click (and release), and then you'll have to click again to move them. In most of Blender you click and hold to ex. move keys. Users would expect the same behavior here.
The main problem with scaling is that the texts aren't scaled. As it is possible to scale the texts in the channel headers, maybe it should be considered in strips too, so all the elements of a strip stays in relative size but are scaled down like the channel headers or the ex. the nodes. This way things will not collide when scaled.
There is also the problem of white text on a potential white background. If you do not like the centered position, then make an area with the same height as the strip text info area, but place it just below it, and then have a semi transparent dark box under the keys and values.
I am not sure how the storage of a selection affects on whether you can or can not reference retiming key.
Seq->retiming_keys
is reallocated when strip is added for example. So if you store pointer to a key to reuse it later it may be invalid after some operations. This is a bit frustrating limitation. Not sure how other data structures deal with this, but I should have a look.If the selection is stored as a bitfield on the
RetimingKey
then I don't see a problem.@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@ -0,0 +149,4 @@
return rect;
}
static bool mouse_is_inside_box(const rctf *box, const int mval[2])
Use
BLI_rctf_isect_pt
, no need to re-implement the condition here.I was mainly testing the patch on the user level so far. Seems like a lot of work still needs to be done. The major points are:
Strip -> Retiming -> Set Speed
menu creates retiming key at a seemingly wrong position (so that part of the strip is played at the given speed, and other part of the strip is played at a seemingly arbitrary speed).This is just from a quick session of trying to use the tool.
it would be nice if you could go through typical workflow steps and make sure they work as an artist would expect them to.
The selection was designed in a way, that you should not need to switch the tool manually. I guess situation you are experiencing is, that you want to retime whole strip so keys are not visible when tool is not active? Perhaps there could be a flag, that indicates whether the tool was activated manually and not switch tools automatically in such case.
I have implemented a "virtual" keys drawing and selection, so I can handle this. When strip is cut, it would be responsibility of cut operator to create keys if strip is retimed. I can do that here.
My goal with this patch was feature parity with old system. These cases were not handled previously and I didn't want to introduce more changes than necessary, even though this would be relatively small change.
Set speed operator would be used probably 90% of time. So will fix the issue with cut strip.
One of the main decisions here is whether this should be a tool or not?
As it is now, Retime options in the Strip menu and the retime keys are continuously displayed and editable, which makes it unnecessary to even select the Retime tool.
If this path is chosen, the next question is how and when to display keys for all other strip options like Volume or Opacity for fades etc. (which users will expect)? Should they also be visible and editable all the time? How then to avoid clutter accidental mis-selection of keys instead of ex. handles or strips(when small)?
If, on the other hand, the visibility and editability of retiming keys should be limited to when the tool is selected, what to do with the operators in the menu? Only displaying the retime-menu when in tool mode, will not make users find it. Should an exception be made and add those operators to the Tool sidebar instead? Is a Retime right click menu something users will be able to find, now the VSE only have one type of Sequencer context menu, ignoring all actual context(like selection, hover position, tool etc.)?
Hi Richard, I created #112343 as a design guideline for this task. This is a summary of past discussions we had, and has been reviewed by other UI stakeholders as well. The main difference from the current implementation is that:
If you could address these 2 points, the patch looks good to me. There are a few bugs, but they can be fixed later.
@blender-bot package
Package build started. Download here when ready.
@blender-bot build
@blender-bot build
@blender-bot package
Package build started. Download here when ready.
On a user level this is definitely a great improvement!
The code is quite large at this point, and only had some limited time to look into it. There are some small points so far.
The patch description needs to:
@ -2910,3 +2910,3 @@
("wm.context_toggle_enum", {"type": 'TAB', "value": 'PRESS', "ctrl": True},
{"properties": [("data_path", 'space_data.view_type'), ("value_1", 'SEQUENCER'), ("value_2", 'PREVIEW')]}),
("sequencer.refresh_all", {"type": 'R', "value": 'PRESS', "ctrl": True}, None),
("sequencer.refresh_all", {"type": 'E', "value": 'PRESS', "ctrl": True}, None),
Do we really need this shortcut? Would be really nice to get rid of this legacy "safety" operator.
There are few cases where this is needed. Notably for scene strips and in some cases when strips are animated (#98432, #90041).
@ -930,0 +942,4 @@
layout.separator()
layout.operator("sequencer.retiming_segment_speed_set")
layout.operator("sequencer.retiming_show")
For a good UI this needs to be a toggle, so that from looking at the menu item it is clear whether retiming is shown or not.
This is per strip option and is applied to selection. Right now it inverts state, which is not great.
I will do it as we do with select all operator - get active strip state, and apply inverted to selection. This way I can show what happens in menu.
@ -1211,2 +1214,4 @@
}
FOREACH_NODETREE_END;
LISTBASE_FOREACH (bScreen *, screen, &bmain->screens) {
It is fine for the development purposes. The patch would need to bump subversion before landing. Otherwise save+reload will enforce retiming visibility despite of possible user desire.
@ -124,2 +124,4 @@
{
/* Keep this block, even when empty. */
FROM_DEFAULT_V4_UCHAR(space_sequencer.keytype_keyframe);
Eventually this would also need to be moved to a subversion check.
@ -1,4 +1,4 @@
/* SPDX-FileCopyrightText: 2008 Blender Authors
/* SPDX-FileCopyrightText: 2008 Blender Foundation
This needs to be "Blender Authors".
@ -10,6 +10,7 @@
#include "DNA_sequence_types.h"
#include "RNA_access.hh"
#include <BLI_vector.hh>
#include "BLI_vector.hh"
@ -555,0 +830,4 @@
for (auto item : SEQ_retiming_selection_get(SEQ_editing_get(scene)).items()) {
strips_to_handle.append_non_duplicates(item.value);
item.key->flag |= DELETE_KEY;
Maintain a local
blender::Set
of keys to be handled instead of introducing a specific temporary flag.@ -0,0 +1,561 @@
/* SPDX-FileCopyrightText: 2022 Blender Foundation
2023 Blender Authors.
@ -0,0 +148,4 @@
return rect;
}
int left_fake_key_frame_get(const bContext *C, const Sequence *seq)
What exactly the fake key is?
It is key that is drawn and can be selected, but does not exist in strip retiming data. This concept is used because when you resize strip, a key can not be created automatically, utherwise you will create unwanted keys. Now fake key is realized when it is clicked. Even better would be to realize these when they are moved, but I am not sure how I would do that right now
@ -0,0 +169,4 @@
const View2D *v2d = UI_view2d_fromcontext(C);
rctf box = keys_box_get(C, seq);
box.xmax += RETIME_KEY_MOUSEOVER_THRESHOLD;
Why these modifications are needed?
Each key has clickable area around it. if the key is at the edge of the strip, I want it to have same clickable area.
Such information needs to be a comment in the code. It is not immediately obvious, and with a bit of time it will not be possible to find the code review discussion.
Wouldn't it be more cleat to expand the bounds in the
keys_box_get
?@ -0,0 +49,4 @@
td2d->loc[0] = SEQ_retiming_key_timeline_frame_get(scene, seq, key);
td2d->loc[1] = key->retiming_factor;
td2d->loc2d = NULL;
nullptr
in C++ code.@ -0,0 +62,4 @@
tdseq->orig_timeline_frame = SEQ_retiming_key_timeline_frame_get(scene, seq, key);
tdseq->key_index = SEQ_retiming_key_index_get(seq, key);
td->extra = (void *)tdseq;
static_cast<void*>(tdseq)
. See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast@ -0,0 +101,4 @@
TransData *td = tc->data;
TransData2D *td2d = tc->data_2d;
/*for elem*/
Not sure how this comment helps.
@ -125,2 +121,2 @@
typedef struct SeqRetimingHandle {
typedef enum eSeqRetimingKeyFlag {
SPEED_TRANSITION_IN = (1 << 0),
These needs to be named more uniqcly to avoid possible confusion with other systems. For example, prefix with
SEQ_
.@blender-bot package
Package build started. Download here when ready.
There are no changes to python API from current implementation. Not sure what you mean by broken API, it seems to work here?
Or do you mean overall design of retiming system currently in main?
The python API change still needs to be stated in the patch description and later in the commit message and release notes. It would also need to be stated some common shortcuts like I key and such, so that it i more clear for a reader how to use this new awesome feature.
Some simple comment in the code. That is the only part which is not really obvious to me.
The rest seemed fine.
Make sure the patch compiles on all platforms and do not introduce new warnings.
@ -0,0 +243,4 @@
for (Sequence *seq : sequencer_visible_strips_get(C)) {
rctf box = keys_box_get(C, seq);
box.xmax += RETIME_KEY_MOUSEOVER_THRESHOLD; /* Fix selecting last key. */
How about the first key?
@ -0,0 +502,4 @@
const SeqRetimingKey *next_key = key + 1;
if (key_x_get(scene, seq, next_key) < start_frame || key_x_get(scene, seq, key) > end_frame) {
return; /* Label out of strip bounds. */
/* Label is out of the strip bounds. */
@ -0,0 +510,4 @@
size_t label_len = label_str_get(seq, key, label_str, sizeof(label_str));
if (!label_rect_get(C, seq, key, label_str, label_len, &label_rect)) {
return; /* Not enough space to draw label. */
/* Not enough space to draw the label. */
@Sergey Ah yes, I have renimed structs and API with that, completely forgot about that. Will mention this is description.
@ -555,0 +879,4 @@
}
strips_to_handle.append_non_duplicates(item.value);
item.key->flag |= SEQ_DELETE_KEY;
From some of the previous inlined comments which seems to have gotten lost: Maintain a local
blender::Set
of keys to be handled instead of introducing a specific temporary flag.I can not do that, because
SEQ_retiming_remove_key()
reallocatesseq->retiming_keys
. So all pointers to keys in a strip will be invalid when any one is deleted.You can have
SEQ_retiming_remove_multiple_keys
which removes all provided keys with a single re-allocation and copy of keys which are not in the set. This avoid multiple re-allocations and the loop-until-nothing-is-deleted code.And another thing you can do is to store indices, not pointers.
@ -0,0 +228,4 @@
for (Sequence *seq : sequencer_visible_strips_get(C)) {
rctf box = keys_box_get(C, seq);
box.xmax += RETIME_KEY_MOUSEOVER_THRESHOLD; /* Fix selecting last key. */
This is not needed anymore?
Just FYI the last update does not work, I had to move to different machine, so I used git to move the work.
That PC is toast :((
@blender-bot builld
Unknown command
builld
. See documentation for details.@blender-bot package
Package build started. Download here when ready.
I only quickly checked the updates. The
SEQ_retiming_remove_multiple_keys
seems a bit tricky, and perhaps I'd do it differently. But at this point is probably better to address the remaining issues and possible bugs after the initial merge.Make sure things are well documented in the release notes.