I'm a bit confused about this section of code.

Do I need anything here to replace the old code?

GeometryResult::Mode::kNormal used to fall through to the GeometryResult::Mode::kPreventOverdraw case and use the kOverdrawPreventionIncrement stencil mode. Was that intentional? What's the purpose of this?

I'm not 100% sure on this. It may have been that kNormal just didn't care about these particular settings because, for instance, it didn't actually run the stencil pass anyway.
@gaaclarke @chinmaygarde ?

If memory serves, GeometryResult::Mode::kNormal means there are no overlapping triangles. Now I don't think you can entirely disregard whats on the stencil buffer and ignore the stencil test directly. Since normal is the simplest case of the lot, I suspect it was lumped together with PreventOverdraw. But disregarding the test entirely does seem off though.

Now that you are no longer using the stencil buffer, lumping it together with either the EO or NZ modes should be fine I think.

I think you're right. I moved the kNormal case to be lumped with kNonZero, and I've verified locally that the broken iOS app scenario tests are now passing with with change applied. I updated the PR to do this.

Should I do anything with the kPreventOverdraw case here? Is that case never expected to happen for a ClipContents? That's my initial thought, but my understanding how all this code works is very incomplete. If that case is unexpected and incompatible with ClipContents, I can add a FML_UNREACHABLE() here. Otherwise, should I also lump kPreventOverdraw in with one of the other cases?

Ok, after studying this a bit, I think I have a much better understanding of how things work for ClipContents.

With the existing behavior, ClipContents always performs a clip by using stencil-and-cover. It does a render pass for the preparation stencil, which sets the stencil buffer according to the geometry. And then it does a second cover render pass that writes the depth (performing the actual clip) and resets the stencil buffer. It uses this stencil even for geometries kNormal and kPreventOverdraw. For those geometries, it uses a stencil mode that always fills in a 1 value regardless of any triangle overlap in the geometry.

My first PR revision removed the stencil step for kNormal and kPreventOverdraw. This was incorrect because it would render an empty stencil for the stencil step. So the cover step would clip everything. This caused the test breakages in my first revision, where the goldens showed a lot of content was inadvertently clipped out completely.

In a subsequent revision, I followed Chinmay's advice to make kNormal and kPreventOverdraw fall back to the kNonZero stencil case. The stencil is now properly filled out and these cases now work pretty close to how they did before.

I realize that we can do a little better specifically for difference clips.

Clipping needs to use stencil-and-cover for nonzero or evenodd geometry cases because those cases have overlapping triangles, and the stencil pass is needed to determine whether each of these overlaps should be included in the depth write.

But for kNormal and kPreventOverdraw, we don't need stencil-and-cover. The kNormal case has no overlapping triangles, so the depth write has no overlapping triangles to deal with. For the kPreventOverdraw case, any overlapping triangles should unconditionally be included in the depth write (unlike the nonzero or evenodd cases). So kNormal and kPreventOverdraw geometries can do a difference clip with a single render pass and does not need to use a stencil.

I just updated my PR to do this.

Edit: It turns out there are slight differences between the kStencilNonZeroFill stencil mode and what we want for kNormal/kPreventOverdraw geometries, which show up as a few small pixel differences in the golden tests. This means we can't fall through to kStencilNonZeroFill for kNormal/kPreventOverdraw. Instead, I made a new kStencilIncrementAll stencil mode which does what we want: set the stencil for everything covered by a triangle regardless of any triangle overlaps. This is the same as what the old kOverdrawPreventionIncrement stencil mode did.

I'm not 100% sure on this. It may have been that kNormal just didn't care about these particular settings because, for instance, it didn't actually run the stencil pass anyway.
@gaaclarke @chinmaygarde ?

Since the new rendering approach uses depth testing to eliminate overdraw, we no longer need to check whether the BlendMode is kSrc. In the past, this check helped skip an extra stencil pass when drawing opaque stroke paths, but overdraw still occurred (Some drivers may optimize this away, but such behavior is not guaranteed). With depth testing, overdraw for opaque stroke paths is avoided entirely.

Actually, it can potentially make things a little faster for blend mode since it avoids storing a new pixel value on a previously rendered pixel.

If we trust this approach well enough then we can eventually just make this all unconditional.

So is the suggestion to remove the blend_mode != kSrc check here?

In line 213 above, when BlendMode is kSrc, then depth_write_enabled is set to true (and depth_compare remains its default kGreaterEqual). The comment says this is "to allow reordering". But I don't fully understand what this means or what this does.

Keeping the blend_mode != kSrc check here preserves this existing behavior when BlendMode is kSrc. I didn't want to change the existing behavior because I didn't know whether changing this behavior would do something unintended.

So current behavior with this PR is:

If the options.blend_mode != BlendMode::kSrc here is removed, the behavior would be:

Is this change in the behavior for the (BlendMode == kSrc) && (kPreventOverdraw == true) case acceptable and/or desired?

Or is the suggestion to both remove the blend_mode != kSrc check here, and also remove setting depth_write_enabled when BlendMode is kSrc? The behavior would be:

So is the suggestion to remove the blend_mode != kSrc check here?

Yes, it is.

In line 213 above, when BlendMode is kSrc, then depth_write_enabled is set to true (and depth_compare remains its default kGreaterEqual). The comment says this is "to allow reordering". But I don't fully understand what this means or what this does.

Impeller automatically rewrites the BlendMode from kSrcOver to kSrc when it detects that the content being drawn is fully opaque. In kSrc mode, the Render backend turns blending off entirely.

Given that, imagine we’re drawing three opaque shapes A → B → C (from back to front). Because blending is disabled for these draws, their visual result no longer depends on the draw order. Once depth writes are enabled, we can safely reorder them—for example, drawing C → B → A instead of the original order.

The benefit of this reordering is that it allows the GPU to reject more fragments early. Drawing the frontmost opaque geometry first lets early-z eliminate a large portion of the fragments from the shapes behind it, which reduces overdraw and lowers fragment-shader cost overall.

If the options.blend_mode != BlendMode::kSrc here is removed, the behavior would be:

Is this change in the behavior for the (BlendMode == kSrc) && (kPreventOverdraw == true) case acceptable and/or desired?

Yes, this change is acceptable — in fact, it’s exactly the behavior we want.

Thanks for the thorough explanation. I removed the blend_mode != kSrc check as you suggested.