Fix coordinates conversion with rotation.

[avandecreme]

That ended up quite big.

This first commit should fix:

• coordinates conversion when there is rotation
• image clipping with rotation
• place holder with rotation but I did not test. What is the best way to test that?

Bugs to fix:

• flickering at certain angles. It seems that it has been introduced by my previous PR. I suspect some float arithmetic issue.
• tile edge smoothing does not work with rotation (the translation computations are off)

[iangilman]

I'm a fan of this change. :)

[avandecreme]

:)

About that method, is it ok to call it from tiledImage: https://github.com/openseadragon/openseadragon/pull/796/files#diff-80bee396a520b6fa750064722bc3f0ceR1335 even though it is "private"?
Or the underscore rather means "internal"?

[iangilman]

Yeah, I've been wresting with that myself. I think I'm feeling the underscore in the context of this library means internal to the library rather than private to this object. What do you think?

[avandecreme]

That works for me. At least for methods.
For properties, that can make it hard to debug if they can be modified from anywhere.

[iangilman]

Agreed.

[iangilman]

This is a big patch! Seems decent in theory...let me know when you've sorted those bugs out.

I think you should be able to test the placeholder by doing an ImageTileSource with a broken URL...that way the placeholder should stay up the whole time.

[avandecreme]

So I tested the place holder and it works fine.
The flickering is fixed.

The only remaining issue is the edge smoothing. I spent quite some time on it without coming up with a fix. I will look again but ideas are welcome :)

[iangilman]

By edge smoothing are you referring to #764? Perhaps @MindFreeze might have some ideas.

Meanwhile, I'm still thinking about function names. scalePointsFromPixels etc. seems pretty good, but it does ignore the fact that in addition to scaling it's also translating. I wonder if there is another option that addresses that. pointsFromPixelsNoRotate might be more accurate (if little less pretty). Anyway, @avandecreme, just mentioning it in case you've got any thoughts on the subject.

[avandecreme]

By edge smoothing are you referring to #764? Perhaps @MindFreeze might have some ideas.

Yes. There is a translation error when there is rotation and I am not sure of the best way to fix that.
To debug that, one can set debugMode: true and zoom in > 110%.
Note that this bug should be present on master as well (but might be easier to fix in this branch thanks to the new conversion methods).

Meanwhile, I'm still thinking about function names. scalePointsFromPixels etc. seems pretty good, but it does ignore the fact that in addition to scaling it's also translating.
I wonder if there is another option that addresses that. pointsFromPixelsNoRotate might be more accurate (if little less pretty). Anyway, @avandecreme, just mentioning it in case you've got any thoughts on the subject.

You are right, scale seems the best term for the delta functions but is not accurate for pointFromPixel.
pointsFromPixelsNoRotate is not pretty but I can't think of a better term either. I guess we should be consistent and name the delta versions with the same pattern.

[iangilman]

Another option would be to go with a flag, so you'd have pointsFromPixels(pt) and pointsFromPixels(pt, { noRotate: true }). That second param is where the immediately param goes; we could make it so you can pass a boolean there (for immediately) or an object (for flags like noRotate)...in the latter case, you should be able to add immediately as a property of the object if you want. Just another thought.

Sounds like the edge smoothing translation error with rotation was already part of @MindFreeze's original patch? Or has this patch made it worse? Either way, hopefully he can weigh in here.

[avandecreme]

Another option would be to go with a flag, so you'd have pointsFromPixels(pt) and pointsFromPixels(pt, { noRotate: true }). That second param is where the immediately param goes; we could make it so you can pass a boolean there (for immediately) or an object (for flags like noRotate)...in the latter case, you should be able to add immediately as a property of the object if you want. Just another thought.

I started by adding a flag but I later felt that the code would be cleaner with separate methods.
An other way to look at the no rotation methods is to consider them like coordinates conversion from browser coordinates to "OSD image ratio" coordinates. With rotation you go from browser to viewport coordinates.

Sounds like the edge smoothing translation error with rotation was already part of @MindFreeze's original patch? Or has this patch made it worse? Either way, hopefully he can weigh in here.

I believe it was already present.

[MindFreeze]

The problem with edge smoothing seems to be related to the scaling. I am looking into it. No luck yet.
But if the problem is there on master as well, you should merge this and create a separate issue for the smoothing thing.

[avandecreme]

I finally found the trick. See avandecreme@eaed2d5

[avandecreme]

@iangilman Any thought on that?

I started by adding a flag but I later felt that the code would be cleaner with separate methods.
An other way to look at the no rotation methods is to consider them like coordinates conversion from browser coordinates to "OSD image ratio" coordinates. With rotation you go from browser to viewport coordinates.

I think the solution I prefer is the NoRotate suffix right now. If that ok with you, I will do the renaming. I think we should then merge this PR and work on the smoothing issue in a new PR.

[iangilman]

Sure, I'm fine with the NoRotate suffix, and I'm down with merging this and doing smoothing in a new PR.

[avandecreme]

Done :)

[iangilman]

Lovely! Does this close any tickets?

[avandecreme]

Lovely! Does this close any tickets?

Actually not. But we should add that to the changelog though. Let me know if you want me to do it.

[iangilman]

I've added this PR to the changelog: 3c44666

[avandecreme]

Excellent 👍