-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Prepare API for multimonitor support #2980
base: master
Are you sure you want to change the base?
Conversation
Do we have any confidence that the team has the expertise and resources to actually implement these features? My concern is that we're building a road to nowhere by creating an API without a plan to ever implement new features with it. |
Without commenting on the "expertise and resources" part, I'd like to say that it's pretty embarassing to not support multiple monitors in a graphics library in 2024, no matter how "simple" it is. |
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.
Some minor improvements to our original code
@L0laapk3: can you help me understand the relationship between Does If I create a |
Before we jump too much into reviewing code that was mostly meant as a jump off point for API discussions, lets focus on that first. |
The way I changed it now, yes.
Each monitor has a list of supported fullscreen VideoModes. In fullscreen mode it must be an exact match with one of the supported modes to work afaik. In windowed/borderless mode it doesn't matter (more on this below)
Nothing is strictly stopping you from entering custom numbers and creating your own As a sidenote, in my opinion the |
Pull Request Test Coverage Report for Build 9033044943Details
💛 - Coveralls |
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.
Looks ready for review to me
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.
Do you expect any of these MonitorImpl
classes to have data members in the future? Currently none of them contain any data which means we could simplify this significantly by converting all these classes into a collection of free functions within a namespace.
@ChrisThrasher yes, they will definitely have data members once they are populated. That is why I went through all the trouble of non-static impl classes :D For reference, I have an unix implementation prepared here: https://github.com/L0laapk3/SFML/pull/1/files |
whoops, that last commit & message was meant to go into my games' repository, not its sfml subrepository. reverted. |
I'm not sure |
Im open to name suggestions and not super attached to any names. |
Multimonitor support
This is my proposal for a plan to support multiple monitors in the future. To keep a good overview about what changes, I propose to split it up in several PRs:
Monitor
implementation, breaking API changes (this PR).MonitorImpl
s.RenderWindow
constructor overload with aMonitor
parameter.Monitor
such as listing all available monitors and getting information about monitors.Comments about the general plan as lined out above are welcome, as well as comments on 1. this PR. However, please try to keep specific comments about PRs 2-4 out of this page if they have no impact on this specific PR. These comments could possibly be directed at #2136.
This PR
The goal of this PR is to only make the necessary breaking changes to the API for multimonitor support, while keeping all functional behavior identical.
Description
Monitor
API class has been created.VideoMode
s functions have been moved toMonitor
as they are all monitor dependent.VideoModeImpl
s have been moved toMonitorImpl
, and this class has been made non-static.MonitorImplTarget
s have been populated with placeholders with identical behavior as before the PR to limit the scope. See PR 2 in the plan above.Tasks
How to test this PR?