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 Windows DPI scaling causing strange window behavior (#679). #681

Merged
merged 2 commits into from Aug 19, 2014

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented Aug 18, 2014

Fixes #679.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 18, 2014

Member

What about

SetProcessDPIAware is available for use in the operating systems specified in the Requirements section. It may be altered or unavailable in subsequent versions. Instead, use SetProcessDPIAwareness

... and

Note SetProcessDPIAware is subject to a possible race condition if a DLL caches dpi settings during initialization. For this reason, it is recommended that dpi-aware be set through the application (.exe) manifest rather than by calling SetProcessDPIAware.

?

Member

LaurentGomila commented Aug 18, 2014

What about

SetProcessDPIAware is available for use in the operating systems specified in the Requirements section. It may be altered or unavailable in subsequent versions. Instead, use SetProcessDPIAwareness

... and

Note SetProcessDPIAware is subject to a possible race condition if a DLL caches dpi settings during initialization. For this reason, it is recommended that dpi-aware be set through the application (.exe) manifest rather than by calling SetProcessDPIAware.

?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 18, 2014

Member

The patch dynamically tries to load the function entry point from user32.dll, so if an operating system does not support it, it won't get loaded or called. It is purely optional.

The second note only applies to DLLs that do what is said... cache dpi settings during initialization. SFML, if used as a dynamic library won't do this, and since it should normally be the only library that has to worry about window management, the race condition won't occur, since no other library would be loaded that has to care about dpi settings.

GLFW already does this, and SDL is in the process of doing this as well, as can be seen here and here. I don't think SDL has full automatic high-DPI support yet, so you won't find any related code in their codebase. High resolution displays are new, and libraries have to catch up with their code. Supporting DPI scaling like this means the user won't have to mess with their manifest, which is a pain to do if you are using MinGW.

Member

binary1248 commented Aug 18, 2014

The patch dynamically tries to load the function entry point from user32.dll, so if an operating system does not support it, it won't get loaded or called. It is purely optional.

The second note only applies to DLLs that do what is said... cache dpi settings during initialization. SFML, if used as a dynamic library won't do this, and since it should normally be the only library that has to worry about window management, the race condition won't occur, since no other library would be loaded that has to care about dpi settings.

GLFW already does this, and SDL is in the process of doing this as well, as can be seen here and here. I don't think SDL has full automatic high-DPI support yet, so you won't find any related code in their codebase. High resolution displays are new, and libraries have to catch up with their code. Supporting DPI scaling like this means the user won't have to mess with their manifest, which is a pain to do if you are using MinGW.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 18, 2014

Member

The patch dynamically tries to load the function entry point from user32.dll, so if an operating system does not support it, it won't get loaded or called. It is purely optional.

I got it, my comment was more about SetProcessDPIAwareness: why don't you use it instead, since it is recommended?

The second note only applies to DLLs that do what is said... cache dpi settings during initialization. SFML, if used as a dynamic library won't do this, and since it should normally be the only library that has to worry about window management, the race condition won't occur, since no other library would be loaded that has to care about dpi settings.

I agree. But since it can be done externally, in the manifest, I was wondering if it could be left to the user. But I guess I have my answer ;)

mess with their manifest, which is a pain to do if you are using MinGW.

Member

LaurentGomila commented Aug 18, 2014

The patch dynamically tries to load the function entry point from user32.dll, so if an operating system does not support it, it won't get loaded or called. It is purely optional.

I got it, my comment was more about SetProcessDPIAwareness: why don't you use it instead, since it is recommended?

The second note only applies to DLLs that do what is said... cache dpi settings during initialization. SFML, if used as a dynamic library won't do this, and since it should normally be the only library that has to worry about window management, the race condition won't occur, since no other library would be loaded that has to care about dpi settings.

I agree. But since it can be done externally, in the manifest, I was wondering if it could be left to the user. But I guess I have my answer ;)

mess with their manifest, which is a pain to do if you are using MinGW.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 18, 2014

Member

I got it, my comment was more about SetProcessDPIAwareness: why don't you use it instead, since it is recommended?

I initially stumbled upon this documentation page, which notes that this function is only available starting from Windows 8.1, when I searched again just now, I found this page, which seems to be older judging my the number in the URL? Since they provide (for me at least) confusing if not conflicting information, I went ahead and checked my user32.dll on Windows 7.
Imgur
So calling SetProcessDPIAwareness on my system would not work, although SetProcessDPIAware does.
If we decide to add SetProcessDPIAwareness, SetProcessDPIAware will still have to be checked as a fallback in case the former is unavailable.

Member

binary1248 commented Aug 18, 2014

I got it, my comment was more about SetProcessDPIAwareness: why don't you use it instead, since it is recommended?

I initially stumbled upon this documentation page, which notes that this function is only available starting from Windows 8.1, when I searched again just now, I found this page, which seems to be older judging my the number in the URL? Since they provide (for me at least) confusing if not conflicting information, I went ahead and checked my user32.dll on Windows 7.
Imgur
So calling SetProcessDPIAwareness on my system would not work, although SetProcessDPIAware does.
If we decide to add SetProcessDPIAwareness, SetProcessDPIAware will still have to be checked as a fallback in case the former is unavailable.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Aug 19, 2014

Ok.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

Ok.

This is ambiguous. Ok what? 😀

Member

binary1248 commented Aug 19, 2014

Ok.

This is ambiguous. Ok what? 😀

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 19, 2014

Member

Ok "I agree with everything and have nothing else to say". What other OK could it be? 😛

Member

LaurentGomila commented Aug 19, 2014

Ok "I agree with everything and have nothing else to say". What other OK could it be? 😛

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

But I said multiple things... hence ambiguity 😛.

  1. Add SetProcessDPIAwareness and SetProcessDPIAware
  2. Just leave it with SetProcessDPIAware

Pick one 😉.

Member

binary1248 commented Aug 19, 2014

But I said multiple things... hence ambiguity 😛.

  1. Add SetProcessDPIAwareness and SetProcessDPIAware
  2. Just leave it with SetProcessDPIAware

Pick one 😉.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 19, 2014

Member

If they recommend SetProcessDPIAwareness because SetProcessDPIAware is kind of deprecated, then we should go with both. Otherwise, SetProcessDPIAware only is fine. The doc is not very clear about the difference between these two functions...

Member

LaurentGomila commented Aug 19, 2014

If they recommend SetProcessDPIAwareness because SetProcessDPIAware is kind of deprecated, then we should go with both. Otherwise, SetProcessDPIAware only is fine. The doc is not very clear about the difference between these two functions...

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

When I hear Microsoft say that "stuff is deprecated by new stuff that came out with our latest operating system" I am always a bit sceptical of whether the old stuff is really "deprecated" or whether it is just another excuse they have for developers to write applications that require an operating system update.

I guess I'll add SetProcessDPIAwareness as the first choice and SetProcessDPIAware as the fallback if it isn't available. It isn't even that much work thanks to runtime loading. 😀

Member

binary1248 commented Aug 19, 2014

When I hear Microsoft say that "stuff is deprecated by new stuff that came out with our latest operating system" I am always a bit sceptical of whether the old stuff is really "deprecated" or whether it is just another excuse they have for developers to write applications that require an operating system update.

I guess I'll add SetProcessDPIAwareness as the first choice and SetProcessDPIAware as the fallback if it isn't available. It isn't even that much work thanks to runtime loading. 😀

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 19, 2014

Member

IMO "deprecated" typically means they're removing it from the SDK some time in the future (and nothing more for the time being). We've got Windows 8.1, yet we can still use leftovers from the Win 3.x API marked as deprecated for ages. So for the time being, I'd use both for now. If they ever remove it, they'll most likely add some emulation/compatibility layer anyway.

Member

MarioLiebisch commented Aug 19, 2014

IMO "deprecated" typically means they're removing it from the SDK some time in the future (and nothing more for the time being). We've got Windows 8.1, yet we can still use leftovers from the Win 3.x API marked as deprecated for ages. So for the time being, I'd use both for now. If they ever remove it, they'll most likely add some emulation/compatibility layer anyway.

@majesty2450

This comment has been minimized.

Show comment
Hide comment
@majesty2450

majesty2450 Aug 19, 2014

Tested it, works. I'll test again if changes are made.

majesty2450 commented Aug 19, 2014

Tested it, works. I'll test again if changes are made.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

Added support for SetProcessDPIAwareness in the latest commit.

Member

binary1248 commented Aug 19, 2014

Added support for SetProcessDPIAwareness in the latest commit.

@majesty2450

This comment has been minimized.

Show comment
Hide comment
@majesty2450

majesty2450 Aug 19, 2014

Tested and had no errors, however it appears that the awareness function is not being called for me, only the aware function. Not sure if this is correct behavior or not.

majesty2450 commented Aug 19, 2014

Tested and had no errors, however it appears that the awareness function is not being called for me, only the aware function. Not sure if this is correct behavior or not.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

Not sure if this is correct behavior or not.

It is... Awareness is only called if your system supports it, which isn't the case, just like my system.

Member

binary1248 commented Aug 19, 2014

Not sure if this is correct behavior or not.

It is... Awareness is only called if your system supports it, which isn't the case, just like my system.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 19, 2014

Member

Fixed a "bug" thanks to information provided by @eXpl0it3r (he uses Windows 8.1). It seems the half of Microsoft's API documentation that I relied on wasn't consistent with what they shipped to customers... so...

If anybody uses Windows 8.1, it should call the right function now.

Member

binary1248 commented Aug 19, 2014

Fixed a "bug" thanks to information provided by @eXpl0it3r (he uses Windows 8.1). It seems the half of Microsoft's API documentation that I relied on wasn't consistent with what they shipped to customers... so...

If anybody uses Windows 8.1, it should call the right function now.

@eXpl0it3r eXpl0it3r merged commit f9ed3fd into master Aug 19, 2014

@eXpl0it3r eXpl0it3r deleted the bugfix/windows_dpi_scaling branch Aug 19, 2014

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone Aug 19, 2014

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Oct 28, 2015

Contributor

I have a question about this. What is the reasoning behind calling SetProcessDpiAwareness with ProcessSystemDpiAware instead of ProcessPerMonitorDpiAware? Just curious.

Contributor

Foaly commented Oct 28, 2015

I have a question about this. What is the reasoning behind calling SetProcessDpiAwareness with ProcessSystemDpiAware instead of ProcessPerMonitorDpiAware? Just curious.

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