dalliance
dalliance copied to clipboard
Rendering on high-DPI screens is broken with new renderer
@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?
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...
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?