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

DPI problem umbrella issue #587

Closed
icefoxen opened this issue Feb 18, 2019 · 22 comments

Comments

@icefoxen
Copy link
Contributor

commented Feb 18, 2019

Overview of issues:

Demonstration:

Mac-specific problems:

Issues reported due to DPI being incorrect (some SDL, some winit):

Maybe related? The Mac OS Weird Screen Size Problem:

@icefoxen icefoxen added this to the 0.5 milestone Feb 18, 2019

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@icefox Hmm, does retina work for ggez on macOS? 
The reason I ask is because I can't see where winit sets NSHighResolutionCapable - searching for that in the repo yields zero results.

Which makes me wonder if ggez has retina support on macOS since you use winit?
afaik NSHighResolutionCapable can't be set programatically
@seivan

This comment has been minimized.

Copy link

commented Feb 19, 2019

@icefoxen The only thing I'm not sure of is if NSHighResolutionCapable is a strict requirement but as of now, I can't get retina (scale factor 1.0) when letting SDL2 setup the window and the Metal Layer.

@icefoxen icefoxen pinned this issue Feb 19, 2019

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Found some pretty good references on OSX stuff: https://developer.apple.com/library/archive/documentation/GraphicsAnimation/Conceptual/HighResolutionOSX/APIs/APIs.html#//apple_ref/doc/uid/TP40012302-CH5-SW2 . Rummage around other things in that section for more info.

I like how they explain their reasoning on why you should never need to use the various tools they give you. It all makes perfect sense if a) pixels are actually invisible, b) it always works the same across every device (and OS), c) Retina displays never get bigger or smaller, and d) you never care about degenerate cases like aliasing or moire effects because e) OSX's Magic Drawing Hints fix everything for you behind the scenes. Unfortunately, the real world is seldom this tidy.

I'm not bitter, honest.

@hughlang

This comment has been minimized.

Copy link

commented Mar 6, 2019

Please see my latest comment on #553. I spent enough time with this to feel like I can help out. And I can test on mac, linux, and windows. Thanks.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

@hughlang I'd love some help, thanks! Sorry for the delay in getting back to you. This is definitely something I'd like help with, it's just tough 'cause I'm so frustrated with it I can't stand to try to deal with it more than once every three weeks apparently. :-P

@hughlang

This comment has been minimized.

Copy link

commented Mar 10, 2019

Thanks! I researched other projects that had a similar hidpi problem. I found this to be a little relevant:

https://github.com/PistonDevelopers/glutin_window/pull/149/files

It seems like they ditched hidpi checking in favor of the to_physical() method call. Maybe the nonsense of hidpi is now fixed upstream.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

It's implied to be fixed in the response to rust-windowing/winit#796 but I just haven't had the time to dig through and see how complete and comprehensive the fix actually is. If winit provides a nice way to actually give us real resolution numbers, we should use it everywhere necessary.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

#[cfg(mooman219)] Today at 8:40 PM
Why do you have a no-hidpi fork of winit
icefox Today at 8:40 PM
Mainly to see what would happen.

What happens is actually kinda interesting, so I'm going to write it down. On any platform, without a hidpi display (hidpi_factor = 1.0) it has no difference, as one would expect.

On Linux, with no active hidpi code, it works as intended and gives you things the size you ask them for without the hidpi scaling.

On OSX with a retina display, it gives you a window that is still scaled for hidpi but the existing graphics bug with scaling gets worse (#553), it draws everything at 1/4 size instead of 1/2.

On Windows, it mostly works but in the astroblasto example at least the text at the top of the screen is half cut off by the window title bar. So something's screwy there. (Actually that happens even with vanilla ggez, it seems. Sigh.)

icefoxen added a commit that referenced this issue Apr 4, 2019
Starting an experiment in removing hidpi.
Basically the idea is to undo all Winit's behavior in favor
of giving the programmer what they ask for, and see how this
goes.  It will probably have multiple parts:

 * Fixing window creation and sizing (done?)
 * Fixing mouse event translations (they use LogicalSize's)
 * Fixing resize events (oh gods) (oh they should get a
   HiDpiFactorChanged event I think)
 * Fixing whatever horror and nightmare this has inflicted upon
   graphics transforms and text sizing.

 See #587
@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Well I started messing around in 5aedef4 and scarily it doesn't seem like complete nonsense, yet. In fact on Linux it seems to work and work really well, though I'm sure there are some whack edge cases that still need handling, as described in that commit message, plus probably more. So... It might be the obvious way forward.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

The "hack around to force no hidpi scaling ever" branch seems to work better than I expected, though it's not flawless. Things that need fixed:

  • Conf::hidpi does nothing at the moment. It needs to be either removed or implemented. ...maybe just removed so we can get on with life in a universe in which 0.5 is stable, since it should be able to be re-added without a breaking API change.
  • HidpiChangeEvent is untested since I don't have a good way to test it at the moment
  • The MacOS Scaling Bug is still there; I should be able to hunt that unimpeded now.
@hughlang

This comment has been minimized.

Copy link

commented Apr 16, 2019

Thanks @icefoxen

@hughlang

This comment has been minimized.

Copy link

commented Apr 17, 2019

I researched the MacOS scaling bug a bunch this morning, since I need a solution for this. Here are my findings. TLDR, you already are on the right path with handling Winit's HiDpiFactorChanged event in process_event.

First, the subverted_size is incorrect for anything outside of DPI factor 1.0. It forces a logical size that is smaller and not changeable without resetting later. I think that part should be removed or locked at 1.0.

1.0 display:

image


The screen size is 1024x768 but it's blurry because it has been upscaled. 

subverted_size w=1024.0 h=768.0
Window created, desired size 1024x768, hidpi factor 1.
  Window logical outer size: 1024x790, logical drawable size: 1024x768
  Asked for   : OpenGl 3.2 Core, vsync: true
  Actually got: Driver vendor: Intel Inc., renderer Intel HD Graphics 4000 OpenGL Engine, version 4.1, INTEL-12.4.7, shading language 4.10
actual initial projection matrix w=1024.0 h=768.0
size_w=1024.0 size_h=790.0
draw_w=1024.0 draw_h=768.0
graphics::hidpi_factor: 1
src/graphics/context.rs resize_viewport
resize_event w=1024 h=768

2.0 display

image

subverted_size w=512.0 h=384.0
Window created, desired size 1024x768, hidpi factor 2.
  Window logical outer size: 512x406, logical drawable size: 512x384
  Asked for   : OpenGl 3.2 Core, vsync: true
  Actually got: Driver vendor: Intel Inc., renderer Intel HD Graphics 4000 OpenGL Engine, version 4.1, INTEL-12.4.7, shading language 4.10
actual initial projection matrix w=1024.0 h=768.0
size_w=1024.0 size_h=812.0
draw_w=1024.0 draw_h=768.0
graphics::hidpi_factor: 2
src/graphics/context.rs resize_viewport
resize_event w=1024 h=768

At the end of the graphics setup, you currently make some calls to set the projection rect/matrix. I assume this is also what you do after you receive the HiDpiFactorChanged event using the new physical size which is double the logical size for macos.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Oh, awesome, thank you so much for looking into this. Can you link your example code somewhere? I'll work on this tonight if I can. Basically on MacOS it looks like the conversion is happening twice for whatever reason, we're left with a window that is the right physical size but thinks it's smaller than it actually is. I don't get this behavior on Windows or Linux, is the weird thing.

@hughlang

This comment has been minimized.

Copy link

commented Apr 17, 2019

I think that was the old behavior when the conversion is happening twice. It depends on how you test it. I've seen some freaky things in how hard-coded floats used in testing have different behaviors than ones assigned to a var first.

For example, I was hacking around in src/graphics/context.rs and was messing with these lines:

        let hidpi_factor = 2.0 as f32;
        let subverted_size = glutin::dpi::LogicalSize::new(
            (window_mode.width / hidpi_factor) as f64,
            (window_mode.height / hidpi_factor) as f64,
        );

This behaves the way shown in the debug output above. However, you get a different result if you hard code numbers inside the formula. It goes through one or more size change alerts.

I can try to provide some test code, but mine is too dense to be useful. In the end, I think you just need to update the projection matrix. It will start treating the original 1024x768 as 2048x1536 internally when projecting pixels at it.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

This behaves the way shown in the debug output above. However, you get a different result if you hard code numbers inside the formula. It goes through one or more size change alerts.

Now that just doesn't make sense. Unless it somehow thinks that the size it's being told to become is not quite the same as the size the window is created with and it has to resize.

@hughlang

This comment has been minimized.

Copy link

commented Apr 17, 2019

I know, but I've also got screenshots and log output that shows that.
Anyway, it's not important. I think the real problem is solvable. It's all explained in the Winit dpi.rs documentation.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Wellllllll. Something WEIRD is happening.

image

That implies that something wrong is going on with what size the OpenGL context is being created. Might be a glutin issue then.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Can't reproduce using gfx_window_glutin, quite, though I didn't try too hard yet. Basically on mac with a hidpi factor of 2.0, ggez gets a request for an 800x600 window. It asks glutin for a 400x300 window and gets an 800x600 window that claims it is 400x300 pixels. However it also gets an OpenGL viewport that is 400x300 pixels, and says that it is 400x300 pixels.

This sort of nonsense happens even when I force hidpi_factor to be 1.0, so I think I'm just going slowly insane.

This has happened before, with me and others, but it's maddening as heck that it's only on mac for some reason: rust-windowing/glutin#287

This may be the culprit: https://docs.rs/glutin/0.20.0/x86_64-apple-darwin/glutin/struct.WindowedContext.html#method.resize

Adding the below to the bottom of resize_viewport() definitely changed things, but didn't exactly fix it:

        let p = self.window.get_inner_size().unwrap().to_physical(self.hidpi_factor as f64);
        self.window.resize(p);

The interactions with fullscreen mode is also frustrating.

@hughlang

This comment has been minimized.

Copy link

commented Apr 18, 2019

Thanks for the research. I will give that a try in the morning. I think the glutin links you post are vaguely similar to the DPI docs on Winit. https://docs.rs/winit/0.18.0/winit/dpi/index.html – Definitely interesting.

I'm eager to try changing the projection matrix as an experiment. I feel certain that the subverted size is going to give exactly what it says it will, and I want to try starting with 1.0 sizing and updating after DPI change event.

Starting with these lines in src/graphics/context.rs, what do you suggest as code to try out for handling DPI change?

        // Calculate and apply the actual initial projection matrix
        let w = window_mode.width;
        let h = window_mode.height;

        let rect = Rect {
            x: 0.0,
            y: 0.0,
            w,
            h,
        };
        gfx.set_projection_rect(rect);
        gfx.calculate_transform_matrix();
        gfx.update_globals()?;

I see a related method set_projection(&mut self, mat: Matrix4) and I see GraphicsContextGeneric has some projection matrix data.

Also, I didn't try 800x600 which was a source of problems for ggez/mac in the past, so I will test that as well.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Glutin is basically a thin wrapper around Winit these days anyway, hence why the docs look similar.

I messed a lot with the projection matrix, to little avail because, I believe, the OpenGL context wasn't getting resized correctly anyway. It looks like it's only covering 1/4 of the screen. Some sort of platform dependent stuff happening there is basically the only way I can explain this problem occuring on mac and not other platforms. The projection matrix itself will be fine if it's given the correct info.

I tried throwing WindowedContext::resize() calls in to Context::process_event() to call it on a resize event, but to no avail so far. That SEEMS like the right thing to do but I guess we'll have to fiddle with it more.

@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

HiDPI pees in my cheerios yet again. See https://github.com/ggez/ggez/blob/master/examples/eventloop.rs#L30 . See https://github.com/ggez/ggez/blob/devel/src/event.rs#L169-L170 . Consider what one has to do to make things not break when you try to combine these two.

@icefoxen icefoxen referenced this issue Apr 23, 2019
@icefoxen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

...what has to be done is use the event returned by Context::process_events, really.

icefoxen added a commit that referenced this issue Apr 23, 2019
Resolve #587.
I'm sick of this and want to just move on with life.  Any other
issue with hidpi anything is going to be closed as "dependency bug"
until winit eventually switches to physical coordinates by default,
which admittedly they're probably going to.

As a consequence, Apple platforms are no longer supported.
See https://drewdevault.com/2017/10/26/Fuck-you-nvidia.html but
search-and-replace NVidia with Apple.  It's the exact same sort
of problem and I'm done with it.

I'm sorry.

@icefoxen icefoxen closed this Apr 23, 2019

@icefoxen icefoxen unpinned this issue Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.