Fix CanvasDrawer when viewport is flipped and a TiledImage is rotated #2511
Conversation
…. fix webgl drawer debug info when viewport is flipped.
|
Nice, so you have separated the logic for flip and rotation, the viewport really only needs to flipped once, which makes it much clearer now! I have tested most combinations of viewport & tiledImage rotation (+/- angles) they seem to be working well! |
src/webgldrawer.js
Outdated
| @@ -1067,63 +1071,85 @@ | |||
| this._clippingContext.restore(); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This is so minor I feel silly mentioning it, but you've got a stray tab here...
src/webgldrawer.js
Outdated
| /** | ||
| * Get the canvas center | ||
| * @private | ||
| * @param {Boolean} sketch If set to true return the center point of the sketch canvas |
There was a problem hiding this comment.
Fixed (jsdoc comment was copied over from CanvasDrawer)
This bug in CanvasDrawer that we're addressing here also happened to affect the debug mode of the WebGLDrawer, because that code was originally copied over from the former canvas implementation. It certainly isn't ideal to have to fix things in two places, but hopefully there will be fewer and fewer remaining issues that affect both drawers after we get the initial kinks ironed out. I thought about avoiding what is essentially duplicate code by making the WebGLDrawer call the CanvasDrawer prototype in order to draw the debug info. However, this has its own downside of coupling the drawer implementations more closely, and would mean that changes in one drawer could influence the other. A bigger refactoring of the debug message code into the base Drawer class might be the best approach... |
There was a problem hiding this comment.
Thanks for the fixes! And thanks for the thoughts... Sounds like we're on the right track. I agree it's good to keep some separation between the two. Probably the best strategy, like you say, is creating small composable functions that can be shared when appropriate, but otherwise leaving the logic separate.
This PR should fix #2477 and incorporates some of the changes proposed in #2510, which addressed some but not all of the problems with the canvas drawing pipeline.
Based on my testing using the drawer comparison demo (http://localhost:8000/test/demo/drawercomparison.html), the two viewers now appear to work equally well in the cases that were causing buggy behavior before. A side benefit is that the code is a bit cleaner and easier to understand now too (though it is still somewhat complicated).
While fixing this, I also updated the debug info drawing code in the WebGLDrawer to incorporate the same types of changes since that was also broken in the same way.