turf icon indicating copy to clipboard operation
turf copied to clipboard

⚠️ Change coordIndex to last Coordinates Array reference

Open DenisCarriere opened this issue 7 years ago • 7 comments

Change coordIndex to last Coordinates Array reference

⚠️ Breaking change for next major release v6.0

Ref: https://github.com/Turfjs/turf/issues/1099 & https://github.com/Turfjs/turf/issues/1092#issuecomment-350579714

TurfJS v5.x coordEach

meta.coordEach(multiPolyWithHole, (coord, coordIndex, featureIndex, multiFeatureIndex, geometryIndex) => {
    //=featureIndex
    //=multiFeatureIndex
    //=geometryIndex
    //=coordIndex
});

TurfJS v6.x coordEach

meta.coordEach(multiPolyWithHole, (coord, indexes => {
    //=indexes.featureIndex
    //=indexes.multiFeatureIndex
    //=indexes.geometryIndex
    //=indexes.coordIndex
});

Index Breakdowns

MultiPolygon with Holes

// MultiPolygon with hole
// ======================
// FeatureIndex => 0
const multiPolyWithHole = multiPolygon([
    // Polygon 1
    // ---------
    // MultiFeature Index => 0
    [
        // Outer Ring
        // ----------
        // Geometry Index => 0
        // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
        [[102.0, 2.0], [103.0, 2.0], [103.0, 3.0], [102.0, 3.0], [102.0, 2.0]]
    ],
    // Polygon 2 with Hole
    // -------------------
    // MultiFeature Index => 1
    [
        // Outer Ring
        // ----------
        // Geometry Index => 0
        // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
        [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]],

        // Inner Ring
        // ----------
        // Geometry Index => 1
        // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
        [[100.2, 0.2], [100.8, 0.2], [100.8, 0.8], [100.2, 0.8], [100.2, 0.2]]
    ]
]);

Polygon with hole

// Polygon with Hole
// =================
// Feature Index => 0
const polyWithHole = polygon([
    // Outer Ring
    // ----------
    // Geometry Index => 0
    // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
    [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]],

    // Inner Ring
    // ----------
    // Geometry Index => 1
    // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
    [[100.2, 0.2], [100.8, 0.2], [100.8, 0.8], [100.2, 0.8], [100.2, 0.2]]
]);

FeatureCollection of LineStrings

// FeatureCollection of LineStrings
const line = lineStrings([
    // LineString 1
    // Feature Index => 0
    // Geometry Index => 0
    // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
    [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]],

    // LineString 2
    // Feature Index => 1
    // Geometry Index => 0
    // Coord Index => [0, 1, 2, 3, 4] (Major Release Change v6.x)
    [[100.2, 0.2], [100.8, 0.2], [100.8, 0.8], [100.2, 0.8], [100.2, 0.2]]
]);

DenisCarriere avatar Dec 10 '17 21:12 DenisCarriere

This is a bit confusing as to what you're proposing here sorry @DenisCarriere .

Anotehr thought is I wonder if rather than sending them through as arguments which is becoming unweildly, whether we construct an object?

Perhaps something like

meta.coordEach(multiPolyWithHole, (coord, indexes) => {
   indexes.featureIndex,
   indexes.geometryIndex
   etc
});

That might make extending these methods a bit simpler as well in terms of reducing breaking changes etc.

rowanwins avatar Dec 10 '17 22:12 rowanwins

That is exactly what I'm proposing... via https://github.com/Turfjs/turf/issues/1099

I just didn't update the JS example here (for now) because this issue only talks about the coordIndex and not about returning an Index Object.

DenisCarriere avatar Dec 10 '17 22:12 DenisCarriere

@rowanwins I've updated the comment with:

TurfJS v5.x coordEach

meta.coordEach(multiPolyWithHole, (coord, coordIndex, featureIndex, multiFeatureIndex, geometryIndex) => {
    //=featureIndex
    //=multiFeatureIndex
    //=geometryIndex
    //=coordIndex
});

TurfJS v6.x coordEach

meta.coordEach(multiPolyWithHole, (coord, indexes => {
    //=indexes.featureIndex
    //=indexes.multiFeatureIndex
    //=indexes.geometryIndex
    //=indexes.coordIndex
});

DenisCarriere avatar Dec 10 '17 22:12 DenisCarriere

Ah - glad we're on the same track (even if I didn't realise it!)

rowanwins avatar Dec 10 '17 22:12 rowanwins

This is a bit confusing as to what you're proposing here

Currently coordEach only counts each coordinates by doing ++ for each coordinates it iterates over, instead it should be resetting itself at each feature/geometry to be able to retrieve the last possible Array of the coordinates.

Example for a FeatureCollection of LineString

const lines = lineStrings([
  [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]],
  [[100.2, 0.2], [100.8, 0.2], [100.8, 0.8], [100.2, 0.8], [100.2, 0.2]]
]);

meta.coordEach(lines, indexes => {
  //=indexes.coordIndex => [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
})

Using this proposal

meta.coordEach(lines, indexes => {
  //=indexes.coordIndex => [0, 1, 2, 3, 4, 0, 1, 2, 3, 4]
})

DenisCarriere avatar Dec 10 '17 22:12 DenisCarriere

ah I see now, that's clearer thanks!

rowanwins avatar Dec 10 '17 22:12 rowanwins

Like #1099, I don't think this is a good idea: it would degrade turf-meta performance just in order to make it 'easier' for core contributors. turf-meta is low-level code: it needs to be fast, not friendly.

tmcw avatar Jun 25 '18 00:06 tmcw