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.

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

⚠️ Change coordIndex to last Coordinates Array reference #1168

Open
DenisCarriere opened this issue Dec 10, 2017 · 7 comments
Open

⚠️ Change coordIndex to last Coordinates Array reference #1168

DenisCarriere opened this issue Dec 10, 2017 · 7 comments

Comments

@DenisCarriere
Copy link
Member

DenisCarriere commented Dec 10, 2017

Change coordIndex to last Coordinates Array reference

⚠️ Breaking change for next major release v6.0

Ref: #1099 & #1092 (comment)

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]]
]);
@rowanwins
Copy link
Member

rowanwins commented Dec 10, 2017

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.

@DenisCarriere
Copy link
Member Author

DenisCarriere commented Dec 10, 2017

That is exactly what I'm proposing... via #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
Copy link
Member Author

DenisCarriere commented Dec 10, 2017

@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
});

@rowanwins
Copy link
Member

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

@DenisCarriere
Copy link
Member Author

DenisCarriere commented Dec 10, 2017

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]
})

@rowanwins
Copy link
Member

ah I see now, that's clearer thanks!

@DenisCarriere DenisCarriere changed the title Change coordIndex to last Coordinates Array reference ⚠️ Change coordIndex to last Coordinates Array reference Dec 11, 2017
@tmcw
Copy link
Collaborator

tmcw commented Jun 25, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants