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

Fast polygon scanning with active edge list #96

Merged
merged 107 commits into from
Nov 20, 2020
Merged

Fast polygon scanning with active edge list #96

merged 107 commits into from
Nov 20, 2020

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Oct 26, 2020

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)

Summary

This PR is intended to narrow the performance gap we have compared to other frameworks by implementing the algorithm described in #90.

The PR is WIP, opened it to get early feedback. Remaining tasks:

  • Review the output of 40+ failing tests with image comparer tool. Check if any of them indicates a bug VS minor difference in output.
  • Deal with any bugs found
  • Implement NonZero intersection rule in PolygonScanner
  • Utilize PolygonScanner in DrawTextProcessor
  • Dedupe DrawTextProcessor / FillRegionProcessor code
  • Deal with text drawing bugs
  • Remove the logic enforcing TessellatedMultipolygon orientation, and make sure the algorithm renders more or less nice output with CCW outermost contours + CW holes (see Fast polygon scanning with active edge list #96 (comment))
  • Remove sandbox code from both product and test (eg. ShapeOptions.UsePolygonScanner)
  • Cleanup and fix Style Cop findings

Implementation

  • The pipeline works as following: IShape -> TesselatedMultipolygon -> ScanEdgeCollection (compact representation of scannable edges) -> PolygonScanner
  • y coordinates are rounded to the scanlines. This allows easy handling of corner cases. See NumericCornerCases.jpg
    • Horizontal lines are not emitted to ScanEdgeCollection. A line counts as horizontal by definition, if the whole line lies on the same scanline after rounding it's y coordinates.
  • For more details about the algorithm, see comments, and the drawings pushed to the repo with documentation purposes.

Results

Fixes #90 fixes #61 fixes #18.
Resolves #5 without offsetting the whole polygon, instead the calculation of coefficients in ScanEdge is centered around zero.

According to my visual observation, the new algorithm has fewer robustness issues now. I suspect the remaining ones are caused by an unhandled corner case rather than by numeric issue:

image

Performance

The performance gains scale with the number of points in the polygon to draw. ( ~8.5x speedup filling Utah's and ~62x speedup filling Missisippi's polygons.)

Configuration:
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-rc.1.20452.10
  [Host]     : .NET Core 3.1.8 (CoreCLR 4.700.20.41105, CoreFX 4.700.20.41903), X64 RyuJIT
  DefaultJob : .NET Core 3.1.8 (CoreCLR 4.700.20.41105, CoreFX 4.700.20.41903), X64 RyuJIT


FillPolygonSmall (Utah): 

|                 Method |         Mean |      Error |     StdDev |  Ratio | RatioSD |
|----------------------- |-------------:|-----------:|-----------:|-------:|--------:|
|          SystemDrawing |    210.91 us |   4.130 us |   6.670 us |   4.72 |    0.19 |
|    ImageSharp_Original | 28,572.78 us | 350.107 us | 292.356 us | 629.88 |   18.67 |
| ImageSharp_ActiveEdges |  3,342.78 us |  63.592 us |  68.043 us |  74.06 |    2.59 |
|              SkiaSharp |     44.79 us |   0.825 us |   1.184 us |   1.00 |    0.00 |


FillPolygonMedium (Mississippi):

|                 Method |         Mean |       Error |      StdDev |    Ratio | RatioSD |
|----------------------- |-------------:|------------:|------------:|---------:|--------:|
|          SystemDrawing |     358.2 us |     7.14 us |    10.68 us |     2.27 |    0.06 |
|    ImageSharp_Original | 187,078.3 us | 3,699.54 us | 6,181.11 us | 1,186.12 |   37.40 |
| ImageSharp_ActiveEdges |   2,884.7 us |    56.27 us |   109.75 us |    18.84 |    0.67 |
|              SkiaSharp |     156.8 us |     2.91 us |     2.43 us |     1.00 |    0.00 |


DrawPolygonMediumThin:


|                 Method |         Mean |       Error |       StdDev |    Ratio | RatioSD |
|----------------------- |-------------:|------------:|-------------:|---------:|--------:|
|          SystemDrawing |     697.7 us |    13.94 us |     25.49 us |     6.08 |    0.27 |
|    ImageSharp_Original | 407,472.0 us | 7,985.96 us | 10,931.27 us | 3,541.83 |  117.29 |
| ImageSharp_ActiveEdges |  18,913.2 us |   327.80 us |    350.75 us |   165.31 |    3.56 |
|              SkiaSharp |     114.6 us |     2.11 us |      1.98 us |     1.00 |    0.00 |


DrawPolygonMediumThick:


|                 Method |       Mean |     Error |    StdDev |     Median |  Ratio | RatioSD |
|----------------------- |-----------:|----------:|----------:|-----------:|-------:|--------:|
|          SystemDrawing |   1.539 ms | 0.0304 ms | 0.0621 ms |   1.515 ms |   0.97 |    0.04 |
|    ImageSharp_Original | 169.697 ms | 3.3095 ms | 4.7463 ms | 168.241 ms | 104.43 |    2.09 |
| ImageSharp_ActiveEdges |  38.795 ms | 0.7716 ms | 1.0562 ms |  38.591 ms |  24.16 |    0.74 |
|              SkiaSharp |   1.625 ms | 0.0158 ms | 0.0132 ms |   1.623 ms |   1.00 |    0.00 |

Comment on lines +127 to +135
// | Method | TextIterations | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
// |-------------- |--------------- |-------------:|-----------:|-----------:|-------:|--------:|----------:|---------:|------:|----------:|
// | SystemDrawing | 1 | 55.03 us | 0.199 us | 0.186 us | 5.43 | 0.03 | - | - | - | 40 B |
// | ImageSharp | 1 | 2,161.92 us | 4.203 us | 3.510 us | 213.14 | 0.52 | 253.9063 | - | - | 804452 B |
// | SkiaSharp | 1 | 10.14 us | 0.040 us | 0.031 us | 1.00 | 0.00 | 0.5341 | - | - | 1680 B |
// | | | | | | | | | | | |
// | SystemDrawing | 20 | 1,450.12 us | 3.583 us | 3.176 us | 27.36 | 0.11 | - | - | - | 3696 B |
// | ImageSharp | 20 | 28,559.17 us | 244.615 us | 216.844 us | 538.85 | 3.98 | 2312.5000 | 781.2500 | - | 9509056 B |
// | SkiaSharp | 20 | 53.00 us | 0.166 us | 0.147 us | 1.00 | 0.00 | 1.6479 | - | - | 5336 B |
Copy link
Member Author

@antonfirsov antonfirsov Nov 13, 2020

Choose a reason for hiding this comment

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

Unless I'm doing something wrong in this benchmark, we are still a lot behind when it comes to draw text.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... Anything obvious when profiling?

Copy link
Member Author

@antonfirsov antonfirsov Nov 13, 2020

Choose a reason for hiding this comment

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

Code is same as with FillRegion. I suppose GDI and Skia are utilizing some further optimizations for text rendering. Probably a global rasterized glyph cache? (This could be good candidate for LRU caching!)

@antonfirsov antonfirsov changed the title [WIP] Fast polygon scanning with active edge list Fast polygon scanning with active edge list Nov 13, 2020
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.

only thing I can see is the fact we don't seem have any need at all for any for the OrientationHandling logic which leaves us with additional maintenance.

public static TessellatedMultipolygon Create(
IPath path,
MemoryAllocator memoryAllocator,
OrientationHandling orientationHandling = OrientationHandling.ForcePositiveOrientationOnSimplePolygons)
Copy link
Member

Choose a reason for hiding this comment

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

ForcePositiveOrientationOnSimplePolygons ends up not actually influencing the algorithm do we need the extra complexity of the other types seeing as none of our library code uses anything but this option

Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanted to keep the flexibility to change this later if we want. Also: having the 3 options kinda documents what are we doing here and why.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

It all looks good to me, just a few questions really. I'll leave @tocsoft to do final approval as he actually understands this stuff! 😝

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Very happy!

tocsoft
tocsoft previously approved these changes Nov 16, 2020
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.

Agreed everything's looking good.... still not 100% convinced we want the extra (unused) logic in TessellatedMultipolygon but I'm happy to approve anyway. But I agree there is some follow up clean up actions to remove some unused/wanted codepaths, pretty much all of internal path can probably be dropped along with the more generic point in polygon/intersection logic etc from the paths.

@antonfirsov
Copy link
Member Author

@tocsoft I will remove the OrientationHandling stuff as part of this PR in the next few days.

@antonfirsov
Copy link
Member Author

@tocsoft all done, I'm letting you have a final look and/or merge.

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.

approved 👍 nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants