VSE: Rounded corners for timeline strips #122576
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
Code Documentation
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
9 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#122576
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "aras_p/blender:vse_rounded_corners"
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?
VSE timeline strips now have rounded corners.
Strip corner rounding radius is 4, 6 or 8px depending on strip height (if strip is too narrow to fit rounding, then rounding is turned off). Note: screenshots below were from slightly different rounding logic that went up to 16px radius.
Details
This is more involved than it sounds like, so here's a longer story:
Previously pretty much all of timeline strip drawing was just boxes and lines, which makes things simple to draw. However, adding rounded corners creates complications in "how to make things not go outside of the rounded corner?", e.g. a strip transition triangle, or the "locked" diagonal stripes overlay. Plus the border and selection outline should also be rounded.
I've decided to approach this by adding a dedicated GPU shader for drawing "most of" timeline strip control. So instead of previous approach of overlaying many semitransparent rectangles (in multiple alpha blending passes), now it is mostly within the fragment shader code. The flow now is this:
This leaves several things that can "go outside" of rounded corners:
There is a behavior change that I did wrt strip thumbnails: now image thumbnails that have transparency are drawn with alpha blending. This was not the case before; thumbnails were always opaque (except when they would be part of a muted strip, then they would respect transparency, lol!). This change is so that a "semitransparent rounded corner" part of the image would actually render as transparency.
Before:
Now:
If this is a problem, I guess an alternative could be to not make the thumbnail corner "transparent", but to try to poke in the background color into there. This would probably work, except while dragging a strip over other strips, you would see the background in the corner area instead of it being transparent.
Another behavior change: in Sprite Fright Edit, there's a bunch of (disabled) audio strips that seem to be "invalid" -- in the current blender main, they are rendered with a funny "too wide" white side, their durations seem to be negative, and you seemingly can't do anything useful with them - e.g. enabling them and trying to transform them "snaps" them to be invisible. They are still rendered in this "weird" way though. In this branch, this "weird rendering" does not happen, so overall timeline of Sprite Fright Edit looks different. I have no idea how such "invalid" strips ended up being there, or is that something that should be addressed somehow. Example:
Performance: about the same in my tests; the new code is a tiny bit faster on the CPU (since it ends up doing fewer draw calls).
Possible TODOs
More Screenshots
Medium zoom (corner rounding about 4px):
Zoomed out (corner rounding is like 2px):
Zoomed in (max corner rounding is 8px):
Timeline of Sprite Fright edit:
Testing file for various strip types/configs that I'm using for testing is attached,
vse_timeline_render.zip
Notes for reviewers:
sequencer_timeline_draw.cc
, that more or less moved into the new GPU shader.unpackUnorm4x8
to decode color from a 32 bit integer. There was a missing "translation" of that into Metal (unpack_unorm4x8_to_float
) so I added that, plus other three related unpacking functions.I have not seen what this looks like yet, but rounded corners on VSE strips were tested some years ago (by Psy-Fi I think) and if my memory serves right, the concern was that start and end frames became ambiguous as a result, especially at low zoom level were frames are densely packed. Is that something you're aware of?
@HadrienBrissaud not aware of that, right now I'm just starting to do this since @pablovazquez asked "hey can we have rounded corners?". My idea is first to get "soemething" working, and then debate the actual details.
The way I imagine things is along the lines of:
But we'll see once I have something actually working. So far it does not even work yet, I just pushed into a branch so that I can move development from my windows box into my mac box :)
24d182b069
to0f32ffb335
@blender-bot build
@blender-bot package
Package build started. Download here when ready.
WIP: VSE: Rounded corners for timeline stripsto VSE: Rounded corners for timeline stripsA lot of work seems to have gone into this, but to be frank all I see this brings is ambiguity as per strips' start and end frames. It also comes with some issues and uncertainties (such as how to make strip offsets work with those changes), for a benefit I can't really see. What is the motivation, do strips look better with round corners?
The advantage I see with sharp corners is they leave no doubt as to when two strips (especially when they're on different tracks) are separated -or not- by a gap :
I feel like my ability to read the timeline is somewhat muddied. Perhaps it's just a matter of habit, or visual cognition? I don't know.
Aesthetically, it's also a little strange to have many varying degrees of "roundness" depending on the strip's horizontal size (although this happens only when zoomed out) :
This is a no-brainer. Aesthetically, it looks much more elegant. And practically, it is the making the cuts much more visible. So, a serious improvement to the VSE. Excellent work and very well tested.
The only extreme corner case I could find is when strips are very tall and narrow, maybe are the corners a bit too rounded for my taste? But anyone having the strips displayed like this, are properly using the VSE wrong... lol.
Maybe the Offsets could start from the center of the strip and thereby be more connected with the strip?
(EDIT: Looking at it, I wonder why it is drawn on bottom and top like that, when it could be just one line. And I even wonder why it is something users should be toggling on and off, it could just be drawn automatically during Transform operations on the selected strips(using handles or Slip), but that's another story)
@aras_p @tintwotin
i was thinking about the rounded corner may put difficulty (effort) if we want to see if two body strips are vertically aligned or not like the picture above or here
my simple ideas is mixing 2 type of corners like the following :
suggestion 1- turn corners to sharp if any strip is selected while letting unselected strip have rounded corners.
suggestion 2- 1- turn corners to sharp if 2 strips or more are selected while letting unselected strip have rounded corners.
I was always against rounded corners myself, but this PR looks OK. Does the radius depend on display DPI? On my machine this looks more like 4px chamfer. Looking at other screenshots here this seems to be quite variable.
Here is mine for reference:
The ambiguity of the strip alignment could be resolved by strips having constant margin in pixels rather than height of 0.8 or what it is now.
I love rounded corners. Besides looking absolutely amazing, main advantage is (as many have mentioned already) that cuts are very easily visible, and thats THE most important UI feedback when editing. That is something I've always struggled a little bit in Blender. This feels so much nicer.
But I think one thing it makes harder is detecting if strips are aligned vertically. To fix that, I think it would be good if strips used more of the vertical space and didn't leave such huge gaps. Strips should almost be touch each other vertically, as they do horizontally, that way groupings are also more easily visible.
Reducing those spaces will also help with offset lines, which now come from underneath the strip, not from it.
I would also increase strip outlines by one pixel. Not only will that help with detecting cuts and alignments, but selection too, which I still think isn't very visible.
Yeah I have tweaked the rounding to be less pronounced upon Francesco's feedback, but did not update the screnshots of the PR yet:
Strip corner rounding radius is 4, 6 or 8px depending on strip height (if strip is too narrow to fit rounding, then rounding is turned off). Note: screenshots below were from slightly different rounding logic that went up to 16px radius
Nika's comment suggests similar. I think it would make sense to have the gap for showing the offset line/widget to be never wider than say 4px. Nika suggests to get rid of it completely, I'm not totally sold on that. To me it feels like the offset being below/above the strips is "by design", so that they would not get hidden by other unrelated strips being nearby. Right?
@blender-bot package
Package build started. Download here when ready.
@HadrienBrissaud I have tweaked rounding logic a bit since then:
@blender-bot package
Package build started. Download here when ready.
I am fine with the approach and I think it is already an improvement over the current state. I cannot comment on the usefulness of the feature as I am not a VSE user myself.
There are some issue with the line width being rounded up or down which creates lines that are 2 px wide while some other are 1 px wide.
The rounded corners also would need some more love to make sure they are indeed the target pixel resolution.
I suggest to look at how the widget shader does it as I believe I faced similar issues in the past.
But these are more like nitpicks and can be fixed later.
Otherwise, just need a small pass over readability and that's it for me.
@ -1597,0 +1372,4 @@
strips_count_ = 0;
}
private:
As per C++ codestyle https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-layout the members should be at the top of the class.
@ -0,0 +1,53 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
Move this to
GPU_shader_shared.hh
.@ -0,0 +29,4 @@
/* Per-strip data for timeline rendering. */
struct SeqStripDrawData {
float content_start, content_end, bottom, top;
Add comment describing what these refers to.
It is useful if someone unfamiliar with the sequencer code is trying to debug a drawing issue 😉.
Same thing for
float left_handle, right_handle, strip_content_top, handle_width
they need more description.@ -0,0 +32,4 @@
float content_start, content_end, bottom, top;
float left_handle, right_handle, strip_content_top, handle_width;
uint flags;
uint col_background;
Add comment these are
ubytes
colors that have been packed using the equivalent ofpackUnorm4x8
.@ -0,0 +44,4 @@
/* Global data for timeline rendering. */
struct SeqContextDrawData {
float pixelx, pixely;
Add comment to what this is. My first impression was that this was the position of a strip or the view. I believe this is the same semantic as the existing code inside the sequencer module but more documentation wouldn't hurt.
@ -0,0 +40,4 @@
vec2 co = coInterp;
SeqStripDrawData strip = strip_data[strip_id];
vec2 bsize = vec2(strip.right_handle - strip.left_handle, strip.top - strip.bottom) * 0.5;
It isn't obvious what the
b
prefix stands for. Try to avoid these 1 letter prefix and use snake case.But maybe it's just legacy code that was copied to the GLSL, so I don't know.
@ -0,0 +48,4 @@
vec2 view_to_pixel = vec2(context_data.inv_pixelx, context_data.inv_pixely);
bsize *= view_to_pixel;
bcenter *= view_to_pixel;
vec2 pxy = co * view_to_pixel;
Rename to
pos
.@ -0,0 +55,4 @@
radius = 0.0;
}
float d = sdf_rounded_box(pxy - bcenter, bsize, radius);
Rename to
sdf
orsigned_distance
. Avoid very small variable name.@ -0,0 +10,4 @@
#include "gpu_shader_create_info.hh"
GPU_SHADER_INTERFACE_INFO(gpu_seq_strip_iface, "")
.no_perspective(Type::VEC2, "coInterp")
Do not use camel case.
@fclem yeah indeed, I suspect this might be because strip positions are not snapped to a pixel grid, so some of the outlines fall "in between" and instead of like 2px outline you get 1 full opacity pixel and two half opacity ones. I should implement snapping/quantization of the strips to pixel coordinates, but during VSE call this was deemed to be acceptable to solve in a followup PR.
Addressed your code style feedback, thanks!
I've tested the patch briefly. So far works quite good. The only thing which I think is happening is the roundness stays constant in the screen space, regardless of the UI scale. I think the roundness should scale with the UI, similarly to the buttons.
Such things can be tweaked during bcon3, if we are all fine with the rest of the code.
Other than the reaction to the UI scale which we can tweak if needed later, can just go ahead and land this PR to ensure it is in the 4.2.
I find this new version much better. The lower corner radius works well, I have no more concerns about readability. Thanks for taking them into account. It now looks like a pleasant aesthetic tweak, not too disruptive.
However it doesn't seem to be influenced by the interface resolution scale right now. I suppose it should?
This is being discussed on the chat right now, and so far it feels like it points to a larger unrelated issue: most of blender, when changing interface zoom factor, actually physically changes the sizes of the "widgets", and so it makes sense that things like rounded corners would also scale with that.
But VSE timeline does not; with changing UI resolution scale the strip widgets stay the same size on screen. Which feels like an oversight that should be addressed first, but that's a larger task/topic.