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

Implement rounded rectangle stroke #111

Merged
merged 2 commits into from Jul 7, 2017
Merged

Conversation

nivkner
Copy link
Contributor

@nivkner nivkner commented Jul 7, 2017

Fixes #38.

The problem with the #82 was the segmentation of the shape, which could result in a mismatch between the different parts. This is addressed by producing one continuous shape, and by finding only the inner points in rounded borders (since the outer ones could be part of other paths).

Copy link
Owner

@nical nical left a comment

Choose a reason for hiding this comment

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

Looks great! if we can move the stroke_border_radius_build into the loop it'd be perfect, otherwise the PR is already in a good shape and we can merge it.

let p4 = point(x_max, y_max - br);

let sides = &[
[p1, p2],
Copy link
Owner

Choose a reason for hiding this comment

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

nit: let's indent this one extra level.

&mut builder,
);
for i in 1..4 {
builder.line_to(sides[i][0]);
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't the end of the previous iteration already going to finish at this position? If we can make it so, then you could even move the line_to and the stroke_border_radius_build above into the loop (and just have the move_to outside the loop).

@nivkner
Copy link
Contributor Author

nivkner commented Jul 7, 2017

moved the first stroke_border_radius_build and line_to into the loop as requested.

Copy link
Owner

@nical nical left a comment

Choose a reason for hiding this comment

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

Neat.

@nical nical merged commit e47d313 into nical:master Jul 7, 2017
@nical
Copy link
Owner

nical commented Jul 7, 2017

Thanks a lot!

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.

None yet

2 participants