make WMS layer WMS 1.3 compatible

[bartvde]

Currently OpenLayers.Layer.WMS is not compatible with WMS version 1.3.0.

This ticket will track changes necessary to make it WMS 1.3 compliant. Notable changes are:

* CRS instead of SRS (although for instance Cubewerx WMS also still accepts SRS in 1.3) * axis sequence in BBOX parameter

By changing the wms-untiled.html example to:

            layer = new OpenLayers.Layer.WMS(
                "OpenLayers WMS",
                "http://demo.cubewerx.com/demo/cubeserv/cubeserv.cgi?",
                {layers: 'RELIEF:Foundation', version: '1.3.0'},
                {singleTile: true}
            );

you can see what is going wrong right now with the axis sequence for EPSG:4326.

[bartvde]

patch to make Layer.WMS WMS 1.3 proof (includes tests)

[bartvde]

All relevant tests pass in IE8 and FF3.5. Thanks for any review.

[ahocevar]

Two thoughts:

• Assuming reversed axis order for epsg codes between 4000 and 5000 seems Mapserver specific to me. IMHO the layer should expose an axisOrder property to the API, so the developer can set it manually. Possible values could include "lonlat", "latlon" and "mapserver", with the latter applying the logic you have now.
• Storing the reversed axis order in a bounds object seems dangerous to me. The best thing to do is probably add a 2nd argument to the Bounds::toBBOX method to request the BBOX with reversed axis order and use that in Layer.WMS::getURL.

[bartvde]

Hi Andreas, thanks for your comments.

Wrt the first comment, the axis order depends on the CRS, so does it really make sense to set it as a property on the layer? If you have a WMS layer in an application and you reproject the map, you would need to also possibly change that property. The Mapserver "hack" (EPSG codes between 4000 and 5000 do have a reversed axis order, but there are a few more EPSG codes with a reversed axis order outside of that range) is a good option (and will work for most of the cases) until the projection object has information on the axis order (not in the near future, see recent discussions on MetaCRS list).

Wrt your second comment, that's what I had in the first place, so I'm happy to make that change.

[bartvde]

Andreas, I did a bit more research on the EPSG database and axis order. I think the EPSG database has 1751 rows with reversed axis order currently. I've used the following SQL:

select coord_ref_sys_code from epsg_coordinatereferencesystem where coord_sys_code in (select coord_sys_code from epsg_coordinateaxis where coord_axis_orientation = 'north' and coord_axis_order = 1)

Would it be an option to store this list in OpenLayers somehow? Although it would really belong in PROJ4JS, but that's kind of hard in the short term seeing the discussion on MetaCRS.

[crschmidt]

Bart,

There are many cases where OpenLayers requires users to duplicate information that could be otherwise determined by an exhaustive lookup. For example, we do not store all of the EPSG projections in OpenLayers; we expect that if users wish to use them, they configure their own projection. When we configure the maxExtent of a layer/map, we are duplication information that is available on the server. When we flip the 'xy' switch in GML parsing, we're repeating information we should know based on the CRS.

An option on the layer is a way for the user to tell OpenLayers a piece of information that complete knowledge of the projection would otherwise provide.

I am strongly against including any kind of CRS information in OpenLayers as a dictionary.

[bartvde]

Hi Chris, thanks, that's clear, what kind of option on the Layer.WMS are you suggesting? An object with the EPSG codes for which the axis order needs to be reversed? So that the application developer can provide that info for the CRS-s he knows he will deal with in his application.

[crschmidt]

Well, generally, I'd just say that something like the 'xy' option would make sense; I guess with different base layers, a WMS might be in different projections so: xy: ['EPSG:4326', 'EPSG:4001'], etc. would allow you to do a 'hasItem' on the list and see if the current projection matches that, and flip if so.

[bartvde]

new patch incorporating comments from ahocevar and crschmidt

[elemoine]

Bart, this is looking good to me, but I'll let crschmidt and ahocevar do the final review since they made the initial comments on this work. Thanks,

[crschmidt]

I've generally been under the impression that we should avoid complex types (lists, objects) in the properties of the class, setting them in initialize instead unless they are always the same across all instances. I don't know if this applies to 'yx', but I thought I would mention it; something related to those properties basically being shared across all of the instances.

I'd also like to add a comment to the 'yx' property that describes it only changes anything if the 'version' is 1.3.0.

Other than that, I don't have any feedback; it looks generally good to me, especially if someone can confirm it works against a real server. Some documentation somewhere might be nice, but doesn't need to be added with this patch, and can be put in docs.openlayers.org or something.

[ahocevar]

Bart, along the lines of what crschmidt said, I'd like to see yx as a prototype constant, something like:

OpenLayers.Layer.WMS.REVERSE_AXIS_1_3_CRS = ['EPSG:4326'];

Also, I would not make this constant an API one, because later we might want to add axis information to the projection object.

[bartvde]

revised patch (also includes example now)

[bartvde]

Thanks for the review guys, I've incorporated all your comments into my latest patch.

[elemoine]

Replying to crschmidt:

I've generally been under the impression that we should avoid complex types (lists, objects) in the properties of the class, setting them in initialize instead unless they are always the same across all instances. I don't know if this applies to 'yx', but I thought I would mention it; something related to those properties basically being shared across all of the instances.

as a constant it makes sense that is shared across all the instances. And if xy is provided as a config option with

new OpenLayers.Layer.WMS("name", "url", {/*params*/}, {xy: ["EPSG:xxxx"]});

then the instance gets its own xy property.

I'd also like to add a comment to the 'yx' property that describes it only changes anything if the 'version' is 1.3.0.

Agreed.

I also agree with Andreas that we should not make the 'xy' property an API property.

[elemoine]

Replying to bartvde:

Thanks for the review guys, I've incorporated all your comments into my latest patch.

as just discussed with Andreas over the IRC, I'm not a big fan with using a constant set in the constructor in this case. In the rest of OpenLayers we set things in the constructor in this way for constants used by the class user to set options passed to the constructor (e.g. the modification modes of the modify feature control). If the objective is to share this constant across all the instances, which makes sense to me, I think the good place is in the prototype, as you did it in your previous patch.

[bartvde]

updated patch after IRC discussion

[bartvde]

(In [9775]) Have Layer.WMS support WMS version 1.3 with the axis order sequence, r=elemoine,crschmidt,ahocevar (closes #2284)