These are chat archives for openseadragon/openseadragon

4th
Nov 2016
John Hoffer
@thejohnhoffer
Nov 04 2016 16:01
It really seems like the fully-loaded-change events are called much too soon for tiledImage.preload === true
John Hoffer
@thejohnhoffer
Nov 04 2016 16:06
Ah- looking at the source code, I think I'm starting to understand. It seems bestTile is never set!
John Hoffer
@thejohnhoffer
Nov 04 2016 16:31
bestTile.png
Which makes sense in a way... there is no 'best tile' to draw... because there are no tiles to draw... but it seems the fully-loaded-change event it based on that fact instead of whether there are no tiles to load...
Ian Gilman
@iangilman
Nov 04 2016 17:32
@thejohnhoffer I doubt that's it, since you're still drawing the tiles: https://github.com/openseadragon/openseadragon/pull/1071#discussion_r86594476
...and at any rate, whether you draw the tiles or not probably doesn't affect the bestTile calculation
At any rate, have you tried the same code with opacity 1 to see how it behaves?
John Hoffer
@thejohnhoffer
Nov 04 2016 17:35
You're right- I was off. It seems to be the case that lastDrawn is populated correctly for tiledImage.preload == true && tiledImage.opacity == 0
Ian Gilman
@iangilman
Nov 04 2016 17:35
Cool
@thejohnhoffer On another topic, have you had a chance to test openseadragon/openseadragon#1029 with the latest master?
John Hoffer
@thejohnhoffer
Nov 04 2016 17:37
I'll try running the same visual with opacity 1. Preload really shouldn't have any impact on any kind of behavior unless opacity is 0...
On the second topic, I'll open my viewer right now in safari to quickly test it...
John Hoffer
@thejohnhoffer
Nov 04 2016 17:40
Ah, wait, we're no longer handling partial opacity through openseadragon for my current project... I'll have to wait until after work to test that. I can rereun it using the same examples I've posted in #1029 just for consistency
Ian Gilman
@iangilman
Nov 04 2016 17:40
Also, if #1035 fixed it, we should mark it as fixed
John Hoffer
@thejohnhoffer
Nov 04 2016 17:41
#1035 looks promising!
Ian Gilman
@iangilman
Nov 04 2016 17:42
:)
Okay, keep me posted on the fully loaded thing...
John Hoffer
@thejohnhoffer
Nov 04 2016 19:20
Should I expect image.lastDrawnto have any particular value after 'fully-loaded-change' is called?
John Hoffer
@thejohnhoffer
Nov 04 2016 19:56
@iangilman Even when opacity == 1, it seems that fully-loaded-change is called way before the tiles load...
Ian Gilman
@iangilman
Nov 04 2016 19:58
@thejohnhoffer Interesting... do you have a link you can share?
...and are you checking the value of fullyLoaded? fully-loaded-change happens every time it goes from true to false or from false to true... you have to verify that it's actually true
...and no, I don't know that you would expect anything particular from image.lastDrawn after fullyLoaded is true.
John Hoffer
@thejohnhoffer
Nov 04 2016 20:00
Yeah, within the fully-loaded-changeevent I check to make sure e.fullyLoaded is true.
I can make a link-- give me a few minutes...
Ian Gilman
@iangilman
Nov 04 2016 20:04
Thanks
John Hoffer
@thejohnhoffer
Nov 04 2016 20:13
This link will only be up for as long as we need it....
This is without any opacity, so each image just displays over the previous image as quickly as possible. The tileSize is the same as the image size as a hacky way to make sure only one tile loads for each image.
John Hoffer
@thejohnhoffer
Nov 04 2016 20:19
My basic claim is that you should be able to see all the fully loadedevents print to the console well before each of the images is shown as '200 OK' in the network tab of the developer console.
To demonstrate this more clearly... maybe I should use a larger tileSize of 2048 instead of 512 just to ensure that you can see the effect of the network lag.
Ian Gilman
@iangilman
Nov 04 2016 20:22
Yeah, it definitely seems to be wrong
Looks like you're using a custom tile source
John Hoffer
@thejohnhoffer
Nov 04 2016 20:23
That's right.
Ian Gilman
@iangilman
Nov 04 2016 20:23
Were you still getting the same behavior when you had multiple tiles per image?
John Hoffer
@thejohnhoffer
Nov 04 2016 20:23
Yep
You can test that yourself by simply changing the url parameters like so
Ian Gilman
@iangilman
Nov 04 2016 20:24
Cool
I wonder if this is related to #1020
Try reversing #1014 and see if that fixes it
John Hoffer
@thejohnhoffer
Nov 04 2016 20:25
Sure- why not- okay..
Ian Gilman
@iangilman
Nov 04 2016 20:29
Yeah, I think that's it... what's happening is you don't get a 'bestTile' because the only tile you need is already loading, but it hasn't finished loading, so we're erroneously saying that's good enough
...and this is happening because #1014 causes draws to happen even when we should be sitting around waiting
John Hoffer
@thejohnhoffer
Nov 04 2016 20:30
So I commented out the this._needsDraw = true under the comment // Load the new 'best' tile in the openseadragon.js included at the link I shared...
Ian Gilman
@iangilman
Nov 04 2016 20:31
Doesn't seem to have made a difference here...
John Hoffer
@thejohnhoffer
Nov 04 2016 20:31
Unfortunately not...
I just confirmed that the line is indeed commented...
Ian Gilman
@iangilman
Nov 04 2016 20:33
Anyway, I think my analysis still holds true, even if it's not caused by that patch. Basically we need an additional piece of information before deciding we're fully loaded, which is how many tiles are currently loading
John Hoffer
@thejohnhoffer
Nov 04 2016 20:34
I'm just trying to parse all this...
So fully-loaded currently means 'We have started loading all tiles we should.'
But it should be recoded to mean 'We have finished loading all tiles we should.'
Ian Gilman
@iangilman
Nov 04 2016 20:36
Correct
So around https://github.com/openseadragon/openseadragon/blob/master/src/tiledimage.js#L900 you could add something like tiledImage._tilesLoading = 0 And then at https://github.com/openseadragon/openseadragon/blob/master/src/tiledimage.js#L1179 add tiledImage._tilesLoading++
John Hoffer
@thejohnhoffer
Nov 04 2016 20:37
Ha! I had a feeling the else if line with only a cryptic shoutout to josh1093 was somehow involved...
Ian Gilman
@iangilman
Nov 04 2016 20:37
...and then https://github.com/openseadragon/openseadragon/blob/master/src/tiledimage.js#L996 could become } else if (tiledImage._tilesLoading === 0) {
Right? Obviously suspicious ;)
John Hoffer
@thejohnhoffer
Nov 04 2016 20:38
Okay- this is all sounding good. I'll make the changes on my working copy first before making a PR...
Will we want to initialize _tilesLoadingsomewhere near the creation of the tiledImage?
Ian Gilman
@iangilman
Nov 04 2016 20:39
Actually, 997 should change to this._setFullyLoaded(this._tilesLoading === 0); instead of changing 996
John Hoffer
@thejohnhoffer
Nov 04 2016 20:40
Snazzy
Ian Gilman
@iangilman
Nov 04 2016 20:40
We could initialize it at the top for completeness sake
Sorry you're finding all these issues, but it's wonderful to be getting to the bottom of them!
John Hoffer
@thejohnhoffer
Nov 04 2016 20:41
Yeah- it's a good feeling! Okay- this sounds like a new pull request... I'll make a new branch on my fork.
Ian Gilman
@iangilman
Nov 04 2016 20:46
Yes, seems worth keeping it separate :)
I'll make the pull request.
Ian Gilman
@iangilman
Nov 04 2016 21:26
Awesome!