Inline Clipper

[JimBobSquarePants]

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

• Introduces a port of the bit of Clipper2 we use that uses Vector2 throughout. This uses half the memory of the default implementation, brings a minor (~5ms for large geo polygon test) speed improvement, plus makes the code much easier to maintain and investigate for optimization opportunities.
• Exposes a ClippingOperation enum to IPath.Clip via ShapeOptions. Fixes Add possibility to make intersection of shapes #10
• Adds tests for vertical and horizontal path clipping. Fixes Flattening the result of a Clip gives different results for horizontal and vertical lines #250

#263 should be merged before this as I'll likely have to update the reference file again.

TODO;

• Finish cleaning up PolygonClipper and PolygonOffsetter to match standards.
• Add tests for the various ClippingOperation values.
• Fix rounding inaccuracy on .NET 472
• Ask @antonfirsov whether PolygonScanner needs to support the Clipper fill rules Positive and Negative

[codecov]

Codecov Report

Merging #265 (208660b) into main (1d451a2) will increase coverage by 17%.
The diff coverage is 63%.

@@          Coverage Diff          @@
##           main   #265     +/-   ##
=====================================
+ Coverage    61%    78%    +17%     
=====================================
  Files       100     97      -3     
  Lines      6427   5067   -1360     
  Branches   1374    932    -442     
=====================================
+ Hits       3923   3985     +62     
+ Misses     2308    898   -1410     
+ Partials    196    184     -12     

Flag	Coverage Δ
unittests	78% <63%> (+17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp.Drawing/Shapes/Path.cs	75% <0%> (ø)
...mageSharp.Drawing/Shapes/PolygonClipper/BoundsF.cs	0% <0%> (ø)
....Drawing/Shapes/PolygonClipper/ClipperException.cs	33% <0%> (+33%)	⬆️
...rp.Drawing/Shapes/PolygonClipper/PolygonClipper.cs	64% <ø> (ø)
...harp.Drawing/Shapes/PolygonClipper/ClipperUtils.cs	36% <36%> (ø)
....Drawing/Shapes/PolygonClipper/PolygonOffsetter.cs	76% <76%> (ø)
...mageSharp.Drawing/Shapes/PolygonClipper/Clipper.cs	97% <95%> (+23%)	⬆️
...rocessing/Processors/Text/RichTextGlyphRenderer.cs	95% <100%> (ø)
src/ImageSharp.Drawing/Processing/ShapeOptions.cs	100% <100%> (ø)
...rc/ImageSharp.Drawing/Shapes/ClipPathExtensions.cs	100% <100%> (ø)
... and 3 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[JimBobSquarePants]

None only seems useful if we allow choosing the operation via Outline. I don't know if it's wise to do so since Clipper.Offset uses Union by default with FillRule.Positive and FillRule.Negative which PolygonScanner does not support.

[JimBobSquarePants]

There's a good chance this can be removed and we can use RectangleF

[antonfirsov]

Finish cleaning up PolygonClipper and PolygonOffsetter to match standards.

I disagree this kind of refactoring is needed. We don't want to maintain (=understand + test) that code. Much better to include the original code as-is (only visibilities changed), and provide a facade on the top, so it is a no-brainer to keep it in sync with the upstream. Refactoring on the other hand gives extra work and has a non-zero chance to introduce bugs. I don't see the value, only downsides.

[JimBobSquarePants]

Finish cleaning up PolygonClipper and PolygonOffsetter to match standards.

I disagree this kind of refactoring is needed. We don't want to maintain (=understand + test) that code. Much better to include the original code as-is (only visibilities changed), and provide a facade on the top, so it is a no-brainer to keep it in sync with the upstream. Refactoring on the other hand gives extra work and has a non-zero chance to introduce bugs. I don't see the value, only downsides.

That horse has bolted I’m afraid. The refactor to half the memory usage and use Vector2 was significant. I’m happy to track upstream and implement any fixes.

[antonfirsov]

refactor to half the memory usage

I didn't know (or missed) that part. If so, it all makes sense. Wasn't able to look at this during the weekend, hope this week I can do it.

[JimBobSquarePants]

refactor to half the memory usage

I didn't know (or missed) that part. If so, it all makes sense. Wasn't able to look at this during the weekend, hope this week I can do it.

No worries. I found a few things to improve upon this evening anyway.

[tocsoft]

lgtm 👍