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

Vector files with z not being handled correctly #913

Merged
merged 1 commit into from
Aug 26, 2013
Merged

Vector files with z not being handled correctly #913

merged 1 commit into from
Aug 26, 2013

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Aug 18, 2013

For example, this geojson linestring http://maps.peterrobins.co.uk/cgi-bin/fs/details/2649
view2d should surely be ignoring z but instead seems to be trying to make a 3d drawing of it - very pretty, but not quite correct :-)

@tschaub
Copy link
Member

tschaub commented Aug 16, 2013

When I try to load that data, I get an assertion error (instead of improper rendering). The shared structures that are currently used for storing geometry coordinates enforce coordinates with all the same dimension, and the vector layer currently restricts this to 2. This is inflexible and not the way that it should be. We could support a dimension option on the layer to make this work right now, but this is not the place where this should be specified.

Alternatives

1. Support a dimension option on the parser.

var vector = new ol.layer.Vector({
  source: new ol.source.Vector({
    parser: new ol.parser.GeoJSON({dimension: 3}), // default is 2
    url: 'data/3d.json'
  })
});

2. Support a dimension option on the source.

var vector = new ol.layer.Vector({
  source: new ol.source.Vector({
    parser: new ol.parser.GeoJSON(),
    dimension: 3,  // default is 2
    url: 'data/3d.json'
  })
});

3. No new options, just make it work.

We could make the current shared structures 3d by default and stop enforcing that incoming coordinates match. The current shared structures are going to go anyway, so if we can't commit to an API to support alternative dimensions, this is the way to go.


I'm partial to Alt 1, but I'm interested to hear from others. The benefit of 1 over 2 is that different formats could have different defaults, and the source would require less user configuration for common cases.

@ahocevar
Copy link
Member

My preference would be Alt. 3 over Alt. 1, and I do not like Alt. 2.

@probins
Copy link
Contributor Author

probins commented Aug 16, 2013

I tried it both in my own program and adapting the vector-layer example to use that file, in both cases using the latest hosted master build. I only tried with GeoJSON, but the same principle should apply to all formats.

I think 3 is the only possibility. If you're using other people's data, for example, a kml file they've created, you have no way of knowing whether there's a 3rd dimension or not.

@probins
Copy link
Contributor Author

probins commented Aug 16, 2013

... and I've seen quite a few kml files which have a 3rd coordinate, but it is always 0 - so it's present, but wrong

@probins
Copy link
Contributor Author

probins commented Aug 17, 2013

@tschaub I'm curious as to why you get an assertion error. I've pushed a small example to http://probins.github.io/ol3docs/osmgir.html - same data but this time with kml rather than geojson. This produces no error, and the same rendering - pretty, but not correct.

I also have feature collections where features have geometries with a different no of dimensions, one feature may have 2, another 3. In my case, this happens because I'm aggregating data from different sources. Unusual perhaps, but there's nothing in the specs that says that all features have to have the same no of dimensions.

@tschaub
Copy link
Member

tschaub commented Aug 17, 2013

The assertions are stripped by the compiler.

@tschaub
Copy link
Member

tschaub commented Aug 18, 2013

Ok, this is the simplest change to just make it work. This doesn't change the default from 2 to 3, and it doesn't allow an easy way to configure the dimension. If we do want to add a dimension option to feature parsers, that can be added later. There is no real reason to add it yet, but when we provide a way to serialize data again from a source (e.g. saving modified features), we'll want to maintain the original dimension of the coordinates.

@tschaub
Copy link
Member

tschaub commented Aug 25, 2013

My preference would be Alt. 3 over Alt. 1, and I do not like Alt. 2.

@ahocevar I ended up going with Alt 3 but left room for Alt 1.

@ahocevar
Copy link
Member

This looks good for now. Please merge.

tschaub added a commit that referenced this pull request Aug 26, 2013
Allow for vector data with unknown or inconsistent dimension.
@tschaub tschaub merged commit eaf4477 into openlayers:master Aug 26, 2013
@tschaub tschaub deleted the dimension branch August 26, 2013 13:00
afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
Simplifying and unhacking the ModifyFeature control. r=@bartvde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants