Fix the jobLimit of the imageLoader #544
Conversation
Currently jobsInProgress was not incremented after adding a job. So it has gone into the - range and was like unlimited jobs.
All files are downcased so far.
| @@ -94,14 +94,16 @@ ImageJob.prototype = { | |||
| * @memberof OpenSeadragon | |||
| * @classdesc Handles downloading of a set of images using asynchronous queue pattern. | |||
| * You generally won't have to interact with the ImageLoader directly. | |||
| * @param {Object} options - Options for this ImageLoader. | |||
There was a problem hiding this comment.
Since options is optional, you can put square brackets around it here in the docs.
|
Beautiful! Just a couple tweaks for the docs and it's good to go. |
|
Thanks a lot, im not good at writing docs! I just took some examples from your other classes. Im back in office on monday and will make the changes there. I had an idea, that i will try on monday. What do you think about changing the queuing system from FIFO (first in first out; currently) to LIFO (last in first out)? I think thats worth a try, so the latest tile when i zoom in for example should be the fastest. Because sometimes it happes that after a drag it queues all tiles and the position where i zoomed in took a long time to appear sharp. Have you already tried that out? |
|
Yeah, learning all the details of jsdocs takes a bit. Mostly it looked great! Thanks for making those last tweaks on Monday. We haven't tried LIFO, probably because we haven't really been using the jobLimit (as you can tell by the fact that the code was completely broken). I'm hesitant to change to LIFO, because we're pretty specific about what order we request tiles in, from most important to least. If you're sitting still, and use LIFO, the tiles are likely to come in in a less than ideal order. On the other hand, like you say, if you're zooming around, then the older tile requests might not be as relevant anymore. Perhaps rather than switching to LIFO, we should have some way to signal that we're on the move and that old tile requests should be discarded. |
|
I just fixed the documentation. Ok, so i won't even try it out. The idea with the event/signal to discard them sounds like a good idea. How would you define "old" requests? |
|
I think any request that's queued but not yet in process should be counted as "old" when the signal comes through. The draw loop will re-add anything that needs adding. I suppose another approach would be to have the TiledImage check to see if the loader queue is full, and then not queue anything up until there's space. That way you never end up with a backlog, and as you move, there will be room soon for the new requests. |
Fix the jobLimit of the imageLoader
|
Thanks for the docs changes! I've merged this now... you can start a new pull request for the additional loader work (if you're up for it). |
|
Thanks for merging, i will try to optimize the tile loading and if its working i will make a new PR. |
|
Excellent :-) |
I totally forgot about the issue #490 and fixed this now in the collections branch. I didn't want to fix it in master, since the structure is different (ImageLoader gets initialized in Drawer there) and so you would have to merge it.
Update
I just fixed another issue that has caused the imageLoaderLimit to not work, it wasn't incremented after starting a job.
Fixes #490