Skip to content

Conversation

@antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Oct 22, 2021

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

The original SkipEdgesBeforeMinY() implementation in #96 forgot to watch cases when all edges of the rendered polygon crossed the top boundary, meaning that all ScanEdge Y0 are < minY, leading to i0 overflow this.sorted0. #119 added a workaround for some cases but broke the merge sort -style coordinate walking algorithm which should always pick the next smaller coordinate to traverse the elements of two sorted lists in ascending order.

The PR adds bounds checks to the original algorithm. Fixes #170, fixes #108.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #171 (8bb6407) into master (eb510f3) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   69.98%   69.98%   -0.01%     
==========================================
  Files          89       89              
  Lines        5201     5204       +3     
  Branches     1062     1065       +3     
==========================================
+ Hits         3640     3642       +2     
  Misses       1347     1347              
- Partials      214      215       +1     
Flag Coverage Δ
unittests 69.98% <85.71%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...arp.Drawing/Shapes/Rasterization/PolygonScanner.cs 96.73% <85.71%> (-1.02%) ⬇️

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 eb510f3...8bb6407. Read the comment docs.

@antonfirsov
Copy link
Member Author

antonfirsov commented Oct 22, 2021

I'm still not big friend of codecov.

image

image

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.

Nice job! 👍

// Do fake scans of the lines that start before minY.
// Instead of fake scanning at every possible subpixel Y location,
// only "scan" at start edge Y positions (defined by values in sorted0) and end Y positions (defined by values in sorted1).
// Walk the two lists simultaneously following mergesort logic.
Copy link
Member

Choose a reason for hiding this comment

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

Veeeery cool!

@JimBobSquarePants JimBobSquarePants merged commit e987bd8 into master Oct 23, 2021
@JimBobSquarePants JimBobSquarePants deleted the af/fix-SkipEdgesBeforeMinY branch October 23, 2021 12:56
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