These are chat archives for openseadragon/openseadragon

23rd
Sep 2015
Grant Echols
@gechols
Sep 23 2015 17:39
@iangilman I have a pretty simple app now that demonstrates the add/remove/add timing issue. How can I best make it available? Can I submit a PR with the code in the test/demo directory?
Ian Gilman
@iangilman
Sep 23 2015 17:49
@gechols How about putting it in a CodePen and linking to it from an issue?
Have you narrowed down what the issue is?
Grant Echols
@gechols
Sep 23 2015 17:52
When I add a tiled image, then call removeAll and then try adding another one (in a unit test at this point, but it also mimics some actual behavior) I end up with two images in the final result.
Ian Gilman
@iangilman
Sep 23 2015 17:54
Sure, but did setTimeout of 1ms fix it? Is there an event you can watch for that'll get you past the timing issue?
Grant Echols
@gechols
Sep 23 2015 17:56
If I add a timeout just before calling removeAll it seems to work but I can't tell if its just a timing issue or if the code really hasn't had a breath to complete its work. We load our images from the network so its very possible that there are still images loading when we need to switch the model (set of images) using removeAll. Since I don't know what's causing the problem I'm not sure what event(s) to listen to.
I'll try to get a CodePen put together and see if that helps make sense of things
Ian Gilman
@iangilman
Sep 23 2015 17:57
Sounds good...I was just curious if you'd learned anything new along the way :)
I'm guessing you're right...if there are still images loading while you do removeAll, they would show up. Perhaps we need some way to cancel in flight loads. Looks like that was discussed here: openseadragon/openseadragon#629
Grant Echols
@gechols
Sep 23 2015 18:02
Is there a convenient location where the latest build of the openseadragon.js file is available for use in the CodePen?
Ian Gilman
@iangilman
Sep 23 2015 18:03
We still need to get around to sticking it on CDN, but meanwhile it's at http://openseadragon.github.io/openseadragon/openseadragon.min.js
Btw, I was just talking with @aeschylus the other day about canceling in-flight loads. The workaround I came up with was to have a success function you pass into addTiledImage that would then check to see if you had done a reset since the load started and then remove the image. Of course having an explicit mechanism would be even better.
Grant Echols
@gechols
Sep 23 2015 18:06
Does this CodePen work for you? http://codepen.io/anon/pen/JYRWPO
Just hit the Add/Remove/Add button to see the problem. Zoom out and its got two images instead of just one.
Ian Gilman
@iangilman
Sep 23 2015 18:10
Yup, beautiful. And yes, looking at this, I think that's exactly what's going on...the add isn't finished until after the remove.
So the fix to OSD would be to add something like viewer.cancelAdds() which you could call at the same time as viewer.world.removeAll(). I suppose it could even be an option to world.removeAll...
Grant Echols
@gechols
Sep 23 2015 18:13
Is there a queue of adds? Is that what the 'Job' structure is?
Ian Gilman
@iangilman
Sep 23 2015 18:16
There is (though the "job" you're talking about is probably the tile loading mechanism); Viewer has a private array called _loadQueue. cancelAdds() (or whatever it should be called) would just need to empty out that array, I think.
Grant Echols
@gechols
Sep 23 2015 19:43
I'm looking at openseadragon._loadQueue and I come across the 'raiseAddItemFailed' function where the for loop has a strange construct (or a missing .length?) - for (var i = 0; i < _this._loadQueue; i++) { // should this read i < _this._loadQueue.length ?
Ian Gilman
@iangilman
Sep 23 2015 20:01
@gechols Ouch! Yes...good catch!
Are you working on a fix for your issue? If so, can you include the fix to that for loop in the same PR?
Grant Echols
@gechols
Sep 23 2015 20:39
Yup. So I'm looking at just setting _loadQueue = []; I assume an image load could complete after that happens which falls into the success handler of the getTileSourceImplementation call. Currently that handler assumes that the first entry is the matching entry and removes it from the _loadQueue. Isn't that risky given that it may now have a new list? If so I believe I should check to make sure the entries match before doing the _loadQueue.splice(0, 1), right?
It's fine...it's just pulling everything off of the queue that it can
That's how we make sure that things end up getting added in the sequence they were called (even if one ajax call comes back slower than the others)
So each time we get a success, we start at the top of the queue and add everybody that's ready to go until we find one that's not, and then we wait until the next success.
Grant Echols
@gechols
Sep 23 2015 20:45
Yes. That's my concern. We don't bother to check to see if the tileSource we successfully loaded is still in the _loadQueue so we go and add it to the world. That's what is currently happening - I've added a new image after removing everything from the world and then an old one gets added
I think we need to check the tileSource against the _loadQueue[0].tileSource BEFORE adding it. If it doesn't match then we just ignore the callback and wait for the next one to happen
Ian Gilman
@iangilman
Sep 23 2015 20:46
No, nothing gets loaded unless it's in the queue...
Grant Echols
@gechols
Sep 23 2015 20:47
The image WAS in the queue, but now I'm going to empty the _loadQueue and clear the world items
Ian Gilman
@iangilman
Sep 23 2015 20:48
Yup, and that should take care of it, because nothing gets added to the world unless it's in the queue at the time the tile source has successfully loaded.
Grant Echols
@gechols
Sep 23 2015 20:48
Where is that check made?
Ian Gilman
@iangilman
Sep 23 2015 20:48
This is the only time a tiled image gets added to the world: https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L1331
i.e. if it's not in the queue it doesn't get into the world
Grant Echols
@gechols
Sep 23 2015 20:49
There will be a NEW image in the queue - but not one that matches the one that just completed
Make sense?
Ian Gilman
@iangilman
Sep 23 2015 20:50
Sure, and if its tile source hasn't come back successfully yet, execution stops at https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L1290
...and will resume when it does come back with success
Perhaps the confusion is that the image request here https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L1284 is not the same as the image request here https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L1304-L1329 even though they're in the same function?
I'm sorry if my code is obtuse :(
Grant Echols
@gechols
Sep 23 2015 20:56
OK...just got it. The myQueueItem is in the 'app thread' so to speak and gets assigned a tileSource in the top of the successCallback. If that queueItem isn't the one at the head of the _loadQueue then it won't have the tileSource set and we will skip the add to the world. So that subtle difference means that if I just wipe out the _loadQueue at the same time I removeAll from the world then we will ignore the 'in flight' result of an earlier image loading...Sorry I'm slow to get this, but now I see what you've done. Thanks. I think I can safely make the changes now.
Ian Gilman
@iangilman
Sep 23 2015 20:58
Cool. Thanks for bearing with me! :)
Grant Echols
@gechols
Sep 23 2015 20:59
I'd like to add a boolean to the world.removeAll(cancelPending) and have the world call its viewer to cancelPendingImages - sound good?
The boolean would be optional and default to the current behavior
Ian Gilman
@iangilman
Sep 23 2015 21:00
I like it, but I actually think we should consider the current behavior a bug, so let's make the default the new behavior
Grant Echols
@gechols
Sep 23 2015 21:01
Sounds fine to me.
Ian Gilman
@iangilman
Sep 23 2015 21:01
cancelPendingImages sounds like a good name btw, better than my idea
Grant Echols
@gechols
Sep 23 2015 21:01
Do you want a public method in the viewer to do the same 'cancelPending'?
Ian Gilman
@iangilman
Sep 23 2015 21:03
Maybe we should keep the Viewer method private for now (_cancelPendingImages) until we decide we might need it outside of this context.
...and in fact let's not even have a flag on removeAll
Grant Echols
@gechols
Sep 23 2015 21:03
OK
Ian Gilman
@iangilman
Sep 23 2015 21:05
And I think #629 will be fixed automatically by this, because open calls close which calls world.removeAll :)
Thank you for digging into this!
Grant Echols
@gechols
Sep 23 2015 21:06
Awesome! Glad to be of help.
Grant Echols
@gechols
Sep 23 2015 21:26
openseadragon/openseadragon#734
Ian Gilman
@iangilman
Sep 23 2015 21:26
Awesome :)
And does it fix your issue?
Grant Echols
@gechols
Sep 23 2015 21:26
Yup
Tried it against my test code as well
Ian Gilman
@iangilman
Sep 23 2015 21:27
Excellent :)