move Layer.OSM in its own file

[elemoine]

Related to issue #138.

[bbinet]

Looks good to me.

[marcjansen]

Do we need some documentation here and for the other properties? Except for this I think the change is perfectly fine for master.

[probins]

Much though I dislike excessive requires, isn't this a case where SphericalMercator should be required? AFAIA, all the OSM renderings use it, so it seems unnecessary to ask people to specifically add it to their builds.

[tschaub]

I agree there should be a @require for SphericalMercator. If you want XYZ sphericalMercator: false, you can use XYZ.

[elemoine]

Good catch. I'll add it.

[probins]

another point is that they plan to change the licence, which according to http://www.osmfoundation.org/wiki/License/We_Are_Changing_The_License should be complete by April 1. So the attribution should change at that time, presumably before the next OL release.

[elemoine]

Thanks @probins. I think we should track this in a separate issue (or pull request). I'll open an issue if you can't get to it before I do.

[probins]

see #149

[elemoine]

@tschaub, @probins, going back to @requires for SphericalMercator here: I have realized that Layer.XYZ and Layer.OSM don't actually use the Layer.SphericalMercator mixin, even if sphericalMercator is true. So do we really want OSM.js to require SphericalMercator.js? SphericalMercator.js is still useful for users who want to transform lonlat coords to spherical mercator coords, but I think those should explicitly add SphericalMercator.js to their build profiles, don't you think? I also note that XYZ.js do not have a @requires for SphericalMercator today, so we're not breaking people's build profiles by not including it in OSM.js.

[elemoine]

@marcjansen a9d919a adds documentations for Layer.OSM.

[probins]

@elemoine the function at the end of SphericalMercator uses it, surely, by adding the transforms to Projection, so without the SphericalMercator file you won't be able to add/transform vectors in another projection (unless you have Proj4JS present, of course).

I have to admit I've always found this rather confusing, as SphericalMercator is not really a Layer, it's just an object defining functions. Perhaps it would be better to rename it?

As for the requires, SphericalMercator is simply a convenience so you can use one of the common raster layers without having to load Proj. On the other hand, if you have Proj anyway, then you don't need it, so perhaps it would be better to make it optional for all those layers. and not required?

[probins]

as a further comment, Google is currently the only layer that actually mixes the SM functions into the layer

[elemoine]

@probins, for you to know in case you haven't noticed: Layer.Yahoo and Layer.MultiMap do use Layer.SphericalMercator as a mixin, but they're deprecated (and now moved into the deprecated.js file).

The code in Layer/OSM.js does not at all rely on code in Layer/SphericalMercator.js, so it really strikes meas odd to have Layer/OSM.js require Layer/SphericalMercator.js. My comment also applies to Layer/Bing.js. (Layer/Bing.js does include a @requires statement for SphericalMercator.js, and I'd suggest to remove it.)

I understand that without Layer.SphericalMercator people won't be able to transform lonlat coords to/from spherical mercator coords, but this is a different issue, requiring a separate fix.

And for that I think I'd suggest moving the mercator/wgs84 transformation-related code from SphericalMercator.js to Projection.js. SphericalMercator.js would just include the mixin for Layer.Google. And people doing mercator/wgs84 transformations would just need Projection.js in their builds, which is fine. The only issue is people using Projection.js for other transformations than mercator to/from wgs84, because those would have unneeded code in their builds. I think this is acceptable, and surely more consistent than what we have today.

[probins]

yes, I would agree with that. Logically, the transform logic belongs in with Projection, not as a pseudo-layer. Perhaps you could have it as a sub-class of Projection?

This also raises the question of whether Google actually needs the mixin and its own special logic. Can it not use the transforms the same as any other projected layer?

I also agree that Bing and OSM should be the same: either require SphericalMercator or not.

[elemoine]

@probins, I was thinking of moving projectForward projectInverse to OpenLayers.Projection (possibly under different names), and moving the auto-executing function calling addTransform and nullTransform at the end of Projection.js. Google.js and Google/v3.js would require both SphericalMercator.js, for the mixin it includes, and Projection.js, for the transformations.

Regarding the mixin: Layer.Google needs it because it redefines the getExtent, getLonLatFromViewPortPx and getViewPortPxFromLonLat functions (defined in Layer.EventPane).

[elemoine]

e160628 adds the @requires SphericalMercator.js statement to Layers/OSM.js. This is to be consistent with Bing.js. But as discussed above, I think this should be revisited (in a separate issue/pull request).

Please tell me if I can merge this.

[tschaub]

Please merge. See #153 for changes that pull transforms out of SphericalMercator.js.