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

Support individual corner radii for kurbo::RoundedRect #391

Merged
merged 11 commits into from
Feb 9, 2021

Conversation

SecondFlight
Copy link
Contributor

@SecondFlight SecondFlight commented Feb 3, 2021

image

This PR adds support for this Kurbo patch. This PR points directly at my Kurbo fork, so it probably shouldn't be merged until the Kurbo PR is merged and a new Kurbo version is published.

piet-coregraphics and piet-cairo both get this feature for free, because both platforms use paths for rounded rectangles. I have tested both platforms and verified this.

piet-web should get this feature for free as well, for the same reason. I have not explicitly tested this, but I feel confident about it since to_path is well-tested and is not platform-specific, and RoundedRect is not mentioned in the code.

In piet-svg, this has been implemented by simply using a bezier curve if the corner radii aren't all equal, and a rounded rectangle if they are. Rounded rectangles in SVG can only have a single radius, and while it's possible to use some clipping magic, I don't think it's worth the effort.

In piet-direct2d, this has been implemented by drawing four rounded rectangles, one for each corner. They are clipped using an axis-aligned clip, which is faster than masking with a shape. If all corner radii are equal, a single rounded rectangle is drawn.

piet-direct2d has a similar implementation to piet-svg.

@cmyr
Copy link
Member

cmyr commented Feb 3, 2021

Will give this a look through shortly, and will do a release when we land this + #389 (and maybe #387?)

@raphlinus
Copy link
Contributor

Will give this a look through shortly, and will do a release when we land this + #389 (and maybe #387?)

I'd love to see #389 go in, though I consider it pretty high risk. I don't think #387 is ready, as there are serious unresolved questions in the Direct2D port in particular (and those are not just implementation details, but point to deeper architectural concerns imho).

@SecondFlight
Copy link
Contributor Author

SecondFlight commented Feb 6, 2021

The most recent commit addresses a major oversight when rendering in Direct2D. See the top-middle here:
image

versus here:
d2d-test-6

Sample image 6 has also been updated. I'll have a PR for piet-snapshots shortly.

@SecondFlight SecondFlight marked this pull request as ready for review February 6, 2021 00:36
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.

One note that is totally not a blocker, otherwise this looks good to me. I've merged the piet-snapshots PR, so you should be able to update the submodule in this PR and then we'll pass CI (except for this funny wasm failure that I don't think is your fault..)

match path_from_shape(self.factory, true, shape, fill_rule) {
Ok(geom) => self.rt.fill_geometry(&geom, &brush, None),
Err(e) => self.err = Err(e),
if let Some(radius) = round_rect.radii().as_single_radius() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this as before, just changing,

if let Some(round_rect) = shape.as_rounded_rect().filter(|r| r.as_single_radius().is_some()) {
}

This isn't a major point, but it cleans up the diff somewhat.

@SecondFlight
Copy link
Contributor Author

Okay, I'll update this soon. I was really hoping for a better way to do that, because my version feels really clunky.

@SecondFlight
Copy link
Contributor Author

That was just sloppy, sorry. I forgot that building and testing piet doesn't build or test piet-direct2d or etc.

Now the snapshots are still failing. Not sure what's going on, since they're freshly updated. I'll poke at this more when I get time next, but that may be a couple days.

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.

Looks good!

@cmyr cmyr merged commit cce3710 into linebender:master Feb 9, 2021
SecondFlight added a commit to linebender/piet-snapshots that referenced this pull request Feb 11, 2021
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.

3 participants