Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
476 stars 75 forks source link

Grafik Resolution for 4k display #1542

Closed JustAnother1 closed 7 months ago

JustAnother1 commented 1 year ago

My display has 3840 x 2160 pixels. In the Settings I can choose 4096 x 2160 what is not working correctly on my display (part of the screen is black(on the left) and part of the game screen is missing (on the right))

I can not choose 3180 x 2160 . The 4096 x 2160 is the only available 4k setting, All others are full HD (1920 x 1080) or less. In the Full HD range there is also 1920 x 1200. So it would make sense to also have 3840 x 2160, right?

Observed on:

Return To The Roots v20220922-0537322eb59cd418697ceeb35f21564150f0bdf4 Compiled with GNU C++ version 8.3.0 for linux 64 Bit

Spikeone commented 1 year ago

You can always set the resolution in the config.ini

JustAnother1 commented 1 year ago

@Spikeone Thnak you! that works. But 3840 x 2160 should probably still go into the list, right?

Spikeone commented 1 year ago

Probably - maybe it's a bug because I'm somewhat confused that your 4k res is missing - since others used and complained about such already :D

Flow86 commented 1 year ago

Hi, we get the supported video modes from your graphics driver. (see method ListVideoModes). So it seems that your driver is reporting this wrong.

JustAnother1 commented 1 year ago

@Flow86 that might be correct. But is it the best solution to depend on the different drivers to report everything they can?

I run Linux Mint on a "AMD Ryzen 5 5600G with Radeon Graphics × 6" graphics reported as "Advanced Micro Devices, Inc. [AMD/ATI] Device 1638". So no separate Graphics Card. I have two monitors connected (3840x2160 + 1920x1080) setup as desktop extension.

in the terminal window i see

Starting in "removed" Directory for user data (config etc.): "removed/.s25rttr" Suche nach Treibern in "removed/lib/s25rttr/driver/video" 1 video drivers found! Videotreiber "(SDL2) OpenGL via SDL2-Library" geladen OpenGL 4.6 wird unterstützt Suche nach Treibern in "removed/lib/s25rttr/driver/audio" 1 audio drivers found! Audiotreiber "(SDL2) Audio via SDL2_mixer-Library" geladen

Would it make sense to only detect the biggest resolution and to then put everything in the list that is smaller than that?

I think that the normal user does not want to change settings in config.ini. Listing standard resolutions even if not reported by the driver would give the user an easy way to fix issues by just selecting a different resolution.

But that is just a suggestion. It now works for me so I can close the issue if you are pleased with the situation as is.

Flamefire commented 1 year ago

Listing standard resolutions even if not reported by the driver would give the user an easy way to fix issues by just selecting a different resolution.

Which would make it possible to (easily) set a resolution not supported by the GPU. We have to rely on the (system) driver to report supported configurations. Note that the "drivers" in the RTTR folder are not really "drivers" but abstractions over interfaces (bad naming...)

JustAnother1 commented 1 year ago

@Flamefire In my case the 4096x2160 was reported to be supported, but did not work. The 3840x2160 was not listed (so probably not reported) but works perfectly. As I see it we are in a world were those "drivers" or "abstractions over interfaces" are not perfect and even sometimes wrong. My "driver" was wrong. For me it seems as if the questions is how to deal with this imperfect situation.

Therefore my suggestion was to make it easier for the user to find a solution that works for the user. With CRTs a wrong configuration could damage the monitor, but I haven't seen any CRTs that could do 4k. Therefore suggesting the other 4K resolutions, if the driver reports one of them should be save. I know that I would not have opened a issue if the 4096x2160 did not work, but the 3840x2160 would have been in the list and worked. I would not had an issue and it would be clear to me that my hardware is the cause for the 4096x2160 is not working.

But again: Changing the config.ini fixed it for me. So tell me to close the issue and I will. Or close it yourselves.

Flamefire commented 1 year ago

As I see it we are in a world were those "drivers" or "abstractions over interfaces" are not perfect and even sometimes wrong. My "driver" was wrong.

Sorry seems I need to be more clear: Our "driver"/abstraction asks an abstraction (here: SDL2) of the system (and system driver) for what is supported. Now either the system/OS driver is reporting a wrong value to SDL2 or SDL2 is reporting a wrong value to us. In either case we have to assume what SDL2 tells us is correct and what it doesn't tell us is not supported either by SDL2 or by the system.

Now of course there could be a bug in our code. The code is here: https://github.com/Return-To-The-Roots/s25client/blob/0537322eb59cd418697ceeb35f21564150f0bdf4/extras/videoDrivers/SDL2/VideoSDL2.cpp#L411

It could well be an issue with the 2 monitors being present. So if you can build an debug that and find the problem we'd be happy to include that fix.

JustAnother1 commented 1 year ago

I don't think that there is a bug in the code. Where bug means that the code is doing something that is not intended.

This is what I do not agree on:

we have to assume what SDL2 tells us is correct and what it doesn't tell us is not supported either by SDL2 or by the system.

I assume that SDL2 is incorrect in reporting the wrong modes. I assume your code is correct. But the code is inconsistent.

If your intention is to avoid using graphic modes that are not reported by the hardware (SDL2,..) then you need to fix the code that reads the graphic mode from the CONFIG.INI file.

If your intention is to allow the user to use modes that are not reported but work, then why does the user has to go into CONFIG.INI for that when changing it in the dialog would be much easier.

In both cases the highlighted code position is not the place to apply a patch.

If you want to make sure that only reported modes are used than a patch in the parsing of the CONFIG.INI file is needed. The resolution value found in it needs to be verified against the reported values. This would then break RTTR for me until my SDL2 driver gets fixed. But hey how else can we get those low level guys to fix their code, right?

If you want to allow for modes that are not reported but work then as suggested previously add the not reported modes. That should then probably not happen in the SDL2 specific code, but more on the hardware independent code.

If you want to go with option 1 then just tell me to close the Issue and I will, as it is working for me.

If you think the second option is better than let us think how to best implement that. If the drop down menu would allow typing then a user could type in the wanted resolution. Adding all resolutions with less pixels might also be an option. Maybe the SDL or lower code just reports the highest possible resolution and assumes that it clear that everything lower than that is supported. Or even just putting the CONFIG.INI work around somewhere in the documentation might be thing to do.

I don't know. It is up to you to decide how RTTR should behave.

falbrechtskirchinger commented 1 year ago

@JustAnother1 Can you show the output of xrandr on your system?

JustAnother1 commented 1 year ago

$ xrandr Screen 0: minimum 320 x 200, current 5760 x 2160, maximum 16384 x 16384 DisplayPort-0 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 598mm x 337mm 1920x1080 60.00+ 50.00 59.94
1680x1050 59.95
1400x1050 59.98
1600x900 60.00
1280x1024 75.02 60.02
1440x900 59.89
1280x960 60.00
1280x800 59.81
1152x864 75.00
1280x720 60.00 50.00 59.94
1024x768 75.03 70.07 60.00
832x624 74.55
800x600 72.19 75.00 60.32 56.25
720x576 50.00
720x480 60.00 59.94
640x480 75.00 72.81 66.67 60.00 59.94
720x400 70.08
DisplayPort-1 disconnected (normal left inverted right x axis y axis) DisplayPort-2 disconnected (normal left inverted right x axis y axis) HDMI-A-0 connected primary 3840x2160+1920+0 (normal left inverted right x axis y axis) 1872mm x 1053mm 3840x2160 60.00
+ 50.00 59.94 30.00 25.00 24.00 29.97 23.98
4096x2160 60.00 50.00 59.94 30.00 25.00 24.00 29.97 23.98
1920x1200 60.00
1920x1080 60.00 50.00 59.94 30.00 25.00 24.00 29.97 23.98
1600x1200 60.00
1680x1050 59.88
1400x1050 59.95
1280x1024 75.02 60.02
1440x900 59.90
1280x960 60.00
1280x800 59.91
1152x864 75.00
1280x720 60.00 50.00 59.94
1024x768 75.03 70.07 60.00
832x624 74.55
800x600 72.19 75.00 60.32 56.25
720x576 50.00
720x480 60.00 59.94
640x480 75.00 72.81 60.00 59.94
720x400 70.08
$

falbrechtskirchinger commented 1 year ago

Thanks! That is quite odd. I'd expect SDL to use the same API and report the same resolutions. Unless, of course, you're not using X11.

One more thing to try would be to export SDL_VIDEODRIVER=x11 before launching the game (but this should be the default anyway :man_shrugging:). Then check the entire list of video resolutions on the options screen, as they may be sorted in an unexpected way and you have quite a large number of supported resolutions.

As to your suggestion to include "common" resolutions that are smaller than the largest reported resolution, I think that's a bad idea. My system, for example, only supports 2 resolutions. It can't even do 1920x1080, which is in between the 2 reported resolutions.

The solution is to figure out why SDL isn't reporting the correct resolutions. If you're willing and able to build a small SDL test application, I'd be willing to write it, to help narrow down where things go wrong. And if it is indeed SDL's fault, you should talk to them.

JustAnother1 commented 1 year ago

I'm using Linux Mint (if that helps you narrow down the X11 thing) and I also use two Monitors. My desktop extends to both screens. DisplayPort-0 is connected to a 27" Monitor that can do 1920x1080 at most. HDMI-A-0 is connected to a large 4K TV that can do the full 3840x2160 My desktop extends over both screens. So possibly different windows on the different screens. Rttr in full screen would leave the 27" Monitor still usable for other things.

Maybe this probably uncommon setup is causing the confusion?

I checked the SDL_VIDEODRIVER variable and it was not set. I did the export before the game and did not see a change. I still have the patched config.ini working so the resolution was correct. But in both cases I was offered 3180x4096 as a possible resolution. I probably have to change back the config.ini, right?

In both cases I see on the console that he is looking for a video driver, finds a video driver((SDL2) OpenGL via SDL2-Library) and says that "OpenGL 4.6" is supported.

I hope that helps.

I suggested the common resolutions just to turn the situation from "I can not chose the right resolution" towards a "some of the resolutions I can select do not work" kind of situation. That for me seems to be better as with the option to select the "3840x2160" it then works for me,...

falbrechtskirchinger commented 1 year ago

Maybe this probably uncommon setup is causing the confusion?

It really shouldn't. The xrandr tool is reporting the desired 3840x2160 resolution and SDL uses the xrandr (X Resize and Rotate) API (same as the command line tool) to obtain supported resolutions. It's puzzling why it isn't showing up.

I checked the SDL_VIDEODRIVER variable and it was not set. I did the export before the game and did not see a change. I still have the patched config.ini working so the resolution was correct. But in both cases I was offered 3180x4096 as a possible resolution. I probably have to change back the config.ini, right?

Without looking at the code, I'd assume so, yes.

I suggested the common resolutions just to turn the situation from "I can not chose the right resolution" towards a "some of the resolutions I can select do not work" kind of situation. That for me seems to be better as with the option to select the "3840x2160" it then works for me,...

Again, the frustrating part here is, that xrandr clearly knows the correct resolutions and SDL should report those same resolutions to the game.

Soon, an alternative might be to use "Windowed Fullscreen"/"Borderless Windowed" (whatever we end up calling it) which simply creates a maximized window without a border and doesn't change screen resolution. That feature should be merged in the next couple of weeks and I can let you know when it's available in a nightly version.

JustAnother1 commented 1 year ago

I removed the complete [video] section from CONFIG.INI

On the next start of rttr i saw this: Return To The Roots v0.9.5-397f2b2315e997504d4958bfbdea0af815ce559a Compiled with GNU C++ version 8.3.0 for linux 64 Bit ... WARNING: Could not use settings from "/removed/.s25rttr/CONFIG.INI", using default values. Reason: File missing or invalid

It defaulted back to 800x600. And then i saw the following settings screen.

Bildschirmfoto vom 2023-08-31 01-45-09

On giving it a second glance the 3840x2160 is on the list. I probably have to apologize. Right now I'm not sure if I missed it before.

So this can be closed. (I would recommend sorting the entries in the drop down).

Thank you to all who helped !

Spikeone commented 1 year ago

@JustAnother1 just asking, aren't you now using a 0.9.5 stable instead of a nightly version?

Flamefire commented 1 year ago

FWIW: It sorts by aspect ratio first: https://github.com/Return-To-The-Roots/s25client/blob/f13f98dc8a6029aa3ac2beade1474c7a57e446de/libs/s25main/desktops/dskOptions.cpp#L655-L676

JustAnother1 commented 1 year ago

@Spikeone You are rigth, so I retested:

Return To The Roots v20230814-e37a322209cffb00fda4afd50c87b8f7e8261083 Compiled with GNU C++ version 10.2.1 20210110 for linux 64 Bit Bildschirmfoto vom 2023-08-31 21-20-09

@Flamefire good find. Sorting by aspect ratio might make sense (not fully convinced) but with the same aspect ratio sorting the bigger resolution on top would be better, right?

My feeling was to just sort it by resolution and with same resolution. This way 3840x2160 is on 2nd position. Due to square pixels the different aspect ratios do not have the same resolution. (1280x800 (16:10) / 1280x720 (16:9) / 1280x1024 (5:4) )

But I'm not an user experience Expert.

Flamefire commented 1 year ago

I'm fine with changing it and descending makes also sense.

My feeling was to just sort it by resolution and with same resolution. This way 3840x2160 is on 2nd position. Due to square pixels the different aspect ratios do not have the same resolution. (1280x800 (16:10) / 1280x720 (16:9) / 1280x1024 (5:4) )

I don't understand "by resolution and with same resolution", can you explain further? Also are the 3 listed resolutions intended to be an example order? If so, what is the sorting logic?

JustAnother1 commented 1 year ago

I myself don't understand anymore what I meant by "by resolution and with same resolution". Sorry.

If we take "3840x2160" then lets call the first number (3840) x and the second number (2160) y.

My suggestion was to sort by x first then y second.

The three listed resolutions should be an example that the same x value gives different y levels for different aspect ratios. Therefore sorting by aspect ratio is not needed.

I think the biggest x value shout be on the top of the list. People usually want to use the highest resolution, right?

If then we sort the same way for y then that would mix up the aspect ratios. See the 1280x examples. The 5:4 would be first followed by 16:10 and 16:9. For me that would be fine, but an alternative would be to sort by aspect ratio second.

I assume people would look at the list, go down to the x value they want and then there is only a small number of entries all next to each other to choose from. Therefore the second level sorting is not so important.

I think different users will have different preferences for sorting. But as long as they can on first glance figure out where they will find their searched for resolution all should be fine.