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

Detect change of devicePixelRatio (hidpi scaling) on web target #1659

Conversation

alvinhochun
Copy link
Contributor

@alvinhochun alvinhochun commented Aug 18, 2020

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

The WASM web target did not handle change of the devicePixelRatio (changing the browser zoom level also causes it to change). This results in the canvas becoming wonky.

The change of devicePixelRatio can be detected using a MediaQueryList, so this is what I used. The tricky part is that a media query can only be used to detect whether a specific devicePixelRatio matches, which means a new MediaQueryList will be needed whenever the scaling has changed. I added a helper type DevicePixelRatioChangeDetector to manage this.

Outstanding issues:

  • Remove console.log debug messages
  • Properly handle the new_inner_size field in WindowEvent::ScaleFactorChanged
    I have no idea how it should be handled.
  • Fix issue where canvas become blurry at certain scales?
    It seems that whenever the CSS width/height in pixels is not an integer, the canvas becomes blurry in that dimension (e.g. when devicePixelRatio == 1.3333333333333333 and the physical width is 1067, the CSS width becomes 800.25 and the rendering is somehow not aligned to the physical pixels). I do wonder if it is more appropriate to handle this in a separate PR.
    Edit: I just realized that this is probably due to winit not firing WindowEvent::Resized events that could be causing some of the UI code to lay out things incorrectly. However I don't believe this is the only reason.

    Edit 2: False alarm, after adding WindowEvent::Resized it seems to work fine.
  • Also make the change for stdweb
    Would be nice if someone else can do it as I really don't know if I can make the change correctly for stdweb. Is there a possibility for this change to be implemented for web-sys only at first and stub out the stdweb side and fix it later in a separate PR?
  • Stubbed out for stdweb.

This change is incomplete but functionally working on web-sys. I would really like some comments and pointers.

@alvinhochun alvinhochun marked this pull request as draft August 18, 2020 15:38
@alvinhochun
Copy link
Contributor Author

alvinhochun commented Aug 18, 2020

I was reading the long thread in the WebGL repo and it seems that there is finally a way to get a 1-to-1 pixel-accurate canvas: KhronosGroup/WebGL#2460 (comment) (check the linked comment and several after). It relies on ResizeObseverer and if winit can use it then this PR will likely be unnecessary.

The downside is that this API is pretty new so not very widespread, and it also requires sizing based on a parent element.

@ryanisaacg
Copy link
Contributor

Feel free to stub out the stdweb side for now.

@alvinhochun
Copy link
Contributor Author

@ryanisaacg How would you suggest to handle the new_inner_size in WindowEvent::ScaleFactorChanged? Given that ControlFlow::Poll defers all events to the next requestAnimationFrame, it seems that the proper way would be to defer the handling until run_until_cleared, which would need quite a bit of refactoring of run_until_cleared and handle_event.

But then a problem arises. The canvas element automatically keeps its logical size in CSS pixels when the devicePixelRatio changes This means technically we have to send a WindowEvent::Resized event together with the ScaleFactorChanged event. In addition, we need to synchronously "fix" the canvas size so that it would properly snap to the device pixel grid and have a framebuffer with the correct size. If we allow the user to modify new_inner_size during requestAnimationFrame, then we potentially have to resize the canvas again.

An alternative is that we defer all the above until requestAnimationFrame. However, this means that any CursorMoved events generated after the change of devicePixelRatio and before actually sending ScaleFactorChanged will need to use the old scale factor for the PhysicalPosition to stay consistent with the event sequence.

Still, if a program relies on window.inner_size() and window.scale_factor() to process the events in the event handler, it may interpret the CursorMoved event incorrectly for some of the events. So these getters should also be synchronized with the event sequence.

@ryanisaacg
Copy link
Contributor

Let me know if this doesn't work, it's been a little while since I had to dig around in the event handling internals:

  1. We notice the devicePixelRatio has changed
  2. We send all events in queue to the user, to make sure that any old events use the correct physical size
  3. We dispatch a ScaleFactorChanged event to the user, with scale_factor * logical_size to calculate new_inner_size
  4. Based on the value of new_innner_size, we have a new physical size. Set the canvas framebuffer size to that value. Calculate the new logical size based on this number divided by scale_factor.
  5. Dispatch a Resized event with the new physical size

This probably will require some custom logic in the event handling internals, because it is the only event that passes a mutable value and reacts based on the result. Additionally, for the reasons you mentioned, it's probably best to not wait for the control flow to pick up the method (because we don't want this sitting in a queue and the newer events getting stale.) This means that you'll want a custom event-scheduling method just for scale factor changes, which may be nontrivial work.

@alvinhochun
Copy link
Contributor Author

@ryanisaacg thanks for the suggestion. I think this should be doable, but as the docs for ControlFlow::Poll states:

For web, events are sent when requestAnimationFrame fires.

Sending all events when devicePixelRatio has changed would mean this is no longer accurate in all cases. Is it fine for me to just modify it to describe the new behaviour?

@ryanisaacg
Copy link
Contributor

ryanisaacg commented Aug 27, 2020

Yup, in this case the docs are descriptive and not prescriptive.

@alvinhochun
Copy link
Contributor Author

I decided to create a new PR #1690 since it has become quite different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants