Implement per-image flipping

[ali1234]

This works by re-arranging the tiles within the image, and then rendering each tile flipped horizontally.

To use, set "flipped" on the image instead of the viewer.

Changing the flip state after loading is not yet supported. It will require rebuilding the tile bounds.

Implements #1553

[ali1234]

This is now working well enough for some review. Note that there is a stealth bug fix in there - viewport clipping now works with wrapped images. This is because of the new getTileBounds wrapper which is aware of wrapping and tiling.

Open questions:

• Where should the getTileBounds wrapper live? Public or private? Unbound or member?
• Should getFlip() even exist? Should there be a setFlip() too?
• Does the 1px fudge for hiding seams when wrapping still work?
• Documentation?

[iangilman]

Fantastic! Thank you for taking this on, @ali1234! My apologies for not reviewing it sooner... 2020 did a number on my schedule.

The code looks good to me. Some thoughts:

• I think getTileBounds should be a private member (until such time as we discover we want to add it to the API officially). We can do that by adding a _ prefix to its name and adding @private to its doc comment.
• I think getFlip is good and yes, we should have a setFlip, so we can change it while the app is running.
• The documentation you've already added looks good. What additional documentation were you thinking of?

[ali1234]

I was thinking more about the website high level docs and examples, rather than the autodocs (which I don't really know if I've done right, not a JS dev here.)

Will have a look at the other stuff at the weekend.

[iangilman]

I was thinking more about the website high level docs and examples, rather than the autodocs (which I don't really know if I've done right, not a JS dev here.)

Well, having a page for flip would certainly be great! We have one for rotation (http://openseadragon.github.io/examples/ui-rotation/) but not for flip. If you're up for tackling that, it's in another repository: https://github.com/openseadragon/site-build/tree/master/www.

The JS docs you did look proper to me!

Will have a look at the other stuff at the weekend.

Great! I'll get back to you sooner than I did last time!

BTW, just a note here: I tried this patch and tested flipping the image by itself, flipping the entire viewport along with the image, and rotating the viewport along with the image, and everything worked properly :)

[ali1234]

Well I know for a fact there is at least one edge case when you flip both the image and the viewport, see

https://al.zerostem.io/~al/dzi/

By default you have an "xray" view through the top layer so you can see the bottom layer (which is flipped so it lines up with the top). If you click "bottom", now the bottom layer is normally shown and the top layer is the "xray". Now click "flip" to flip the viewport (so writing on the bottom layer is not reversed), and pan around. It all works except now the xray view moves opposite to the panning direction.

This could be my code, but I couldn't figure out a nice way to fix it. I think it happens in some of the examples too, if you modify them slightly to use per-image flipping.

[iangilman]

Hmm... looks like just hitting the flip button is enough to get that to happen (you don't need to click "bottom" first).

One thing we do sometimes with major features like this is make a new page in the test/demo folder that shows off the various possibilities and provides a good test bed for situations like this. It might be worth creating such a page for "flip" and including a simplified version of this scenario, for easier debugging (and to be able to keep an eye on regressions). That would make it easier for other people to take a look at figuring out the issue as well. Anyway, thank you for spotting this issue... definitely worth fixing!

[ali1234]

Rebased and added an example.

The problem with images panning the wrong way actually seems to be #1900 which I forgot I reported. You can see it in the example by toggling rotate on either image while viewport flip is on. It seems to happen without the changes I've made in this pull request.

[ali1234]

Regarding getTileBounds: There is already a public getTileBounds method on TiledSource, the new one in TiledImage is a wrapper around it to support flipping (which the source is not aware of). It gets called in unbound methods (eg updateLevel) that previously called the public TiledSource version directly, so I think the new one needs to be public too? Otherwise a huge amount of refactoring will be required. Not sure how to proceed.

[ali1234]

Above setFlip implementation is incomplete. It needs to recalculate the bounds of every tile, as flipping an image rearranges the tiles. It does not do that, so each individual tile flips, but stays in the same position.

[ali1234]

So I found _raiseBoundsChange but this doesn't seem to be enough to make it recalculate the tile bounds.

[ali1234]

Question: do I need to do anything special when deleting a cached tile from tilesMatrix?

[ali1234]

Perhaps a better way to explain what that last change does:

Previously getTile() was like this:

if tile does not exist in tilesMatrix:
    create a new tile
    insert tile in tilesMatrix

return tile from tilesMatrix

and I have changed it to:

if tile not in tilesMatrix OR tile is in tilesMatrix but it has the wrong flip:
    create a new tile
    insert tile in tilesMatrix *possibly replacing an old one*

return tile from tilesMatrix

This makes individual tile bounds get recalculated when flip changes, so it makes setFlip() work. Is this a good approach? Do I need to do anything more to clean up the old, replaced tiles?

[iangilman]

Rebased and added an example.

Thank you for adding the example! It really helps.

Rebasing, though, makes it harder to continue the code review from where I last left off; instead I have to reread all of the code again. I know some projects prefer rebasing, but I like the continuity of being able to see the full change history. Not a big deal, just letting you know :)

One minor thing on the example... if you load the images when you create the viewer (rather than using addTiledImage afterwards), it'll properly position both images in the viewport (rather than starting out a little zoomed in). Something like this:

var viewer = OpenSeadragon({
    // debugMode: true,
    id: "contentDiv",
    prefixUrl: "../../build/openseadragon/images/",
    showNavigator: true,
    tileSources: [
        {
            tileSource: "../data/testpattern.dzi",
            x: 0,
            y: 0,
        }, {
            tileSource: "../data/testpattern.dzi",
            x: 1,
            y: 0,
            flipped: true,
        }
    ]
});

The problem with images panning the wrong way actually seems to be #1900 which I forgot I reported. You can see it in the example by toggling rotate on either image while viewport flip is on. It seems to happen without the changes I've made in this pull request.

I see! I've confirmed that this patch does not introduce the issue, so we don't need to hold up this patch on a fix for that issue (though it would of course be lovely to fix at some point).

Regarding getTileBounds: There is already a public getTileBounds method on TiledSource, the new one in TiledImage is a wrapper around it to support flipping (which the source is not aware of). It gets called in unbound methods (eg updateLevel) that previously called the public TiledSource version directly, so I think the new one needs to be public too? Otherwise a huge amount of refactoring will be required. Not sure how to proceed.

I'm not suggesting that it be literally private in a code sense, so the unbound method can continue to call it. I'm just saying we don't necessarily want to indicate to people using the library that it's something for them to use. Prefixing the method name with _ and adding @private accomplishes that signaling without hindering our ability to call it from the unbound methods. Unless you think there is a reason for users of the library to call it?

This makes individual tile bounds get recalculated when flip changes, so it makes setFlip() work. Is this a good approach? Do I need to do anything more to clean up the old, replaced tiles?

Very clever! That seems like a fine solution. I can't think of anything else that would need to be done. When I tested it in the demo, it responded quite nicely. I suppose it would be interesting to know how it looks with a lot of tiles over a slower connection. That said, it sure looks like it just uses the tiles that are already in memory.

So, it looks like this patch is probably ready to land! Is there anything else you think it needs? I suppose it would be nice to have some unit tests for this new functionality, but only if you're up for it... I wouldn't consider it a requirement.

Thank you for making this happen!

[ali1234]

As far as I can tell tilesMatrix only caches the metadata of a particular tile instance in a tiledImage. The actual image file is cached in tileSource using a different mechanism.

For getTileBounds - if anyone is already using the existing public version in tiledSource then they will need to switch to using the new one in tiledImage, otherwise they will get the wrong answer for flipped images. I can't really think of a reason why anyone would be using it though.

[iangilman]

As far as I can tell tilesMatrix only caches the metadata of a particular tile instance in a tiledImage. The actual image file is cached in tileSource using a different mechanism.

I think this is good... if you flip an image, you'll end up with twice the tile records (the flipped version and the non-flipped version), but they will all share images at the tileCache level. I could be wrong, but I think that's a reasonable way to work it.

For getTileBounds - if anyone is already using the existing public version in tiledSource then they will need to switch to using the new one in tiledImage, otherwise they will get the wrong answer for flipped images. I can't really think of a reason why anyone would be using it though.

Yeah, makes sense. Let's stick with it being public, then.

Your recent changes look good. What say you, shall we land this?

[ali1234]

I'm happy for this to be merged.

[ali1234]

Just found a bug. setFlip() does not update the navigator.

[iangilman]

Good catch!

OK, I'll merge this soon. Thank you for all the hard work on this!