Lines with a 180 degree angle between them should be considered parallel (turf-boolean-parallel)

[mracette]

Checklist

• Use a meaningful title for the pull request. Include the name of the package modified.
• Have read How To Contribute.
• Run npm test at the sub modules where changes have occurred.
• Run npm run lint to ensure code style at the turf module level.

Description

booleanParallel currently does not recognize when parallel lines have opposite directions. This can be verified with a new test that compares the lines [ [0, 0], [0, 1] ] and [ [0, 1], [0, 0] ].

The slopes for these lines are 90 and 270 degrees, so they fail the slope1 === slope2 check. But according to the conditions for parallelism these 2 lines should be considered parallel.

A new condition is added in this PR that checks if the lines have an angle of 180 degrees between them. If so, they are considered parallel.

[mracette]

Another issue I've identified is that lines that are parallel to the y-axis have a slope of NaN. As a result, two vertical lines are not considered parallel either because NaN !== NaN. I believe this is due to the way calculateRhumbBearing works. If we can't change that function, maybe we should do a check for NaN in booleanParallel.

The question is - are there situations where both lines have slope of NaN and they are not parallel? Seems like it would be better to update calculateRhumbBearing if possible

[smallsaucepan]

Hi Mark. Going to review this for you if I can. Think you're right though in that calculateRhumbBearing might need some improvements, and would like to find out if we should tackle that first in case it has implications for your PR.

For a bit more background, the geodesy lib function calculateRhumbBearing is based on has an explicit check that returns NaN if the points are the same i.e. the line has no length so can have no bearing. Might return NaN in other cases too, though not sure what they would be.

The question is - are there situations where both lines have slope of NaN and they are not parallel? Seems like it would be better to update calculateRhumbBearing if possible

If either slope is NaN they can't be parallel as at least one line has an unknowable bearing. That might be why NaN was chosen as return type - even if both lines are illegitimate the caller is less likely to accidentally to do a simple compare and think they're parallel.

Another issue I've identified is that lines that are parallel to the y-axis have a slope of NaN.

Do you have some example data for this? Would like to reproduce that locally if I can as a first step.

[mracette]

Thanks @smallsaucepan!

Do you have some example data for this? Would like to reproduce that locally if I can as a first step.

Hm, actually I can't seem to reproduce this right now. I added a very simple vertical line test (60a9925) which is passing. If I come across the situation where I was seeing booleanParallel return false due to NaN slopes, I will let you know.

[smallsaucepan]

Looks good to me.

[mracette]

@smallsaucepan I found the failing test case, it involves 3d lines: 8c2d806

Any idea what might be causing this to fail?

[smallsaucepan]

Thanks @mracette. These coords are failing because they're outside the generally expected values for lon and lat: -180 to 180, and -90 to 90 respectively. Removing the altitude component gives the same NaN result. Here's what they look like projected:

Now whether turf-rhumb-bearing (which is what returns NaN) should be more forgiving, I can't say. Most calculators I've seen though insist on inputs being within those ranges.

[mracette]

Thanks @mracette. These coords are failing because they're outside the generally expected values for lon and lat: -180 to 180, and -90 to 90 respectively.

Ah, thanks for pointing that out @smallsaucepan. I think that clears everything up then, and I think we can merge this as an isolated fix for isParallel. If all looks good to you, then I believe I need an additional approval in order to move forward.

[mracette]

Hi @smallsaucepan, would you like to take another look at this before merge?

[smallsaucepan]

Looks good to me Mark. Think a maintainer also needs to approve for it to progress though.

[smallsaucepan]

Hi @mracette - you will need to update the coordinates in verticals3d.geojson as mentioned above, or the tests will keep failing.

[mracette]

@smallsaucepan Thank you, that should be fixed. I'm not seeing anything about code review in the contribution guidelines, but if you know who the maintainers are that need to approve this would you mind looping them in?

[twelch]

Thanks @mracette! You can expect it in the next release.