blender/blender

UI: Sequencer Overlays popover layout improvements #121591

Merged

Pablo Vazquez merged 2 commits from pablovazquez/blender:ui-vse-overlay-panel into main

2024-05-13 12:25:49 +02:00

Pablo Vazquez commented

2024-05-08 20:07:06 +02:00

Member

With the recent addition of the Cache overlay, it was clear that the options in the Overlays popover were added as they were coded, without a specific grouping in mind. The popover needed a bit of re-organization.

In general I made it to follow the layout of the 3D Viewport overlays.

Make it wider, to fit two-column layout and allocate room for longer strings in certain translations.
Move general settings on top, such as guides.
Move strips-related overlays into their own subpanel (similar to "Objects" in the 3D Viewport overlays)
Move Thumbnails higher up so it's more easily reachable.
Rename F-Curves to Animation Curves.
Remove redundant label from waveform style, I think it's implied by context.

Before	After

Gray out "Waveform Style" when set to Off:

Example how certain translations wouldn't fit before:

Spanish Before	Spanish After

With the recent addition of the Cache overlay, it was clear that the options in the Overlays popover were added as they were coded, without a specific grouping in mind. The popover needed a bit of re-organization. In general I made it to follow the layout of the 3D Viewport overlays. * Make it wider, to fit two-column layout and allocate room for longer strings in certain translations. * Move general settings on top, such as guides. * Move strips-related overlays into their own subpanel (similar to "Objects" in the 3D Viewport overlays) * Move `Thumbnails` higher up so it's more easily reachable. * Rename `F-Curves` to `Animation Curves`. * Remove redundant label from waveform style, I think it's implied by context. |Before|After| |----|----| |![before](/attachments/5b2d719e-7f18-4eeb-966a-6a6dc1149fc0)|![after](/attachments/8ae62c8c-9a96-4a27-b2fc-736311609ce2)| Gray out "Waveform Style" when set to Off: ![Waveform Style grayed out](/attachments/d1a73a4d-4f0a-4918-9875-bda40abd670a) Example how certain translations wouldn't fit before: |Spanish Before|Spanish After| |----|----| |![spanish before](/attachments/8d063c6d-21dd-44ce-913b-74f586dc9746)|![spanish after](/attachments/1dfe103d-8a32-42d1-a9ac-6778b485aeb7)|

104 KiB

92 KiB

105 KiB

20 KiB

91 KiB

👍 1 👀 1 ❤️ 5 🎉 1 🚀 1

Pablo Vazquez added the

Video Sequencer

labels 2024-05-08 20:07:06 +02:00

Pablo Vazquez added 1 commit 2024-05-08 20:07:21 +02:00

UI: Sequencer Overlays popover layout improvements b7e523a9c0

With the recent addition of the cache toggle, it was clear that the
Sequencer Overlays popover needed a bit of re-organization.

Richard Antalik approved these changes 2024-05-08 21:45:12 +02:00

tintwotin commented

2024-05-10 07:06:30 +02:00

First-time contributor

As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off?

Maybe it would be more in consistency to have to convert enums to check-boxes:

[X] Waveforms
     [X] All (grayed out if Waveforms are not checked)
     [X] Full (grayed out if Waveforms are not checked)

For a better separation, the options could be grouped according to the areas they're located:

Header:
- Name
- Source
- Duration
- Color Tags (Maybe they should be limited to the header area(when the header is visible), so they will not interfer with ex. animation curves or waveforms?)

Content:
- Thumbnails
- Waveform (Strip, Half)
- Animation Curves
- Retiming 

Sequencer:
- Offsets
- Grid
- Cache

If the most important elements should be on top (which is the paradime in the VSE elsewhere), I would place the Grid & Cache at the bottom.

The Retiming option, I'm not really sure, it should be an overlay option, since it is displaying elements users can interoperate with, as opposed to the other options? In addition to this, as it is now, there are 3 places to check if retiming points should be visible: retime menu, time subpanel and overlay menu, and this is bound to be a source of confusion.

![image](/attachments/753d21eb-8d2a-4a79-af67-69f0df3d5796) As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off? Maybe it would be more in consistency to have to convert enums to check-boxes: ``` [X] Waveforms [X] All (grayed out if Waveforms are not checked) [X] Full (grayed out if Waveforms are not checked) ``` For a better separation, the options could be grouped according to the areas they're located: ``` Header: - Name - Source - Duration - Color Tags (Maybe they should be limited to the header area(when the header is visible), so they will not interfer with ex. animation curves or waveforms?) Content: - Thumbnails - Waveform (Strip, Half) - Animation Curves - Retiming Sequencer: - Offsets - Grid - Cache ``` If the most important elements should be on top (which is the paradime in the VSE elsewhere), I would place the Grid & Cache at the bottom. The Retiming option, I'm not really sure, it should be an overlay option, since it is displaying elements users can interoperate with, as opposed to the other options? In addition to this, as it is now, there are 3 places to check if retiming points should be visible: retime menu, time subpanel and overlay menu, and this is bound to be a source of confusion.

19 KiB

Richard Antalik commented

2024-05-10 17:57:26 +02:00

Member

As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off?

Maybe it would be more in consistency to have to convert enums to check-boxes:
[X] Waveforms
     [X] All (grayed out if Waveforms are not checked)
     [X] Full (grayed out if Waveforms are not checked)

IMO current setup is reasonable. If anything, we could remove per-strip option.

> ![image](/attachments/753d21eb-8d2a-4a79-af67-69f0df3d5796) > As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off? > > Maybe it would be more in consistency to have to convert enums to check-boxes: > ``` > [X] Waveforms > [X] All (grayed out if Waveforms are not checked) > [X] Full (grayed out if Waveforms are not checked) > ``` IMO current setup is reasonable. If anything, we could remove per-strip option.

John Kiril Swenson commented

2024-05-11 21:06:40 +02:00

Contributor

As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off?

Maybe it would be more in consistency to have to convert enums to check-boxes:
[X] Waveforms
     [X] All (grayed out if Waveforms are not checked)
     [X] Full (grayed out if Waveforms are not checked)

I think it makes sense to have on/strip/off as a radio button instead of a boolean toggle. There is an inherent grouping of three things here, regardless of the wording used --

a). Have waveforms on for all strips
b). Have waveforms on for only the strips manually chosen by the user
c). Have waveforms off for all strips

Booleans only really make sense for a clear black-and-white situation, but there are three possibilities here.

The suggested change (having "all" be a sub-checkbox of "turning on waveforms") has multiple problems, e.g.

What would it mean to turn off "all" waveforms? Which waveforms would be left? The term "strip" indicates clearly that waveforms would be shown on a strip-by-strip basis.
What would disabling "full" do? What is the opposite of "full"? Empty? It's unclear.

If the most important elements should be on top (which is the paradime in the VSE elsewhere), I would place the Grid & Cache at the bottom.

I'd be inclined to agree, but I think the change is meant to be consistent with the Viewport Overlays (where general guides are placed at the top)

The Retiming option, I'm not really sure, it should be an overlay option, since it is displaying elements users can interoperate with, as opposed to the other options? In addition to this, as it is now, there are 3 places to check if retiming points should be visible: retime menu, time subpanel and overlay menu, and this is bound to be a source of confusion.

The Toggle Retiming Keys option in Strip > Retiming and the Show Retiming Keys option in the time subpanel do effectively the same thing, which is to disable the retiming keys but still show them on the strip with a lower opacity. The option in the overlay actually disables the visibility of the retiming keys completely.

I agree that this is a source of confusion, but I think a better solution would be to rename Toggle Retiming Keys and Show Retiming Keys to something like Lock Retiming Keys.

> > ![image](/attachments/753d21eb-8d2a-4a79-af67-69f0df3d5796) > As I understand that this was born as an enum, and here the enum is displayed as radio buttons, but from a user perspective it seems to be strange to a two different bool buttons for On and Off? > > Maybe it would be more in consistency to have to convert enums to check-boxes: > ``` > [X] Waveforms > [X] All (grayed out if Waveforms are not checked) > [X] Full (grayed out if Waveforms are not checked) > ``` I think it makes sense to have on/strip/off as a radio button instead of a boolean toggle. There is an inherent grouping of three things here, regardless of the wording used -- a). Have waveforms on for all strips b). Have waveforms on for only the strips manually chosen by the user c). Have waveforms off for all strips Booleans only really make sense for a clear black-and-white situation, but there are three possibilities here. The suggested change (having "all" be a sub-checkbox of "turning on waveforms") has multiple problems, e.g. - What would it mean to turn off "all" waveforms? Which waveforms would be left? The term "strip" indicates clearly that waveforms would be shown on a strip-by-strip basis. - What would disabling "full" do? What is the opposite of "full"? Empty? It's unclear. > If the most important elements should be on top (which is the paradime in the VSE elsewhere), I would place the Grid & Cache at the bottom. I'd be inclined to agree, but I think the change is meant to be consistent with the Viewport Overlays (where general guides are placed at the top) > The Retiming option, I'm not really sure, it should be an overlay option, since it is displaying elements users can interoperate with, as opposed to the other options? In addition to this, as it is now, there are 3 places to check if retiming points should be visible: retime menu, time subpanel and overlay menu, and this is bound to be a source of confusion. The `Toggle Retiming Keys` option in Strip > Retiming and the `Show Retiming Keys` option in the time subpanel do effectively the same thing, which is to disable the retiming keys but still show them on the strip with a lower opacity. The option in the overlay actually disables the visibility of the retiming keys completely. I agree that this is a source of confusion, but I think a better solution would be to rename `Toggle Retiming Keys` and `Show Retiming Keys` to something like `Lock Retiming Keys`.

tintwotin commented

2024-05-12 12:55:49 +02:00

First-time contributor

The options "on" and "off" are commonly used to represent binary choices, such as enabling or disabling a property, which is why check boxes are used the represent this:
[X] Waveform

Radio buttons are suitable when users need to select one option from a small set of mutually exclusive choices of a similar character ex.:
[X] Red
[ ] Green
[ ] Blue

It is poor design to mix these two different concepts.

The options "on" and "off" are commonly used to represent binary choices, such as enabling or disabling a property, which is why check boxes are used the represent this: [X] Waveform Radio buttons are suitable when users need to select one option from a small set of mutually exclusive choices of a similar character ex.: [X] Red [ ] Green [ ] Blue It is poor design to mix these two different concepts.

John Kiril Swenson commented

2024-05-12 22:45:12 +02:00

Contributor

I don't think that there is a clear binary choice in either case here. In order for a checkbox to be completely unambiguous, there needs to only be two possibilities and disabling the checkbox has to have a clear opposite meaning.

The opposite of "full" is not "half," so having that be a toggleable checkbox doesn't make much sense. You can't immediately infer what disabling the option would do.

Additionally, there are three states for waveform visibility regardless of which design you use: with radio buttons, the three states are easily apparent (all waveforms, some waveforms, no waveforms). If you use checkboxes instead, as in

[X] Waveforms
     [X] All (grayed out if Waveforms are not checked)

there are still three states here, but they're heavily obscured to the user in an unnecessary way. The states are:

Waveforms unchecked
Waveforms checked, All unchecked
Waveforms checked, All checked

Again, the opposite of "all" is not "only the strips that are selected by the user," so you can't immediately tell what unchecking it would do.

I think you might be taking the button wording at face-value instead of considering the underlying states. Sure, "on" and "off" usually suggest a checkbox, but the buttons could just as easily be named something like "all" "strip" and "none". Then it should be clear that there are three mutually exclusive options.

So, I'm +1 on renaming "on" and "off" to "all" and "none" while keeping the radio buttons. Otherwise, I'm fine with either wording. I just think checkboxes muddy the intent here.

I don't think that there is a clear binary choice in either case here. In order for a checkbox to be completely unambiguous, there needs to only be two possibilities *and* disabling the checkbox has to have a clear opposite meaning. The opposite of "full" is not "half," so having that be a toggleable checkbox doesn't make much sense. You can't immediately infer what disabling the option would do. Additionally, there are three states for waveform visibility regardless of which design you use: with radio buttons, the three states are easily apparent (all waveforms, some waveforms, no waveforms). If you use checkboxes instead, as in ``` [X] Waveforms [X] All (grayed out if Waveforms are not checked) ``` there are still three states here, but they're heavily obscured to the user in an unnecessary way. The states are: 1. Waveforms unchecked 2. Waveforms checked, All unchecked 3. Waveforms checked, All checked Again, the opposite of "all" is not "only the strips that are selected by the user," so you can't immediately tell what unchecking it would do. I think you might be taking the button wording at face-value instead of considering the underlying states. Sure, "on" and "off" usually suggest a checkbox, but the buttons could just as easily be named something like "all" "strip" and "none". Then it should be clear that there are three mutually exclusive options. So, I'm +1 on renaming "on" and "off" to "all" and "none" while keeping the radio buttons. Otherwise, I'm fine with either wording. I just think checkboxes muddy the intent here.

Pablo Vazquez commented

2024-05-13 12:21:55 +02:00

Author

Member

Thanks everyone for the feedback!

I would place the Grid & Cache at the bottom.

The reason these are on the top is to match the 3D Viewport Overlays panel.

If anything, we could remove per-strip option.

Now that waveforms are fast and even turned on by default, I think we could consider this. Otherwise at least renaming On/Strip/Off to All/Strip/None would fit better with the rest of Blender.

I agree with some of the points brought up here. However, whether the Retiming overlay should be an option at all, or refactoring the way Waveform are set is out of the scope of this patch. It would require more thinking, new/removing options, versioning, and so on.

Since this PR is simply moving existing items around the panel, I’ll proceed as is to get it out of the way. New design/patch discussions can be made on a per-feature basis.

Thanks everyone for the feedback! > I would place the Grid & Cache at the bottom. The reason these are on the top is to match the 3D Viewport Overlays panel. > If anything, we could remove per-strip option. Now that waveforms are fast and even turned on by default, I think we could consider this. Otherwise at least renaming `On/Strip/Off` to `All/Strip/None` would fit better with the rest of Blender. I agree with some of the points brought up here. However, whether the Retiming overlay should be an option at all, or refactoring the way Waveform are set is out of the scope of this patch. It would require more thinking, new/removing options, versioning, and so on. Since this PR is simply moving existing items around the panel, I’ll proceed as is to get it out of the way. New design/patch discussions can be made on a per-feature basis.

Pablo Vazquez added 1 commit 2024-05-13 12:22:45 +02:00

Merge branch 'main' into ui-vse-overlay-panel 252cdde471

Pablo Vazquez merged commit 6e42c3d920 into main

2024-05-13 12:25:48 +02:00

Pablo Vazquez referenced this issue from a commit

2024-05-13 12:25:49 +02:00

UI: Sequencer Overlays layout improvements

Pablo Vazquez deleted branch ui-vse-overlay-panel

2024-05-13 12:25:51 +02:00

John Kiril Swenson referenced this pull request

2024-06-02 22:57:53 +02:00

UI: Fix polling in sequencer preview overlays #122638

Pablo Vazquez referenced this issue from a commit

2024-06-03 16:39:00 +02:00

UI: Fix polling in sequencer preview overlays

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#121591

No description provided.