openlayers / ol2

OpenLayers v2 - deprecated!
Other
1.48k stars 771 forks source link

adding a wms layer atop osm should be easier #350

Open elemoine opened 12 years ago

elemoine commented 12 years ago

Today @pgiraud told me that the following does not work, but should work:

            map = new OpenLayers.Map('map');
            osm = new OpenLayers.Layer.OSM( "Simple OSM Map");
            wms = new OpenLayers.Layer.WMS( "OpenLayers WMS",
                    "http://vmap0.tiles.osgeo.org/wms/vmap0", {layers: 'basic'},{isBaseLayer: false});
            map.addLayers([osm, wms]);
            map.setCenter(...)

and I agree.

It works if the projection or maxExtent is set in the map. It also works if you set displayOutsideMaxExtent to true in the WMS layer.

But it should work without this of course.

elemoine commented 12 years ago

Here's what happens:

Tile.shouldDraw always returns false for the WMS layer, because we ask intersectsBounds to compare EPSG:900913 bounds (this.bounds) to EPSG:4326 bounds (layer.maxExtent)!

layer.maxExtent is -180,-90,180,90 because the layer got its projection from the map, which defaults to EPSG:4326.

I don't know how to fix that, but the current situation sucks!

ahocevar commented 12 years ago

Before we introduced the defaults tied to the projection, I would have agreed. But now I think it is ok if people have to do

        map = new OpenLayers.Map('map', {projection: "EPSG:900913"});
        osm = new OpenLayers.Layer.OSM( "Simple OSM Map");
        wms = new OpenLayers.Layer.WMS( "OpenLayers WMS",
                "http://vmap0.tiles.osgeo.org/wms/vmap0", {layers: 'basic'},{isBaseLayer: false});
        map.addLayers([osm, wms]);
        map.setCenter(...)
elemoine commented 12 years ago

@ahocevar I see what you mean. The ambiguity is that we tell users that the base layer determines the projection, max extent, and resolutions, while it's not the case here for maxExtent (or this particular use of maxExtent).

Another way to see the problem is that the static values for the layer are taken from the map, while the dynamic values are taken from the base layer. I'm not sure I'm clear...

probins commented 12 years ago

"we tell users that the base layer determines the projection" AFAICS only on Map.baseLayer, and AFAICS this only applies to vectors, which can be transformed, whereas WMS tiles cannot. Layer.projection says "to override the default projection string this layer", which ISTM should say "to override the default projection specified on the map". Map.projection says "Set in the map options to override the default projection string this map" which IMO should say "Set in the map options to specify the default projection for layers added to this map". WMS layers do not have a default projection (arguably, it should be 4326), so surely in this case projection should be set on the wms layer, as you want the tiles to be in the same projection as the OSM baselayer. This is a different case from vector overlays, which can be reprojected to match the baselayer.

elemoine commented 12 years ago

GetMap requests sent for WMS overlays include the baselayer's projection (for the SRS param), whatever the value of layer.projection. See Layer.Grid.getFullRequestString. This means that you don't need to specify the projection of WMS overlays. But actually you need to, for maxExtent to be correct; this is what this issue is complaining about.

For Layer.projection I'd say: "Specifies the projection of the layer. Can be set in the layer options. If not specified in the layer options it is set to the projection of the map, when the layer is added to map.".

I agree with you for Map.projection.

elemoine commented 12 years ago

To go back to this issue's use-case: an OSM base layer and a WMS overlay. The WMS overlay doesn't need to be configured with an EPSG:900913 projection object, as the base layer's projection will be used for the SRS param in GetMap requests. But it needs to have an EPSG:900913 max extent, or Tile.redraw will compare apples to oranges. That does not make too much sense to me.

probins commented 12 years ago

See Layer.Grid.getFullRequestString

Layer.WMS.getFullRequestString So it ignores both map.projection and layer.projection. Fine, fine :-) That too should be documented more prominently.

It's not just maxExtent which is wrong; the resolutions are all wrong too. I don't know whether that actually affects anything, but it's certainly confusing. If you really want to enable this (and personally I can't see the problem in defining projection on either map or layer), then effectively you are saying that the default projection/extent/resolutions for WMS overlay is not that of the map, but of the baseLayer. If you give WMS its own setMap to set this, then it should work, no?

elemoine commented 12 years ago

Yes, setMap always calculates resolutions, even for overlays. And yes, an overlay does not need resolutions. But "overlay" is a state, which can change. With the allOverlays map option any layer can become a base layer.

ahocevar commented 12 years ago

For what it's worth, I tried to address this issue in #177 a while ago.

elemoine commented 12 years ago

@ahocevar What do you think about Tile.shouldDraw not doing the intersectsBounds test and returning true if the layer and map projections don't match (!layer.projection.equals(map.getProjectionObject()))?

elemoine commented 12 years ago

Here's how I'd rewrite shouldDraw (untested):

if (this.displayOutsideMaxExtent) {
    return true;
}
var maxExtent = this.layer.maxExtent;
if (!maxExtent) {
    return true;
}
var map = this.layer.map;
if (!this.projection || !this.projection.equals(map.getProjectionObject()) {
    return true;
}
var worldBounds = map.baseLayer.wrapDateLine && map.getMaxExtent();
if (this.bounds.intersectsBounds(maxExtent, {inclusive: false, worldBounds: worldBounds})) {
    return true;
}
return false;

See my shoulddraw branch.