CSS-based tile animation #127

Merged
merged 30 commits into from Jan 18, 2012

5 participants

@elemoine
OpenLayers member

This pull request suggests adding CSS-based animations when displaying tiles. To enable tile animations the tileAnimation property should be set to true in the map instance (default is true). Some CSS rule defining transitions on the opacity property should also be present. This pull request adds the following rule to the default theme's style.css file:

.olMapTileAnim .olTileImage {
    -webkit-transition: opacity 0.2s linear;
    -moz-transition: opacity 0.2s linear;
    -o-transition: opacity 0.2s linear;
    transition: opacity 0.2s linear;
}

People not relying on OpenLayers' default theme will need to add some CSS rule of this sort, or they'll need to disable tile animations by setting tileAnimation to false in the map options. (I'll document this in the 2.12 release notes if my commits are merged into master.)

All tests pass in FF8, Chromium 15, and IE8.

One thing I'm not sure is how tile animations look like in Android. Any test would be much appreciated.

FWIW, I'd like also to add CSS-based zoom animations in the future. I've been thinking about redesigning our backbuffer implementation to work at the layer container level instead of at the layer level. I've also imagining introducing a zoomAnimation option at the map level, which would be consistent with the tileAnimation option introduced by this pull request.

@elemoine elemoine commented on an outdated diff Jan 1, 2012
lib/OpenLayers/Map.js
@@ -500,6 +508,9 @@ OpenLayers.Map = OpenLayers.Class({
}
OpenLayers.Element.addClass(this.div, 'olMap');
+ if (this.tileAnimation) {
@elemoine
OpenLayers member
elemoine added a line comment Jan 1, 2012

The olMapTileAnim class could be added only if transitions are supported (OpenLayers.TRANSITION is not false).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine
OpenLayers member

I'd like to change the implementation a bit. I want to not rely on the transitionend event. Relying on the transitionend event causes problems if tileAnimation is true and no CSS transition rule is defined. There's certainly lots of people not using OpenLayers' default theme out there. For those, because tileAnimation defaults to true (which I think we want), their tiled layers won't behave correctly because loadend will never get triggered and the layer's backbuffer will
never be removed. People using the loading panel add-in will also have issues.

@tschaub
OpenLayers member

This is a really nice enhancement. I'm working on some changes to handle the transition end events.

@elemoine
OpenLayers member

With eb924c8 we no longer use transitionend. We prevent flashes by delaying the removal of the back buffer with a simple setTimeout call. This is to avoid problem when no css transition rule is defined, which is the case for people not using OpenLayers' default theme (style.css). And there's an another benefit: the code is simpler.

@tschaub
OpenLayers member

The loadend event is always triggered synchronously now.

tschaub added some commits Jan 2, 2012
@tschaub tschaub Removing the tileAnimation map property.
Edit CSS to change the style.
3c910c8
@tschaub tschaub Increase timeout to avoid flashes.
Without this change, flashes are fairly frequent while zooming around a full screen map.
f2168d1
@tschaub
OpenLayers member

This is really looking good. I'm still seeing some flashes with the 800ms timeout - they get much less frequent at 2500ms for me. I also think we can get rid of the added map property. These changes are in elemoine#3.

Please merge if you agree. Thanks for this great enhancement.

@elemoine
OpenLayers member

The problem with large timeout values is that we re-use the old back buffer while we have a new tiles, and, therefore, a better potential back buffer at our disposal. Was 2500 the smallest value you came up with, or can we lower it a bit?

@ahocevar
OpenLayers member

This looks great!

@elemoine
OpenLayers member

@ahocevar I'm interested in a review of the new "set opacity" code from you.

@ahocevar
OpenLayers member

I see frequent flickers in Chrome when zooming in/out in the transition.html example with the 4th layer set as base layer.

@elemoine
OpenLayers member

@ahocevar flickers? Like flashes?

@ahocevar
OpenLayers member

The transition.html example is a good one to test this new code. When using the 1st layer as base layer, I see the whole map disappear frequently after dragging. I think this is related to the flicker issues. Maybe having the backbuffer fade out instead of using setTimeout to remove it would improve things?

@ahocevar
OpenLayers member

@elemoine With flickers, I meant the whole layer or parts of it disappearing for a moment.

@elemoine
OpenLayers member

Thanks for testing @ahocevar. The problem is related to the delayed removal of the back buffer. There's no problem if one goes very slow and always pans after the back buffer has been removed. I haven't looked at how to fix that for now.

@elemoine
OpenLayers member

@ahocevar, and can you confirm that you observe these "flickers" only with the single tile layer?

@ahocevar
OpenLayers member

@elemoine the "flickers" are related to the backbuffer. So they appear after zoom changes also in tiled layers. I think the best way to fix this would be to let the backbuffer fade out with an animation before removing it. The root of the problem is that many browsers fire the loadend event for images really after it finished loading, but before it is displayed. This is why we previously removed the tile based back buffer on the layer's loadend event, and not on the tile's loadend.

@elemoine
OpenLayers member

@ahocevar then I don't understand how/why the problem relates to tile animation.

@ahocevar
OpenLayers member

@elemoine You're right, it's not related. Scheduling the removal of the backbuffer and then clearing the timeout when a new buffer has to be created causes the issue - I see the disappearing tiles only when I wait less than 2.5 sec before dragging (for singleTile layers) or zooming (for other layers) again.

@elemoine
OpenLayers member

@ahocevar moveTo removes the current back buffer only in the (zoom changed, no resize transition, single tile) case, but without clearing the timer. That may be one problem. I still don't understand what's going on for non single tile layers.

@elemoine
OpenLayers member

I may know what the issue is now: a back buffer is being reused while it is scheduled for removal (from a previous zoom). In that case the back buffer can be removed during the transition, i.e. while new images are loaded.

So solve the problem we may just need to clear the timer when a back buffer is about to be (re-)used (in applyBackBuffer).

@tschaub
OpenLayers member

For what it's worth, I didn't see flashes when I was waiting to remove the back buffer until the newly loaded tiles transitionend (and similarly named) event. It wouldn't be too hard to reproduce that work if other methods don't work.

@elemoine
OpenLayers member

Two options at the time moveTo is called and the back buffer has been scheduled for removal:

  1. Clear the timer and remove the back buffer immediately. This may cause flashes as we remove the back buffer earlier than planned (tile animation may still be going on). This may be mitigated by the fact that we use a large timeout value.

  2. Clear the timer and keep the back buffer. This prevents flashes, but results in the reuse of the old back buffer while we may have new tiles.

I'd favor option 2, to avoid flashes. But both options should be tried out.

Some pseudo-code for option 2 (mostly for myself):

moveTo:
    ...
    if zoom changed and
       no resize transition and
       single tile:
         remove back buffer
    ...

applyBackBuffer:
    if no back buffer:
        create back buffer
        if no back buffer:
            return
    else if removal is scheduled:
        clear timer
    position and scale back buffer

tile loadend listener:
    if no more loading tiles:
        if back buffer:
             schedule removal (set timer)

remove back buffer:
    if back buffer:
        if back buffer has parent:
             remove back buffer from parent
        set back buffer to null (no back buffer)
        if removal is scheduled:
             clear timer
@elemoine
OpenLayers member

39f2ddc implements option 1, as option 2 didn't yield anything good.

@ahocevar I'd appreciate if you could test the new code, and see if you can still reproduce the issues you've seen.

I'll still need to add tests for the new code.

@lordi

Hey, wonderful patch, works great.

The only thing that bothers me is that my transparent PNGAs will have fuzzy black pixels in IE7/8, e.g. the transparency no longer works correctly. This surely is an IE bug and is due to setting the filter="alpha(opacity=X)" property.

What would be the nicest way to solve this problem?
Set the opacity filter only when view port div has the olMapTileAnim class?

@lordi

Basically, a way to disable tile animation would be nice.
Why was the previously introduced property "tileAnimation" removed again?

@elemoine
OpenLayers member

@lordi thanks for testing this patch. I'll try to reproduce the IE issue. Do you by any chance have live example?

Regarding the tileAnimation option: we removed it because we think tile animation should be entirely configurable using CSS. This is a CSS setting. This patch adds notes to the 2.12.md file, which describe how to disable tile animation entirely. But this won't make the IE issue go away.

@lordi

@elemoine thanks, I can send you links to live examples if you give me an email of yours or another way to PM you.

The following stackoverflow question is related, but I wasn't able to fix it with that.

http://stackoverflow.com/questions/1251416/png-transparency-problems-in-ie8

@elemoine
OpenLayers member

@lordi, both the current code and my code make use of the opacity filter, so this is a bit strange that you don't have these fuzzy black pixels with the current code. And I don't seem to be able to reproduce the issue using the layer-opacity.html example. (Also, I'm observing that this example doesn't work properly in IE6, but I'm not yet sure this is because of my patch.)

@lordi lordi commented on an outdated diff Jan 10, 2012
lib/OpenLayers/Tile/Image.js
style.position = "absolute";
- if (this.layer.opacity < 1) {
@lordi
lordi added a line comment Jan 10, 2012

modifyDOMElement will apply a filter property, only if opacity is < 1, thats why master works for my code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lordi

In current master, opacity is set but filter="alpha(opacity=X)" is set only when opacity < 1.
In my example, opacity is always 1, since the PNGs have an inherit alpha channel.

I have marked the line in the diff: https://github.com/openlayers/openlayers/pull/127/files#diff-4

It think this is the problem, but you can't get around it easily, because for tile fading the filter property must be set necessarily.

@elemoine
OpenLayers member

@lordi, does d3db301 make any difference?

@elemoine
OpenLayers member

FWIW, http://www.openlayers.org/dev/examples/layer-opacity.html does not exhibit the expected behavior in IE6 either, so the IE6 issue isn't related to the new code.

@lordi

Yep, with d3db301 it works alright in IE8!
Thanks for your efforts, is this changeset compatible with the other browsers?

@elemoine
OpenLayers member

@lordi, yes. The code looks even better with d3db301. Thanks a lot for reporting the problem and providing me with live examples.

@elemoine
OpenLayers member

@ahocevar, have you had a chance to test the latest changes? I hope I got it right this time and I can merge this patch. Thank you.

@elemoine elemoine Merge branch 'master' into tile-fade-in
Conflicts:
	tests/Tile/Image.html
99ca325
@tschaub
OpenLayers member

Eric, this is such a huge user experience improvement! Thanks for all the work on it. I've tested it in full-screen maps with multiple layers and transition effects. Everything looks great. Though I'm sure there could be additional items exposed, I think it is worth getting this into master and letting more people have a chance to test it out.

@ahocevar
OpenLayers member

@elemoine: I think this is good to merge. I did some heavy panning and zooming with both untiled and tiled layers, and only saw a minor glitch (a single tile disappearing for a fraction of a second) every now and then with tiled layers. Thanks for your efforts on this.

@elemoine elemoine merged commit 37d2127 into openlayers:master Jan 18, 2012
@virix

In Firefox 10 the fading doesn't work well: the transition starts with a black background.

@elemoine
OpenLayers member

@vrifino thanks for reporting this. Does the same problem exist in Leaflet?

@tschaub
OpenLayers member

I've just tested Firefox 10 on OSX and the fade transitions work well. Got a report of the same working on Windows.

@elemoine
OpenLayers member

I now have FireFox 10 on my Ubuntu laptop and the tile transitions work well for me. Tested on the fullScreen example.

@virix

I've tested Firefox 10 on Windows 7 and the transition starts with a black background. It seems a Firefox 10 bug: the same problem exists in Leaflet. Firefox 8 works well. If you increase the transiction interval, the problem is shown more clearly:

.olTileImage {
-webkit-transition: opacity 0.2s linear;
-moz-transition: opacity 10s linear;
-o-transition: opacity 0.2s linear;
transition: opacity 0.2s linear;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment