stamen / modestmaps-js

Modest Maps javascript port
http://modestmaps.com
566 stars 152 forks source link

coordLimits #88

Closed tmcw closed 12 years ago

tmcw commented 12 years ago

1.0.0 more or less abandons the previous approach to restricting zoom levels based on layers - only map.coordLimits can be specifically set on the map object. I think this is a pretty serious step back - going to take a look at inheriting this from layers.

RandomEtc commented 12 years ago

Which bit is a step back in your opinion?

It was a pretty high priority for me to clear up the difference between interaction limits (now coordLimits on the map, limiting center coordinates) and the metadata on a provider that describes the tiles that exist (now tileLimits, limiting the urls that will be generated). Basically the provider data is "facts" that you want to set when you make the provider and initialize the tiles. The map data is "settings" which you should configure on a per-project basic, perhaps inheriting from the provider or perhaps not.

They are the same format so you can set coordLimits to be the tileLimits from a provider and it should "just work". I can see the need for a convenience method (or something) that wires this up for you but a "step back" is a bit harsh :-p

Use-cases I had in mind:

Use-cases I don't think we can cover (interaction-wise) without serious forethought:

This last case is why tileLimits are separate from mapLimits and not automatically wired up. The priority was to not generate bad URLs by default, but to keep the panning/zooming behavior squarely under the control of people using the library. map.setZoomRange(min,max) and provider.setZoomRange(min,max) are new and useful and require way less understanding of the internals than previous versions.

tmcw commented 12 years ago

The problem I'm currently looking at is that most of the time I'm automatically configuring a provider with Wax and that provider has correct limits coming from JSON. All of the code that uses that pattern is broken by this change. This basically addressed two of the three use-cases you had pointed out.

It's also unclear how to use the new tileLimits API when you don't have immediate access to the Map object, as is the case with wax.mm.connector.

Potentially, I could write something like

var map = new MM.Map('map', new wax.mm.connector(tilejson));
map.coordLimits = wax.mm.limits(tilejson);

I'll try out something of that sort and see if it addresses the problem.

RandomEtc commented 12 years ago

Or if you don't want to write a new wax limits function, I think this should work too?

var provider = new wax.mm.connector(tilejson);
var map = new MM.Map('map', provider);
map.coordLimits = provider.outerLimits();

Sorry this (breaking) change wasn't communicated more clearly. Semver or no, the switch to 1.0 should really have involved better documentation of issues like this (from whoever introduced the issue, me in this case).

tmcw commented 12 years ago

Any recommendations for the tileLimits API without access to the Map object? All of the examples I'm seeing have ready access to Map, but that isn't generally the case when you're calculating these externally to the map, and don't seem to lack access to .locationCoordinate() from providers?

RandomEtc commented 12 years ago

Not sure - is it too much to introduce a dependency on a projection instance? Not every provider needs one but if you do it's easy to make one (doesn't need a DOM like a map instance would at the moment).

tmcw commented 12 years ago

More detail:

It looks like this segment of provider is calculating whether coord.row is greater than the size of the extent, rather than whether it's outside of the extent. Unclear how this works - changing it to compare against just BR.row somewhat alleviates the problem. (there's also the accidental binary |, which I'll fix in a second).

Stashing this work for the time being because I'm hitting a bit of a wall in understanding what should do what: http://bl.ocks.org/1642008 The code in question is https://github.com/mapbox/wax/blob/master/connectors/mm/waxconnector.js#L6 - both setting tileLimits and coordLimits breaks the map in unusual ways.

RandomEtc commented 12 years ago

Eek on accidental binary |, good catch.

Also you're right, the comparison to vSize is too specific for the default behavior and checking against TL and BR would be more correct.

I now realise that my advice before (to assign provider.tileLimits to map.coordLimits) wasn't right because of the nature of sourceCoordinate :-/ See the enforce-limits example for an overridden one that correctly deals with extents that don't wrap around.

We probably need a more intelligent sourceCoordinate that can handle both cases (wrap around the whole world, don't wrap around cropped regions). Modest Maps for Flash used to detect this by setting wrappable limits to Infinity - the problem with this is that you can't then use the limit itself to wrap the coordinate used to generate the tile url.

RandomEtc commented 12 years ago

BTW, the check against vSize was for non-square plate carrée projections, but it's been a while since I've followed through with an example for one of those. Anyone got tiles?

tmcw commented 12 years ago

Hmm, okay - can you go into a bit more detail about the varieties of layers you're looking to support here? I don't have any non-square tilesets around, and am trying to fix up this part of the code today. Is the assumption that MapProvider can expect the same coord sizes (# tiles/zoom) across projections?

RandomEtc commented 12 years ago

Honestly I'm not sure - I'd have to work through the exercise of setting up a map in a different projection to be certain. But 100% certain that other projections should be possible, and definitely were possible in the first public releases of the lib.

If you don't have any non-Mercator maps, and Stamen don't have any, it can't be that big a priority :)

(I'm not able to work on mm.js at the moment so don't let me be a blocker)

tmcw commented 12 years ago

Okay, coordLimits and enforcing extent was fixed in 34db7980dc6ce73b334a63efe4fc82a0f4f39a4b - not seeing any more actions here, closing.