-
Notifications
You must be signed in to change notification settings - Fork 57
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
[GEN][ZH] Allow selecting resolutions with aspect ratios different from 4:3 #460
base: main
Are you sure you want to change the base?
[GEN][ZH] Allow selecting resolutions with aspect ratios different from 4:3 #460
Conversation
Great job! I had a chance to review your work and noticed this commit valeronm@d597b7f . I was wondering if it addresses the issue mentioned below. If so, would you be want to submitting another PR? |
Yes, definitely. The commit just combines the code from #78 with some hacks around control bar. I'm going to submit separate PRs for these changes. |
Any reason why are not supporting 21:9, 32:9? |
As 16:10 displays are beginning to get a lot more common on premium laptops, is there any chance to allow that as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Outdated
Show resolved
Hide resolved
Are you sure this fixes #453 ? To test it, you need to go to Options.ini in MyDocuments, set an uncommon resolution, launch game in windowed mode, and then reproduce 453. |
It's easy to change or remove the check for aspect ratio, and I just used the aspect ratios described in #72 as a baseline for the PR. Will be happy to change it to any values. There is also an issue with fixed FOV of 50° in the game, which effectively zooms everything on resolutions wider than 4:3, so even on 16:9 we can see less than on 4:3, and I imagine that on 32:9 we'll see only several units until #78 will be fixed.
16:10 is already supported by the code. It covers all resolutions between 5:4 (aspect ratio 1.25) and 16:9 (aspect ratio 1.778), so 16:10 (1.6) and 3:2 (1.5) are also supported.
I'm sure that this code doesn't support custom resolutions specified directly in options.ini. But I see that in #453 GenTool prevents overriding standard resolution (1920 x 1080), while in VS6 it's reset to the first available 4:3 resolution just because the game doesn't see 1920 x 1080 as an option. With this change, the problem will be resolved because 1920 x 1080 will be available and selected in the list of resolutions. |
ok, resolutions appear: And changing to those resolutions work as intended. But if we change the resolution to a invalid one manually (like described on #453 ) the game crashes on launch with error : |
I think that supporting non-standard resolutions should be done separately. One reason for that - non-standard resolutions will work only in windowed mode, so there should be conditional logic to differently work in full screen and windowed mode. If we think that #453 is about non-standard one, then it's better to unlink this PR from the ticket. This PR is only about extending the aspect ratio filter implemented in the game to support a wider range of resolutions. |
I think supporting all resolutions supported by the display device should be there. Arbitrarily restricting the aspect ratio will cause issues for people who have for example this monitor: LG DualUp |
Yes. If that is a bug of the retail game. I prefer to keep changes small, and this change has perfect size and scope. If the custom resolution issue can be addressed separately from this change, then that can be done in a separate pull request. |
Is it necessary to add a yaml file with these fixes? As they affect the user experience, even though users hardly remember this bug since it was fixed in gentool :-) |
Yes this change is user facing and will need to be documented, but Vitex is still working on the change log pipeline. We are not submitting these kind of changes yet, not until the foundation is done. |
9ff7433
to
56e614b
Compare
56e614b
to
56509dd
Compare
The game only allows select resolutions with 4:3 aspect ratio and the logic between Generals and Generals ZH is slightly different. With these changes, it's possible to select resolutions with aspect ratios between 5:4 and 16:9 (similar to what GenTool does based on #72)
Changes:
W3DDisplay::getDisplayModeCount
andW3DDisplay::getDisplayModeDescription
into single functionisResolutionSupported
;Screenshot
Fixes #72