Streamline handling of pixel center offsets

[LaurenzV]

This resolves a TODO, such that we don't need "do and then undo" the center shift transform in vello_hybrid. It's not entirely clear to me why it doesn't work for blurred rounded rectangles, but unfortunately I don't really understand the algorithm in the first place, so I can't explore this further in the short term.

[grebmeg]

Could you investigate which case this is? If the algorithm is inherently center-sampled, the comment should document that understanding rather than leaving the uncertainty.

From what I can tell, this is the "algorithm already assumes centered sampling" case. The pixel center offset is hardcoded directly in the math here v1 = 0.5 in SimdRoundedBlurredRect::new, which then feeds into the distance field computation here:

// Equivalent to j + r.v1 - r.v1 * r.height  →  j + 0.5 - 0.5 * height
let y = j - r.v1.mul_sub(r.height, r.v1);
// Equivalent to i + r.v1 - r.v1 * r.width   →  i + 0.5 - 0.5 * width
let x = i - r.v1.mul_sub(r.width, r.v1);

[LaurenzV]

I did play around with this, but always got unreasonable test failures after changing. I think it's somehow encoded into the algorithm itself, but I can take another stab at investigating this, maybe I missed something.

[LaurenzV]

Okay you are right, the fix seemed pretty easy! I had to regenerate one of the snapshots, but I believe this is because the current snapshot is more correct than the previous one. The previous one would apply the 0.5 offset in the current user space (meaning it would be affected by the current scaling/skewing transform), while now it's always applied in pixel space (which should be the correct one).

[grebmeg]

One thing I'm curious about: did you consider an alternative where the encoding never bakes the +0.5 shift into the transform, and instead the CPU fine rasterizer applies it at the consumption site? Something like:

let start_pos = image.transform
    * Point::new(f64::from(start_x) + 0.5, f64::from(start_y) + 0.5);

The upside would be keeping EncodeExt::encode_into's signature unchanged (no new parameter), and making the responsibility clear: the consumer that iterates integer pixel coordinates is the one that decides where those integers fall in continuous space. It would also avoid the BlurredRoundedRectangle question entirely: no special-casing needed since the +0.5 would be applied uniformly to all paints at the same place.

[LaurenzV]

Yes, I briefly mentioned this in one of the comments, the reason I didn't do it this way is that we need that this will add two additions to every single wide tile command, instead of only needing it to apply once for the whole path and the transform automatically being propagated to all wide tiles. It's probably not going to have a noticeable impact, but since it seemed like an easy thing to avoid I chose the other way for now.

But if you prefer this, I'm happy to change it. The only downside is that it requires us to hardcode this change into every single painter kernel, so I'm not sure if it's better in that regard.

[LaurenzV]

The cleanest option is to create sampling_x and sampling_y f32 variables here with the offset applied, and then use it in the given paint samplers. What do you think?:

            Paint::Indexed(paint) => {
                let color_buf = &mut self.paint_buf[x * TILE_HEIGHT_COMPONENTS..]
                    [..TILE_HEIGHT_COMPONENTS * width];

                let encoded_paint = &encoded_paints[paint.index()];

                let start_x = self.wide_coords.0 * WideTile::WIDTH + x as u16;
                let start_y = self.wide_coords.1 * Tile::HEIGHT;

I think I agree now that it's probably more clean to apply that change in vello_cpu instead. WIll create a new commit that does this, but if you think the older version is better I can revert that.

[LaurenzV]

I think I like that new approach better, let me know what you think!

[grebmeg]

Nice!! I think it’s much cleaner now, thank you!

[LaurenzV]

Thanks for the great idea!