vello: make Scene clip/layers honor fill rule (even-odd clips)

[waywardmonkeys]

Scene layer APIs now take a peniko::StyleRef for the clip shape (push_layer, push_clip_layer, push_luminance_mask_layer), fixing the long-standing hardcoded Fill::NonZero clip behavior.

This is required for correctness in CSS/SVG semantics (e.g. clip-rule, fill-rule, and masking/clipPath content that relies on even-odd holes).

Using StyleRef also enables clipping to a stroke (pass &Stroke), so clips can be defined by a stroked outline as well as a filled interior.

To make even-odd clipping work end-to-end, plumb the fill rule through the clip pipeline:

• Give BEGIN_CLIP a 1-word info slot so clip draw flags can be stored.
• Populate clip draw_flags for BEGIN_CLIP in draw_leaf.
• In clip_leaf, propagate info_offset from BEGIN_CLIP to the corresponding END_CLIP.
• In coarse, apply clip layers with the correct fill rule by propagating BEGIN_CLIP draw flags through to END_CLIP, and by updating the “clip-zero” fast path to respect even-odd backdrop parity (GPU + CPU paths).

Existing callsites can continue passing Fill::NonZero to preserve prior behavior; Fill::EvenOdd selects even-odd clips.

[waywardmonkeys]

This was done with LLM assistance as it apparently understands some of the Vello internals better than me.

[tomcur]

This looks right to me, but I've not looked much at Vello Classic's shaders before, so it probably makes sense to have at least one more approval.

I was first confused at how this was able to work at all with the following include_tile logic, but have since convinced myself that that was a case of vestigial documentation of old logic (which I propose a fix for in #1333). The actual checks there and what the even-odd-filled clipping does seem to match.

vello/vello_shaders/shader/coarse.wgsl

Lines 335 to 344 in c15340d

	// If this draw object represents an even-odd fill and we know that no line segment
	// crosses this tile and then this draw object should not contribute to the tile if its
	// backdrop (i.e. the winding number of its top-left corner) is even.
	let backdrop_clear = select(tile.backdrop, abs(tile.backdrop) & 1, even_odd) == 0;
	let include_tile = n_segs != 0u || (backdrop_clear == is_clip) || is_blend;
	if include_tile {
	let el_slice = el_ix / 32u;
	let el_mask = 1u << (el_ix & 31u);
	atomicOr(&sh_bitmaps[el_slice][y * N_TILE_X + x], el_mask);
	}

[DJMcNab]

I'm fairly comfortable with this change, especially given the testing that's done.

As I say, I'd also like a screenshot test, which would correct our missing test for clip_test.

Obviously, I'm not a massive fan of the extra API complexity for this fairly niche use case. But I don't see how to avoid that in an ergonomic way, unless we introduce helpers which ignore this, which is also pretty nasty.

This is a light approval; in combination with Tom's review I think this gets it over the line. I would also quite like one of Raph or Chad to give it a once over; I'm not really a domain expert on this code. But I don't think we can usefully make that a blocker.

[tomcur]

Obviously, I'm not a massive fan of the extra API complexity for this fairly niche use case. But I don't see how to avoid that in an ergonomic way, unless we introduce helpers which ignore this, which is also pretty nasty.

I'm somewhat less worried about API complexity here, it's an important feature for correctness/completeness, but there's a good general question here about function parameter growth in these kinds of APIs versus, say, the wgpu-style of introducing something like LayerDescriptor structs.

One very nitty detail (which doesn't really solve the issue in your comment) is that it may be nice to name the parameter clip_fill_rule (as it only applies to the clip), and, while the current order of arguments agrees with e.g. Scene::fill, it may then be nice to order the clip_fill_rule parameter to be just in front of clip.

[waywardmonkeys]

One very nitty detail (which doesn't really solve the issue in your comment) is that it may be nice to name the parameter clip_fill_rule (as it only applies to the clip), and, while the current order of arguments agrees with e.g. Scene::fill, it may then be nice to order the clip_fill_rule parameter to be just in front of clip.

I've renamed it. I did go with this order to match Scene::fill ... but I guess we have until we do a release to bikeshed the ordering. :)

[tomcur]

Looks good!

[dfrg]

I would also quite like one of Raph or Chad to give it a once over

The logic LGTM.

If we're adding a new parameter, I wonder if we should go all the way and take a peniko::StyleRef instead? This would allow clipping to a stroke as well.

[waywardmonkeys]

I would also quite like one of Raph or Chad to give it a once over

The logic LGTM.

If we're adding a new parameter, I wonder if we should go all the way and take a peniko::StyleRef instead? This would allow clipping to a stroke as well.

At first, I didn't really understand how this would work ... but with some LLM assistance, I think I have it working.

This deserves a bit of review, especially the bit about handling an empty stroke.

[tomcur]

This looks correct to me.

I do have a follow-up that enables dashed stroked clipping (like Scene::stroke) and rejigs some of the invalid/empty path handling, but I think it makes sense to do that as a separate PR: the current PR works fine as-is, and this would be a bit of a larger diff to Scene::push_layer_inner (plus questions about potentially factoring out some shared logic with Scene::stroke).

For completeness:

--- a/vello/src/scene.rs
+++ b/vello/src/scene.rs
@@ -212,34 +212,57 @@ impl Scene {
     ) {
         let t = Transform::from_kurbo(&transform);
         self.encoding.encode_transform(t);
-        let (is_fill, stroke_for_estimate) = match clip_style {
+
+        // The logic for encoding the clip shape differs between fill and stroke style clips, but
+        // the logic is otherwise similar.
+        //
+        // `encoded_result` will be `true` if and only if a valid path has been encoded. If it is
+        // `false`, we will need to explicitly encode a valid empty path.
+        let encoded_result = match clip_style {
             StyleRef::Fill(fill) => {
                 self.encoding.encode_fill_style(fill);
-                (true, None)
+                #[cfg(feature = "bump_estimate")]
+                self.estimator.count_path(clip.path_elements(0.1), &t, None);
+                self.encoding.encode_shape(clip, true)
             }
             StyleRef::Stroke(stroke) => {
                 let encoded_stroke = self.encoding.encode_stroke_style(stroke);
-                (false, encoded_stroke.then_some(stroke))
+                if !encoded_stroke {
+                    // If the stroke has zero width, encode a fill and indicate no path was
+                    // encoded.
+                    self.encoding.encode_fill_style(Fill::NonZero);
+                    false
+                } else {
+                    if stroke.dash_pattern.is_empty() {
+                        #[cfg(feature = "bump_estimate")]
+                        self.estimator
+                            .count_path(clip.path_elements(0.1), &t, Some(stroke));
+                        self.encoding.encode_shape(clip, false)
+                    } else {
+                        // TODO: We currently collect the output of the dash iterator because
+                        // `encode_path_elements` wants to consume the iterator. We want to avoid calling
+                        // `dash` twice when `bump_estimate` is enabled because it internally allocates.
+                        // Bump estimation will move to resolve time rather than scene construction time,
+                        // so we can revert this back to not collecting when that happens.
+                        let dashed = peniko::kurbo::dash(
+                            clip.path_elements(0.1),
+                            stroke.dash_offset,
+                            &stroke.dash_pattern,
+                        )
+                        .collect::<Vec<_>>();
+                        #[cfg(feature = "bump_estimate")]
+                        self.estimator
+                            .count_path(dashed.iter().copied(), &t, Some(stroke));
+                        self.encoding
+                            .encode_path_elements(dashed.into_iter(), false)
+                    }
+                }
             }
         };
-        if stroke_for_estimate.is_none() && matches!(clip_style, StyleRef::Stroke(_)) {
-            // If the stroke has zero width, encode a valid empty path. This suppresses
-            // all drawing until the layer is popped.
-            self.encoding.encode_fill_style(Fill::NonZero);
-            self.encoding.encode_empty_shape();
-            #[cfg(feature = "bump_estimate")]
-            {
-                use peniko::kurbo::PathEl;
-                let path = [PathEl::MoveTo(Point::ZERO), PathEl::LineTo(Point::ZERO)];
-                self.estimator.count_path(path.into_iter(), &t, None);
-            }
-            self.encoding.encode_begin_clip(parameters);
-            return;
-        }

-        if !self.encoding.encode_shape(clip, is_fill) {
-            // If the layer shape is invalid, encode a valid empty path. This suppresses
-            // all drawing until the layer is popped.
+        if !encoded_result {
+            // If the layer shape is invalid or a zero-width stroke, encode a valid empty path.
+            // This suppresses all drawing until the layer is popped.
             self.encoding.encode_empty_shape();
             #[cfg(feature = "bump_estimate")]
             {
@@ -247,10 +270,6 @@ impl Scene {
                 let path = [PathEl::MoveTo(Point::ZERO), PathEl::LineTo(Point::ZERO)];
                 self.estimator.count_path(path.into_iter(), &t, None);
             }
-        } else {
-            #[cfg(feature = "bump_estimate")]
-            self.estimator
-                .count_path(clip.path_elements(0.1), &t, stroke_for_estimate);
         }

[tomcur]

This should now be updated, something like the following.

Suggested change

	- Breaking change: Make `Scene` clip / layers honor fill rule (even-odd clips). ([#1332][] by [@waywardmonkeys][])
	When pushing a layer, you should use `Fill::NonZero` as the clip fill rule to achieve the same behavior as previous versions.
	- Breaking change: Allow setting `Scene` layer clipping style, adding even-odd filled path clipping and stroked path clipping. ([#1332][] by [@waywardmonkeys][])
	When pushing a layer, you should use `StyleRef::Fill(Fill::NonZero)` as the clip fill rule to achieve the same behavior as previous versions.

[waywardmonkeys]

It takes Into<StyleRef> so Fill::NonZero works and is more concise.

[tomcur]

Good point, didn't realize we had the From, but that makes sense.