Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dasmoth opened this issue Oct 31, 2016 · 3 comments
Open

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

dasmoth opened this issue Oct 31, 2016 · 3 comments

Comments

@dasmoth
Copy link
Owner

dasmoth commented Oct 31, 2016

image

@dasmoth dasmoth reopened this Oct 31, 2016
@dasmoth
Copy link
Owner Author

dasmoth commented Oct 31, 2016

@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
Copy link
Contributor

chfi commented Nov 2, 2016

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
Copy link
Owner Author

dasmoth commented Nov 2, 2016

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 dasmoth closed this as completed in 2665363 Nov 4, 2016
@dasmoth dasmoth reopened this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants