Add FeatureCollection mutate tests (not failing) #897
Conversation
| coordEach(geojson, function (coord) { | ||
| coordEach(geojson, function (coord, coordIndex, featureIndex, featureSubIndex) { | ||
| // Ignore scaling on first coordinate of Polygons (Issue #895) | ||
| if (isPolygon && mutate === true && featureSubIndex === 0) return; |
There was a problem hiding this comment.
@stebogit Might want to add this type of behavior in a few of the transform modules.
There was a problem hiding this comment.
@DenisCarriere it looks ok, and clearly it does work. However I can not understand why we don't want to scale all the points of the ring(s) of a polygon, whether we clone it or not.
I really can't wrap my head around it... 🤔
There was a problem hiding this comment.
@DenisCarriere it clearly works, but the idea we need to skip a point if we clone geojson (and only for polygons?) seems so weird to me... 🤔
| origin = defineOrigin(geojson, origin); | ||
|
|
||
| // Shortcut no-scaling | ||
| if (factor === 1 || isPoint) return geojson; | ||
|
|
||
| // Scale each coordinate | ||
| coordEach(geojson, function (coord) { | ||
| coordEach(geojson, function (coord, coordIndex, featureIndex, featureSubIndex) { |
There was a problem hiding this comment.
I noticed coordIndex and featureSubIndex are always equals, throughout the entire test suite, and featureIndex is always 0.
Inside coordEach this never logs:
if (coordIndex !== featureSubIndex || featureIndex > 0) console.log('Woah!');
Shouldn't the indices change at least for multi features?
EDIT: Got it, geojson is a polygon with one ring, i.e. featureIndex = 0 and featureSubIndex = coordIndex
| coordEach(geojson, function (coord) { | ||
| coordEach(geojson, function (coord, coordIndex, featureIndex, featureSubIndex) { | ||
| // Ignore scaling on first coordinate of Polygons (Issue #895) | ||
| if (isPolygon && mutate === true && featureSubIndex === 0) return; |
There was a problem hiding this comment.
@DenisCarriere it looks ok, and clearly it does work. However I can not understand why we don't want to scale all the points of the ring(s) of a polygon, whether we clone it or not.
I really can't wrap my head around it... 🤔
| @@ -59,18 +61,24 @@ module.exports = function (geojson, factor, origin, mutate) { | |||
| * @param {Feature|Geometry} geojson GeoJSON Feature/Geometry | |||
| * @param {number} factor of scaling, positive or negative values greater than 0 | |||
| * @param {string|Geometry|Feature<Point>|Array<number>} [origin="centroid"] Point from which the scaling will occur (string options: sw/se/nw/ne/center/centroid) | |||
| * @param {boolean} [mutate=false] allows GeoJSON input to be mutated | |||
There was a problem hiding this comment.
Here mutate does not actually define if the feature is going to be mutated or not, that was handled before.
I can't find a good description though 😅
cloned last vertex in turf-hex-grid; reverted changes in transform-scale; fixed JDoc types;
|
@DenisCarriere I found out the real issue: as you said
because when I modified (locally) that line and all seems to be good with the original |
Ahh... that makes sense! Was wondering where that issue came from. |
| }; | ||
|
|
||
| /** | ||
| * Scale Feature/Geometry | ||
| * | ||
| * @private | ||
| * @param {Feature|Geometry} geojson GeoJSON Feature/Geometry | ||
| * @param {GeoJSON} geojson GeoJSON Feature/Geometry |
There was a problem hiding this comment.
GeoJSON means it also handles FeatureCollection & GeometryCollection.
This doesn't? The main method handles that part.
| * @param {number} factor of scaling, positive or negative values greater than 0 | ||
| * @param {string|Geometry|Feature<Point>|Array<number>} [origin="centroid"] Point from which the scaling will occur (string options: sw/se/nw/ne/center/centroid) | ||
| * @returns {Feature|Geometry} scaled GeoJSON Feature/Geometry | ||
| * @returns {GeoJSON} scaled GeoJSON object |
There was a problem hiding this comment.
If the input is Geometry it outputs a scaled Geometry, we know the outputs/inputs can ONLY be Feature or Geometry.
Add test cases for
@turf/transform-scaleIssue
On (Multi)Polygon the first & last coordinate gets repeated when using
coordEach(not sure if this was intentional incoordEach).Before
After