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

Update to glutin 0.17 #3

Merged
merged 3 commits into from Jul 12, 2018
Merged

Conversation

IvoWingelaar
Copy link
Contributor

It doesn't compile without warnings, because of changes in the virtual
keycodes.

LMenu maps to "AltLeft" in src/native_keycode.rs, but this isn't defined by glutin 0.17. I assume the fix is simply mapping LAlt to "AltLeft" (ditto for RMenu) and adding the three missing patterns Copy, Cut, and Paste?

I would have already done this, but we currently map LAlt to "", so I'm quite confused as to the purpose of LMenu and LAlt.

It doesn't compile without warnings, because of changes in the virtual
keycodes.
@edwin0cheng
Copy link
Member

  1. For the "AltLeft" key issue, are you talking about the virtual key code or scan code? For virtual key code, it is safe to remove the LMenu/RMenu if glutin is not supported that.
  2. Would you mind to add a boolean in AppConfig and use that to control the behavior of set_resizeable ?

@IvoWingelaar
Copy link
Contributor Author

  1. The virtual key code. More specifically in translate_virtual_key(..) -> &'static str. I've followed the instructions in the changelog of winit that describes that I should use {L,R}Alt instead.
  2. I will do that.

I'm having some trouble getting the right viewport sizes in the examples of unrust. Tomorrow I'll look further into it: I'm quite sure it's related to the DPI scaling not happening properly.

@jice-nospam
Copy link
Collaborator

Works fine on linux. Looks good to me

@IvoWingelaar
Copy link
Contributor Author

The examples in unrust now work with the code in this branch. I've set the default for the resizable flag in AppConfig::new to true to preserve previous behavior.

If you are using a screen with a higher DPI factor you should notice that native windows and GUI text are now scaled properly.

Do we want this last behavior to also be controllable with a bool in AppConfig?

@IvoWingelaar
Copy link
Contributor Author

@jice-nospam Does the DPI scaling not affect the visual quality of native graphics in doryen-rs?

@jice-nospam
Copy link
Collaborator

@IvoWingelaar I don't see any difference but I'm using a Linux VM. I'll double check on native Windows asap (also potential performance impact)

@jice-nospam
Copy link
Collaborator

everything's ok on windows, and no impact on perfs.

@jice-nospam
Copy link
Collaborator

@edwin0cheng shall we merge ?

@edwin0cheng
Copy link
Member

edwin0cheng commented Jul 11, 2018

Do we want this last behavior to also be controllable with a bool in AppConfig?

Sure

And anything else looks good to me.
@IvoWingelaar Do you want me to merge now or wait for the higher DPI factor flag first ?

@IvoWingelaar
Copy link
Contributor Author

The current code should work correctly, and can be merged. Properly implementing DPI scaling (and making it configurable) is kinda tricky, as explained here. Currently I simply scale all Logical* values to their Physical* variants with the DPI factor such that all consumers can directly use the values in OpenGL functions (like `, but this seems a little hacky.

Proper DPI support would require exposing only Logical* values with the DPI factor and let the consumer choose what to do with them. I would like to hear your thoughts on this before I start rewriting code, as it would imply an API change.

@jice-nospam
Copy link
Collaborator

Failure to account for the DPI factor can create a badly degraded user experience. Most notably, it can make users feel like they have bad eyesight, which will potentially cause them to think about growing elderly, resulting in them entering an existential panic.

Priceless !

As a 3D game engine, I don't think unrust should expose this to users. The game engine should use 3D (opengl) coordinates in the range -1...1 and your image scales to fill the game window (which in most cases should cover the whole screen). Your UI should do the same and thus, be HiDPI insensitive (and even screen-resolution insensitive).
Am I missing something ?

@IvoWingelaar
Copy link
Contributor Author

@jice-nospam That works in 95% of the cases, but I've previously had some bad experiences with software not being aware of HiDPI and drawing UI elements way too small (like, unreadable). So I kinda agree with glutin's docs here.

When drawing stuff that needs to be pixel perfect (like doryen-rs, and coincidentally a related side project of mine), this is actually a big problem: my screen has a DPI of 1.66, so asking a window with a size of 800x600 gives me a window with a size of 1280x960, roughly speaking. This "stretches" some pixels and as such those squares become rectangles.

That's why I brought up the flag to disable this behavior, as there are legitimate use cases for it to be optional. Disabling this behavior is not possible in glutin 0.17, but I believe it to be somewhat trivially implemented in uni-app: I only need to take DPI in consideration when creating the window, and when I get a resize command, or a DPI change event.

But as @edwin0cheng has actually written code that tries to take DPI into consideration for the UI in unrust, I think we should properly support this. My suggestion is the following:

  1. Mouse coordinates and Window sizes should be returned and stored as Logical* values. These are DPI, and DPI-change insensitive.
  2. If using for example glViewport, you simply multiply these values with a hidpi factor.
  3. When drawing things that need to be pixel-perfect, either disable the DPI flag I proposed, or divide the coordinates/locations by the hidpi factor.

This should cover almost everything, I believe. When first reading the docs of glutin I was less than enthusiastic about this (because I imagined it to be a larger problem), but I've warmed up to it now: changes to unrust, doryen-rs, my own project, and uni-app will be minimal.

If given the green light, I can try and implement this.

@jice-nospam
Copy link
Collaborator

I suggest we merge this glutin-0.17 upgrade as-is (just add a flag to AppConfig to disable hidpi scaling), then create another PR for adding hidpi factor in the API.

@edwin0cheng
Copy link
Member

@IvoWingelaar, @jice-nospam Indeed, I would prefer to discuss all these changes in issue page first and make it easy to track.

On the other hand, Here is my two cents about DPI related issues:

Mouse coordinates and Window sizes should be returned and stored as Logical* values. These are DPI, and DPI-change insensitive.
If using for example glViewport, you simply multiply these values with a hidpi factor.

I think it is okay in general, but how to handle multiple-monitor situation ? Of course currently unrust did not handle it, but for Native values we do not require to handle it (except UI).

Or, How about we embed the DPI information in all these Logical values (e.g. a tuple / struct (x,y, w,h,dpi), or an enum which provide the information and conversion ) ?

When drawing things that need to be pixel-perfect, either disable the DPI flag I proposed, or divide the coordinates/locations by the hidpi factor.

This one is a little bit tricky. I would recommend us to provide a logical to native conversion utility, or as above mentioned, use an Enum to represent all DPI related information.

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

Successfully merging this pull request may close these issues.

None yet

3 participants