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

Revert self intersection behavior: line-intersect #2742

Merged
merged 11 commits into from
Dec 14, 2024
Merged

Revert self intersection behavior: line-intersect #2742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f5df121
Update self intersection argument, doc gen, add self intersection geo…
pm0u Oct 30, 2024
39e8236
Merge branch 'master' of github.com:pm0u/turf into pm0u/self-intersec…
pm0u Dec 4, 2024
a0be0c2
Remove additional param clarifications
pm0u Dec 4, 2024
7a54fec
Add fixture for self intersecting multi-line in non-intersecting coll…
pm0u Dec 4, 2024
e77c500
More test cases
pm0u Dec 4, 2024
f82e274
Merge branch 'master' into pm0u/self-intersections-line-intersect
smallsaucepan Dec 5, 2024
2c8d081
Merge branch 'master' of github.com:pm0u/turf into pm0u/self-intersec…
pm0u Dec 11, 2024
8559032
revert removal of example, add comment explaining test
pm0u Dec 11, 2024
92a39b4
Merge branch 'pm0u/self-intersections-line-intersect' of github.com:p…
pm0u Dec 11, 2024
cc80f9b
re include newline in example
pm0u Dec 11, 2024
542bba7
update readme
pm0u Dec 11, 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
2 changes: 1 addition & 1 deletion packages/turf-line-intersect/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Takes any LineString or Polygon GeoJSON and returns the intersecting point(s).
* `options` **[Object][2]** Optional parameters (optional, default `{}`)

* `options.removeDuplicates` **[boolean][3]** remove duplicate intersections (optional, default `true`)
* `options.ignoreSelfIntersections` **[boolean][3]** ignores self-intersections on input features (optional, default `false`)
* `options.ignoreSelfIntersections` **[boolean][3]** ignores self-intersections on input features (optional, default `true`)

### Examples

Expand Down
4 changes: 2 additions & 2 deletions packages/turf-line-intersect/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { sweeplineIntersections as findIntersections } from "./lib/sweepline-int
* @param {GeoJSON} line2 any LineString or Polygon
* @param {Object} [options={}] Optional parameters
* @param {boolean} [options.removeDuplicates=true] remove duplicate intersections
* @param {boolean} [options.ignoreSelfIntersections=false] ignores self-intersections on input features
* @param {boolean} [options.ignoreSelfIntersections=true] ignores self-intersections on input features
* @returns {FeatureCollection<Point>} point(s) that intersect both
* @example
* var line1 = turf.lineString([[126, -11], [129, -21]]);
Expand All @@ -40,7 +40,7 @@ function lineIntersect<
ignoreSelfIntersections?: boolean;
} = {}
): FeatureCollection<Point> {
const { removeDuplicates = true, ignoreSelfIntersections = false } = options;
const { removeDuplicates = true, ignoreSelfIntersections = true } = options;
let features: Feature<G1 | G2>[] = [];
if (line1.type === "FeatureCollection")
features = features.concat(line1.features);
Expand Down
65 changes: 57 additions & 8 deletions packages/turf-line-intersect/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,9 @@ import path from "path";
import { loadJsonFileSync } from "load-json-file";
import { writeJsonFileSync } from "write-json-file";
import { truncate } from "@turf/truncate";
import {
featureCollection,
// geometryCollection,
lineString,
polygon,
} from "@turf/helpers";
import { featureCollection, lineString, polygon } from "@turf/helpers";
import { lineIntersect } from "./index.js";
import { Feature, LineString, Point } from "geojson";

const directories = {
in: path.join("test", "in") + path.sep,
Expand All @@ -21,14 +17,18 @@ const fixtures = fs.readdirSync(directories.in).map((filename) => {
return {
filename,
name: path.parse(filename).name,
geojson: loadJsonFileSync(directories.in + filename),
geojson: loadJsonFileSync(
directories.in + filename
) as GeoJSON.FeatureCollection<LineString>,
};
});

test("turf-line-intersect", (t) => {
for (const { filename, name, geojson } of fixtures) {
const [line1, line2] = geojson.features;
const results = truncate(lineIntersect(line1, line2));
const results: GeoJSON.FeatureCollection<LineString | Point> = truncate(
lineIntersect(line1, line2)
);
results.features.push(line1);
results.features.push(line2);

Expand Down Expand Up @@ -132,3 +132,52 @@ test("turf-line-intersect - polygon support #586", (t) => {
t.equal(results.features.length, 1, "should return single point");
t.end();
});

/**
* ensures that the self intersection param behaves as expected -
* since it cannot be verified in the fixture format.
*/
test("turf-line-intersect - self intersection behavior", (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

Are there advantages to running the same features through both "in code" and as in/out fixture tests?

Would you mind adding some simple tests for MultiLineString, Polygon, and MultiPolygon as well e.g.

Screenshot 2024-11-13 at 11 04 55 PM

To be honest I'm not sure what the expected behaviour of those are. Should ignoreSelfIntersections apply to the entire MultiLineString, or only take effect within each individual LineString? If you're got the time let's take this opportunity to embed whatever the behaviour is into some baseline tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably no reason to have both - is there a preference for one or the other? Its not clear to me why some tests are based on fixtures - is this preferred for simple T/F style testing?

Copy link
Contributor Author

@pm0u pm0u Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Remembering my logic here - The reason was that the tests below specifically test the behavior of the ignoreSelfIntersections param. Because the in/out fixture tests all use the same setup it can't be validated in that setting.

If we think this approach makes sense I can add a note as to why this is set up this way.

Copy link
Member

Choose a reason for hiding this comment

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

Fixtures were probaby chosen to avoid 1000s of lines of geojson in code. If that approach doesn't fit nicely though, by all means add normal test cases.

const line1: Feature<LineString> = {
type: "Feature",
properties: {},
geometry: {
type: "LineString",
coordinates: [
[0, 0],
[0, 2],
[2, 1],
[-1, 1],
],
},
};
const line2: Feature<LineString> = {
type: "Feature",
properties: {},
geometry: {
type: "LineString",
coordinates: [
[3, 3],
[4, 4],
[5, 5],
],
},
};

const ignored = lineIntersect(line1, line2);
t.equal(
ignored.features.length,
0,
"self intersections should be ignored by default"
);

const included = lineIntersect(line1, line2, {
ignoreSelfIntersections: false,
});
t.equal(
included.features.length,
1,
"self intersections should be included when ignoreSelfIntersections set to false"
);
t.end();
});
30 changes: 30 additions & 0 deletions packages/turf-line-intersect/test/in/self-intersecting-line.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[0, 0],
[0, 2],
[2, 1],
[-1, 1]
]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[3, 3],
[4, 4],
[5, 5]
]
}
}
]
}
36 changes: 36 additions & 0 deletions packages/turf-line-intersect/test/in/self-intersecting-multiline.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "MultiLineString",
"coordinates": [
[
[0, 0],
[1, 2],
[0, 4]
],
[
[1, 0],
[0, 2],
[1, 4]
]
]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[3, 3],
[4, 4],
[5, 5]
]
}
}
]
}
52 changes: 52 additions & 0 deletions packages/turf-line-intersect/test/in/self-intersecting-multipoly.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {
"fill": "#ed333b",
"fill-opacity": 0.5
},
"geometry": {
"coordinates": [
[
[
[2.8208987964653716, 5.206515064293427],
[2.8208987964653716, -4.530167455797994],
[6.383916833323553, -4.530167455797994],
[6.383916833323553, 5.206515064293427],
[2.8208987964653716, 5.206515064293427]
]
],
[
[
[-0.33343506038161763, 3.67541603033537],
[-0.33343506038161763, 2.1793088174136273],
[8.775841349855597, 2.1793088174136273],
[8.775841349855597, 3.67541603033537],
[-0.33343506038161763, 3.67541603033537]
]
]
],
"type": "MultiPolygon"
},
"id": 0
},
{
"type": "Feature",
"properties": {},
"geometry": {
"coordinates": [
[
[-7.951422534380413, 5.231149187754099],
[-7.951422534380413, -4.552832252613101],
[-5.701165954657256, -4.552832252613101],
[-5.701165954657256, 5.231149187754099],
[-7.951422534380413, 5.231149187754099]
]
],
"type": "Polygon"
}
}
]
}
44 changes: 44 additions & 0 deletions packages/turf-line-intersect/test/in/self-intersecting-poly.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"coordinates": [
[
[-0.8680870854922205, 3.0368343508535673],
[-1.321147431010786, 5.333329344976903],
[1.0099462525165848, 5.611566335081363],
[0.47439299724965167, -1.9282285399474688],
[3.2162894164922875, -1.8166964568157766],
[2.707220996335252, 7.192842763774053],
[-3.8645185945755713, 6.519248925309753],
[-3.8571038955967083, 0.3483632770754781],
[-0.5093053180867742, 0.39824548653076874],
[4.4119165583018685, 0.21046777461485533],
[3.9804563423212755, 3.7937082620644844],
[-0.8680870854922205, 3.0368343508535673]
]
],
"type": "Polygon"
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"coordinates": [
[
[-7.4564971746842446, 6.278543929857676],
[-13.631981273628753, 6.323585458965155],
[-12.43687692686214, -2.589094309688946],
[-6.376915352576532, -3.0767722870515115],
[-7.4564971746842446, 6.278543929857676]
]
],
"type": "Polygon"
}
}
]
}
30 changes: 30 additions & 0 deletions packages/turf-line-intersect/test/out/self-intersecting-line.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[0, 0],
[0, 2],
[2, 1],
[-1, 1]
]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[3, 3],
[4, 4],
[5, 5]
]
}
}
]
}
36 changes: 36 additions & 0 deletions packages/turf-line-intersect/test/out/self-intersecting-multiline.geojson
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "MultiLineString",
"coordinates": [
[
[0, 0],
[1, 2],
[0, 4]
],
[
[1, 0],
[0, 2],
[1, 4]
]
]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [
[3, 3],
[4, 4],
[5, 5]
]
}
}
]
}
Loading
Loading