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

Add multi-display device support #8072

Open
wants to merge 57 commits into
base: master
Choose a base branch
from

Conversation

MaMadDl
Copy link
Contributor

@MaMadDl MaMadDl commented Nov 23, 2023

Adding Multi Monitor Support
Tested on Windows (Win 11)
Untitled

@ihhub
Copy link
Owner

ihhub commented Nov 23, 2023

Hi @MaMadDl , can you please explain what problem this pull request solves?

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Nov 23, 2023

Hi @ihhub
when you have more than one monitor
and you want the game to run on your second (or third) Monitor you can't really change it.
the Ctrl+Alt+WinKey doesnt always work
so I added the functionality to do so .
adding this needed changing layout of dialog_graphics_setting to add a new button
for the button image couldn't find anything better in AGGs

@Mr-Bajs
Copy link
Contributor

Mr-Bajs commented Nov 23, 2023

Is this only a windows feature, or does this also work for MacOS, Linux (both X11 and Wayland?)

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Nov 23, 2023

It should cause we are using SDL to handle monitor coordinates and SDL handles different window systems
I have linux (ubuntu 20.04) as well but haven't got to test it yet

@ihhub
Copy link
Owner

ihhub commented Nov 23, 2023

Hi @MaMadDl , what do you mean I can't change it if it is possible to drag a window in between monitors? I'm just trying to understand the problem before trying to analyze a solution for it.

Could you also elaborate "the Ctrl+Alt+WinKey doesnt always work"? In which cases it works and in which it doesn't?

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Nov 23, 2023

when you are in fullscreeen (and you want to play in fullscreen not windowed) game is pretty much stuck on the first monitor (primary monitor) connected.
usually some engines allow moving between monitors using Crtrl+Alt+ ArrowKey (Not Win Key MyBad!) to move game window between monitors. this engine doesn't ATM.
so in order to play the game on fullscreen on any other monitors than the first monitors you need to add functionality on engine level. and that's what i did.

if you got a second monitor try this for yourself. try to open the game on fullscreen on your second (non-primary monitor)

[-Wreorder-ctor] and  [-Werror=reorder]
Compiled with C5038 enabled as error
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/screen.h Outdated Show resolved Hide resolved
src/engine/screen.h Outdated Show resolved Hide resolved
MaMadDl and others added 3 commits November 24, 2023 01:42
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@MaMadDl
Copy link
Contributor Author

MaMadDl commented Nov 24, 2023

@Mr-Bajs on Ubuntu 20.04
Apparently X11 can correctly handle screen switch internally ( I could move game window between monitors using WinKey+Shift+ArrowKey)

include <SDL_video.h> for analyzer
@zenseii zenseii added improvement New feature, request or improvement ui UI/GUI related stuff labels Nov 24, 2023
@zenseii zenseii added this to the 1.0.11 milestone Nov 24, 2023
@zenseii
Copy link
Collaborator

zenseii commented Feb 17, 2024

@MaMadDl,

could you get the higher resolution before the change ? sadly both my displays are 1920x1080 so I cannot test this myself.

Yes before that I could. I'm basically plugging my laptop to my TV.

but as far as I could tell getAvailableResolutions was always checking for available display resolutions for first display. I have changed this to calculate list of resolutions based on the display the window is in. After a quick google search it appears that SDL has issues with HIDPI on window for 4k displays but I'm not an expert in SDL.

I'll test your new changes when they are uploaded then :)

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Feb 17, 2024

@zenseii
changes are already commited in 0287e22

@zenseii
Copy link
Collaborator

zenseii commented Feb 17, 2024

@MaMadDl

@zenseii changes are already commited in 0287e22

I have changed this to calculate list of resolutions based on the display the window is in.

I see. Then I misunderstood.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/screen.cpp Outdated Show resolved Hide resolved
@MaMadDl
Copy link
Contributor Author

MaMadDl commented Mar 4, 2024

Hi @zenseii

the error on startup is fixed.

also for the 4K resolution I think I found the problem :
in getAvailableResolutions() the lambda function is calculated at the startup once (with default display index 0) and after that every time its called we get the result from startup
line #855:
static const std::vector<fheroes2::ResolutionInfo> filteredResolutions = [=]() {
can you check for your resolutions after removing static const
like std::vector<fheroes2::ResolutionInfo> filteredResolutions = [=]() {

@zenseii zenseii self-requested a review March 10, 2024 10:20
@zenseii
Copy link
Collaborator

zenseii commented Mar 12, 2024

Hi, @MaMadDl.

can you check for your resolutions after removing static const

I tested this and now the list of resolutions correctly change to match the display.

Meanwhile, I found another bug. When you have your screens set up to extend the primary screen, and end fheroes2 while it is being displayed on what is the second screen, then if you change to single screen mode, which means you're only using the 2nd screen or the first screen, then the App will throw an exception in xstring.cpp line 401 when clicking on the Graphics option in settings:

image

The way to "fix" this is to turn on extended screen mode again, and then switch to the first monitor before switching to only use one of the screens.

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Mar 16, 2024

Hi @zenseii

the error happens because SDL trying to render on a display that does not exists.

because this could happen in any stage of the game and not just in options
the only solution I could think of is using SDL_AddEventWatch on SDL_DisplayEvent and re-allocate the engine in case the display goes out of bound

this bug is not exclusive only to multi-display setups and could happen on single displays as well.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/localevent.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/localevent.cpp Outdated Show resolved Hide resolved
@MaMadDl
Copy link
Contributor Author

MaMadDl commented Mar 28, 2024

Meanwhile, I found another bug. When you have your screens set up to extend the primary screen, and end fheroes2 while it is being displayed on what is the second screen, then if you change to single screen mode, which means you're only using the 2nd screen or the first screen, then the App will throw an exception in xstring.cpp line 401 when clicking on the Graphics option in settings:

Hi @zenseii
to fix this I have added SDL_DISPLAYEVENT to localevent polling. in any stage of the game a display disconnect this event is called and allocating last available display in case current display is out of bound.

currently this index is not being saved. so on rebooting the game, it will start on original display.

I'm also getting this message in the console upon booting the game:

fheroes2 engine, version: 1.0.11
13.02.2024 16:37:32: [ERROR]    `anonymous-namespace'::RenderEngine::getMaximumDisplays:  Failed to Get Number of Displays, description: Video subsystem has not been initialized

Also this error is handled.

@zenseii
Copy link
Collaborator

zenseii commented Apr 1, 2024

Hi @zenseii to fix this I have added SDL_DISPLAYEVENT to localevent polling. in any stage of the game a display disconnect this event is called and allocating last available display in case current display is out of bound.

currently this index is not being saved. so on rebooting the game, it will start on original display.

Also this error is handled.

Hi, @MaMadDl. I tested the latest commits and the only issue I could find now is that whenever I start the app the resolution is set to 640x480, even if I change it to something like 1920x1080 before closing the app. I believe we need to save the new resolution when closing the app.

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Apr 1, 2024

Hi @zenseii
I'm unable to reproduce this error. can you check the conf file to see if resolution is saved or changed?
I haven't changed how resolution is saved or even works. (it's in line #261 in dialog_graphics_settings.cpp)

@zenseii
Copy link
Collaborator

zenseii commented Apr 18, 2024

@MaMadDl. I've done a clean rebuild and made some tests on the latest commit. I managed to trigger an assertion however it is pretty much an edge case, but it does help clarify the bug I encountered earlier.

Basically you need to display the game on the 2nd extended monitor and set a resolution that is higher than your primary resolution supports. Then disconnect the video cable while the app is running. This triggers the assertion in screen.cpp line 1378. As I said this is kind of an edge case.

image

That brings us to the previous bug I described about the resolution not changing from 640x480. If you do the above, or if you end the app on the second monitor with an unsupported resolution for the primary monitor and disconnect the 2nd monitor, then when you restart fheroes2 the resolution will be set to 640x480, which is fine. The problem is that you cannot change this resolution unless you click on the monitor button and the screen flashes for a brief moment.

This is what I show in this video:

2024-04-18.15-08-41.mp4

@ihhub ihhub modified the milestones: 1.1.0, 1.1.1 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature, request or improvement ui UI/GUI related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants