Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow independent corner radii for RoundedRect #166

Merged
merged 6 commits into from
Feb 3, 2021

Conversation

SecondFlight
Copy link
Contributor

@SecondFlight SecondFlight commented Jan 30, 2021

Closes #138.

This PR allows RoundedRect to be created with four radius values instead of one, and updates the Shape impl accordingly.

All functions that take a f64 radius value now take an impl Into<RoundedRectRadii> value. RoundedRectRadii implements the from trait for f64, so existing code should not need to change in most cases.

Supporting this in Piet is not trivial as best I can tell. I took a cursory look at the breaking changes and noticed that, while Direct2D supports rounded rectangles, it only supports them with a single radius for all corners. I haven't looked at other platforms.

I am taking the existing unit test for to_path as evidence of correctness, but I'd like to ask my reviewer(s) to double-check the ordering of the corners in that function. If I messed up the ordering, then I believe the unit test would still pass.

Please let me know if there's anything I can fix or improve!

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks pretty good to me. I'm a little worried about all the edge cases (radii adding up to more than the rectangle side), but overall this looks like a careful job.

src/rounded_rect.rs Outdated Show resolved Hide resolved
src/rounded_rect.rs Outdated Show resolved Hide resolved
src/rounded_rect.rs Outdated Show resolved Hide resolved
src/rounded_rect_radii.rs Show resolved Hide resolved
src/rounded_rect_radii.rs Show resolved Hide resolved
@SecondFlight
Copy link
Contributor Author

SecondFlight commented Jan 30, 2021

I'm a little worried about all the edge cases (radii adding up to more than the rectangle side)

I don't believe this is possible.

RoundedRectRadii doesn't know how big the associated rectangle is, so the code to handle that edge-case is in rounded_rect.rs, specifically line 56:

let radii = radii.abs().clamp(shortest_side_length / 2.0);

clamp sets all radii to radius.max(clamp_value). I believe this addresses the edge-case, but I've been more wrong in the past 😅

@SecondFlight
Copy link
Contributor Author

I'll address the remaining comments next time I work on this.

@raphlinus
Copy link
Contributor

I believe this addresses the edge-case

Ok, I think it means nothing can go wrong, but it's also more conservative than it needs to be. For example, in a w x h rectangle, radii of (w, w, 0, 0) are possible, and this is a useful shape to draw. The logic to enforce that is not trivial, and there may not be an unambiguous "correct" solution as in the case of a single radius.

@tirix
Copy link

tirix commented Jan 30, 2021

The logic to enforce that is not trivial

I think enforcing would not be a big issue. It may either mean panicking:

assert radii.top_left + radii.top_right <= self.width;
assert radii.bottom_left + radii.bottom_right <= self.width;
assert radii.top_left + radii.bottom_left <= self.height;
assert radii.top_right + radii.bottom_right <= self.height;

Or clamping in a different way, arbitrarily giving priority to some corner over others:

radii.top_left.clamp(width);
radii.top_left.clamp(height);
radii.top_right.clamp(width - radii.top_left);
radii.top_right.clamp(height);
radii.bottom_left.clamp(width);
radii.bottom_left.clamp(height - radii.top_left);
radii.bottom_right.clamp(width - radii.bottom_left);
radii.bottom_right.clamp(height - radii.top_right);

However allowing the radii to go past the half dimensions will make several other calculations invalid, this is where it will not be trivial anymore.

@SecondFlight
Copy link
Contributor Author

I believe I've addressed all the review comments. Please let me know if I can add or change anything else.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, and I think the API is the right choice; I had initially thought it would make more sense to have separate methods for the non-uniform radii case, but this feels good.

@SecondFlight SecondFlight changed the title Independent corner radii for RoundedRect Allow independent corner radii for RoundedRect Feb 3, 2021
@raphlinus raphlinus merged commit 52fc659 into linebender:master Feb 3, 2021
@cmyr
Copy link
Member

cmyr commented Feb 3, 2021

I'll look into doing a kurbo release shortly.

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.

Enable RoundedRect to change the radius on all four corners
4 participants