VSE: Rounded corners for timeline strips #122576

Merged
Aras Pranckevicius merged 16 commits from aras_p/blender:vse_rounded_corners into main 2024-06-04 20:05:49 +02:00

VSE timeline strips now have rounded corners.

image

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:

  • Draw "background" part of the strips: background, darker hold-still regions, color band, transition triangle. All of this masked/clipped by the rounded rectangle shape.
  • Draw "complex in the middle" things mostly in the same way as before: offsets, meta contents, thumbnails, fcurve overlays, waveforms.
  • Draw "foreground" part of the strips: missing media, locked stripes, effect input highlight, left/right handles, border and outline. All masked/clipped by the rounded dectangle.

This leaves several things that can "go outside" of rounded corners:

  • Audio waveforms. I opted to do nothing about this, since the waveform near start & end of strip would have to be at max volume to go outside of the corner, and that probably "almost never" happens. If it does, "oh well" is probably good enough for now.
  • Animation curve darkening overlays. These just darken what is under them "a bit", and I haven't noticed that this would be a visible problem.
  • Image/movie thumbnails. These are a problem, and they are drawn with OCIO shaders that makes it hard to "also plug in some round corner masking" into that. What I chose to do, is to manually "make corners transparent" of the actual thumbnail image whenever start or end of it crosses the strip rounded corner area. Right now the thumbnails are drawn in "very inefficient way" anyway, so this extra cost seems okay. It does create some visual artifacts at very large zoomed in levels, where pixels of the thumbnail are large, e.g. look at this corner:
  • Meta strip contents. Right now I indent the contents by the border round radius. If that is not a good enough approach, a more proper solution could be implemented.

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

  • Audio waveform drawing could be moved into part of the GPU shader itself. I'd imagine the audio waveforms for all the visible strips would be put into an SSBO, each strip entry would know which part of that SSBO to fetch waveform from, and use that. This would probably be faster.
  • Fcurve overlay could be moved into GPU shader itself in a similar way.
  • The whole thumbnail drawing is a larger task, that should be addressed at some point. Since unrelated to this PR, thumbnail rendering is slow due to all the GPU textures being re-created from scratch every frame. Thumbnails should perhaps be cached into some GPU "atlas/cache" and kept around for some frames. Maybe OCIO things could be applied on the CPU side during thumbnail creation, so that rendering of actual thumbnails could go though "our" shaders too.
  • Strip offsets no longer "join" the strip body due to the rounded corner. Perhaps someone who's good at design should think of how strip offsets should actually be rendered.

More Screenshots

Medium zoom (corner rounding about 4px):
image

Zoomed out (corner rounding is like 2px):
image

Zoomed in (max corner rounding is 8px):
image

Timeline of Sprite Fright edit:
image

Testing file for various strip types/configs that I'm using for testing is attached, vse_timeline_render.zip

Notes for reviewers:

  • I have tested the code myself on OpenGL (Windows, NVIDIA) and Metal (Mac, M1), seems to work :)
  • There's a lot of code removed from sequencer_timeline_draw.cc, that more or less moved into the new GPU shader.
  • GPU shader does signed distance to rounded rectangle for the corners / outlines, and otherwise follows/replicates the logic of the whole sequencer strip widget drawing.
  • GPU shader note: I am using GLSL4 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.
  • On CPU side the drawing is done by instancing a quad, and setting up two UBOs - one with "global" data, and another one with "per strip" data. This is quite similar to how icon or UI button batches are done.
VSE timeline strips now have rounded corners. ![image](/attachments/03d87f8e-a5b7-416f-9498-72cf4dde7cf7) 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: - Draw "background" part of the strips: background, darker hold-still regions, color band, transition triangle. All of this masked/clipped by the rounded rectangle shape. - Draw "complex in the middle" things mostly in the same way as before: offsets, meta contents, thumbnails, fcurve overlays, waveforms. - Draw "foreground" part of the strips: missing media, locked stripes, effect input highlight, left/right handles, border and outline. All masked/clipped by the rounded dectangle. This leaves several things that can "go outside" of rounded corners: - Audio waveforms. I opted to do nothing about this, since the waveform near start & end of strip would have to be at max volume to go outside of the corner, and that probably "almost never" happens. If it does, "oh well" is probably good enough for now. - Animation curve darkening overlays. These just darken what is under them "a bit", and I haven't noticed that this would be a visible problem. - Image/movie thumbnails. These are a problem, and they are drawn with OCIO shaders that makes it hard to "also plug in some round corner masking" into that. What I chose to do, is to manually "make corners transparent" of the actual thumbnail image whenever start or end of it crosses the strip rounded corner area. Right now the thumbnails are drawn in "very inefficient way" anyway, so this extra cost seems okay. It does create some visual artifacts at _very large_ zoomed in levels, where pixels of the thumbnail are large, e.g. look at this corner: ![](/attachments/ed40813a-4972-4f8a-a71d-fd10072a7aeb) - Meta strip contents. Right now I indent the contents by the border round radius. If that is not a good enough approach, a more proper solution could be implemented. ![](/attachments/6358cf00-281e-451e-a478-1bd6a06b17a0) 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: ![](/attachments/3c22d0ef-869f-4860-9b26-9a76bc628b8f) Now: ![](/attachments/e5e7a547-580f-4a59-805b-4cfcc38e9ad2) **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: ![](/attachments/f979f3ef-221c-4227-9185-e6f9bbc5c96d) 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 - Audio waveform drawing could be moved into part of the GPU shader itself. I'd imagine the audio waveforms for all the visible strips would be put into an SSBO, each strip entry would know which part of that SSBO to fetch waveform from, and use that. This would probably be faster. - Fcurve overlay could be moved into GPU shader itself in a similar way. - The whole thumbnail drawing is a larger task, that should be addressed at some point. Since unrelated to this PR, thumbnail rendering is slow due to all the GPU textures being re-created from scratch every frame. Thumbnails should perhaps be cached into some GPU "atlas/cache" and kept around for some frames. Maybe OCIO things could be applied on the CPU side during thumbnail creation, so that rendering of actual thumbnails could go though "our" shaders too. - Strip **offsets no longer "join" the strip body** due to the rounded corner. Perhaps someone who's good at design should think of how strip offsets should actually be rendered. ![](/attachments/a7c80370-5e37-48cd-9647-93ad0e466de6) ### More Screenshots Medium zoom (corner rounding about 4px): ![image](/attachments/03d87f8e-a5b7-416f-9498-72cf4dde7cf7) Zoomed out (corner rounding is like 2px): ![image](/attachments/3c92b453-03d1-439b-92ec-4a296e0c6c78) Zoomed in (max corner rounding is 8px): ![image](/attachments/2d5f5cfb-db3c-4ede-b4bf-d7952975cea0) Timeline of Sprite Fright edit: ![image](/attachments/44aa3d88-66fb-4735-8a5d-6a75a35d57d3) Testing file for various strip types/configs that I'm using for testing is attached, `vse_timeline_render.zip` ### Notes for reviewers: - I have tested the code myself on OpenGL (Windows, NVIDIA) and Metal (Mac, M1), seems to work :) - There's a lot of code removed from `sequencer_timeline_draw.cc`, that more or less moved into the new GPU shader. - GPU shader does signed distance to rounded rectangle for the corners / outlines, and otherwise follows/replicates the logic of the whole sequencer strip widget drawing. - GPU shader note: I am using GLSL4 `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. - On CPU side the drawing is done by instancing a quad, and setting up two UBOs - one with "global" data, and another one with "per strip" data. This is quite similar to how icon or UI button batches are done.
First-time contributor

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?

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?
Author
Member

@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:

  • When you're "zoomed out" (i.e. vertical size of a strip is small) then there's very little (or none) of corner rounding.
  • Corner rounding only gets more pronounced when you're zoomed in "enough" where there'd be no downsides of the rounding.

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 :)

@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: - When you're "zoomed out" (i.e. vertical size of a strip is small) then there's very little (or none) of corner rounding. - Corner rounding only gets more pronounced when you're zoomed in "enough" where there'd be no downsides of the rounding. 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 :)
Aras Pranckevicius force-pushed vse_rounded_corners from 24d182b069 to 0f32ffb335 2024-06-01 17:26:19 +02:00 Compare
Aras Pranckevicius added 3 commits 2024-06-02 09:35:17 +02:00
Aras Pranckevicius added 1 commit 2024-06-02 12:50:04 +02:00
VSE: Cleanup strips batcher code
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
4ae19abf3a
Aras Pranckevicius added 1 commit 2024-06-02 20:41:26 +02:00
VSE: Apply rounded corners to thumbnail top too, naming/formatting
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
b848a322a8
The top corners might need rounding when there are no  title bars
due to name/source/time overlays being off
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122576) when ready.
Aras Pranckevicius added 1 commit 2024-06-03 09:36:26 +02:00
Aras Pranckevicius changed title from WIP: VSE: Rounded corners for timeline strips to VSE: Rounded corners for timeline strips 2024-06-03 09:47:32 +02:00
Aras Pranckevicius added this to the Video Sequencer project 2024-06-03 09:47:45 +02:00
First-time contributor

A 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 :

rndst_02.png

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) :

rndst_01.png

A 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 : ![rndst_02.png](/attachments/63565623-73c0-4b4e-a415-52317ee18bec) 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) : ![rndst_01.png](/attachments/ffe4618e-fc8c-4624-9526-5e0241fe5f6a)
Aras Pranckevicius added 1 commit 2024-06-03 10:18:37 +02:00
First-time contributor

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.
image

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. ![image](/attachments/8793a19c-06ba-4e17-91c6-7ddd5f021c5f)
First-time contributor

Maybe the Offsets could start from the center of the strip and thereby be more connected with the strip?
image

(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)

Maybe the Offsets could start from the center of the strip and thereby be more connected with the strip? ![image](/attachments/3a45cdae-8da4-4680-ab8c-ecd0239a4b8f) (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)
5.0 KiB
First-time contributor

@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
image
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.

@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 ![image](/attachments/02453143-fce1-4a1b-a8e4-3e4c7f892e8d) 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.
Aras Pranckevicius added 1 commit 2024-06-03 20:04:03 +02:00
Tweak strip rounding math
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
5765eb91c5
- Pick between none, 4px, 6px or 8px rounding radii based on height
- When strip becomes too narrow, use no rounding on it at all

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:
Screenshot_2024-06-04_02-50-30.png

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 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: ![Screenshot_2024-06-04_02-50-30.png](/attachments/7bec7df9-ab98-4f34-a7b8-90fa9d748000) 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.
Contributor

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.

image

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.

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. ![image](/attachments/3265518c-864a-4cec-bd65-3b7e0373f036) 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.
417 KiB
Aras Pranckevicius added 1 commit 2024-06-04 09:05:58 +02:00
Merge branch 'main' into vse_rounded_corners
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
730c3dbbb6
# Conflicts:
#	source/blender/editors/space_sequencer/sequencer_timeline_draw.cc
Author
Member

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.

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

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.

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?

> 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. 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` > 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. 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?
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122576) when ready.
Aras Pranckevicius added 1 commit 2024-06-04 11:43:57 +02:00
Remove old non-rounded VSE strip corners rendering path
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
5dd4350b5a
Aras Pranckevicius requested review from Sergey Sharybin 2024-06-04 12:29:15 +02:00
Author
Member

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)

@HadrienBrissaud I have tweaked rounding logic a bit since then:

  • Overall rounding does not go that large (was up to 16px before, up to 8px now),
  • When strip becomes too narrow to fit rounded corners, it turns rounding off for itself. Previously it was using variable rounding to fit. This feels like it reduces the amount of "variable rounding" to me
> 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) @HadrienBrissaud I have tweaked rounding logic a bit since then: - Overall rounding does not go that large (was up to 16px before, up to 8px now), - When strip becomes too narrow to fit rounded corners, it turns rounding off for itself. Previously it was using variable rounding to fit. This feels like it reduces the amount of "variable rounding" to me
Aras Pranckevicius requested review from Clément Foucault 2024-06-04 12:56:48 +02:00

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122576) when ready.
Clément Foucault requested changes 2024-06-04 16:23:54 +02:00
Dismissed
Clément Foucault left a comment
Member

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.
image

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.

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. ![image](/attachments/1867d662-5d5b-4eb5-86fc-e220a4f219ea) 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.

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.
aras_p marked this conversation as resolved
@ -0,0 +1,53 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors

Move this to GPU_shader_shared.hh.

Move this to `GPU_shader_shared.hh`.
aras_p marked this conversation as resolved
@ -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.

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.
aras_p marked this conversation as resolved
@ -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 of packUnorm4x8.

Add comment these are `ubytes` colors that have been packed using the equivalent of `packUnorm4x8`.
aras_p marked this conversation as resolved
@ -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.

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.
aras_p marked this conversation as resolved
@ -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.

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.
aras_p marked this conversation as resolved
@ -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.

Rename to `pos`.
aras_p marked this conversation as resolved
@ -0,0 +55,4 @@
radius = 0.0;
}
float d = sdf_rounded_box(pxy - bcenter, bsize, radius);

Rename to sdf or signed_distance. Avoid very small variable name.

Rename to `sdf` or `signed_distance`. Avoid very small variable name.
aras_p marked this conversation as resolved
@ -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.

Do not use camel case.
aras_p marked this conversation as resolved
Aras Pranckevicius added 1 commit 2024-06-04 17:55:57 +02:00
Author
Member

There are some issue with the line width being rounded up or down which creates lines that are 2px wide while some other are 1px wide

@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!

> There are some issue with the line width being rounded up or down which creates lines that are 2px wide while some other are 1px wide @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!
Aras Pranckevicius requested review from Clément Foucault 2024-06-04 18:00:01 +02:00

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.

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.
Clément Foucault approved these changes 2024-06-04 19:32:32 +02:00
Sergey Sharybin approved these changes 2024-06-04 19:50:01 +02:00
Sergey Sharybin left a comment
Owner

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.

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.
Aras Pranckevicius merged commit 7fdfa47f23 into main 2024-06-04 20:05:49 +02:00
Aras Pranckevicius deleted branch vse_rounded_corners 2024-06-04 20:05:52 +02:00
First-time contributor

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)

@HadrienBrissaud I have tweaked rounding logic a bit since then:

  • Overall rounding does not go that large (was up to 16px before, up to 8px now),
  • When strip becomes too narrow to fit rounded corners, it turns rounding off for itself. Previously it was using variable rounding to fit. This feels like it reduces the amount of "variable rounding" to me

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?

> > 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) > > @HadrienBrissaud I have tweaked rounding logic a bit since then: > - Overall rounding does not go that large (was up to 16px before, up to 8px now), > - When strip becomes too narrow to fit rounded corners, it turns rounding off for itself. Previously it was using variable rounding to fit. This feels like it reduces the amount of "variable rounding" to me 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?
Author
Member

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.

> 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.
Sign in to join this conversation.
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 Assignees
9 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#122576
No description provided.