Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for blurred rounded rectangle. #665

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

msiglreith
Copy link
Collaborator

Implements the blurred rounded rectangle approximation outlined in https://raphlinus.github.io/graphics/2020/04/21/blurred-rounded-rects.html. Closes #640

The convolution is mostly taken verbatim from https://git.sr.ht/~raph/blurrr translated to wgsl. Added input bounds for the erf function due to artifacts at std_dev < 0.1.

Possible optimizations would include moving parts of the precomputation to the CPU side, right now each tile recalculates the same values.

Scene output:
grafik

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good, and works well!

The main blocking concern is copying the logic into the CPU shaders

examples/scenes/src/test_scenes.rs Show resolved Hide resolved
vello/src/scene.rs Outdated Show resolved Hide resolved
vello/src/scene.rs Outdated Show resolved Hide resolved
transform: Affine,
brush: Color,
size: impl Into<Size>,
radius: f64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not because I expect you to do this, but because it's important that it's raised, but is it at all plausible for the corner radii to be different? I.e. make this function accept a RoundedRect - which despite the docs, can have different radii on each corner since linebender/kurbo#166

I don't have a good understanding of the underlying maths to make this work

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this as well but seems tricky at least.
If it would be dependent on the (continuous) distance alone it would be fine in my opinion but the parameters (e.g squircle shape) derived from the corner radius would introduce dicontinuities at the quadrant boundaries, probably visible at larger filter lengths. We could derive shared parameters for the whole shape with some tradeoff in quality but without larger adjustments to the approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - that's fine. I've not thought about it deeply, and I don't think that it is a necessary feature.

Thought it would be important to at least ensure that it is discussed, as much for when Raph comes through to gut-check this.

Copy link
Contributor

@nicoburns nicoburns Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it is a necessary feature.

FWIW, this definitely is a necessary feature for full CSS box-shadow support. Not sure how common they are in the wild, but the spec certainly allows authors to specify such shadows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to emulate it with multiple rounded rectangles, each in their own clip layer. That means that you would be responsible for the behaviour in the degerenate cases. I don't think these clip layers would need to be nested.

I think it's possible you would get conflation artifacts, though...

Copy link

@jkelleyrtp jkelleyrtp Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/DioxusLabs/blitz/blob/main/packages/blitz/src/renderer/multicolor_rounded_rect.rs

we implemented the maths here for rounded-rect, you might be able to find some of it helpful

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rounded rect can be done without too much difficulty in a distance field context by, basically, setting the radius by quadrant. But in the blurred case you will definitely see discontinuities depending on the exact parameters. I think it is possible to fix, but will require some doing.

Of course another possibility is to render the rounded rect as a vector shape (as in the code Jon linked) and blur it, but (a) that's going to be slower than doing it in a shader, even with heavier math, and (b) we don't have the infrastructure in place. It would be nice to have though, as you do need that for text shadows.

I don't recommend using clips for this, it adds complexity and possible performance issues, as well as potential conflation artifacts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly tested how the artifacts look like and there are indeed visible discontinuities at larger kernel sizes (see figure below). It seems to be primarily caused by the scale factor. Using a fixed or interpolated value would be an option here. But I would postpone it for now as followup.
grafik

vello_encoding/src/encoding.rs Outdated Show resolved Hide resolved
examples/scenes/src/test_scenes.rs Outdated Show resolved Hide resolved
vello_shaders/shader/coarse.wgsl Outdated Show resolved Hide resolved
@msiglreith
Copy link
Collaborator Author

Thanks for the review! I added the snapshot tests along with support in the cpu pipeline (besides fine rasterization)

@DJMcNab
Copy link
Member

DJMcNab commented Aug 16, 2024

One last thought: I'd like to see this used in stats displays for background. I've put my implementations on Zulip, because I'm running into issues.

Otherwise, we're just waiting on Raph for licensing confirmation on the code used from him, I think

@msiglreith
Copy link
Collaborator Author

thanks for testing, should be fixed. A fill style was missing resulting in the rect path being interpreted as stroke.

@DJMcNab DJMcNab requested a review from raphlinus August 16, 2024 15:20
@jrmoulton
Copy link

I've been testing this in my vello test branch for floem.

Works great!

It would be nice to have some docs for the std dev arg. I can see that it affects blur but that's as much as I can see. I imagine most people will be approaching this API from a web-dev background and looking for blur and won't have an understanding of the terminology in terms of gaussian blur.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! I'm not sure exactly what needs to be done to fix the snapshot issue. Also, there's been a bit more discussion about multiple radii, that would be a good followup issue (or, better, PR).

transform: Affine,
brush: Color,
size: impl Into<Size>,
radius: f64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rounded rect can be done without too much difficulty in a distance field context by, basically, setting the radius by quadrant. But in the blurred case you will definitely see discontinuities depending on the exact parameters. I think it is possible to fix, but will require some doing.

Of course another possibility is to render the rounded rect as a vector shape (as in the code Jon linked) and blur it, but (a) that's going to be slower than doing it in a shader, even with heavier math, and (b) we don't have the infrastructure in place. It would be nice to have though, as you do need that for text shadows.

I don't recommend using clips for this, it adds complexity and possible performance issues, as well as potential conflation artifacts.

vello/src/scene.rs Outdated Show resolved Hide resolved
@DJMcNab
Copy link
Member

DJMcNab commented Aug 27, 2024

@msiglreith a reminder that the policy is author-merged, and I believe you have permission to press the button.

I'm not sure exactly what needs to be done to fix the snapshot issue

I don't follow what this is referring to though

@msiglreith msiglreith added this pull request to the merge queue Aug 27, 2024
Merged via the queue into linebender:main with commit aaa9f5f Aug 27, 2024
17 checks passed
@msiglreith msiglreith deleted the blur-rect branch August 27, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop shadows
6 participants