fix: Filter expansion logic for transforms with scale/skew and for clipped layers #1303
Conversation
|
Maybe we could add a new test for this? |
|
Sure, I’ve added another one. Here’s the diff to show exactly what was wrong with the previous code. The left side shows the current version and the right side shows the previous one. Since we were rendering only the clipped geometry, the blur was applied only to that area, causing lighter regions around the geometry edge and creating noticeable artifacts along the vertical clip edges. 
|
| # Enable all debug utilities | ||
| debug = ["debug-cmds"] | ||
| # Enable command listing and introspection utilities | ||
| debug-cmds = [] |
There was a problem hiding this comment.
Could we maybe instead use the log crate and print the debugging things using debug!? I think it's a bit unfortunate having this feature, but maybe not the end of the world.
| /// True when currently rendering inside a filtered layer with a clip path. | ||
| /// When set, command generation uses full viewport bounds instead of clip bounds | ||
| /// to ensure filter effects can process the full layer before applying the clip. | ||
| in_clipped_filter_layer: bool, |
There was a problem hiding this comment.
Not for this PR, but I'm wondering whether we should switch vello_cpu to always use the new clipping approach. Especially with filters now it seems like there are a lot of edge-case interactions between layer clipping and filters, which would likely be completely eliminated if we always used the new algorithm. On the other hand, it would probably be problematic to lose the parity between vello_cpu and vello_hybrid...
| // The expansion rect is centered at origin: | ||
| // - Negative coordinates (x0, y0) represent left/top expansion | ||
| // - Positive coordinates (x1, y1) represent right/bottom expansion | ||
| let left_px = (-expansion.x0).max(0.0).ceil() as u16; |
There was a problem hiding this comment.
Don't we need floor for the left and top pixels?
| // Reset the flag after popping the filtered layer with clip | ||
| if layer.filter.is_some() && layer.clip { | ||
| self.in_clipped_filter_layer = false; | ||
| } |
There was a problem hiding this comment.
What if we have two nested filter layers with clip? Won't that reset it for the first layer as well? Don't we need a stack of booleans here again?
| /// // 3: FillPath | ||
| /// // 4: PopBuf | ||
| /// ``` | ||
| pub fn list_commands(&self) -> String { |
There was a problem hiding this comment.
Does this need to be pub or is pub(crate) enough? And as mentioned, I think I personally would prefer not having a feature gate for this and just using the log crate.
|
|
||
| // Save the filtered pixmap to disk for debugging. | ||
| #[cfg(feature = "debug-filters")] | ||
| save_filtered_layer_debug(&pixmap, *layer_id); |
There was a problem hiding this comment.
I know it's a bit annoying if you want to use this multiple times, but maybe it would be better to just store the snippet locally and paste it whenever you need it?
| ctx.set_transform(Affine::IDENTITY); | ||
| ctx.set_paint(WHITE); |
There was a problem hiding this comment.
Do we need to undo these in the end?
| let mut tile_x = 0.0; | ||
| while tile_x <= max_width { | ||
| // Draw a thin vertical line | ||
| let line_width = 1.0; |
There was a problem hiding this comment.
Can be lifted out of the loop.
The previous expansion calculation extracted only x/y scales from the transform and applied them separately to each direction. This failed for rotated filters (e.g., drop shadows, blurs) where the axis-aligned bounding box needs to account for the full transform matrix including rotation and shear.
Additionally, clipped filter layers were being processed incorrectly: the filter only saw the clipped region instead of the full layer. Filters must process the entire layer first, then apply the clip, otherwise effects like blurs get cut off at clip boundaries.