Draw colored rectangle when the tile is not yet loaded

[philipgiuliani]

Here my first try to draw a colored placeholder rectangle when the tile is not yet loaded. This is currently not building because I re-defined the box, topLeft and size var. But its buildable as minbuild. I used the included collections demo to test it. Will test it soon in my other project.

There is a lot of duplication with the setClip method.

Tasks:

• Create first prototype
• Add option to define the color, and if its null (default), disable this feature
• Add possibility to pass a function as placeholderFillStyle to draw a gradient or image instead of a single color
• DRY it up (move rect calculation to drawer)
• Documentation & Refactoring
• Testing
  • Check if options are passed correctly
  • Test with a large demo

Closes #623

[iangilman]

What's the purpose of passing the context into the function?

I wonder if there are other things worth passing into the function. For instance, we could pass in the TiledImage (in which case this line would probably happen in tiledimage.js), in case someone used the same function for multiple TiledImages but still wanted to differentiate between them.

What do you think? Anything else?

[philipgiuliani]

I passed the context into the function to be able to draw a gradient or a image like this:

placeholderFillStyle: function(context) {
    var gradient = context.createLinearGradient(0, 0, context.canvas.width, 0);
    gradient.addColorStop(0,"black");
    gradient.addColorStop(1,"red");

    return gradient;
}

[iangilman]

Gotcha; I didn't realize the context was necessary for that. Good to have then.

[iangilman]

I still think it would be useful to pass the TiledImage into the callback. What do you think?

[iangilman]

This is a great start!

[philipgiuliani]

Thanks a lot, i will try to do the changes as soon as possible. Do you have a suggestion of the method name that returns the rect? It should go into the tiledImage or, and not into the drawer? (you said drawer in the issue)

[iangilman]

I'm thinking it would be Drawer.viewportToDrawerRectangle(). We'd probably also want a viewportToDrawerPoint(). The Drawer has a reference to the Viewport, so it should have everything it needs.

[philipgiuliani]

Could you look over the documentation? I know its very bad, but im not good at writing docs :(
I have changed everything else already.

[iangilman]

Doing this means the overall setting would override the setting passed directly into the function. It should be the other way around. You could swap options and the object literal and assign the result back to options, or go with something like:

if (options.placeholderFillStyle === undefined) {
  options.placeholderFillStyle = _this.placeholderFillStyle;
}

[philipgiuliani]

Oh damn... Thanks a lot, i had not tried this functionality yet... I was very confused when passing the options 😆

[iangilman]

All the docs look good :)

[philipgiuliani]

I hope I have understood correctly the proposals. The drawer is now saving and restoring the context in the drawPlaceholder method!

[philipgiuliani]

What could i do with the information of the tiledImage? Would you pass only the tiledImage instread of the context, or both? To style every tiledImage different you can pass the function directly in addTiledImage.

[iangilman]

What if I wanted to have a blue gradient for one of my tiledImages and a green gradient for another one? One solution would be to pass a different function for each when you first created them, but if you were relying on the single placeholderFillStyle that's attached to the viewer, you would need a way to distinguish between the different tiledImages. One way would be to pass the tiledImage into the fillStyle function. This serves a different purpose than the context, so you would pass them both.

Or we could say if you want different gradients for different tiledImages, you always need to pass different functions.

At any rate, I figure we should consider that scenario so we know what the proper way to handle it is.

[philipgiuliani]

Ok i understand what you mean, you want me to change it to function(context, tiledImage)? :) I dont know whats more important, how would you order it?

[iangilman]

How about tiledImage, context ?

[philipgiuliani]

Hi, its done! I moved the fillStyle to the tiledImage, seemed cleaner to me.

[iangilman]

Looks beautiful! Thank you.

I see on your checklist you still have some testing to do?

[philipgiuliani]

Oh forgot to check it, i already tested it in my project, but i havent deployed it yet, just offline. I will upgrade it tomorrow. Offline it worked great :)

[philipgiuliani]

One last thought... What you think about renaming drawPlaceholder to drawRectangle in the drawer? So the method could be used for other rectangles also.

[iangilman]

Yes, Drawer.drawRectangle sounds good.

[philipgiuliani]

Great, its done :)

[iangilman]

Lovely. Are we ready to merge it or do you have more testing to do?

[philipgiuliani]

No i think its ready :) Im currently at home and I have the viewer in the office. Tomorrow i will upgrade OpenSeadragon ;)

[iangilman]

Cool. I'm glad to have this feature in; thanks for tackling it. :-)

[philipgiuliani]

I had some time to try this out today and noticed that its very flickering when the tiles come.
Here a .gif where you can see it. You have any idea? The white rectangles are the tiledImages of course ;)

Recorded it with the Chrome Devtools Network speed option :)

[avandecreme]

I suspect that the opacity is varying.
In tile.js the opacity is set on the canvas but not restored:

openseadragon/src/tile.js

Line 261 in 0d05614

context.globalAlpha = this.opacity;

Could you try restoring it once the tile is drawn?

[philipgiuliani]

Yes this line seems to cause the issue. When commenting ontext.globalAlpha = this.opacity; out its not flickering anymore 👍 But of course the nice effect is also not working now. I will try your suggestion!

[iangilman]

Indeed...as each new tile comes in we fade it over top of the older tiles. However we only draw the placeholder color if no tile exists. We should continue to draw that color until the low res tile has completely faded in.

Another solution would be to always draw the placeholder underneath everything, but that would negatively affect performance.

So...the trick is to detect when we have our first tile but it hasn't fully faded in. One strategy would be to assume that if we have even one fully opaque tile that's enough, since we load the low res tiles first. If so, you could add a new flag, say _hasOpaqueTile, to TiledImage. Set it to false at the beginning of each draw, around here:

openseadragon/src/tiledimage.js

Line 583 in 0d05614

// Reset tile's internal drawn state

...and set it to true when you find an opaque tile, here:

openseadragon/src/tiledimage.js

Line 1027 in 0d05614

if ( opacity == 1 ) {

...and then test it instead of lastDrawn.length here:

openseadragon/src/tiledimage.js

Line 1173 in 0d05614

if ( tiledImage.placeholderFillStyle && lastDrawn.length === 0 ) {

[iangilman]

@philipgiuliani Have you had a chance to work on the flickering issue yet?