Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to Leaflet #121

Merged
merged 40 commits into from Nov 23, 2012
Merged

Conversation

jfirebaugh
Copy link
Member

I got so fed up with the poor two-finger zoom behavior of OpenLayers that I started to work on this. I have to say, it feels much nicer.

I don't have a full port completed ("Browse Map Data" is the major missing piece), but I wanted to gauge interest and figure out what the requirements would be to cut over.

@systemed
Copy link
Contributor

systemed commented Oct 9, 2012

Wow.

@tomhughes
Copy link
Member

I haven't looked at any of the details, but I don't have a problem with this in general terms, if it can be made to support everything that OL currently does.

One concern is that I think it only has SVG and Canvas renderers, which presumably means no vector overlays in IE before version 9 which may be a bit of a problem.

Something I did notice is that you have removed vendor/assets/openlayers but we will probably need to keep that for now even if we're not using it, as there are third party sites that load OpenLayers from us I believe - certainly there are many which load the OpenStreetMap,js file and I think some load the whole thing.

@Jonobennett
Copy link

Could the choice of viewer library be a user-configurable option? I have no objection to Leaflet being the default.

@tomhughes
Copy link
Member

Well of course it could but it would create a massive amount of work to maintain it - any new features that used the map at all would need to be written (in part at least) twice.

@tmcw
Copy link
Contributor

tmcw commented Oct 9, 2012

One concern is that I think it only has SVG and Canvas renderers, which presumably means no vector overlays in IE before version 9 which may be a bit of a problem.

No, there's a VML renderer

@jfirebaugh
Copy link
Member Author

While I was writing this I see that others chimed in, but anyway:

Leaflet also has a VML renderer, and it supports IE 7-9 and even IE 6 partially.

I have no problem leaving the OL assets around for third party sites, but it would be a lot of work to support two slippy map implementations simultaneously, and it strikes me as overconfigurability to offer that option. We should compare the two on their merits and make a decision.

To state what I see as the case for Leaflet in the most general terms: it offers a better user experience (smoother zoom and pan, faster rendering performance), has a smaller download footprint, and is easier to learn for developers without prior OL or Leaflet experience.

@tomhughes
Copy link
Member

Excellent. I was misled by something I saw in the docs, and didn't find the actual list of renderers for some reason.

@mourner
Copy link

mourner commented Oct 12, 2012

Hi, I'm Leaflet maintainer — let me know if you need any help or have any questions regarding the library and this work, I'm at your service!

Regarding features and browser support, they're outlined on this page: http://leaflet.cloudmade.com/features.html (BTW, touch support in IE10 and tablet Metro apps is almost finished too). In particular, vector layers work perfectly in all browsers, including IE6, with great rendering performance due to algorithmic optimizations such as real-time simplification and clipping.

@jfirebaugh
Copy link
Member Author

Thanks @mourner, I appreciate the help.

I do have a question about the vector performance on IE. Due to performance concerns, we currently limit the number of features displayed in OSM's browse mode to 100 for IE <= 7, 500 for IE 8, and 2000 for other browsers. Do you have any guidance as to what would be reasonable limits (if any) for Leaflet?

To update everyone on the status here, I ported the data browser yesterday. The remaining pieces that are missing are:

  • Feature limits in data browser (see above).
  • A resizable crop control in export. I use Leaflet.draw for dragging the initial rectangle, but in the existing export you can adjust the result via draggable handles, and I haven't implemented that.
  • Image scale control in export. I need to figure out how to calculate the map scale with Leaflet; it isn't a built in function like it is with OpenLayers.
  • Fix a few bugs relating to base layer changes. I'm hoping Leaflet will add a baselayerchange event to make this easier.
  • Convert embed.html to Leaflet.

And there is one blocking bug in Leaflet that I'm waiting on a fix for.

I've updated my branch. Is it possible to get it deployed on a dev server so people can test this out?

@tomhughes
Copy link
Member

I've setup http://leaflet.apis.dev.openstreetmap.org/ which is shadowing your branch, but it's failing at the moment because it's in production mode and leaflet.css is not being precompiled.

@jfirebaugh
Copy link
Member Author

Thanks Tom. Will it update with new changes to this branch automatically?

@tomhughes
Copy link
Member

Yes - it gets updated every time chef runs, which is every half hour.

@jfirebaugh
Copy link
Member Author

It's still 500'ing. Any idea what the issue is?

@tomhughes
Copy link
Member

I think you got unlucky and got caught by me mucking about with the chef config so it didn't update properly - it's working now.

@mourner
Copy link

mourner commented Oct 12, 2012

@jfirebaugh data browser is not working for me on that link (it doesn't load anything but no error too).

@jfirebaugh
Copy link
Member Author

Yeah, it's running against an empty database, so you won't be able to test any vector-based functionality.

@tomhughes
Copy link
Member

Not quite - there's a couple of roads at http://leaflet.apis.dev.openstreetmap.org/?lat=51.76486&lon=-0.00762&zoom=16&layers=M that I added.

@mourner
Copy link

mourner commented Oct 12, 2012

Ah, I see. :) It's quite hard to suggest feature limits without testing as I was mostly looking at performance of several features with lots of points (say 50k-point route, like the debug/vector/vector.html example), not sure how it will behave with OSM-type data (but certainly not worse than OL :)

@mourner
Copy link

mourner commented Oct 12, 2012

@jfirebaugh btw, maybe data should be cleared on new data load, not on moveend, so that there's no flickering.

@mourner
Copy link

mourner commented Oct 12, 2012

Also, there are errors about setCenter in console on resize.

@mourner
Copy link

mourner commented Oct 12, 2012

One more missing piece would be zoom slider. Some people say that it is very useful for UI because it indicates current zoom in relation to min/max visually, and because you can zoom out more quickly. On other hand, many people never use the control (preferring mouse wheel), including me, so it only takes screen space. I'm sure that the panning buttons are unnecessary now though.

@jfirebaugh If you do include the slider, I guess the current style for it (by @tmcw AFAIR) should be ported too.

@wboykinm
Copy link

The touch performance is super-pleasant as well. I don't mean to be an OL hater, but this seems like an overdue porting. Thanks for diving in.

@jfirebaugh
Copy link
Member Author

Thanks to @mourner and @danzel for being responsive with the leaflet fixes. It looks like we have a lead on a workaround for the FF grid artifact.

Remaining TODO items:

  • Test performance of data browser, see if we can simply remove the feature limit. Leaflet should be better performing as it does line simplification and IIRC OpenLayers does not.
  • Port the custom Pan/Zoom control and layer switcher. @tmcw mentioned he thought he could do this fairly quickly. I think we should keep existing controls unchanged for now. Discussions about dropping the pan control or using the default layer switcher can happen separately. (Interesting aside: at SOTMUS one of the speakers related an anecdote of a user who did not know how to pan the map by dragging, but did figure out how to do so using the pan control.)

Anything else?

@mourner
Copy link

mourner commented Oct 17, 2012

Regarding vector performance testing, I'm not sure if vector simplification will improve things much as the type of data on close zoom (mostly lots of buildings) doesn't provide a lot of room for simplification. But try tweaking it like this:

  1. L.Path.CLIP_PADDING = <number> — sets the padding around screen view for vectors (it's 0.5 — half of the screen — for modern browsers), you can try setting lower values to improve performance (less features to render).
  2. L_PREFER_CANVAS = true global switch before including Leaflet makes it prefer Canvas over SVG for rendering in modern browsers, it may increase performance in case of lots of features.

Looking forward to see the testing results!

@mourner
Copy link

mourner commented Oct 17, 2012

By the way, just discovered https://github.com/tripbirds/leaflet-locationfilter/ — which I guess can be used in place of Leaflet.draw for the export feature.

@jfirebaugh
Copy link
Member Author

That locationfilter plugin is great -- now using it in export and browse modes.

I've also restored the maxFeatures check -- IE just can't cope.

Pan/Zoom control is the last remaining item, working on that now.

@openstreetmap-website
Copy link

Wow, I didn't realize that the data view was already ported as well. That's
awesome work.
If you want to try it out, I have created some features in the dev api DB
around
http://leaflet.apis.dev.openstreetmap.org/?lat=40.759521&lon=-111.886708&zoom=18&layers=M

Martijn

On Fri, Oct 19, 2012 at 12:09 PM, John Firebaugh
notifications@github.comwrote:

That locationfilter plugin is great -- now using it in export and browse
modes.

I've also restored the maxFeatures check -- IE just can't cope.

Pan/Zoom control is the last remaining item, working on that now.


Reply to this email directly or view it on GitHubhttps://github.com//pull/121#issuecomment-9610434.


rails-dev mailing list
rails-dev@openstreetmap.org
http://lists.openstreetmap.org/listinfo/rails-dev

martijn van exel
http://oegeo.wordpress.com

@mourner
Copy link

mourner commented Oct 22, 2012

Wow, looks awesome!

@jfirebaugh
Copy link
Member Author

Ok, I believe all outstanding issues are fixed. And I made nice leaflet-ish markers.

@mourner
Copy link

mourner commented Nov 9, 2012

Great! BTW, you can update Leaflet master as well, there are some nice panning inertia improvements, a bit better zoom animation and better dragging cursors. :)

@jfirebaugh
Copy link
Member Author

@tomhughes Anything else that needs to happen here?

@apmon
Copy link
Member

apmon commented Nov 20, 2012

It looks like the layer chooser is not in sync after initial load. The layer chooser always shows the "standard" layer is selected when the page is loaded even if the layer shown (due to the cookie) is a different one.

@tomhughes
Copy link
Member

I've fixed the problem @amon found locally (upstream pull is https://github.com/CloudMade/Leaflet/pull/1169) and a few other minor things and I have pushed that version at http://tomh.apis.dev.openstreetmap.org/ and if there are no more problems found I plan to merge it to master tomorrow.

@openstreetmap-mirror openstreetmap-mirror merged commit 078da13 into openstreetmap:master Nov 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Pull request is not ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet