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.

Sign up for GitHub

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

Added special handling to booleanPointOnLine for zero length lines #2751

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added special handling to booleanPointOnLine for zero length lines #2751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dfb4c5
Added special cases for zero length lines. booleanPointOnLine was ret…
smallsaucepan Nov 20, 2024
3c5a1d8
Merge branch 'master' into bool-on-zero-length-line
smallsaucepan Nov 20, 2024
cf9cfc1
Merge branch 'master' into bool-on-zero-length-line
smallsaucepan Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/turf-boolean-point-on-line/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,25 @@ function isPointOnLineSegment(
} else if (cross !== 0) {
return false;
}

// Special cases for zero length lines
// https://github.com/Turfjs/turf/issues/2750
if (Math.abs(dxl) === Math.abs(dyl) && Math.abs(dxl) === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this math need to be done with some small epsilon for the float comparison instead of ===?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be ok without. The new section is guarding against the old (lower) code getting too many 0s for dxl, etc. They don't seem to behave if they do.

If they get any value above zero, no matter how small, they cope. For example, did some experiments changing the test line string to:

lineString([
  [1, 1],
  [1 + Number.EPSILON, 1],
])

and all is well.

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 could try incorporating additional checks into the existing return dxl > 0 ? x1 <= x && x <= x2 : x2 <= x && x <= x1; type code but keeping them separate seemed neater.

// Zero length line.
if (excludeBoundary) {
// To be on a zero length line pt has to be on the start (and end), BUT we
// are excluding start and end from possible matches.
return false;
}
if (pt[0] === lineSegmentStart[0] && pt[1] === lineSegmentStart[1]) {
// If point is same as start (and end) it's on the line segment
return true;
} else {
// Otherwise point is somewhere else
return false;
}
}

if (!excludeBoundary) {
if (Math.abs(dxl) >= Math.abs(dyl)) {
return dxl > 0 ? x1 <= x && x <= x2 : x2 <= x && x <= x1;
Expand Down
35 changes: 34 additions & 1 deletion packages/turf-boolean-point-on-line/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import path from "path";
import { fileURLToPath } from "url";
import test from "tape";
import { loadJsonFileSync } from "load-json-file";
import { booleanPointOnLine as pointOnLine } from "./index.js";
import { point, lineString } from "@turf/helpers";
import booleanPointOnLine, {
booleanPointOnLine as pointOnLine,
} from "./index.js";

const __dirname = path.dirname(fileURLToPath(import.meta.url));

Expand Down Expand Up @@ -36,3 +39,33 @@ test("turf-boolean-point-on-line", (t) => {
});
t.end();
});

test("turf-boolean-point-on-line - issue 2750", (t) => {
// Issue 2750 was that in the first test below where point is on a different
// longitude to a zero length line booleanPointOnLine gave the correct result,
// while the second test where a point on the SAME longitude, but nowhere
// near, that zero length line incorrectly returned true.
t.false(
booleanPointOnLine(
point([2, 13]),
lineString([
[1, 1],
[1, 1],
])
),
"#2750 different longitude point not on zero length line"
);

t.false(
booleanPointOnLine(
point([1, 13]),
lineString([
[1, 1],
[1, 1],
])
),
"#2750 same longitude point not on zero length line"
);

t.end();
});
Loading