fonttools / skia-pathops

Python bindings for the Skia library's Path Ops
https://skia.org/docs/dev/present/pathops/
BSD 3-Clause "New" or "Revised" License
47 stars 14 forks source link

test incorrectly compares actual and expected results #58

Closed mklca closed 2 years ago

mklca commented 2 years ago

I ran into an issue building and testing this package with compiler architecture set for Skylake CPUs. This allows Skia to exploit vector processing capabilities which have a higher throughput and performance-per-watt but lower precision.

Some prior work 39dd41cab82314f9e7aab01a7a2bdf77bb89ae17 addressed the issue but introduces a numeric bug. Firstly, the actual and expected coordinates are rounded to a given grid precision:

https://github.com/fonttools/skia-pathops/blob/39dd41cab82314f9e7aab01a7a2bdf77bb89ae17/tests/pathops_test.py#L133-L134

These values are subsequently compared for exact equality:

https://github.com/fonttools/skia-pathops/blob/39dd41cab82314f9e7aab01a7a2bdf77bb89ae17/tests/pathops_test.py#L137

Here's an example of the failure from PathTest.test_transform:

((<PathVerb.MOVE: 0>, ((283.5499, 248.1945),)), (<PathVerb.CUBIC: 4>, ((323.1479, 208.5966), (323.1479, 156.2706), (288.4997, 121.6224))), (<PathVerb.CUBIC: 4>, ((242.5377, 75.6604), (201.5255, 89.8026), (163.3417, 127.9864))), (<PathVerb.CLOSE: 5>, ()))
((<PathVerb.MOVE: 0>, ((283.5499, 248.1945),)), (<PathVerb.CUBIC: 4>, ((323.1479, 208.5966), (323.1479, 156.2707), (288.4997, 121.6224))), (<PathVerb.CUBIC: 4>, ((242.5377, 75.6605), (201.5255, 89.8026), (163.3417, 127.9864))), (<PathVerb.CLOSE: 5>, ()))

There are changes at the fourth decimal digit in a few values: 156.2706 vs 156.2707, 75.6604 vs 75.6605.

Rounding the values before comparison can cause values that are within a unit of rounding to end up at different values. For instance, 0.499 and 0.501 differ by 0.002 but rounded to the nearest integer by the usual conventions we end up with 0 and 1.