Get source projection from parser #880

Merged
merged 19 commits into from Aug 5, 2013

Conversation

Projects
None yet
2 participants
@ahocevar
Owner

ahocevar commented Jul 24, 2013

Instead of assuming a data projection in VectorSource, the projection should be read from the parser.

@ahocevar

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 22, 2013

Owner

@bartvde and I are currently working on this in the https://github.com/openlayers/ol3/tree/parser-projection branch.

Owner

ahocevar commented Jul 22, 2013

@bartvde and I are currently working on this in the https://github.com/openlayers/ol3/tree/parser-projection branch.

src/objectliterals.jsdoc
@@ -348,7 +348,9 @@
/**
* @typedef {Object} ol.parser.GMLOptions
* @property {string|undefined} axisOrientation The axis orientation as
- * specified in Proj4. The default is 'enu'.
+ * specified in Proj4. If this is not provided, 'enu' will be used for GML2,

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

I wonder where this logic came from? I don't think there is a difference between GML2 and GML3 wrt axis order (the difference is at the WFS level AFAIK, i.e WFS 1.0 always uses enu)

@bartvde

bartvde Jul 29, 2013

Owner

I wonder where this logic came from? I don't think there is a difference between GML2 and GML3 wrt axis order (the difference is at the WFS level AFAIK, i.e WFS 1.0 always uses enu)

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

This logic came from playing with the wfs-states example data. But I didn't check if the GeoServer output conforms to the spec.

@ahocevar

ahocevar Jul 29, 2013

Owner

This logic came from playing with the wfs-states example data. But I didn't check if the GeoServer output conforms to the spec.

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

Likely the GML2 data was requested with a WFS 1.0.0 request, and the GML3 data with a WFS 1.1.0 request?

@bartvde

bartvde Jul 29, 2013

Owner

Likely the GML2 data was requested with a WFS 1.0.0 request, and the GML3 data with a WFS 1.1.0 request?

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

And are you sure that srsName is even part of the GML2 spec? If this is influenced by the WFS version, it should probably be handled there instead of here.

@ahocevar

ahocevar Jul 29, 2013

Owner

And are you sure that srsName is even part of the GML2 spec? If this is influenced by the WFS version, it should probably be handled there instead of here.

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

It's part of the schema: http://schemas.opengis.net/gml/2.1.2/geometry.xsd but not sure if this is what you mean here.

@bartvde

bartvde Jul 29, 2013

Owner

It's part of the schema: http://schemas.opengis.net/gml/2.1.2/geometry.xsd but not sure if this is what you mean here.

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

Once we have WFS parsers, these could extend GML, and set the appropriate axis orientation. So if I understand things correctly, for now we should remove the code that determines the axis orientation from the srs, because that should be done in the WFS parser. Would you agree?

@ahocevar

ahocevar Jul 29, 2013

Owner

Once we have WFS parsers, these could extend GML, and set the appropriate axis orientation. So if I understand things correctly, for now we should remove the code that determines the axis orientation from the srs, because that should be done in the WFS parser. Would you agree?

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

See also Axis Order Policy and Recommendations: http://www.ogcnetwork.net/node/491 not sure what the way out is though

@bartvde

bartvde Jul 29, 2013

Owner

See also Axis Order Policy and Recommendations: http://www.ogcnetwork.net/node/491 not sure what the way out is though

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

Looking at the policy referenced above, it seems both the interface and/or the payload can determine the axis order. What's your interpretation?

@bartvde

bartvde Jul 29, 2013

Owner

Looking at the policy referenced above, it seems both the interface and/or the payload can determine the axis order. What's your interpretation?

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

I don't get why srsName is mandatory for geometry collections but optional for geometries in GML 2. In WFS 1.0.0, srsName cannot be specified with the request (although GeoServer handles this as vendor param). In WFS 1.1.0, srsName can be specified with the request. If they mean "WFS" with the interface and "GML" with the payload in that policy, it is still not defined whether the interface or the payload takes precedence. Assuming that the server respects the srsName specified with a WFS 1.1.0 request, the srsName of the returned GML payload should be the same, in which case case 1 of that policy would apply. For WFS 1.0.0, the policy would apply whenever srsName is specified for simple geometries, and always for geometry collections. This is clearly not the case with the topp-states WFS 1.0.0 example data.

@ahocevar

ahocevar Jul 29, 2013

Owner

I don't get why srsName is mandatory for geometry collections but optional for geometries in GML 2. In WFS 1.0.0, srsName cannot be specified with the request (although GeoServer handles this as vendor param). In WFS 1.1.0, srsName can be specified with the request. If they mean "WFS" with the interface and "GML" with the payload in that policy, it is still not defined whether the interface or the payload takes precedence. Assuming that the server respects the srsName specified with a WFS 1.1.0 request, the srsName of the returned GML payload should be the same, in which case case 1 of that policy would apply. For WFS 1.0.0, the policy would apply whenever srsName is specified for simple geometries, and always for geometry collections. This is clearly not the case with the topp-states WFS 1.0.0 example data.

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

"I don't get why srsName is mandatory for geometry collections but optional for geometries in GML 2."
From the spec: the srsName attribute is optional since a Point element may be contained in other elements that specify a reference system.
A stand-alone point will always need an srsName, but they couldn't encompass this logic in the schema.

@bartvde

bartvde Jul 29, 2013

Owner

"I don't get why srsName is mandatory for geometry collections but optional for geometries in GML 2."
From the spec: the srsName attribute is optional since a Point element may be contained in other elements that specify a reference system.
A stand-alone point will always need an srsName, but they couldn't encompass this logic in the schema.

src/ol/parser/ogc/gml.js
+ * @private
+ * @type {string|undefined}
+ */
+ this.srsName_ = goog.isNull(this.srsName) ? undefined : this.srsName;

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

I assume this is for storing the srsName read from the data, which can potentially be different from the srsName used for writing?
Maybe we should use inSrsName and outSrsName (or something similar) to make the difference here more apparent? Thoughts?

@bartvde

bartvde Jul 29, 2013

Owner

I assume this is for storing the srsName read from the data, which can potentially be different from the srsName used for writing?
Maybe we should use inSrsName and outSrsName (or something similar) to make the difference here more apparent? Thoughts?

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

Maybe I am failing to understand why we need both srsName and srsName_ so any clarification on this would be welcome

@bartvde

bartvde Jul 29, 2013

Owner

Maybe I am failing to understand why we need both srsName and srsName_ so any clarification on this would be welcome

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

srsName is the user provided one, and srsName_ is the one we get from the data in a read(). Note that srsName_ will be cleared after a read().

@ahocevar

ahocevar Jul 29, 2013

Owner

srsName is the user provided one, and srsName_ is the one we get from the data in a read(). Note that srsName_ will be cleared after a read().

src/ol/parser/ogc/gml.js
+ var srsName = child.getAttribute('srsName');
+ if (goog.isDef(srsName)) {
+ this.srsName_ = srsName.replace(ol.parser.ogc.GML.regExes.epsg,
+ 'EPSG:');

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

I wonder why we want to get rid of valuable info here? Before I was just adding extra flavours to the projection code e.g.:

  new ol.proj.EPSG4326('EPSG:4326', 'neu'),
  new ol.proj.EPSG4326('urn:ogc:def:crs:EPSG:6.6:4326', 'neu'),

but am open for discussion on this.

@bartvde

bartvde Jul 29, 2013

Owner

I wonder why we want to get rid of valuable info here? Before I was just adding extra flavours to the projection code e.g.:

  new ol.proj.EPSG4326('EPSG:4326', 'neu'),
  new ol.proj.EPSG4326('urn:ogc:def:crs:EPSG:6.6:4326', 'neu'),

but am open for discussion on this.

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

This would work too, but then my next comment below should be considered as well.

@ahocevar

ahocevar Jul 29, 2013

Owner

This would work too, but then my next comment below should be considered as well.

src/ol/parser/ogc/gml.js
+ if (goog.isDef(srsName)) {
+ this.srsName_ = srsName.replace(ol.parser.ogc.GML.regExes.epsg,
+ 'EPSG:');
+ if (!goog.isDef(this.axisOrientation_)) {

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

if I am understanding things correctly here, if someone configured a parser with an srsName (for output), this will always determine the axis orientation? I'd say the data itself would be a better indicator here than the provided srsName for writing? Although there is really a rare chance of these differing I guess.

@bartvde

bartvde Jul 29, 2013

Owner

if I am understanding things correctly here, if someone configured a parser with an srsName (for output), this will always determine the axis orientation? I'd say the data itself would be a better indicator here than the provided srsName for writing? Although there is really a rare chance of these differing I guess.

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

This discussion makes me feel like srsName should not be an instance property, but an option to read() and write(). Would you agree with that?

@ahocevar

ahocevar Jul 29, 2013

Owner

This discussion makes me feel like srsName should not be an instance property, but an option to read() and write(). Would you agree with that?

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

That's a very good point, I think this would make more sense indeed.

@bartvde

bartvde Jul 29, 2013

Owner

That's a very good point, I think this would make more sense indeed.

src/ol/parser/ogc/gml.js
+ * Constants for regExes.
+ * @enum {RegExp}
+ */
+ol.parser.ogc.GML.regExes = {

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

Wondering if we should make the regExes in xml.js also static?

@bartvde

bartvde Jul 29, 2013

Owner

Wondering if we should make the regExes in xml.js also static?

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

Probably a good idea, but unrelated to this pull request I'd say.

@ahocevar

ahocevar Jul 29, 2013

Owner

Probably a good idea, but unrelated to this pull request I'd say.

src/ol/parser/ogc/gml.js
+ */
+ol.parser.ogc.GML.regExes = {
+ epsg: new RegExp('(http:\/\/www\.opengis\.net\/gml\/srs\/epsg.xml#|' +
+ 'urn:x-ogc:def:crs:EPSG:)', 'i')

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

x-ogc was replaced by ogc so we should support both here?

@bartvde

bartvde Jul 29, 2013

Owner

x-ogc was replaced by ogc so we should support both here?

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

The wfs-states v3 test data uses x-ogc, and v2 uses the url. I haven't checked what GeoServer uses now. If we go the route of creating a separate projection object for each alias we know, we need a way to determine equality of projections. For OpenLayers' own projections, this can be done with an instanceOf check, but for proj4js projections this would not work. So projections should have an equals() method like in OpenLayers 2 if we decide to take this route.

@ahocevar

ahocevar Jul 29, 2013

Owner

The wfs-states v3 test data uses x-ogc, and v2 uses the url. I haven't checked what GeoServer uses now. If we go the route of creating a separate projection object for each alias we know, we need a way to determine equality of projections. For OpenLayers' own projections, this can be done with an instanceOf check, but for proj4js projections this would not work. So projections should have an equals() method like in OpenLayers 2 if we decide to take this route.

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

both are still in use AFAIK (x-ogc and ogc), see also: http://www.ogcnetwork.net/node/1287

@bartvde

bartvde Jul 29, 2013

Owner

both are still in use AFAIK (x-ogc and ogc), see also: http://www.ogcnetwork.net/node/1287

src/ol/parser/ogc/gml.js
+ */
+ol.parser.ogc.GML.prototype.getAxisOrientation = function() {
+ return goog.isDef(this.axisOrientation_) ?
+ this.axisOrientation_ : this.axisOrientation;

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

again, the difference between the private axisOrientation_ and the public one is a bit confusing for me here
Do we really need to store it temporarily, or can we just deduct it from srsName_ ?

@bartvde

bartvde Jul 29, 2013

Owner

again, the difference between the private axisOrientation_ and the public one is a bit confusing for me here
Do we really need to store it temporarily, or can we just deduct it from srsName_ ?

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

Again, this would all be easier to understand if srsName and axisOrientation were options to read() and write() instead of instance properties. That way, we'd use what we get from the data only if srsName and axisOrientation are not configured.

@ahocevar

ahocevar Jul 29, 2013

Owner

Again, this would all be easier to understand if srsName and axisOrientation were options to read() and write() instead of instance properties. That way, we'd use what we get from the data only if srsName and axisOrientation are not configured.

@bartvde

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Jul 29, 2013

Owner

what change was needed to the test data? i.e. test/spec/ol/parser/ogc/xml/gml_v3/topp-states-gml.xml

Owner

bartvde commented Jul 29, 2013

what change was needed to the test data? i.e. test/spec/ol/parser/ogc/xml/gml_v3/topp-states-gml.xml

@ahocevar

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Jul 29, 2013

Owner

The winding order of the coordinates was wrong way around for a round-trip with the same coordinates string.

Owner

ahocevar commented Jul 29, 2013

The winding order of the coordinates was wrong way around for a round-trip with the same coordinates string.

ahocevar and others added some commits Jul 22, 2013

getProjection method on parsers
With this, vector sources/layers do not need to make
assumptions on the data projection.
Using single argument for callback and string for projection
This makes the asynchronous and synchronous versions of
readFeatures work with the same data structures, and leaves
projection handling outside the parsers.
Get projection and axis order from GML data
To make tests pass, the winding order of the coordinates in the
test data had to be reversed to conform to common gml practice.
Fixing typo
Thanks @bartvde for spotting this.
Renaming readFeaturesWithMetadata* back to readFeatures*
Since this is not exported and we do not have a counterpart that
does not return the metadata, it should be fine to use a shorter
name.
@ahocevar

This comment has been minimized.

Show comment Hide comment
@ahocevar

ahocevar Aug 4, 2013

Owner

@bartvde I'm happy with this now. Are you happy too?

Owner

ahocevar commented Aug 4, 2013

@bartvde I'm happy with this now. Are you happy too?

@bartvde

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Aug 5, 2013

Owner

nice, didn't know this existed ;-)

nice, didn't know this existed ;-)

@bartvde

This comment has been minimized.

Show comment Hide comment
@bartvde

bartvde Aug 5, 2013

Owner

LGTM @ahocevar please merge

Owner

bartvde commented Aug 5, 2013

LGTM @ahocevar please merge

ahocevar added a commit that referenced this pull request Aug 5, 2013

@ahocevar ahocevar merged commit 759e6ff into master Aug 5, 2013

1 check passed

default The Travis CI build passed
Details

@ahocevar ahocevar deleted the parser-projection branch Aug 5, 2013

@probins probins referenced this pull request Aug 25, 2013

Merged

Non-4326 GeoJSON #921

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