Skip to content

Conversation

danzel
Copy link
Member

@danzel danzel commented Jun 19, 2012

When zooming, animate all markers/overlays moving like the map!
This makes the awesome stretch zoom effect way more sexy.

Implemented for Marker, Popover, Path.SVG, Path.Canvas.
Plugins that mess with markers may need minor css updates (I've done a branch of @jacobtoye's Leaflet.iconlabel https://github.com/danzel/Leaflet.iconlabel)

Tested to work on latest: chrome, firefox, safari, opera.
Tested to not do anything different on IE9.

TouchZoom works 99%. There is sometimes an animation error when you release your finger when using SVGs (they slide one way and then slide back). I've been bashing my head against this all day with no progress, so help here is appreciated!
I'm pretty certain Map.TouchZoom._onTouchEnd calculates center wrong, or the maths in Path.SVG._animatePathZoom is incorrect for how ZoomAnimation does the zoom with the value passed here.

Check it out:
http://danzel.github.com/Leaflet/debug/vector/feature-group-bounds.html
http://danzel.github.com/Leaflet/debug/vector/vector-canvas.html

danzel added 24 commits June 19, 2012 09:20
… and reappearing. Currently implemented as a hack in Map.ZoomAnimation.
… is tidy now.

Conflicts:

	src/map/anim/Map.ZoomAnimation.js
…of testing each browsers 3d everywhere. Gives opera 3d support!

Not totally happy with L.Browser.any3d not being in the closure, but it works.
…w while it is happening or the animation will get messed up
…ome weird movements (two finger scroll on iPad) it makes the animation get weird.
…e to _zooming in Map.TouchZoom. Minor other tidy ups.
@mourner
Copy link
Member

mourner commented Jun 19, 2012

This is one the best Leaflet pulls ever — thanks Dave, this made my day!

I'll try to figure out the touch zoom issue before merging. Meanwhile, the last touch would be to make this configurable, at least for markers — path root is animated as one but transitions for markers are individual, so there may be a considerable performance hit if you, say, add 10000 markers on the map. I'm not sure if this should be a per-map setting (like tile fade animation) or per-marker, or both.

@mourner
Copy link
Member

mourner commented Jun 19, 2012

Refactored a bit (animation on a separate zoomanim event instead of zoomstart, simpler calculation for markers and popups, fixed popup autopanning bug, removed some repetition in code): CloudMade@45d9421

@mourner
Copy link
Member

mourner commented Jun 19, 2012

Still no luck with the touch-zooming issue though...

@mourner
Copy link
Member

mourner commented Jun 19, 2012

I got a guess! Maybe iOS can't correctly do transition between two different transforms. I'll try refactoring so that instead of transition between touchTransform and animEndTransform it does transition between touchTransform and touchTransform + transformBetweenTouchAndAnimEnd.

@mourner
Copy link
Member

mourner commented Jun 19, 2012

No, didn't manage to make this work — got too confused and stuck with the math. :(

@mourner
Copy link
Member

mourner commented Jun 19, 2012

One more weird bug on iOS: double-click-zoom and then immediately do pinch-zoom-out. You'll see a wacky zoom animation twice and after this all pinch-zooming will act like this.

@danzel
Copy link
Member Author

danzel commented Jun 20, 2012

Heres a test page for the iOS zoom issue so it can be reproduced on desktop chrome:
http://danzel.github.com/Leaflet/debug/vector/touchzoomemu.html

The actual transforms are at fault, not iOS! This should make debugging it easier too

@danzel
Copy link
Member Author

danzel commented Jun 20, 2012

Alright, TouchZoom fixed :-)
For why this works, check out this fiddle: http://jsfiddle.net/cstZS/13/
The transforms are equivalent, but the animations that happen are not.

danzel added 2 commits June 20, 2012 16:48
…lse and markers will disappear instead of animating when the zoom animation plays.
@danzel
Copy link
Member Author

danzel commented Jun 20, 2012

Option added too. You either have markers animate or not, in which case they disappear while the zoom animates.

This should be all good to pull in now I hope :)

@mourner
Copy link
Member

mourner commented Jun 20, 2012

The transform fix was really smart, great that you got it fixed!

@mourner
Copy link
Member

mourner commented Jun 20, 2012

I also noticed (looking at your fork of Leaflet.iconlabel) that style changes for different overlays are similar, so maybe we need to DRY it up a little by making one class for all animatable overlays (e.g. leaflet-anim-overlay).

Also, I planned to refactor layers code in future to make all layers to be inherited from a base layer class that contains repeated logic (like animation). We could make two animation mixins then, for example, one for scaled animated layers (like vectors and image overlays) and other for static animated layers (like markers and popups).

@mourner
Copy link
Member

mourner commented Jun 20, 2012

I'll still merge the pull, these are just some thoughts for future. :)

@mourner
Copy link
Member

mourner commented Jun 20, 2012

Fixed panning animations in Firefox/Opera in my branch, tested a bit more... Chrome is butter-smooth, a pity that Firefox doesn't work such well.

Some more thoughts... I think there may be a lot of users that have several hundreds of markers on the map or more, and they may experience great zoom lags after upgrade, especially on Firefox. Maybe it would be nice to make the defaults more intelligent by tracking the number of markers that were added to the map and automatically switching off marker zoom animation above a certain threshold. E.g. there would be two options — maxAnimatedMarkers (set by default depending on the browser - e.g. 400 for Chrome, 100 for Firefox) and animateMarkerZoom (not set by default, that would override the max behavior and manually turned it off/on).

@danzel
Copy link
Member Author

danzel commented Jun 20, 2012

Will do another pass on the css bits today to tidy them up.

On the markers limit I would be hesitant to add it as the real limits also vary based on browser version and computer spec. We could provide a method so users can turn the animation on or off?
I'll test a bit more today and see if I still think the same later, chrome on my box handles ~800 markers fully animated no worry.

…win8 touch tablet, now I just need someone to give me one to test on :-)
@danzel
Copy link
Member Author

danzel commented Jun 20, 2012

I've looked at the limits.

Chrome on my box is smooth up to around 1000.
Safari on a Macbook Air is smooth at 1000.
Firefox is smooth to about 200 (with firebug disabled).
Opera is smooth to 1000 also, but it doesn't reliably fire the transition end event past around 250 (Add 1000 points, hit the zoom out button. It zooms out but doesn't fire transition end). So we would probably want to limit this also.

It would be a bit weird if a user using a map gets the animations, then suddenly after adding another marker and going past the limit the animations disappear.
If the client application handled this (always have the animation on these browsers, never have on these), based on how many markers they expect to have, then this would provide a better user experience.

@danzel
Copy link
Member Author

danzel commented Jun 21, 2012

The Future is now!

Changed to use classes for hiding/animating. Leaflet.iconlabel has the same changes.
Much DRYer :-)

@mourner mourner merged commit 4bc3455 into Leaflet:master Jun 21, 2012
@mourner
Copy link
Member

mourner commented Jun 21, 2012

Merged into master, yay :)

@mourner
Copy link
Member

mourner commented Jun 21, 2012

Please submit a pull to gh-pages-master with corresponding docs updates.

@@ -156,6 +156,7 @@ L.DomUtil = {
L.Util.extend(L.DomUtil, {
TRANSITION: L.DomUtil.testProp(['transition', 'webkitTransition', 'OTransition', 'MozTransition', 'msTransition']),
TRANSFORM: L.DomUtil.testProp(['transformProperty', 'WebkitTransform', 'OTransform', 'MozTransform', 'msTransform']),
BACKFACEVISIBILITY: L.DomUtil.testProp(['backfaceVisibility', 'WebkitBackfaceVisibility', 'OBackfaceVisibility', 'MozBackfaceVisibility', 'msBackfaceVisibility']),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, did you cover backface visibility in other browsers intentionally? As far as I remember, I added it as a nasty Android bugfix, so it doesn't need to be used in non-webkit browsers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, just made it generic as it was before. Probably wants a comment as to why it is as it is. I'll add one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants