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

Prevent PathGradientBrush from throwing an error with corner cases #1007

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Prevent PathGradientBrush from throwing an error with corner cases #1007

merged 3 commits into from
Sep 12, 2019

Conversation

mysticfall
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

While testing the initial implementation of PathGradientBrush with a more complex scenario, I found that there are such cases where it fails to find intersection points for a given position.

The previous code assumed it would be always possible to calculate an intersecting point of a line between the center position and an arbitrary point within the paint area. But it seems that the assumption was wrong for very small polygons, for a reason that I have yet to understand perfectly.

So, I just made a small correction to ignore such corner cases which prevents an error from halting the paint process. And it looks like ignoring them doesn't affect the output image noticeably.

As such, I think we could address this problem with the proposed change without breaking any existing code (that is, if anyone beside me is already using the new gradient brush), although further investigation might be needed in future.

There can be such cases where it fails to find intersection points for
a given position, especially when the polygon is small.

Such an input would result in an error previously, but now it renders
correctly.
@mysticfall
Copy link
Contributor Author

Not sure why one of the build fails, but I think it's not related to the changes I made?

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #1007 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1007   +/-   ##
=======================================
  Coverage   89.85%   89.85%           
=======================================
  Files        1098     1098           
  Lines       48863    48863           
  Branches     3433     3433           
=======================================
  Hits        43908    43908           
  Misses       4254     4254           
  Partials      701      701
Impacted Files Coverage Δ
...ImageSharp.Drawing/Processing/PathGradientBrush.cs 90.9% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b73284...1eea497. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #1007 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1007   +/-   ##
=======================================
  Coverage   89.85%   89.85%           
=======================================
  Files        1098     1098           
  Lines       48863    48863           
  Branches     3433     3433           
=======================================
  Hits        43908    43908           
  Misses       4254     4254           
  Partials      701      701
Impacted Files Coverage Δ
...ImageSharp.Drawing/Processing/PathGradientBrush.cs 90.9% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b73284...1eea497. Read the comment docs.

@mysticfall
Copy link
Contributor Author

Also, I changed type of the constructor argument from ILineSegment[] to PointF[].

The reason is, users would normally start with points and their associated colors, so it can be quite cumbersome to construct line segments by hand. The equivalent class in .NET API (System.Drawing.Drawing2D) also expects an array of points, so I think this change could make the API look more intuitive.

Also, I realized that LinearLineSegment can potentially have more than 2 points, and the case was not considered in the original codebase.

I'm not sure if I can make a breaking change to the existing API at this stage. So, if it's not allowed, I'll update the PR without the relevant part.

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍.
I would agree and think the breaking change is fine in this case.

@tocsoft tocsoft merged commit a4a7feb into SixLabors:master Sep 12, 2019
@mysticfall
Copy link
Contributor Author

Thanks for the quick review! :)

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
Prevent PathGradientBrush from throwing an error with corner cases
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants