Change the implementation of getWinding() again to pass all unit tests.

[iconexperience]

This new version does not count any intersections if the point is on a y-monotonic curve, but at the final calculation it sets the winding to at least 1 if the point is on a curve.

[iconexperience]

This was expected to fail because the unit test Path.contains() (straight curves with zero-winding) counts some points on a curve as being outside the path.

@lehni: Maybe I should have combined the two pull requests. Combined they should have passed the Travis checks.

[iconexperience]

Here is another test that fails with the current code, but not after the new change:

var c1 = new Path.Circle({center: [200, 200], radius: 100});
var c2 = new Path.Circle({center: [150, 200], radius: 50});
var cp = new CompoundPath({children: [c1, c2], strokeColor: "blue"});
var point = new Point(100, 200);
new Path.Circle({center: point, radius: 3,  fillColor: "red"});
console.log("contains point: " + cp.contains(point));

Here is the Sketch

[lehni]

So I have a question:

On line 396, you test if the point is on the horizontal curve like this, comparing against zero with < 0. I don't fully understand this check, could you explain it?

(px - x) * (px - prevXEnd) < 0)

On line 409, you have the same check, but with <= 0. intentional?

(px - values[0]) * (px - values[6]) <= 0

[lehni]

And regarding #944 (comment), I shall turn this case into a unit test! We have to protect ourselves against regressions in edge cases like these.

[lehni]

@iconexperience and yes, it would have been better to just create one PR. a PR can contain many commits. In fact, even after opening a PR here, all the new commits to your branch that you created the PR for will appear here, until I merge the PR in. And since the two topics are so closely related, one PR would have been more suitable.

Another topic: It's good if the commit messages are quite short. It's fine to write more information into a commit message, but it's better to keep the first line short, and then add a 2nd paragraph with the details, like this:

Change the implementation of getWinding() again, so we pass all tests

This new version does not count any intersections if the point is on a y-monotonic curve, but at the final calculation it sets the winding to at least 1 if the point is on a curve.

This is also to prevent GitHub force-breaking the first line. But it's just considered good form, generally.

[iconexperience]

@lehni If the distances px - x and px - prevXEnd are have opposite signs, it means that x is between px and prevXEnd. By using <0instead of <=0 we exclude the cases px == x and prevXEnd == x, but these are already handled in line 383, so we save one character :)

Please note that we combine this with the condition py === yStart (which implies py === prevEndY), so this test checks if p is on the horizontal line between [yStart, xStart] and [prevXEnd, prevYEnd].

But in line 409 we need to include the end points of the line, otherwise we would miss this point, because there is no non-horizontal monocurve between two horizontal curves:

[iconexperience]

And thanks for all the help on becoming a better Github citizen :)

[lehni]

: )