Skip to content

Conversation

@valadaptive
Copy link
Contributor

"Round these floating-point bounds outwards to integer coordinates" is a common operation for 2D graphics libraries, and half of the ones I see implement it incorrectly, as did tiny-skia.

Let's say we have a path that is 2px wide, at x = 0.5:
image

It at least partially covers columns 0, 1, and 2. That's 3 columns, so its rounded bounding box is 3 pixels wide. However, taking the ceiling of the width independent of its position would say it's 2 pixels wide!

We need to take the fractional position of the bounding box into account when calculating its rounded width and height. Or to put it another way, we need to convert the bottom and right edges into absolute coordinates before taking the ceiling, and then convert those back into relative coordinates.

Not changed in this PR, but now I'm suspicious of it: instead of always rounding the width and height up to 1, should we be returning None if they are not strictly positive?

@valadaptive valadaptive requested a review from LaurenzV August 22, 2025 16:56
@valadaptive valadaptive changed the title Fix Rect::round_out Fix Rect::round(_out) Aug 22, 2025
These tests seem to be...kinda broken? I mean, we explicitly use
saturating casts to *avoid* failure. Because we now saturate every
absolute coordinate, I think these methods are infallible for now.
@valadaptive
Copy link
Contributor Author

Alright; I think I've gotten this in a state where it can be reviewed. Sorry for the churn.

I've just removed the round_overflow rect tests for the time being. I believe that doing saturating casts on all the absolute coordinates makes the conversion infallible. As I mentioned at the bottom of the PR description, we might want to return None if the coordinates are out-of-bounds or the dimensions are negative, though, so I'll keep the signature for now. Let me know what your opinion is on this.

@valadaptive
Copy link
Contributor Author

Also note that this did require regenerating a snapshot test; rendering a rectangle with floating-point coords with antialiasing turned off is visually different now. I believe the new render is the correct one.

@jneem
Copy link
Member

jneem commented Aug 28, 2025

I don't know how much we care, but this does lose precision when there's a large coordinate: if the original x is i32::MAX / 2 and the original width is i32::MAX then the rounded rect will truncate the width to i32::MAX / 2 (because we'll truncate right to i32::MAX). I think you can get the best of both worlds by first saturate_rounding x, casting it back to f32, computing the width in f32, and then saturate_rounding the new resulting width.

@valadaptive
Copy link
Contributor Author

I don't know how much we care, but this does lose precision when there's a large coordinate: if the original x is i32::MAX / 2 and the original width is i32::MAX then the rounded rect will truncate the width to i32::MAX / 2 (because we'll truncate right to i32::MAX). I think you can get the best of both worlds by first saturate_rounding x, casting it back to f32, computing the width in f32, and then saturate_rounding the new resulting width.

Skia itself stores SkIRect coordinates in absolute form. I believe we only store width/height here to preserve the invariant that they're non-zero.

Our IntRect also promises:

x+width and y+height does not overflow.

So I believe truncating the width to i32::MAX / 2 is correct.

@valadaptive valadaptive merged commit 624257c into linebender:main Aug 28, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants