Improved best first level calculation #1198
Conversation
|
When I apply this change to the test page I made in https://github.com/rmcloughlin/osd-tile-loading, this PR fixes the problem perfectly; no low layer images are fetched at all. However in my actual application things still act a bit strange. My image is about 60,000 (w) by 40,000 (h) with tile size 1024 and overlap 1. Before the fix is applied (using OSD 2.2.1). The tiles are loaded in this order: When I use OSD that was built from this PR, the tiles are loaded in the following order: So in this case it's still loading the low-layer tiles but it's getting them after loading the more appropriate tiles. |
|
@rmcloughlin That second issue you're seeing is the one fixed by #1193. To see both this patch and the other patch together you can use this combined branch: https://github.com/openseadragon/openseadragon/tree/ig-both Could you try that one and verify that it does the right thing? |
|
Yes that one (ig-both) works better. It makes fewer requests and doesn't request all the low layer images (like 0/0_0.jpeg, 1/0_0.jpeg). |
|
@rmcloughlin Awesome, thank you for testing it! I'll leave this open for a few more days to see if @avandecreme (or anyone else) has anything to add. |
src/tilesource.js
Outdated
| ); | ||
|
|
||
| if( tiles.x + 1 >= tilesPerSide.x && tiles.y + 1 >= tilesPerSide.y ){ | ||
| if (tiles.x > 1 || tiles.y > 1) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
In the line below, shouldn't we return Math.max(this.minLevel, i - 1)?
Also, we could start the loop at i = this.minLevel + 1.
Fixes #1197
The issue hinges around the
levelVisibilitycalculation that's based in part onTileSource.getClosestLevel. With 254px tiles, the response from getClosestLevel is reasonable, but with 500px tiles it just returns 0.I'm not actually sure why getClosestLevel compares against the container size. It seems to me the best thing to do is just find the highest level where the entire image can fit on a single tile, so I changed it to do that, and in my testing the results are good: regardless of the tile size or the viewer size, the first tile that's loaded is the largest single-tile level, and then higher level tiles are loaded to fill in the remainder.
If you force the
defaultZoomLevelto be something small, it will start with a lower level, since it doesn't need a tile as large as the largest single-tile level. If you force defaultZoomLevel to something big, it still starts with the largest single-tile level (unless you haveimmediateRenderon), since we don't want to risk having holes. All seems good to me.Even though getClosestLevel is technically a documented function, I'm not terribly concerned about changing it like this; it's not really a function people would use, and it still produces the same kind of value (even if the logic is different now), and ignoring the old argument isn't really a breaking change. Also note that this function is currently called in only one place. I suppose I could at least improve its documentation.
That said, I'd love a second opinion... @avandecreme, what do you think?
@rmcloughlin, can you verify that this fixes the issue you found?