classic: Implement inverse blurred rounded rects#1718
Conversation
nicoburns
added
enhancement
d98ee7f to
3d69137
Compare
Signed-off-by: Nico Burns <nico@nicoburns.com>
3d69137 to
48e029d
Compare
|
I've updated this encode the invert bit into the the unused sign bit from the std_dev parameter rather than adding a whole new 32bit parameter |
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I think the actual wiring is pretty much fine, except for the single-rect constructor. I'm not really happy with how this is documented. I am also aware that the non-inverse rounded rectangle is also poorly documented, but I feel like this is misleadingly documented rather than just under-documented.
I do agree that this is a valuable feature, and hope this feedback makes sense.
| let shape: Rect = rect.inflate(kernel_size, kernel_size); | ||
| self.draw_blurred_rounded_rect_in(&shape, transform, rect, brush, radius, std_dev); | ||
| let inflated_rect = if invert { | ||
| rect.inflate(rect.origin().x.abs(), rect.origin().y.abs()) |
There was a problem hiding this comment.
This is a wild thing to drop without an explanatory comment... it seems to want to try to expand to the top-left corner (but not the bottom left corner?), and only if you have no transform?
In theory, you want this to have infinite extents, which I do know aren't really practical in current Vello Classic.
There was a problem hiding this comment.
@DJMcNab See discussion in #1721 (same API for sparse strips). The key thing that this achieves is to offset the position of the paint relative to the position of the clip. This directly corresponds to the offset (x/y) parameters of a CSS box-shadow (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/box-shadow).
We get away without this for "outset" box-shadows because we're painting the area "outside" the clip (i.e. the hole of a rounded-rect-donut clip) anyway (Blitz is applying it's own clip to clip this out which does account for the "offset").
Because "inset" box shadows are painted "inside the box" we are able to specify a tighter clip region, but this means that we have to account for the offset.
Agree that this needs more thought put into the API though.
| /// | ||
| /// When `invert` is `true`, the inverse (`1 - alpha`) of the blur coverage is painted: the | ||
| /// brush is fully opaque outside the blurred rounded rectangle and fades to transparent inside | ||
| /// it. This can be used to implement inset box shadows. |
There was a problem hiding this comment.
Does "outside" here mean 'approximately 2.5 std deviations outside', or something else?
There was a problem hiding this comment.
The are effectively two rounded rects, offset from each other by a user-specified (x/y) offset (currently encoded by the rect parameter's origin):
- A clip rect
- A paint rect
Then:
- The area which is outside the "clip rect" doesn't get painted at all (duh)
- The area in the intersection of the two rects gets the inverted blurred_rounded_rect paint
- The area which is inside the "clip rect" but outside the "paint rect" get a solid fill (this actually falls out of the definition of the inverted blurred_round_rect paint and isn't special cased in the implementation)
Probably this comment could be reworded.
You may find it helpful to play around with https://box-shadow.dev (with invert enabled) to get an intuition of the intended effect. In particular, consider the case where the blur radius is 0 (and potentially the spread too), but there is an offset.
There was a problem hiding this comment.
This way of encoding the parameters feels:
- entirely unjustified
- kind of unhinged
- extremely unintuitive
It's an impressive combination of results!
There was a problem hiding this comment.
That is fair! It does have one redeeming quality, which is that it's non-API-breaking. But yeah, I agree that we ought to change this.
| brush: Color, | ||
| radius: f64, | ||
| std_dev: f64, | ||
| invert: bool, |
There was a problem hiding this comment.
I think the cleanest option is to actually just not define the 'invert' parameter to this method, and force you to define the extents yourself.
There was a problem hiding this comment.
I think I'm in agreement that the user probably ought to define extents (I think this amounts to allowing the user to specify a custom impl Shape?), but I think we still need the invert parameter to invert the actual paint (apply 1 - alpha in the shader).
There was a problem hiding this comment.
Yes... That's the null comment from my suggestion?
I'm saying you should only have the invert parameter on the blurred_rounded_rect_in method, and leave it completely off the not-in one.
| invert: bool, | ||
| ) { | ||
| // The `invert` flag is packed into the sign bit of `std_dev`, which is otherwise | ||
| // always non-negative. |
There was a problem hiding this comment.
'always'? We should probably at least have this as a validity requirement in the underlying docs.
There was a problem hiding this comment.
Do you mean internal docs? If so, that makes sense.
In terms of public API, we could certainly document it, but we probably also ought to clamp the value to zero in case it is negative rather exposing weird behaviour to a user-facing API.
| // The invert flag is packed into the sign bit of `std_dev`, which is otherwise | ||
| // always non-negative. |
There was a problem hiding this comment.
This 'always non-negative' is the kind of comment which raises more questions than it answers.
2 participants
Followup to #1715 for Vello Classic. Implements inverse blurred rounded rects for Vello Classic. Also ports the tests from the sparse strips test suite. Fixes #1374
Only 41 lines of non-test code. The main thing I'm unsure about is whether this is a sensible representation of the
invertbit in the encoding / GPU data layout.I have verified that this produces identical output as the sparse strips implementation when rendering my test shadow scene in Blitz (pictured below)
Generated with Opus 4.8 High with manual review.