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 draw_blurred_rounded_rect_in (intended for box shadows) #700

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Sep 24, 2024

This avoids needing a clip layer for a rounded rectangle which doesn't need to draw the inner parts (e.g. a transparent background with a box shadow).

The previous test image was oversized, so this also fixes that (and makes any future such image fail the tests as expected). I imagine that the author of that test manually copied the value into the place, not realising the size of the image.

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.

Very cool! Just a couple comment suggestions.

examples/scenes/src/test_scenes.rs Outdated Show resolved Hide resolved
vello/src/scene.rs Outdated Show resolved Hide resolved
@DJMcNab DJMcNab added this pull request to the merge queue Sep 25, 2024
Merged via the queue into linebender:main with commit 0850de8 Sep 25, 2024
17 checks passed
@DJMcNab DJMcNab deleted the blur-cutout branch September 25, 2024 19:26
@nicoburns
Copy link
Contributor

I was intending to do a proper review, but my comments would be:

  • looks good!
  • I assume there is some efficiency advantage to this over a short-lived layer?
  • Because it looks like the hard (well, more just awkward) bit (creating the clip shape) is still left to user code.
  • It would definitely nice to get some kind of helper for this "rounded rect donut" shape

Finally a question:

  • Would this work for inset shadows? I guess it would and I could just use a regular rect as the clip?

@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 25, 2024

  • thanks

  • Yes, I believe so - it reduces the number of segments which need to be processed, and avoids some copies of data in fine.

  • Yeah - I didn't want to look into the exact semantics of box shadow's shapes

  • I did originally have this utility, but it made some really ugly looking boundaries. I don't think that such a niche shape is a meaningful addition to the imaging model

  • Yeah, just using the rounded rectangle as the drawing shape should work, if inset shadows are what I think they are.

@nicoburns
Copy link
Contributor

Yes, I believe so - it reduces the number of segments which need to be processed, and avoids some copies of data in fine.

Awesome.

Yeah, just using the rounded rectangle as the drawing shape should work, if inset shadows are what I think they are.

Excellent, I'll try this soon :)

I did originally have this utility, but it made some really ugly looking boundaries. I don't think that such a niche shape is a meaningful addition to the imaging model
Yeah - I didn't want to look into the exact semantics of box shadow's shapes

Perhaps instead of some generic/composable utility in kurbo there ought to some super-simple box_shadow method on Scene that just generates shape from rect as in your example code above and then defers to draw_blurred_rounded_rect_in? Which we could add once we've nailed down exactly what the clip path should be for a given rect/shadow I guess.

@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 26, 2024

Again, I don't really see the advantage of that - that was part of my original implementation, but I removed it for simplicity.

As you say, that wouldn't work for inset shadows; I don't really think it is very useful.

@nicoburns
Copy link
Contributor

My motivation for such a feature is that:

let kernel_size = 2.5 * std_dev;
let shape = BezPath::from_iter(
    rect.inflate(kernel_size, kernel_size)
        .path_elements(0.1)
        .chain(
            RoundedRect::from_rect(rect, radius)
                .to_path(0.1)
                .reverse_subpaths(),
        ),
);

is not particularly simple or obvious to construct to those without knowledge of vector graphics who just want to a draw a box shadow with sensible defaults. It also requires quite a strong knowledge of kurbo to get it that simple (consider the code we currently have in Blitz for this).

As the shape can be derived from the rect and shadow parameters, it would be convenient if there was a method that encapsulated that knowledge. Then the API for consumers of Vello would be the same simpler API as that of draw_blurred_rounded_rect. For inset shadows we'd need an additional method.

But eh, it's not the most important thing in the world. As-is, this seems like a nice improvement on what we had before.

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.

3 participants