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

A Big Back Buffer approach #16

Merged
merged 34 commits into from
Nov 2, 2011
Merged

A Big Back Buffer approach #16

merged 34 commits into from
Nov 2, 2011

Conversation

elemoine
Copy link
Member

@elemoine
Copy link
Member Author

For the record, builds are slightly smaller with the bigbackbuffer patch, but it's (obviously) not significant:

$ ls -lh *.js
-rw-rw-r-- 1 elemoine elemoine 957K 2011-10-21 09:58 OpenLayers-bigbackbuffer-full.js
-rw-rw-r-- 1 elemoine elemoine 123K 2011-10-21 09:59 OpenLayers-bigbackbuffer-lite.js
-rw-rw-r-- 1 elemoine elemoine 958K 2011-10-21 09:58 OpenLayers-master-full.js
-rw-rw-r-- 1 elemoine elemoine 124K 2011-10-21 09:57 OpenLayers-master-lite.js

@elemoine
Copy link
Member Author

With this patch we still reposition every backbuffer tile (see Grid.createBackBuffer). The gain though is that we don't do lonlat-to-pixel conversions for each tile, we do one conversion only for positioning the big backbuffer.

@ahocevar
Copy link
Member

Great work Eric. Your approach is a hybrid between my initial layer backbuffer attempts and what we currently have. And a good one, because it avoids all the pitfalls I ran into. And it simplifies the code indeed.

Testing this I only see one issue: with singleTile layers and no transitionEffect, when zooming right after dragging, the old image is shown until the new tile is loaded. To see this, go to the transition.html example, drag the map and then quickly zoom in.

@elemoine
Copy link
Member Author

Andreas, thanks for the review, and for catching the issue with singleTile and no transitionEffect. This is now fixed in the branch. Please tell if I can now merge this branch into master.

@ahocevar
Copy link
Member

Hm. Maybe this is related to your recent changes, but now in tiled mode, I can break the backbuffer display if I change zoom levels, drag and change zoom levels again while tiles are still loading. What happens then is that the backbuffer shows an incorrect region of the map. Also I noticed that no backbuffering will happen when zooming while tiles are still loading, unless there is a backbuffer being displayed already.

@elemoine
Copy link
Member Author

I don't think that's related to my recent changes. My recent changes only apply if singleTile is set.

I'm not able to reproduce the "backbuffer shows an incorrect region of the map" at this point. I'll try harder.

And yes, backbuffering won't happen if tiles are being loaded and we don't have a backbuffer yet. The comment in Grid.createBackBuffer makes this clear I think. Do you think that's a real issue?

@ahocevar
Copy link
Member

For testing I set transitionEffect: "resize" on the WMS layers in the fullScreen.html example and use a slow (i.e. mobile) internet connection. Maybe this hint helps you to reproduce the issue.

Regarding the fact that you don't create a backbuffer at all when tiles are still loading is a big issue in my opinion, because the most pan/zoom combinations happen when people navigate to the area on the map they are interested in. This is where backbuffers are needed most, so people know where they currently are. And with the behavior after your change, this is exactly where people get no backbuffer.

Looking at your code more closely, I see that the name cloneMarkup for the method that gets the backbuffer of a tile is misleading. Because it does not clone the markup, it steals the markup away from the tile (i.e. the tile does not have an image any more).

@elemoine
Copy link
Member Author

Ok, I'll try in fullScreen.html.

I think I can easily change the strategy, to just skip tiles that are loading when creating the back buffer. This will cause holes in the back buffer, but I guess this corresponds to the current behavior. I'll give it a try, and request your feedback again.

I agree for "cloneMarkup". How about "createBackBuffer"? I've been trying to have the notion of "backbuffer" only at the layer-level, but I think that's ok.

@ahocevar
Copy link
Member

Here is a proof of the issue. What you see in this screenshot happened after a lot of zooming out, dragging, zooming in and the same over again while tiles are loading. I'm not 100% sure what's going on, but it seems what you see in the top left corner is a part of the old backbuffer that is not covered by new tiles that are currently loading.

@ahocevar
Copy link
Member

Yes, I think "createBackBuffer", "createBackBufferMarkup" or "toBackBuffer" would be good names for the current "cloneMarkup" method. Choose one :-).

@elemoine
Copy link
Member Author

Ok, I can now reproduce the problem. Fixing it is another story...

@elemoine
Copy link
Member Author

I just pushed two changes:

  • the first fixes the following case: zoom in and immediately zoom out. On zoom out backbuffering SHOULD be applied, to unscale the backbuffer. Comparing the backbuffer resolution and the current resolution to know if backbuffering should be applied was wrong.
  • create backbuffer even if there are tiles that are still loading (as you requested it Andreas)

These changes break the tests, so I'll need to go back to the tests.

Andreas, do those changes improve the situation for you, or are you still seeing misplaced old backbuffers?

@elemoine
Copy link
Member Author

Actually yes the issue still triggers.

@elemoine
Copy link
Member Author

I think I've found the bug. New commits to come.

@elemoine
Copy link
Member Author

Andreas, please tell me if you're still seeing phantom back buffers with the latest code. Thank you very much.

@ahocevar
Copy link
Member

ahocevar commented Nov 1, 2011

Thanks Eric for your continued effort on this. I can confirm that backbuffers now work in examples and all tests pass in Safari5.1 and Internet Explorer 8. Please pull.

@elemoine
Copy link
Member Author

elemoine commented Nov 2, 2011

Thank you for the great collaboration on this Andreas.

For the record, one improvement could involve invalidating (i.e. removing) the back buffer when it doesn't cover/intersect the current map extent. In this way if new tiles have been loaded a better back buffer could be applied.

elemoine pushed a commit that referenced this pull request Nov 2, 2011
@elemoine elemoine merged commit 0bf1169 into openlayers:master Nov 2, 2011
@ahocevar
Copy link
Member

ahocevar commented Nov 8, 2011

Hey @elemoine: there is still a regression. See openlayers/openlayers@7944407#commitcomment-703587. This can be seen live on http://openlayers.org/dev/examples/mobile-wmts-vienna.html

@ahocevar
Copy link
Member

ahocevar commented Nov 8, 2011

Oh, sorry for the noise. I just tried with a master version before the big buffer change, and with 2.11 (before my backbuffer changes), and all versions show the same behavior.

@elemoine
Copy link
Member Author

elemoine commented Nov 8, 2011

Too late. I had an heart attack already.

@elemoine
Copy link
Member Author

elemoine commented Nov 9, 2011

More seriously, I don't see any solution to this problem. We should maybe mention in the doc that transitionEffect:'resize' may cause undesired effects when applied on transparent layers.

@ahocevar
Copy link
Member

ahocevar commented Nov 9, 2011

With @fredj's tile fading code, things will be nicer though (if also used to fade out the backbuffer).

@elemoine
Copy link
Member Author

elemoine commented Nov 9, 2011

@ahocevar, right, I also think we could mitigate the issue by removing the back buffer a bit earlier.

elemoine pushed a commit that referenced this pull request May 15, 2012
use t.geom_eq in GPX read test
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.

None yet

2 participants