Fix geojson write GeometryCollection

[probins]

I tested my changes mentioned in #3953 on a GeometryCollection, and found they didn't work. However, when I tried doing the same with the current master, I found that didn't work either. examples/geojson has a GeometryCollection so if you do something like

format=new ol.format.GeoJSON();
format.writeFeature(vectorSource.getFeatures()[6], {featureProjection:'EPSG:3857'});

you can see that the coordinates are all screwy. The reason for this is that writeGeometry_ is getting run for the Collection as a whole and then for each individual geometry. writeGeometry_ runs ol.format.Feature#transformWithOptions(), so the coordinates for each geometry are transformed twice. This PR calls write for the individual geoms without the projection options, so the 2nd transform doesn't occur.

From the fact that no-one's noticed this before, I conclude that GeometryCollections aren't used very much. :-) I think tests should be added for this case, but would prefer to include that in my fixes for #3953 so the rounding can be tested as well.

[ahocevar]

Probably safer to do something like

var options = ol.object.assign({}, opt_options);
delete options.featureProjection;
return ol.format.GeoJSON.writeGeometry_(geometry, options);

[probins]

shorter too :-) ok, will change this

[ahocevar]

I think tests should be added for this case, but would prefer to include that in my fixes for #3953 so the rounding can be tested as well.

I'd feel better if you added a test for this pull request too. You can use something like exepct(1.1).to.roughlyEqual(1.10001, 0.0001);.

[marcjansen]

What exactly would that test?

[tschaub]

What exactly would that test?

I think @ahocevar is just saying that you can use the roughlyEqual() matcher in testing (e.g. expect(point.getCoordinates()[0]).to.roughlyEqual(10, 1e-4)).

Thanks for catching this and providing a fix @probins. I agree we should have tests before merging.

[tschaub]

Would it make sense to delete options.dataProjection as well?

[probins]

that's not necessary. If featureProjection isn't set, no transform is performed.

[probins]

I think @ahocevar is just saying that you can use the roughlyEqual() matcher in testing (e.g. expect(point.getCoordinates()[0]).to.roughlyEqual(10, 1e-4)).

where is roughlyEqual defined? I couldn't find any docs

[marcjansen]

https://github.com/openlayers/ol3/blob/master/test/test-extensions.js

[probins]

ok. Basic test added, based on the Point test above.

[ahocevar]

You can also use roughlyEquals here.

[probins]

yeah, I was wondering why the Point test wasn't using roughlyEquals for both coordinates

[ahocevar]

roughlyEqual is just for numbers, so you have to test x and y of a coordinate separately or in a forEach function.

[probins]

the 'transforms and encodes a point' test is using roughlyEqual for the x coord and this Math.abs formula for the y. I'm wondering why.

[probins]

test changed as suggested, so this tests a Point and the first coordinate of a LineString within a GeometryCollection, which should be sufficient.

I'll submit another PR to change the Point test to use roughlyEqual for both coords - might as well make these things consistent.

[ahocevar]

I'll submit another PR to change the Point test to use roughlyEqual for both coords - might as well make these things consistent.

Thanks. I think a roughlyEqualCoordinate method could also make sense.