dasmoth / dalliance

Interactive web-based genome browser.
http://www.biodalliance.org/
BSD 2-Clause "Simplified" License
226 stars 68 forks source link

Rendering on high-DPI screens is broken with new renderer #204

Open dasmoth opened 7 years ago

dasmoth commented 7 years ago

image

dasmoth commented 7 years ago

@chfi small bug in the new renderer.

A quirk in the canvas system is that transformations persist across redraws. So if you call context.scale(2,2) repeatedly, you double the size of things every time you re-render. Biodalliance gets around by wrapping the whole render operation in a context.save()/context.restore() pair, and the restore() call had got lost somewhere in the refactor.

The patch I've just applied (a98690e) appears to fix this, but I'd appreciate if you could take a look over this as well to confirm that I'm not doing something that breaks the "new-renderer" patterns. I'm not 100% happy with it because the save call is in one method and the restore call is in another -- IMO it's much clearer if you can keep saves and restores paired up in one method -- but separating out the logic in prepareViewport makes that a bit tricky. Any thoughts?

chfi commented 7 years ago

I think that looks fine, but I agree that it'd better to keep saves and restores matched in methods. It'd be nice if there was a single point where the canvas was scaled for retina displays, though I'm not sure what the best place to do that would be...

dasmoth commented 7 years ago

One possibility might be replace prepareViewport with function that takes a callback to invoke with the prepared canvas context, e.g.:

     withViewport(
             tier, canvas, retina, true, vOffset,
             paint.bind(null, tier, vOffset)
     );

The hypothetical withViewport function ends up nearly the same as prepareViewport with a couple of extra lines at the end to invoke the painting callback then canvas.restore().

@chfi: Does this still fit with your model of extensible renderers? It looks to me as though it should. What do you think?