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

Fixed it so that the drawing when hidpi is correct (OSX) #212

Conversation

sumibi-yakitori
Copy link
Contributor

@sumibi-yakitori sumibi-yakitori commented Nov 25, 2020

With this PR, rendering in HiDPI mode on MacOS is rendered correctly in high definition and pixels blur-free.

I briefly tested this fix in Windows as well, and there seemed to be no problem.

@17cupsofcoffee
Copy link
Owner

Thank you for the PR!

Proper high DPI support is something I'd definitely like to add in the future (although I might make it optional, as I don't have access to a high DPI screen myself and don't want to force people to use something I can't test very well).

I'm a little uneasy about the workaround for resizes, however - I'd rather solve the issue properly to avoid creating more issues down the line.

Questions:

  1. What behaviour do you see without the workaround? If you could include a screenshot, that would be good.
  2. What version of OSX and SDL2 are you using? If you're on Catalina or higher, make sure you have SDL 2.0.12 or higher, as there's a DPI bug in older versions which ended up being the root cause of Rendering on MacBook Pro only covers quarter of the screen #147.
  3. If the issue isn't caused by an old version of SDL2, does using this code instead of your workaround fix the issue? I think passing the window size (rather than the drawable size) to graphics::update_window_projection is incorrect in high-DPI mode.
WindowEvent::SizeChanged(width, height) => {
    ctx.window.window_width = width;
    ctx.window.window_height = height;

    let (pixel_width, pixel_height) = ctx.window.sdl_window.drawable_size();

    graphics::update_window_projection(
        ctx,
        pixel_width as i32,
        pixel_height as i32,
    );

    state.event(ctx, Event::Resized { width, height })?;
}

@17cupsofcoffee 17cupsofcoffee added Area: Graphics Issues related to graphics/rendering. Area: High DPI Support Issues relating to high DPI rendering. The bane of every engine dev's life. Area: Platform Issues relating to the platform layer Help Wanted Issues that the maintainers would like help with resolving Platform Specific: macOS Issues that are specific to systems running macOS. Type: Feature Request Improvements that could be made to the code/documentation. labels Nov 25, 2020
@sumibi-yakitori
Copy link
Contributor Author

sumibi-yakitori commented Nov 26, 2020

Thanks for the detailed advice.
I agree with you about making HiDPI mode an option.

Answers:


  1. This is what happens. In my actual application it looks like this

    Screenshot 1
  2. macOS Mojave, sdl2 2.0.12_1
  3. Screenshot 2

With the fix you presented, it was drawn correctly.
However, after resizing the window, MouseMoved { delta } is half of the correct value originally expected.

In Screenshot 1 as well, MouseMoved { delta } is half the value after the window is resized, but if you move the window, it becomes normal.

@sumibi-yakitori
Copy link
Contributor Author

sumibi-yakitori commented Nov 26, 2020

I noticed the anomaly inMouseMoved { delta } right away, because what I was making now was an image viewer, but there may be other values returned by SDL2 that will change.

And even if you multiply that anomalous value by something like dpi scale, it won't be able to deal with the situation both before and after the window resizes.

@17cupsofcoffee
Copy link
Owner

That's very weird 🤔 The delta for mouse events comes straight from SDL, so that sounds like it could be an SDL bug. Do you get the correct values for the position in the event, or is that halved as well?

@sumibi-yakitori
Copy link
Contributor Author

sumibi-yakitori commented Nov 26, 2020

position seems to return the correct value.

In Screenshot 2, after the window resizes, the drawing area is correct, but the texture being drawn is 1/4 the size.

Screenshot 1 also has this problem, If this is the case, it can be cured by moving the window.
In the case of Screenshot 2 moving the window will not fix it.

Earlier I had misidentified the problem because I hadn't noticed it.

So it's not that there is something wrong with MouseMoved { delta }, but rather that the drawing scale(coordinate space) seems to be halved after the window resizes.

@17cupsofcoffee
Copy link
Owner

Ah, I think I understand why all of this is happening now 😄

  • At startup, the projection matrix is set to match the window size. The GL viewport is left as the default, which I believe SDL sets to the drawable size.
  • Without any workaround/fix, on resize, both the projection matrix and the viewport are set to the window size. This leads to the 'bottom-left corner' issue you're seeing in screenshot 1, as the viewport is meant to match the drawable size.
    • I'm guessing that moving the window resets the GL viewport to the drawable size, and this is why your workaround works, but I'm not 100% sure!
  • With my code changes, on resize, both the projection matrix and the viewport are set to the drawable size. This allows the viewport to access the whole screen, but it effectively halves the size that things are being drawn, since the matrix has changed!

So I think the solution to get rendering to look the same in both high DPI and low DPI mode would be to always match the startup behaviour - i.e. set the projection matrix to the window size, and the viewport to the drawable size, effectively scaling everything to match the target DPI. I like this approach, as it should mean that existing code will 'just work' in high DPI mode.

I've been looking at Love2D's code while we've been discussing this, and I think this is the approach they take as well:

image

I will create a branch with these changes applied - hopefully then you can test them with your code 🙂

@17cupsofcoffee
Copy link
Owner

I have pushed a branch called hidpi - please let me know if this works as you'd expect 👍

@sumibi-yakitori
Copy link
Contributor Author

Wonderful!
Your fixes in the hidpi branch seem to work fine.

@17cupsofcoffee
Copy link
Owner

Excellent! I've merged hidpi into main - for now, I've put it behind an off-by-default flag on ContextBuilder so that people's existing code will work the same, but I might flip the default in later versions depending on how well it works.

Thank you so much for your help in figuring out the right way forward with this 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Graphics Issues related to graphics/rendering. Area: High DPI Support Issues relating to high DPI rendering. The bane of every engine dev's life. Area: Platform Issues relating to the platform layer Help Wanted Issues that the maintainers would like help with resolving Platform Specific: macOS Issues that are specific to systems running macOS. Type: Feature Request Improvements that could be made to the code/documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants