-
Good evening everbody!
I'm starting another attempt at the good old multi monitor setup issue :) But this time I am bringing a fully functional reference implementation for windows with me! You can check it out here: https://github.com/Foaly/SFML/tree/multi-monitor-windows (commit d3bfdbfb (https://github.com/Foaly/SFML/commit/d3bfdbfb4b94e9527335ce28ce017498b69cb58a))
This weekend I dug through the old discussions and the MSDN and implemented a working multi monitor API for windows. It follows the ideas brought up inprevious threads. I designed the API so that it is fully backwards compatiple with the current SFML master. The usage looks like this:
#include <SFML/Window.hpp>
#include <iostream>
int main()
{
std::cout << "# of connected Screens: " << sf::VideoMode::getScreenCount() << std::endl;
// display all supported fullscreen resolutions for all monitors
for (unsigned int id = 0; id < sf::VideoMode::getScreenCount(); ++id)
{
std::vector<sf::VideoMode> modes = sf::VideoMode::getFullscreenModes(id);
std::cout << "Valid resolutions for screen " << id << std::endl << std::endl;
for(std::size_t i = 0; i < modes.size(); ++i)
{
sf::VideoMode mode = modes[i];
std::cout << "Mode #" << i << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl;
}
std::cout << std::endl;
}
// display the desktop resolution for all screens
for (unsigned int i = 0; i < sf::VideoMode::getScreenCount(); ++i)
{
sf::VideoMode mode = sf::VideoMode::getDesktopMode(i);
std::cout << "Desktop mode for screen #" << i << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl << std::endl;
}
// get infos about the second screen
sf::Screen screen = sf::VideoMode::getScreenInfo(1);
std::wcout << "Monitor 1 name: " << screen.realName << std::endl
<< "x: " << screen.geometry.left << " y: " << screen.geometry.top << " width: " << screen.geometry.width << " height: " << screen.geometry.height << std::endl
<< "framerate: " << screen.frameRate << "Hz" << std::endl;
// open a window on the second monitor
sf::VideoMode videoMode(1280, 1024, 32, 1);
sf::Window window(videoMode, "Multi Monitor Example!" /*, sf::Style::Fullscreen*/);
// run the program as long as the window is open
while (window.isOpen()) {
// check all the windows events
sf::Event event;
while (window.pollEvent(event)) {
// "close requested" event: we close the window
if (event.type == sf::Event::Closed)
window.close();
else if (event.type == sf::Event::KeyReleased) {
// close if escape key was pressed
if (event.key.code == sf::Keyboard::Escape) {
window.close();
}
}
}
// display the windows content
window.display();
}
return 0;
}
Of course this is a proof of concept rather than a polished and finished implemention. It's supposed to be a base for further discussion about the API and the implementation. I know there are some dirty hacks in there (like including sf::Rect from the graphics package, exposing platform specific details in the public API or breaking compilation on Linux/Mac... ::)) but I'd still love to hear feedback (both on the implementation and the API). Also the code needs testing. I tryed it with my setup (win7 + NVIDIA GTX 970 + 3 monitors) and everything works as expected, but there are loads of different configurations and the winapi is quiet complicated, so if some more people with different configuration would test this, that would be awesome!
Ok I'd love to hear improvents, comments or criticsim. Let's get this feature implemented!
edit: also just noticed 400th post yay :D
-
I like it! Reminds me a bit of how GLFW does it.
I see 2 things I'd like to comment on:
1. I'd rename sf::Screen::frameRate to sf::Screen::refreshRate, to be a bit more semantically (is that the right usage? can never remember...) correct.
2. On that note, refresh rate is more of a video mode concept, not really a monitor concept. A monitor will define its max refresh rate, but a monitor can support more than 1 refresh rate. For instance, my monitors claim to support 60, 59, 50, 30, 29, and 25 (the last 3 being interlaced). But also support 75 Hz at 1280x1024 and below. So maybe refresh rate should be an addition to sf::VideoMode, ideally.
Now that I'm looking at it again...
3. I don't think sf::Screen should be a member/accessible through sf::VideoMode. I think you did for backwards compatibility, but that kind of feels backward. A video mode is dependent on a screen/monitor, not the other way around. That's my thoughts on it. I think it might make more sense to have sf::VideoMode through sf::Screen. For example:
std::size_t totalMonitors = sf::Screen::count();
for(std::size_t i = 0; i < totalMonitors; ++i)
{
std::vector<sf::VideoMode> modes = sf::Screen::get(i).getFullscreenModes();
for(auto& vm : modes)
{
std::cout << "Screen #" << i << ": " << mode.width << "x" << mode.height << " - " << mode.bitsPerPixel << " bpp @ " << mode.refreshRate << " Hz\n";
}
}
That's my idea for an API at least. That would allow for less modification to sf::VideoMode as well. Minus the additional refreshRate member. sf::Screen having a "maxRefreshRate" or something might not be a bad thing. Maybe a bit redundant though.
I do think sf::VideoMode should still be accessible without sf::Screen though. That would keep things backwards compatible. Just use the primary monitor maybe?
Anyways, those are my (much more numerous than I initially thought!) thoughts as of now... I'm interested in what other people say!
If/when a final public API is decided on, I'd love to contribute, maybe with a Linux implementation?
EDIT: Downloading/compiling your fork now.
EDIT 2: Working great here! (Win 8.1, 3 monitors, GTX970, unfortunately quite similar to your config, haha.) Changed which monitor to open the window on and that works great as well.
-
Thank you for testing and the feedback!
I like your thoughts about the API. Having sf::Screen seperate from sf::VideoMode sounds interesting. Although it might make the backwards compatibility confusing. (Being able to create video modes through sf::VideoMode and sf::Screen) But let's wait and see what other people think.
About the sf::Screen::frameRate. I agree that refreshRate is the more fitting name in this case. But I don't know if it should be kept in the struct. I initially thought you could use it to sync your rendering with the monitors refresh rate (using sf::Window::setFramerateLimit()), but of course that's what sf::Window::setVerticalSyncEnabled() is for...
Also I thought that I'm putting the monitors nativ refresh rate in there, but looking at the code again, I am actually putting the desktop modes refresh rate in. I don't know how useful that is. Maybe it could still be kept for informational purpose (maybe rename it to defaultRefreshRate).
I was able to get rid of the biggest hack. The platform specific device name is no longer exposed in the public API. I added a new priv::getDisplayDeviceFromId(unsigned int screenId) function to the windows implementation and seperated the deviceName into it's own vector. The API is now cleaner and platform-independent. Here is the new commit 86a6eb93 (https://github.com/Foaly/SFML/commit/86a6eb9367664f4a079acf4991e19ca1bf19f5a4).
I wanted to wait with the Linux implementation until we have decided on a final API, to keep the rewriting work low. I only had a quick look at the Linux code and it seems even more complicated :D Also I don't have a proper multi-monitor linux setup yet, so any help is appreciated.
-
I (also) think that hijacking the sf::VideoMode is not a very good design. Starting just with the intuition of "a screen has video modes" and not "video modes have screens", it could be quite confusing to use, because people would probably try to create sf::Screen objects on their own.
As for the backwards compatibility, there are multiple ways to add things and we can also mark some functions as deprecated etc. So maybe first worry about the nice API and then how it could potentially be added.
-
Hmm yeah the more I think about it the less confusing and better it seems to me to have a seperate sf::Screen class. So I am making a new proposal. The code API would look something like mentioned in dabbertorres post. A seperate class sf::Screen with two static methods for getting the screen count and a screen object based on its ID. A screen object would contain all the info (name, geometry, etc.) and the desktop and fullscreen modes. The VideoMode class would only be used to store the actuall video mode data (width, heigth, bpp, etc.). The VideoMode::getFullscreenModes() and VideoMode::getDesktopMode() would still return the modes to the primary screen to keep backward compatibility, but they would be marked as deprected and could be removed later.
Here is what the code would look like:
class Screen
{
public:
static unsigned int count();
static const Screen& get(unsigned int id);
const std::string name;
const FloatRect geometry;
const unsigned int frequency;
const std::vector<VideoMode> fullscreenModes;
const VideoMode desktopMode;
private:
Screen(std::string Name, FloatRect Geometry, unsigned int Frequency);
}
class VideoMode
{
public:
VideoMode(unsigned int modeWidth, unsigned int modeHeight, unsigned int modeBitsPerPixel = 32, unsigned int screenID = 0);
bool isValid() const;
SFML_MARKED_AS_DEPRECATED(static VideoMode getDesktopMode(););
SFML_MARKED_AS_DEPRECATED(static const std::vector<VideoMode>& getFullscreenModes(););
unsigned int width; ///< Video mode width, in pixels
unsigned int height; ///< Video mode height, in pixels
unsigned int bitsPerPixel; ///< Video mode pixel depth, in bits per pixels
unsigned int screenId; ///< The Id of the screen this video mode is assosiated with
}
So tell me your opinion. I would love to hear some feedback! What does the rest of the team think?
-
It looks nice! Kudos.
A few thoughts of mine:
- With Joystick, we have one class with only static member functions that takes an id as parameter. For consistency in the API, we could either have a similar pattern for sf::Screen or in the future (understand SFML 3) change the Joystick API to have a static get method. (Dunno which is best though.)
- The DPI could be added to the properties of sf::Screen.
- You use the term `geometry`. I'm just curious why. Is it because it's used in Windows API? (just curiosity here; on OS X it's `frame` but I'm fine with any of them).
- Some screen have some "widgets" (OS bar, "Dock" on OS X, ...) that take space where no window can be displayed. On OS X we have -[NSScreen visibleFrame] property for that -- would be nice to have something similar here too.
- The current API has no notification system for screen configuration change, such as scaling factor change (kind of DPI change) or virtual space reordering. This could be interesting to have in SFML as well.
-
Thanks for your thoughts!
I was looking at the Joystick API when I wrote the new proposal. But to me it seemed cleaner to have an immutable screen object, because a screens properties don't change (except for reconfiguration, but I come to that in a minute). So using sf::Screen::get(unsigned int) gets you an object with only const members and you can't construct one yourself.
For sf::Joystick it might make sense to use the other approach, because the joystick objects are constantly updated by the joystick manager and their values change frequently. Then again it might only be a leftover from the 1.6 sf::Input class (http://www.sfml-dev.org/documentation/1.6/classsf_1_1Input.php) which has not been refactored properly when it was put into a seperate class. I don't know. So maybe we should take a closer look for SFML 3.
I added DPI to the class. Good idea! ;)
I actually took the term 'geometry' from Qt ::) I thought it was quiet fitting because it includes both the position and the size. I don't think 'frame' would be better because it is associated with lots of other things. But I'm of course open for suggestions.
Of course we can add a property that specifies the usable area. I left it out originally, because in my first draft the screen was associated with a VideoMode and used mostly for the fullscreen stuff, so it didn't make sense. Now that we have sf::Screen seperate, I'll put it back in. In Qt it's called 'availableGeometry' by the way.
I really like the idea of being able to react to hotplugging/screen resolution changes/rearrangement in virtual screen space/scaling factor changes. I think it would really improve SFML in terms of a multimedia library. You can hotplug all input devices (mouse, keyboard, joystick, sound input), so why not have it for the output devices? I already looked into it for windows and it is defintly possible. But I guess it's not that important for most of the use cases (games). It is more something for multimedia installation or multi-screen apps involving beamers and multiple screens. Also this brings up new questions. For example: Should we put screen events into the window event queue? So I guess we leave that for later and focus on getting basic multi monitor support running. Reacting to events can always be added later.
So if noone else has anything to add I would start rewriting the API/windows implementation to match the proposal. I have some time during the next two weeks that I would like to dedicate to this. Also set up a linux testing rig, so I might start the linux implementation too.
-
I'll just chime in and say I agree with what eXpl0it3r suggested. It indeed makes more sense to have screens containing video modes and not the other way around (it is the way I would design the API).
One thing I would suggest however, add a flag (boolean) to the screen struct that will determine if it is the primary screen or not. Also is the Screen::geometry supposed to be the bounds of the monitor? If it is wouldn't a IntRect make more sense as the type? And I would also suggest naming it 'bounds' / 'workingBounds' or 'area' / 'workingArea' compared to 'geometry'. Or maybe I am just biased... (https://msdn.microsoft.com/en-us/library/system.windows.forms.screen.aspx) ::)
-
Then again it might only be a leftover from the 1.6 sf::Input class which has not been refactored properly when it was put into a seperate class. I don't know. So maybe we should take a closer look for SFML 3.
Yep, most probably. But that's also a nice way to deal with changes/invalidation. Actually we would have some issues if the system is reconfigured and we don't notify the user: what would happen if a screen is moved around, or unplugged? With the current API we would have a static view of the screen system when the application is started.
Should we put screen events into the window event queue? So I guess we leave that for later and focus on getting basic multi monitor support running. Reacting to events can always be added later.
Currently we have joystick events connected to the window so we could do the same for now (or leave it out for a second pass -- I'm totally fine with that) but at the same time having those notifications polled from a window feel kind of weird... They are more system-wide than window-specific.
I would also suggest naming it 'bounds' / 'workingBounds' or 'area' / 'workingArea' compared to 'geometry'. Or maybe I am just biased... (https://msdn.microsoft.com/en-us/library/system.windows.forms.screen.aspx) ::)
Actually, now that you mention 'bounds'... it would be more consonant with `getGlobalBounds` than `geometry`.
-
I think screen events should be received by all windows. Input events only by the active one. (This might change current behavior? I'm not sure right now.)
-
What I meant by system wide was a system that will notify the user even if no windows are opened.
-
Ah, I see. That could be tricky (and would be something completely new).
But if you think about it, sensors should go into something global like that as well. You can't have a set of sensors per window (assuming you could own more than one window on mobile ports).
-
And continuing the logic of how inputs are global.... <joke>Mouse events are given in desktop coordinates and then converted to window coords so mouse events should be global also.</joke> Just employ KISS and handle all events in each window.
If do think about adding 'global events' add to the that fonts installed/uninstalled, power mode changes, logout/shutdown, and time changed.
https://msdn.microsoft.com/en-us/library/microsoft.win32.systemevents%28v=vs.110%29.aspx
-
Alrighty it took a little longer than I thought, but here is the new implementation: commit 3902d95d (http://"https://github.com/Foaly/SFML/commit/3902d95d22199da2b4051012668f666078c2ab83") on a new branch (http://"https://github.com/Foaly/SFML/tree/multi-monitor").
Here is my testing code:
#include <SFML/Window.hpp>
#include <iostream>
int main()
{
// query screen count
std::size_t totalMonitors = sf::Screen::count();
std::cout << "# of connected Screens: " << totalMonitors << std::endl;
// enumerate over all screens
for(std::size_t id = 0; id < totalMonitors; ++id)
{
sf::Screen screen = sf::Screen::get(id);
const std::vector<sf::VideoMode> modes = screen.getFullscreenModes();
std::cout << "Valid resolutions for screen " << modes[0].screenId << std::endl << std::endl;
// fullscreen modes
for(std::size_t i = 0; i < modes.size(); ++i)
{
sf::VideoMode mode = modes[i];
std::cout << "Mode #" << i << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl;
}
std::cout << std::endl;
// desktop mode
sf::VideoMode mode = screen.getDesktopMode();
std::cout << "Desktop mode for screen #" << id << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl << std::endl;
// screen information
const sf::IntRect bounds = screen.getBounds();
const sf::IntRect workingArea = screen.getWorkingArea();
std::wcout << "Monitor " << screen.getId() << " name: " << screen.getName() << std::endl
<< "x: " << bounds.left << " y: " << bounds.top << " width: " << bounds.width << " height: " << bounds.height << std::endl
<< "x: " << workingArea.left << " y: " << workingArea.top << " width: " << workingArea.width << " height: " << workingArea.height << std::endl
<< "refreshRate: " << screen.getRefreshRate() << "Hz"
<< " dpi: " << screen.getDpi().x << "x" << screen.getDpi().y << std::endl << std::endl << std::endl;
}
// backward compatibility with depreaction note
sf::VideoMode::getDesktopMode();
// open a fullscreen window on the second screen
sf::VideoMode videoMode(1024, 768, 32, 1);
sf::Window window(videoMode, "Multi Monitor Example!" , sf::Style::Fullscreen);
window.setFramerateLimit(sf::Screen::get(videoMode.screenId).getRefreshRate());
// run the program as long as the window is open
while (window.isOpen()) {
// check all the windows events
sf::Event event;
while (window.pollEvent(event)) {
// "close requested" event: we close the window
if (event.type == sf::Event::Closed)
window.close();
else if (event.type == sf::Event::KeyReleased) {
// close if escape key was pressed
if (event.key.code == sf::Keyboard::Escape) {
window.close();
}
if (event.key.code == sf::Keyboard::Return) {
window.create(videoMode, "Multi Monitor Example", sf::Style::Resize);
window.setFramerateLimit(sf::Screen::get(videoMode.screenId).getRefreshRate());
}
}
}
// display the windows content
window.display();
}
return 0;
}
As you can see I had to change the API a little bit. Originally I wanted sf::Screen to be an immutable object with public const attributes (see above). Apperently there is no way to make an object like this copy-/assignable in C++. So I had to go with an object that has private members and getters. If somebody has a better suggestion I'd be glad to hear it!
So this is what the current API looks like:
class Screen
{
public:
static std::size_t count();
static const Screen& get(unsigned int id);
std::wstring getName() const;
unsigned int getId() const;
const IntRect& getBounds() const;
const IntRect& getWorkingArea() const;
unsigned int getRefreshRate() const;
Vector2u getDpi() const;
bool isPrimary() const;
const std::vector<VideoMode>& getFullscreenModes() const;
const VideoMode& getDesktopMode() const;
private:
friend class priv::ScreenImpl;
Screen(std::wstring name, unsigned int id, IntRect bounds, IntRect workingArea, unsigned int refreshRate, Vector2u dpi, bool isPrimary, std::vector<VideoMode> fullscreenModes, VideoMode desktopMode);
void updateId(unsigned int id);
////////////////////////////////////////////////////////////
// Member data
////////////////////////////////////////////////////////////
std::wstring m_name; ///< Name of the screen
unsigned int m_id; ///< Id of the screen
IntRect m_bounds; ///< Bounds of the screen in virtual screen space
IntRect m_workingArea; ///< Working area of the screen in virtual screen space
unsigned int m_refreshRate; ///< Refresh rate of the screen
Vector2u m_dpi; ///< Dpi (dots per inch) of the screen
bool m_isPrimary; ///< Is this the primary screen?
std::vector<VideoMode> m_fullscreenModes; ///< Supported fullscreen modes of this screen
VideoMode m_desktopMode; ///< Desktop mode of the screen
};
I changed a couple things: Reworked the code into the new design, changed the implementation quiet a bit (added dpi and workingArea for example), added a SFML_DEPRECATED macro, moved <SFML/Graphics/Rect.hpp> to <SFML/System/Rect.hpp>, removed the obsolete <Window/Win32/VideoModeImpl.hpp> and some more stuff.
I found the names bounds and workingArea most fitting, so I used those :)
Since quiet a lot changes I'd be happy when some people would test my code and give me some feedback. (For example if the dpi is working on pre win8). It would also be nice if someone from the team could take a look at the implementation and tell me if I got implementation right (especially the inheritance for the platform specific implementation).
[...]we would have a static view of the screen system when the application is started.[...]
Yep thats the current state. I did a bt of reading and it seems that on windows a WM_DEVICECHANGE message (https://msdn.microsoft.com/en-us/library/aa363480%28VS.85%29.aspx) is send when a monitor is plugged in or out. So we could react to that and reinitilize the screens. I didn't find anything concerning rearranging in virtual screen space yet.
Like I said I am all for supporting a way to deal with change. I also think we can stick it into the window event loop, because realistically what are you gonna do as a reaction to a screenChangedEvent if not adjusting your windows?
But of course if there is more global event stuff (like sensors) it might be worth rethinking the window event queue. But that's for somebody else to decide :D
I'd say for now we should focus on getting the API and the implementation setteled. The reaction to events can easily be added later.
So check it out and tell me what you think!
-
Originally I wanted sf::Screen to be an immutable object with public const attributes (see above). Apperently there is no way to make an object like this copy-/assignable in C++. So I had to go with an object that has private members and getters. If somebody has a better suggestion I'd be glad to hear it!
Const instances containing non-const members. Which means that you can construct and manipulate a sf::Screen instance as you wish, but those returned by the SFML API are read-only.
-
OK I am not sure if I understand it correctly. Do you mean I should have the members (name, bounds, etc.) public and non-const and have Screen::get() return a const reference? Are the member of a object obtain by get not changeable in this case?
-
Do you mean I should have the members (name, bounds, etc.) public and non-const and have Screen::get() return a const reference?
Yes. Or a copy, it doesn't really matter.
Are the member of a object obtain by get not changeable in this case?
You can't modify a const [reference/pointer to an] object, that's what this keyword is made for ;)
-
Thanks for the clarification! I sometimes get a little confused with the const rules ::)
Just to be sure here: you also think that public members instead of getters are the better design, right? If so I'll change that tonight.
-
you also think that public members instead of getters are the better design, right?
Yes.
-
Alright! I implemented the change. The new commit can be found here (https://github.com/Foaly/SFML/commit/ee27023318d223de9dc91ef4e149aa31d0a19f5f).
Updated testing code:
#include <SFML/Window.hpp>
#include <iostream>
int main()
{
// query screen count
std::size_t totalMonitors = sf::Screen::count();
std::cout << "# of connected Screens: " << totalMonitors << std::endl;
// enumerate over all screens
for(std::size_t id = 0; id < totalMonitors; ++id)
{
sf::Screen screen = sf::Screen::get(id);
const std::vector<sf::VideoMode> modes = screen.fullscreenModes;
std::cout << "Valid resolutions for screen " << modes[0].screenId << std::endl << std::endl;
// fullscreen modes
for(std::size_t i = 0; i < modes.size(); ++i)
{
sf::VideoMode mode = modes[i];
std::cout << "Mode #" << i << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl;
}
std::cout << std::endl;
// desktop mode
sf::VideoMode mode = screen.desktopMode;
std::cout << "Desktop mode for screen #" << id << ": "
<< mode.width << "x" << mode.height << " - "
<< mode.bitsPerPixel << " bpp" << std::endl << std::endl;
// screen information
const sf::IntRect bounds = screen.bounds;
const sf::IntRect workingArea = screen.workingArea;
std::wcout << "Monitor " << screen.id << " name: " << screen.name << std::endl
<< "x: " << bounds.left << " y: " << bounds.top << " width: " << bounds.width << " height: " << bounds.height << std::endl
<< "x: " << workingArea.left << " y: " << workingArea.top << " width: " << workingArea.width << " height: " << workingArea.height << std::endl
<< "refreshRate: " << screen.refreshRate << "Hz"
<< " dpi: " << screen.dpi.x << "x" << screen.dpi.y << std::endl << std::endl << std::endl;
}
// backward compatibility with depreaction note
sf::VideoMode::getDesktopMode();
// open a fullscreen window on the second screen
sf::VideoMode videoMode(1024, 768, 32, 1);
sf::Window window(videoMode, "Multi Monitor Example!" , sf::Style::Fullscreen);
window.setFramerateLimit(sf::Screen::get(videoMode.screenId).refreshRate);
// run the program as long as the window is open
while (window.isOpen()) {
// check all the windows events
sf::Event event;
while (window.pollEvent(event)) {
// "close requested" event: we close the window
if (event.type == sf::Event::Closed)
window.close();
else if (event.type == sf::Event::KeyReleased) {
// close if escape key was pressed
if (event.key.code == sf::Keyboard::Escape) {
window.close();
}
if (event.key.code == sf::Keyboard::Return) {
window.create(videoMode, "Multi Monitor Example", sf::Style::Resize);
window.setFramerateLimit(sf::Screen::get(videoMode.screenId).refreshRate);
}
}
}
// display the windows content
window.display();
}
return 0;
}
It would be cool if someone with win7 or older could test this (even with 1 monitor) to see if the dpi is working correctly. Maybe also someone with a high dpi mointor. As always I am open for feedback :)
-
First, thanks a lot for your effort on suggesting a new API and writing the code. I like the overall approach, but I have some comments regarding your implementation:
Please keep the deprecation of existing features separate from your feature's pull request. We haven't fully decided what approach to choose yet, as can be seen from this discussion (http://en.sfml-dev.org/forums/index.php?topic=17888.msg128426#msg128426). I'll try to come up with some actual code on the weekend, as there are meanwhile quite a few features requiring deprecation. If people agree, you could then base your code on that :)
If you really need to move <SFML/Graphics/Rect.hpp> to <SFML/System/Rect.hpp>, make sure Git recognizes it as a rename and not add+delete, so that the history of the file is preserved. And do it in a separate commit, too.
#warning is not a portable preprocessor directive.
Furthermore, a public constructor with 9 parameters is not useful. It's too long, and calls don't look expressive at all, as the arguments are not named. Why is it even public? I think users are supposed to access sf::Screen instances only via static get() method?
The name Screen::count() is not consistent with other SFML functions returning an array size.
What is the "Id" (should be "ID" in the doc, "Id" in code is fine) of the screen? The documentation should make this more clear. Are the integers continuous, starting from 0?
Top-level const qualifiers in return types are useless:const std::wstring getDisplayDeviceFromId(unsigned int screenId);
-
If IDs are actually indices, I think they should be named "index", this makes it clearer that they are in range [0 .. count()[ and not some random unique identifiers.
Why std::wstring? It's the least defined/portable/usable thing in the C++ world, and we have sf::String.
-
Thanks for the review and the feedback! It's nice to hear my work is appreciated :)
I must have missed that there was a separate discussion for that. I left a post with my code there. Of course that belongs in a separate commit! I'll remove it, leave a comment in the doc and wait until something proper has been added.
Since sf::IntRect is needed for the sf::Screen class, I thought it would be the best to move Rect.hpp into the System class. It seemed to be most fitting since sf::Vector is already there. But of course I will rework it into a separate commit using git mv, so the history isn't lost.
I know #warning isn't a standard preprocessor directive, but all the available compilers support it and I didn't know any other way to issue a compile time warning.
I know a constructor with that many arguments is ugly and not expressive, but I didn't really know how else to do it. Do you have a suggestion? I can make it private again, but the way I understood Laurent it's OK if the user can construct and manipulate a sf::Screen instance as they wish as long as the ones returned by SFML are immutable.
What do you suggest for a consistent naming? The sf::Shape classes have getPointCount(), sf::SoundBuffer has getSampleCount(), etc. but I think that was chosen because it's not clear what sf::Shape::count() or sf::SoundBuffer::count() is supposed return. For sf::Screen::count() it's quiet clear I'd say. But of course I am open for a name that fits better into the consisting API. Also returning std::size_t would be a more consistent right?
I haven't written all of the documentation yet, because I didn't want to rewrite everything while the API is still changing. The idea behind the ID is that you can find the screen belonging to a videomode (to get the screen from mode and to verify the screen supports the mode). The ID's are continuous in range [0 .. count()[. The primary screen is always at index [0] and the rest is sorted from left to right in virtual screen space. I will add some documentation about that. What do others think is index the better name for this?
About const std::wstring ... That used to be a reference, so the const made sense back then. I'll remove it. Of course std::wstring doesn't make sense in the public API, since there is sf::String for that. It's also a unfinished left over and I'll change that as well.
-
I know a constructor with that many arguments is ugly and not expressive, but I didn't really know how else to do it. Do you have a suggestion? I can make it private again, but the way I understood Laurent it's OK if the user can construct and manipulate a sf::Screen instance as they wish as long as the ones returned by SFML are immutable.
What about keeping only the default constructor and setting manually the attributes? For such data record it feels alright (nice tradeoff between simplicity and cleanness). Also it's consistent with VideoMode which has a default ctor and public, mutable attributes.
and the rest is sorted from left to right in virtual screen space.
I would remove this requirement for simplicity and avoiding ambiguities (e.g. all screens are vertically stacked).
-
Other options (not saying they are good/bad, just pointing them out) would be:
- a constructor with many arguments but many of them with a default (I would actually consider this bad, but it's an option).
- a constructor taking a single struct rather than many individual arguments.
- multiple constructors taking various number of arguments and then having the object being limited in what it can "answer" based on what ctor was called.
Someone else can hopefully provide other/better options, but I just wanted to point out that there are many ways to "cut the cake" :)
-
Alright I found some time to implement the discussed changes. New commit. (https://github.com/Foaly/SFML/commit/d3ce2a2b3854ae8b8cb13fd844bd96f1244301db)
The deprecation macro and std::wstring are gone. I added quiet a bit of documentation (mostly about the sf::Screen and sf::VideoMode class). If someone could proof read it, that would be great!
It finally made click in my head and I understood that the sf::Screen object returned by SFML through get() is protected by being a const reference. So there is no need have a seperate constructor. I removed it like Hiura suggested. Something was scrambly there in my head :D
I made a seperate commit (https://github.com/Foaly/SFML/commit/b706e0c4fedcbf77195178f88d5b49c58b7946bb) for moving sf::Rect, but it didn't go as expected. Eventough I used git mv include/SFML/Graphics/Rect.hpp include/SFML/System/Rect.hpp and created a new file, it still looks like I edited the files content on GitHub. Apparently git doesn't like it if you move and create a file with the same name. Also GitHub doesn't show a moved files history. If I use git log --follow include/SFML/System/Rect.hpp the history is all there, but on GitHub there is nothing to see. I asked on SO (https://stackoverflow.com/questions/32738168/how-to-move-a-file-while-preserving-the-history-and-create-one-with-the-same-n), but there seems to be no solution.
About the sorting: I think we should provide a consistent order in which the monitors are returned. Otherwise it will be dependend on the OS or even be at random, which I don't think is very clean. Since most setups on a desk are side by side, I thought sorting them from left to right would be the way to go. But of course I can also add a check for sorting them top to bottom incase they have the same x value.
If some more people could do some testing and check if everything works like they'd expect, that would be awesome. Even if you only have 1 monitor or if you have a DPI screen or a win7 or a TV/projector connected as a second screen. Anything would help :)
-
Now sf::Screen still has a public default constructor... Why is that needed? Let's not just add it for convenience -- if we can enforce class invariants (initialized state) with the get() method, let's do that. We can still provide constructors later, if we clearly see a need for it. Don't hesitate to use friend to increase encapsulation. Contrary to popular belief, friendship is not bad design per se.
About count(), as stated earlier, it's inconsistent naming -- it is a verb that counts the number of screens, as opposed to getCount() which returns the count (=number, a noun).
There's quite a few unneeded sf:: prefixes.
I'll try to have a look at the rest and the functionality later.
-
Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl. The ensureScreenListInitilized() is implemented as a function in an anonymous namespace in src/SFML/Window/Win32/ScreenImpl.cpp. So if sf::Screens constructor was private I would have to move ensureScreenListInitilized() to a private method in sf::ScreenImpl and make the two classes friends. priv::getDisplayDeviceFromId() needs access to ensureScreenListInitilized so I would have to move that into sf::ScreenImpl too. But it's a windows specific function, so I don't know if that's very clean... Do you think that would be better? I agree that it doesn't make much sense to let users create their own sf::Screen objects.
I think sf::Screen::count() makes sense and is expressive, but if it's inconsistent with the rest of the API I will change it. Do other think the same?
I'll removed all the unnecessary sf:: I could find. Tell me if I missed any.
Awesome! I'll be glad to hear some feedback.
Here is the new commit (https://github.com/Foaly/SFML/commit/9d5a3d6924bc34f762f68301111187daf083c842).
-
About sorting: it can be added later for all OSes in sf::Screen::get if we really want to. The thing is, monitors don't need to be aligned so a top-down, left-right ordering doesn't always results in something meaningful.
Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl.
How so? `sf::Screen` is currently empty (https://github.com/Foaly/SFML/commit/9d5a3d6924bc34f762f68301111187daf083c842#diff-8a6c0417bb38e32eac574a2c9c5ab06eR35) as far as I can tell so removing it completely should not be an issue.
Think of the get function as a factory method that will properly set up all attributes. Even if they are initially initialized to garbage (i.e. with the compiler generated constructor) that's not important since they are then properly updated.
-
Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl.
The encapsulation is actually worse: you expose a method to the public API that is only needed by the implementation.
friend is really the way to go here. There's a nice idiom that avoids additional dependencies just for friendship: accessor structs.
namespace priv
{
struct ScreenAccess
{
static Screen construct();
};
}
class Screen
{
friend struct ScreenAccess;
};
I think sf::Screen::count() makes sense and is expressive, but if it's inconsistent with the rest of the API I will change it. Do other think the same?
The coexistence of get() and getCount() is not really nice indeed. There are not many APIs of this style in SFML, so it's a bit difficult to compare... We should keep in mind that in the future, we would need to name others similarly, too.
Awesome! I'll be glad to hear some feedback.
I tested the API on Windows 10 with two screens, it works great. Thanks a lot for your contribution.
I created a Gist (https://gist.github.com/Bromeon/6c60546f8a1d8fe6f0e8) based on your code, which shows all the screens' capabilities. Everybody can use that for future testing.
Feedback -- some tips are really minor and not too relevant for this example, but maybe they'll help you also in other code:
- There's hidden console output that must be removed. Any error output must go to sf::err(), not standard streams.
- As mentioned by Laurent, the screen's index should be called "index" and not "id", as that indicates continuous integer ranges starting at 0.
- I wouldn't use std::wstring unless really necessary. We have sf::String.
- "initilized" -> "initialized"
- Currently the window constructor falls back to the default screen if the specified screen index is not supported. This is probably okay regarding that we have no error reporting mechanism, but for SFML 3 we might consider an exception.
- The difference between bounds and working area (without task bar) is not clear from the documentation.
- if conditions with a single statement should not be wrapped in {}, according to SFML's code style.
- Sometimes two empty lines, "if(" and other minor syntactical mistakes.
- ensureScreenListInitialized() is huge, what about splitting it into multiple functions? Then we can initialize different sf::Screen attributes right ahead, instead of copying everything at the end of the function.
- Instead of if (!std::find(/* videoMode */)) modes.push_back(videoMode) you can simply push_back() all modes, and use std::sort() + std::unique() + std::remove().
-
Thanks for the review and the detailed feedback!
Accessor structs look like a nifty little pattern. I've never heared about those. Thanks for the tip! I went ahead made the constructor private and added a ScreenAccess struct. I put it into the Screen.hpp because having two seperate files for such a small thing seemed like overkill to me. (I kept the constructor because I think an object should always be in a valid state and not have any bogus data, even if it is overwritten directly afterwards.)
The coexistence of get() and getCount() is not really nice indeed.
I agree with that. So what's you verdict on this? Leave it as it is?
- That was for developing. I took it out and redirected any errors to sf::err().
- Replaced.
- Done.
- Done.
- Well I thought that that would be the best behaviour in case of an error. But it could definitly be considert for an excetion.
- Do you mean here (https://github.com/Foaly/SFML/blob/a57066ff32b1ab8fbf6023c4df475e39858ba755/include/SFML/Window/Screen.hpp#L154)? I rewrote it a bit, but I don't really know how else to put it. What's not clear?
- I went over the style guide again and I hope I didn't miss anything.
- Same.
- Hmm I admit its pretty big, but the problem is that some calls depend on each other and some calls have to be made in a certain order, so it's not that easy to separate them. I documented everything to make it as clear as possible. But I'll look into it again.
- I took the code from sf::VideoMode::getFullscreenModes(), but yours is more elegant and efficient. Done
I tested the API on Windows 10 with two screens, it works great. Thanks a lot for your contribution.
Thank you! Thats great to hear.
I rewrote the example in the documentation based on your gist.
So here is the new commit (https://github.com/Foaly/SFML/commit/a57066ff32b1ab8fbf6023c4df475e39858ba755).
I'd like to know what others think about the sorting. Should it be removed or not?
I also made another commit (https://github.com/Foaly/SFML/commit/6e43e75284849ca96cb34085bf74fd6f36fb5f73) that removes the limitation of being able to create only one fullscreen window. So you can now create a fullscreen windows on each of your screens at the same time! I left it in a seperate commit because it is easier to review this way. I'll squash them once it is reviewed.
-
Accessor structs look like a nifty little pattern. I've never heared about those. Thanks for the tip! I went ahead made the constructor private and added a ScreenAccess struct.
That's a nice pattern, but there's no need to overuse it when it's not necessary -- it's also good if we can avoid the extra code (especially if you put it in a public header, which I guess was not what Nexus implied ;)). Here it doesn't hurt to have the whole sf::ScreenImpl as a friend of sf::Screen, with a simple forward-declaration of it in the public header.
I agree with that. So what's you verdict on this? Leave it as it is?
No. A function should be a verb, that's a mandatory rule of the style guide (well, in case it's not, it should). Maybe it's "get" which should be renamed? Or maybe we can simply merge them in a single accessor which returns a std::vector<sf::Screen>, like sf::VideoMode does?
-
I kept the constructor because I think an object should always be in a valid state and not have any bogus data, even if it is overwritten directly afterwards.
But, an empty user-defined default constructor is exactly equivalent to the one the compiler will generate for you if you leave it out. With the little twist that user-defined constructors are, by definitian, never trivial (even if they are empty) but the compiler generated one is (if left out or defined as "= default;").
-
I don't understand why you all want to remove the default constructor. Since the class has primitive type members which won't get initialized properly, to me it is mandatory to write a constructor to ensure a valid state -- which is exactly what Foaly argued.
-
I suggested removing the ctor initially because there already is a factory method: get. Having this extra struct only add one indirection. This approach would be similar to sf::Event and pollEvent/popEvent combo.
Also, having it private prevents people from having a screen instance that gets properly setup later on, doesn't it?
-
6. Do you mean here? I rewrote it a bit, but I don't really know how else to put it. What's not clear?
I didn't see that, I assumed the member would only be documented in the member's documentation itself. Your description is fine (except for "screens bounds" which should be "screen's bounds").
9. Hmm I admit its pretty big, but the problem is that some calls depend on each other and some calls have to be made in a certain order, so it's not that easy to separate them. I documented everything to make it as clear as possible. But I'll look into it again.
Why are dependencies and order a problem? You can of course preserve them. I'm aware that you may need to pass several arguments, but often this is still better than a massive block of code, because it makes clear which function changes what.
I don't understand why you all want to remove the default constructor. Since the class has primitive type members which won't get initialized properly, to me it is mandatory to write a constructor to ensure a valid state -- which is exactly what Foaly argued.
Yes, but it should be private. Ironically, the argument "valid state" is actually a counterargument, because the default constructor creates a semantically invalid screen instance, which refers to no real screen. The whole point behind constructors however is to initialize instances to a valid state.
Whether the attributes are in a defined but undocumented state, or in an undefined state, doesn't matter in practice. Users can't make any assumptions about uninitialized instances. All they can do is assign a valid one.
Also, having it private prevents people from having a screen instance that gets properly setup later on, doesn't it?
It does, but it's not a library writer's task to provide a null state for every object. In my opinion, this is already used far too much for convenience, with the effect that we give up class invariants. In this case, it's even worse because there is no officially documented way to check for a null state. This whole discussion has a bigger scope than just sf::Screen, as there are several classes in SFML doing the same. Maybe we should bring it up again for SFML 3...
Java designers also thought making every object reference nullable would be a good idea, with the result that NullPointerException is an omnipresent bug in Java development. The effect was so severe that various libraries began to provide half-hearted solutions (@Nullable and @NonNull annotations for static code analyzers). This is something you don't have in C++, and I'm really glad about it.
In a near-future C++ version (probably an intermediate library fundamentals technical specification), we will have std::optional which is exactly designed to provide optional state. boost::optional has already existed for ages.
A different question: Does it even make sense when the user creates copies of sf::Screen objects? There is no point in modifying one. If every screen is saved globally, wouldn't const pointers and references suffice?
-
If every screen is saved globally, wouldn't const pointers and references suffice?
It would. But it would force users to use pointers sometimes, are you ok with that?
Unrelated comment: wouldn't a static getPrimaryScreen (or index) function be better than a isPrimary member? You usually want to get the primary display directly, not know if a given display is primary or not.
So we could imagine...
class Screen
{
public:
static const Screen* getPrimary();
static std::vector<const Screen*> getAll();
private:
...
};
I'm not sure if I prefer this design, this is just an idea...
-
It would. But it would force users to use pointers sometimes, are you ok with that?
If there's no point in making a copy, yes. sf::Screen being noncopyable also makes clear that users are supposed to treat it as an immutable object retrieved only through SFML, not something they can instantiate and control themselves.
In other words, it's then very similar to the classes sf::Mouse, sf::Keyboard, sf::Joystick, sf::Sensor, sf::Touch and future sf::Clipboard: global access points to features that exist only once on the system, i.e. kind of singletons. The only reason why we can't make sf::Screen the same is because there can be multiple screens, so we need the get() indirection. I don't see why anybody would save a screen, if SFML stores it already and modifications to copies make no sense.
Unrelated comment: wouldn't a static getPrimaryScreen (or index) function be better than a isPrimary member? You usually want to get the primary display directly, not know if a given display is primary or not.
That sounds good. We should return references though, to signal that they're never null. People who need to be flexible can still take the address.
I'm not sure if the isPrimary variable should be left in the class. Already now, the index variable is strictly speaking redundant. But I can imagine situations where both can be handy.
Alternative method names to get would be getNth (borrowed from std::nth_element()), getAt, getByIndex... But none of them except maybe the first pleases me too much. Providing a STL container interface on the other hand allows for things like:
for (const sf::Screen* screen : sf::Screen::getAll())
{
screen->xy;
}
auto&& screens = sf::Screen::getAll();
std::for_each(screens.begin(), screens.end(), function);
boost::for_each(sf::Screen::getAll(), function);
instead of:
for (std::size_t i = 0; i < sf::Screen::getCount(); ++i)
{
const sf::Screen* screen = sf::Screen::getNth(i);
screen->xy;
}
??? // No STL or Boost algorithm can be applied
-
Commuting right now so ill make even shorter than usual :P
Having a custom ctor, even if it is made non copyable, doesn't allow us to enforce any invariant: the data is still publicly modifiable. The way I see it, sf::Screen is just some POD, nothing more. Which means we don't need an 'invalid' state: an 'undefined' state is enough (which is not probable). It simple enough to let users take care themselves of using valid data by calling get. Would that be really a bad design IYO?
Also, I fear that using ref/ptr/static data will make our job harder latter if we want to introduce screen events /state change /data invalidation. We should probably look more into what I mentioned in one of my first posts: the joystick API with only static function to access information (which, as a bonus, would solve the issue of invariant).
-
Having a custom ctor, even if it is made non copyable, doesn't allow us to enforce any invariant: the data is still publicly modifiable.
No. We have no public constructor, and the static sf::Screen methods only give away const pointers and const references. Users thus cannot modify the objects.
We should probably look more into what I mentioned in one of my first posts: the joystick API with only static function to access information (which, as a bonus, would solve the issue of invariant).
Yes, but:
In other words, it's then very similar to the classes sf::Mouse, sf::Keyboard, sf::Joystick, sf::Sensor, sf::Touch and future sf::Clipboard: global access points to features that exist only once on the system, i.e. kind of singletons. The only reason why we can't make sf::Screen the same is because there can be multiple screens, so we need the get() indirection.
Also, I fear that using ref/ptr/static data will make our job harder latter if we want to introduce screen /state change /data invalidation.
Hm, really good point. What if the user plugs in another screen while the program runs?
Maybe it's not a good idea to store the current screen configuration inside sf::Screen in the first place. Instead, it would be recomputed on every call, and the user is responsible for storing it. In that case, value semantics would make sense again.
-
What if the user plugs in another screen while the program runs?
On OS X it's even easier to change the screen configuration: we can change the "scaling factor" of the screen.
This fact is behind my whole reasoning: with static data in the implementation and indirect access (ptr/ref), even if it's read-only, we are stuck with the old dangling pointer thingy and UB. Returning copies of the data would simplify just A LOT the impl and users' life.
Just to be clearer on what I meant by the Joystick API, in case there was some misunderstanding, is that you fetch each information through a getter such as getAxisPosition(index, axis) or getIdentification(index), which returns a POD and is somewhat similar to the currently proposed sf::Screen::get(index).
-
This fact is behind my whole reasoning: with static data in the implementation and indirect access (ptr/ref), even if it's read-only, we are stuck with the old dangling pointer thingy and UB. Returning copies of the data would simplify just A LOT the impl and users' life.
I totally agree. Initially, I didn't think of configurations changing at runtime.
Just to be clearer on what I meant by the Joystick API, in case there was some misunderstanding, is that you fetch each information through a getter such as getAxisPosition(index, axis) or getIdentification(index), which returns a POD and is somewhat similar to the currently proposed sf::Screen::get(index).
Okay... However this is one step back from object-oriented programming, when users are not able to treat attributes in a combined way.
Classes with only static functions are nothing more than namespaces. That's not bad per se, but sometimes singletons would be more flexible. sf::Mouse::getInstance() for example would allow things like lazy initialization, and it could be fitted with state that is not purely global but depending on the object (e.g. for multiple mice). Same for joystick, it might have been more intuitive to provide sf::Joystick::get() and let the user access actual sf::Joystick instances. But that's another discussion...
It is however important to see that analogous to the sf::Joystick API would not be sf::Screen::get(index) but rather sf::Screen::getName(index), sf::Screen::getDpi(index), sf::Screen::getFullscreenModes(index), etc.
I'm not sure whether we should use the sf::Joystick API for sf::Screen. I think it mostly depends on whether a user is supposed to store some of the values (which would be much easier if there were an object), or should we merely provide a global access point for currently connected screens, with their current configuration? Maybe that's not the worst idea, as that would make sure users retrieve always the latest, most up-to-date information.
-
Yeah, I agree, the usage is different between Screen and Joystick: in the latter case you are interested most of the time in one information only (one button, the identification data of the device, ...), while you pretty much want to know everything about your screen settings. Having one getter that returns a POD with name, dpi and whatnot, like we actually do with Joystick::getInformation, would make it easier for the user to compare screens.
or should we merely provide a global access point for currently connected screens, with their current configuration?
Like the getAll() function Laurent mentioned? I think that would be good: the impl and the users wouldn't have to deal with screen reconfigurations between calls to get(index), even though it's relatively unlikely. But more importantly, the user will probably loop on the Screen to find the best one anyway.
-
I think that would be good: the impl and the users wouldn't have to deal with screen reconfigurations between calls to get(index), even though it's relatively unlikely.
Good point again. I wonder however what happens if a screen is reconfigured during such a call? I don't think there's a way to provide atomicity at this level...
So far, I like Laurent's API the most. Taking into account that screen configurations may change during the program, it may really be worthwhile to work with copies and not cache any data within sf::Screen.
class Screen
{
public:
static Screen getPrimary();
static std::vector<Screen> getAll();
};
The alternative is that the sf::Screen implementation has a means of noticing configuration changes, and can update its internal structures (automatically or on demand). This adds more complexity to the implementation and still introduces usability issues ("is the pointer I have saved still valid?"), so I'm not sure if we should go with that.
Interestingly, the issue of changing configuration in-use exists for sf::Joystick, too. So far, probably no one was unlucky enough to experience it :)
-
First of all I'd like to say that it's great to see an active discussion on this topic. I'm not a library designer but I'll still throw in some of my thoughts.
I like the idea that you can use getAll() and a range based for loop to iterate over the screens. Although I can't think of many use cases where you'd want to use STL algorithms on screens. Never the less I think it's good practice to support them.
getPrimary() is also a good idea, because right now there are already some spots in the code where sf::Screens::get(0) is used to get the primary screen. getPrimary() is definitly more expressive and that's what people will use most of the time, because most people have one screen.
I think the data should be cached internally. It makes screens a lighter resource. And when you think about it screen changes are unlikely to happen while the program runs. Usually you set your screens up, before you start a program. So if a screen change event occurs you could simply set a flag and recompute the internal screen data at the next use. So we have a resource that doesn't change very often and we can easily be notified about change once we have screen events. These are ideal conditions for caching. If you only hand out copys or references you also don't have problems with invalid pointers.
To keep a getByIndex() function would be helpful I think. I can think of cases where users want to get a screen from it's index (for example get the screen corresponding to a video mode), instead of copying the whole vector and the only pick one element. Also we do that internally when switching to fullscreen.
There are two other things I'd like to discuss. First is I'd like to suggest a sf::Screen::contains(sf::Vector2) function. I can think of use cases where the user wants a window to go fullscreen on the monitor it's currently on. Right now there is no way to do it. Also this function would be usefull here (https://github.com/Foaly/SFML/blob/eb8b04625876c4852ed96f6407ac16a44f90836a/src/SFML/Window/Window.cpp#L145) (construct a window from handle) to get the bits per pixel of the correct monitor (right now it just takes the primary one).
The other thing is a commit I made, that removes the restriction that you can only have one fullscreen window at a time. That made sense when we didn't have multi screen support, but I think we can savely remove it now. I made a seperate commit (https://github.com/Foaly/SFML/commit/5738cd81f83bd84cbca73c42a7fd51e685f7ef0a), so it's easier to review. I'll squash them once it's reviewed.
-
And when you think about it screen changes are unlikely to happen while the program runs. Usually you set your screens up, before you start a program.
It doesn't matter as long as they do happen. The library shouldn't just break down because we didn't consider this case.
So if a screen change event occurs you could simply set a flag and recompute the internal screen data at the next use.
How do you detect a change in configuration? How do you signal it to the user?
I think the data should be cached internally. It makes screens a lighter resource. [...] These are ideal conditions for caching. If you only hand out copys or references you also don't have problems with invalid pointers.
Yes, but either you return a copy of the whole vector (which is expensive and partially negates the caching advantage), or you return a reference (which brings back the problem of changing configuration during usage).
I'd personally weigh the performance argument less important than the validity one, so I tend to return a copy.
To keep a getByIndex() function would be helpful I think. I can think of cases where users want to get a screen from it's index (for example get the screen corresponding to a video mode), instead of copying the whole vector and the only pick one element.
I don't think this is necessary. If the user is really concerned about performance, he can store the vector himself.
Another reason why I wouldn't provide an index-based function is that it brings back the danger of outdated information: it's possible that the index which the user memorized refers to a meanwhile different screen.
There are two other things I'd like to discuss. First is I'd like to suggest a sf::Screen::contains(sf::Vector2) function. I can think of use cases where the user wants a window to go fullscreen on the monitor it's currently on. Right now there is no way to do it.
What about screen.bounds.contains(vector)? What if the user wants the working area instead of bounds? It's too specific in my opinion.
In general, I don't like introducing specific functionality just for convenience reasons. As always in designing a new API, we should start with the minimal set of functionality, and extend it later based on user's needs. It's difficult to anticipate everything, and deprecating APIs because of wrong assumptions is much worse than introducing them a bit later.
There are really many parallels to the sf::Joystick API. How do we handle joystick changes at runtime meaningfully? The more I think about it, the more it seems that we've made some design mistakes there. We should definitely reconsider this whole topic for SFML 3, also to unify the differently looking APIs.
-
And when you think about it screen changes are unlikely to happen while the program runs. Usually you set your screens up, before you start a program.
This assumption will, I believe, be incorrekt sooner than you think. On OS X you can already play with the DPI to have more or less "space" on your screen. But that's not why I think it will change: with the recent shift in device usage, more and more tablets are being used and solution to plug in a screen to your tablet are coming rapidly to have a better nomad usage of our computers (think of AirPlay, Microsoft Surface docking stations and whatnot).
There are really many parallels to the sf::Joystick API. How do we handle joystick changes at runtime meaningfully? The more I think about it, the more it seems that we've made some design mistakes there. We should definitely reconsider this whole topic for SFML 3, also to unify the differently looking APIs.
Yes, definitely.
How do you detect a change in configuration? How do you signal it to the user?
Currently we only have events accessible through windows. So there's basically two solutions I can think of: a) continue to use this system, send a screen configuration change to all opened windows and document that sf::Screen's data should be used with as little delay as possible to create a window (*), or b) add a static sf::System::pollEvent for non-window specific events.
As I said before, I think in a first time we can just omit those configuration change event and focus on how the Screen's data is used/accessed.
(*) if it's done at the application start, it's unlikely to be an issue since no decent user should launch an app and at the same time plug a display.