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

WIN32: Add scummvm.exe.manifest file and disable HiDPI scaling (on hold) #1541

Closed
wants to merge 6 commits into from

Conversation

lotharsm
Copy link
Member

@lotharsm lotharsm commented Mar 19, 2019

This PR adds an Windows Application Manifest file that Windows XP+ uses for setting various options during the execution of the program.

This manifest file disables the HiDPI scaling on Windows 7+, to fix the blurry output on HiDPI displays. This is currently most noticable when using the GUI and adds a slight blur to the games.

By declaring ScummVM as "DPI aware", we permit Windows do perform it's own scaling. Disabling scaling by the OS is already the default on Linux, maybe @criezy can comment on the macOS side of things.

A test build is available through https://cloud.serra.network/index.php/s/jsJCjs7X6dnsBwa/download.

In this screenshot, the upper screenshot shows the current behavior on a HiDPI display (look at the GUI scaling), where the lower screenshot shows the behavior with this manifest file on the exact same machine.
scummvm-hidpi-scaling

The screenshots give you a vague representation, on a real HiDPI display the difference is quite obvious.

@dafioram
Copy link
Contributor

Does this fix Trac#10587?

@lotharsm
Copy link
Member Author

I haven't tested this yet since I don't have the build mentioned in this ticket anymore.

But since I could "fix" this by disabling hidpi scaling for the ScummVM executable through the compatibility options, it is very likely that #10587 will be fixed too.

@lotharsm
Copy link
Member Author

@dafioram, regarding Trac#10587: I ran a few tests and the current master branch still showed this crash once. Seems slightly better with SDL 2.0.9 now.

However, during those tests, disabling the HiDPI scaling via this PR seems to fix the issues for me since I couldn't replicate the crash here.

@dafioram
Copy link
Contributor

Sounds like a win.

@lotharsm
Copy link
Member Author

At least I couldn't spot any unwanted side effects during my tests. If the application is run on Windows XP which doesn't understand the DPI awareness setting in the manifest file, it will simply ignore this manifest directive.

@lotharsm lotharsm requested review from bgK, criezy, sev- and Kirben March 21, 2019 17:07
Copy link
Member

@bgK bgK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good compromise to disable dpi scaling until our own GUI is able to scale itself according to the OS level UI scale. Until then, people will complain about the GUI being too small on 4K displays though.

dists/win32/scummvm.exe.manifest Outdated Show resolved Hide resolved
dists/win32/scummvm.exe.manifest Outdated Show resolved Hide resolved
Copy link
Member

@criezy criezy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer the question about the behaviour on macOS, I have actually no idea what it is. I don't have any hight resolution screens myself to test, and it looks like we don't do anything special in ScummVM. So the behaviour is probably whatever is the default for SDL applications on macOS.

The change on Windows as it is presented in this PR seems a good one to me (with the caveat that I am not a Windows user).

dists/scummvm.rc Outdated Show resolved Hide resolved
@lotharsm
Copy link
Member Author

lotharsm commented Mar 23, 2019

After merging this PR, we could tackle the issues with the GUI being too small on 4K displays using SDL_GetDisplayDPI by providing a "highres" theme and do the GUI scaling there (e.g. starting with a 640x400 display and providing larger fonts right in the theme file), just like we already do for "lowres"...

Thinking about it, we even could increase the GUI window to something like 800x600 on the SDL backend and provide a proper theme for it, since I don't think people are using resolutions so low they can't handle 800x600. But providing the proper resolution/windows size based on the DPI surely is a better way to solve this. I think I just found a new project.

@bluegr
Copy link
Member

bluegr commented Mar 23, 2019

I'm using Windows 10, with a UHD resolution (3200x1800). By disabling HiDPI scaling, ScummVM looks miniscule in this resolution, and is practically unreadable. I've even tried to use a 3x filter (Control-Alt-+), but the results are still bad.

It's possible to disable HiDPI scaling from Windows, by right-clicking scummvm.exe and going to Properties->Compatibility->Change High DPI settings->set High DPI scaling override to "Application"

Thus, I believe that it's wrong to just disable HiDPI scaling, without the application actually supporting such high resolutions. And since it's possible for users to override this behavior, IMHO it's wrong to disable it like this.

We can disable it in the future, once we can support arbitrary scalers (e.g. an 8x scaler). Have a look at pull requests #1255 and #271. Until then, my opinion is that this feature should remain enabled as it is right now.

@bluegr bluegr self-requested a review March 23, 2019 19:10
Copy link
Member

@bluegr bluegr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I stated, my opinion is that it's wrong to just disable this feature, without actually providing support for HiDPI resolutions

@lotharsm
Copy link
Member Author

lotharsm commented Mar 23, 2019

I'm using Windows 10, with a UHD resolution (3200x1800). By disabling HiDPI scaling, ScummVM looks miniscule in this resolution, and is practically unreadable. I've even tried to use a 3x filter (Control-Alt-+), but the results are still bad.

Thanks for your feedback! Are you referring to the GUI here or also to the games themselves? Regarding the GUI, I am currently working on proper DPI detection via SDL_GetDisplayDPI(), creating a windows with 800x600 and then using the theme file to upscale the GUI.

Very early POC:
hidpi-demo

@bluegr
Copy link
Member

bluegr commented Mar 23, 2019

@lotharsm: Yes, I'm talking about the GUI. I usually like to play games in fullscreen, but that's me :)

@sluicebox
Copy link
Member

Adding the manifest this way breaks Visual Studio builds atm. The VS project is set to automatically generate a manifest and add it to the resource script. This a creates duplicate resource, linker errors. The VS project settings need to be changed to disable manifest generation if we're explicitly adding one.

manifest

I'm also concerned about this making the default window size impractically small on high dpi. The machine I'm at now is Win7 2560x1440 with text size at "Medium - 125%" in the Windows parlance, I didn't see any difference, maybe I need to have it higher, but I think we need to understand the ramifications of this more. I already feel like the initial size isn't that useful to me at this resolution.

@lotharsm
Copy link
Member Author

@sluicebox, thanks for your comment regarding VS! I wasn't aware that VS is not smart enough to ask the user what to do at this point... ;)

I must confess that I'm a bit lost regarding the proper GUI size for now...

@sluicebox
Copy link
Member

Making things look nice at all dpi settings is a chore and you're a hero for taking it on. Every time I've done it I felt like I was starting over from scratch since it's all so app-specific.

Although it looks like I'm not affected by this yet, I'm leaning towards what @bluegr is saying and holding off until there's scaling to compensate. Even that first screenshot of Windows doing the scaling looks fine to me, but maybe that's a limitation of screenshots, or maybe I just have bad taste. =)

@lotharsm
Copy link
Member Author

lotharsm commented Mar 25, 2019

Making things look nice at all dpi settings is a chore and you're a hero for taking it on. Every time I've done it I felt like I was starting over from scratch since it's all so app-specific.

Thanks :) Yep, everything is app-specific in this regard. However, I already figured out where I could hook up the DPI detection in main.cpp (or is it possible to change the window resolution at a later point?) for the SDL ports and then, based on the new window size, provide a proper theming through the theme files. You can check out https://user-images.githubusercontent.com/11882577/54870798-a6852200-4dab-11e9-872c-f21c8cd1f284.PNG for a first look.

@lotharsm lotharsm changed the title WIN32: Add scummvm.exe.manifest file and disable HiDPI scaling WIN32: Add scummvm.exe.manifest file and disable HiDPI scaling (on hold) May 12, 2019
@sev-
Copy link
Member

sev- commented Dec 30, 2019

@lotharsm Do I understand correctly, that it is OK to close this PR, and wait until the runtime detection is implemented?

@lotharsm
Copy link
Member Author

@sev- Correct - this PR is more or less obsolete.

@lotharsm lotharsm closed this Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants