dalliance icon indicating copy to clipboard operation
dalliance copied to clipboard

Rendering on high-DPI screens is broken with new renderer

Open dasmoth opened this issue 7 years ago • 3 comments

image

dasmoth avatar Oct 31 '16 19:10 dasmoth

@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?

dasmoth avatar Oct 31 '16 20:10 dasmoth

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...

chfi avatar Nov 02 '16 15:11 chfi

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?

dasmoth avatar Nov 02 '16 20:11 dasmoth