Use marchingsquares lib for turf-isoband and turf-isoline, fixes "first break" bug

[smallsaucepan]

Refactoring and fixes to turf-isoband and turf-isoline. Use third party marchingsquares module instead of hosting a copy of it internally. Resolves #1797 (sort of - it was already fixed, but was still open and we've added a regression test for it here) and also chose to resolve #2129 while we were in there.

Please fill in this template.

• 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.

Submitting a new TurfJS Module.

n/ahttps://docs.github.com/articles/closing-issues-using-keywords

[stebogit]

👍🏻

[stebogit]

just a guess here, but if the problem is related to an unexpected mutation using coordEach, have you tried to replace that function with code that does not mutate anything, instead of cloning the entire isoline object? This for sake of performance.
At the very least you could instead clone just each coord array during the loop instead of the entire (potentially very large) isoline object.

[stebogit]

on a second thought, maybe the resize function should just not mutate the values in the first place, but return a new array:

const resize = (point: number[]) => [
  point[0] * scaleX + x0,
  point[1] * scaleY + y0,
];

[smallsaucepan]

I think the resize function has to mutate in place - coordEach discards the return value of the callback.

It's more a quirk of the marchingsquares library which seems to re-use the first coordinate array as the closing point as well. So the callback passed to coordEach is being unexpectedly run twice on a single set of numbers i.e.

What we get back from marchingsquares:

[0] -> [1, 1]
[1] -> [2, 2]
[2] -> [3, 3]
[3] -> [1, 1] // <-- not another array that also contains [1, 1], the exact same array referred to by [0]

What we end up with after coordEach (if callback appends an "x" for the sake of visualisation):

[0] -> [1xx, 1xx]
[1] -> [2x, 2x]
[2] -> [3x, 3x]
[3] -> [1xx, 1xx] // in our case making one point way off target

I'll tweak the workaround to maybe only clobber the last coord in each linestring with a cloned value, then run the resize function over that.

[smallsaucepan]

Looks like this might have been a red herring. Pulled out the cloning operation to rework it and the tests still passed. Suspect adding linearRing: false to marchingsquares.isoContours() may have fixed it already. CI tests have passed so will merge. Thanks for questioning it @stebogit 👍

[stebogit]

what's the purpose of using these internal modules? That is, why not using the npm versions?

[smallsaucepan]

For matrix-to-grid the file committed to turf differs from the one in npm. Looks like @DenisCarriere included a tweak when it was originally added? See https://github.com/Turfjs/turf/blame/2faafab9162bcadd35a79ec329f578881038b5ec/packages/turf-isolines/lib/matrix-to-grid.js#L45

More than happy to use the NPM version. Do you want to roll in that change and re-release? I'm not sure what the original justification was though.

For now will go with the internal modules and we can refactor this depending on what you'd like to do.