Add support for Multiple Monitors in jme-LWJGL3 #2030 #2031
Conversation
Add support for multiple monitors. Add a feature so that when a "Full Screen" window is created, that you can tell it which monitor to create the window on. Add a feature so that the application can call context to get a list of monitors that OPENGL found. It returns them in an ArrayList so that the programmers can select a monitor from the list. JME will take the pos of the monitor from the arraylist to get its handle. So if you have 2 monitors, you will have 2 in the list. So to tell JME which monitor to create the window on it would be 0 or 1. The array position in the list. The thought behind this is the program gets a list of monitors and then they can use that list in their settings for the user to select which monitor to us. Since the ID of the monitor changes between each launch, I went with the position from the arraylist that it returned. So many if user changes the order of the monitors then the program will launch on a different screen. Minor. Added in AppSettings a way to get/set Monitor. Monitor value is used only when creating a Full Screen.
Add support for multiple monitors. Add a feature so that when a "Full Screen" window is created, that you can tell it which monitor to create the window on. Add a feature so that the application can call context to get a list of monitors that OPENGL found. It returns them in an ArrayList so that the programmers can select a monitor from the list. JME will take the pos of the monitor from the arraylist to get its handle. So if you have 2 monitors, you will have 2 in the list. So to tell JME which monitor to create the window on it would be 0 or 1. The array position in the list. The thought behind this is the program gets a list of monitors and then they can use that list in their settings for the user to select which monitor to us. Since the ID of the monitor changes between each launch, I went with the position from the arraylist that it returned. So many if user changes the order of the monitors then the program will launch on a different screen. Minor. Added in AppSettings a way to get/set Monitor. Monitor value is used only when creating a Full Screen.
|
Thank you for implementing the feature for us. I don't have a multi-monitor setup myself, so it didn't occur to me that this might be wanted. I assumed it would usually be easier to create windows in the primary display and use a window manager to move them to where they're wanted. Is that not the case? I don't understand why this PR modifies There are style violations in the added files: javadoc issues, braces missing or positioned unconventionally, 3-space indentation of code blocks, extra blank lines in Also I think the added files should have 2023 copyright dates. |
I think it is impossible in many windowing systems. This is the way to go. |
| * number is the number in the list of monitors GlfwGetMonitors returns. | ||
| * | ||
| * <p>This setting is used only with LWJGL3, it defines which monitor | ||
| * to use when creating a OpenGL window. its default value is -1. |
There was a problem hiding this comment.
its default value is -1.
defaults.put("Monitor", 0);
But looks like it is 0?
There was a problem hiding this comment.
Need to update comment.
|
Do modern games usually let you select the monitor from the game settings? Edit: |
|
Yes, this is for full screen only. You can't select a monitor for "WINDOW", it is forced to "primary OS monitor". When doing full screen, if you don't select it will do it on primary. If your primary is the window you want, then it is fine. But for it to launch and then move a FULL SCREEN is not nice. When you can cause it to launch on the correct window. I'll update the copyright. |
|
Forgot.. TestResizableMonitor was failing after creating a window in full screen when using a different monitor. I didn't look into way JME was calling reship before it did the initialization. |
|
I updated the comments. Let me know if you find anything else. |
|
I missed the LegacyApplication update for getMonitors() |
There was a problem hiding this comment.
Hello Kevin, thanks for this feature, I would like to request some changes as a part of consistency with jMonkeyEngine, please correct me if you think there are untrue statements or you disagree with something, many of these requested changes have duplicates across the PR that I am not referring to it here, so please pay attention to them as far as you can.
| * @return returns a list of monitors and their information. | ||
| */ | ||
| public Monitors getMonitors() | ||
| { |
There was a problem hiding this comment.
Hello Kevin, thanks for this work, according to the Google style of code (that we are following here), the curly brackets of a code block shouldn't be on a new line, so please, Can you refactor the K&R bracket style to Java Style on all the code involved in this PR?
| settings.setWindowYPosition(0); | ||
|
|
||
| try | ||
| { |
There was a problem hiding this comment.
Please use the Java Google coding style here too, it's also the style used in OpenJDK.
You can refer to:
https://google.github.io/styleguide/javaguide.html
| @Override | ||
| public Monitors getMonitors() | ||
| { | ||
| // TODO Auto-generated method stub |
There was a problem hiding this comment.
I think it's far better to throw a UnSupportedOperation Execption here instead of returning a null pointer.
For example:
public Monitors getMonitors() {
// paranoia, feature not supported!
throw new UnSupportedOperationException("Multiple Monitors feature is not supported on LwjglCanvas!");
return new Monitors(); // return empty monitors
}Please refactor the K&R bracket style to the Java bracket style.
There was a problem hiding this comment.
I think it doesn't really matter if you return a null or an empty object, what matters here is there should be a transparent message indicating that this feature is not supported.
There was a problem hiding this comment.
And you don't really need a return statement, it's an unreachable code anyway.
|
|
||
| @Override | ||
| public int getPrimaryMonitor() | ||
| { |
There was a problem hiding this comment.
I think an UnSupportedOperationException should be also thrown here.
| * @see #setMonitor(long) | ||
| */ | ||
| public int getMonitor() { | ||
| return getInteger("Monitor"); |
There was a problem hiding this comment.
It's better to wrap the "Monitor" string key in a constant object for future refactoring and testing purposes, you can use the MonitorInfo to define default static Keys there.
| * | ||
| * @author Kevin Bales | ||
| */ | ||
| public class MonitorInfo { |
There was a problem hiding this comment.
This should be marked as final, and I don't know if it needs to use the conventional style of getters and setters since their advantage here is not very obvious, but I think encapsulation should be the rule here, in case direct changes need to be applied to the member variables in the future.
|
As a general comment, this new API feels a little weird to me. Having a Monitors class that is essentially just a list of monitor infos with one odd control method feels off. Even weirder if this is ever extended to provide display information about the monitors other than "what it is right now". What I would have expected from this feature is some way to ask JME what monitors are available that returns a List of info objects... then some method to set the current monitor on the same thing I asked for the list of monitors. something like: And even that might be a dead-end design if we ever want to support changing things about a particular monitor (like current display settings or whatever). Instead of MonitorInfo we could have a Monitor interface with the getters (no setters necessary) then different system implementations can implement the Monitor interface as they see fit. (An abstract version could be provided to make this easier for the system implementations that only support one display... and perhaps Display is a better name than Monitor.) So: Regardless of all that, a MonitorInfo class with all public fields becomes a maintenance burden forever and should be avoided. |
|
@pspeed42, The monitors do 2k 170hz on both of them, but they report is not even close. So I thought if we listed resolution, people might think that is the only option because it doesn't report correctly. I can change the Monitor classes, I could create getter/setters and even a interface class. That would all be easy. Doing a setCurrentDisplay() would be very hard to do. The monitor selection is a "WINDOW CREATION" so this is before you get into the App simpleInitApp(). So the only option to set which monitor in full screen is through the ini. A restart of the application is require after you allow user to select a monitor. I didn't see an LWJGL command to move monitor to another monitor, just during window creation. Let me know if someone knows something I don't and I can look into it and change it. |
|
The point about the resolution settings is that someday the OS/drivers/etc. may fix those issues and those APIs would work like we expect... so it's better not to automatically design dead-ends into JME's API. As to setCurrentDisplay(), I was confused by this which seemed to be essentially doing the same thing: But maybe change what I said to setPrimaryDisplay() and it makes more sense? Maybe it should be on AppSettings? Either way, I prefer a "request the available options" and "set the option I want" API versus a "get the available options and somehow call something on those options to set my option" And having setters on MonitorInfo is probably confusing when they don't really do anything except let callers mess up the values. Thanks for the work so far. I don't mean to be picking on every thing. I'm just trying to forestall some future maintenance burden. |
|
@pspeed42, There is no way to do a "setPrimaryDisplay()" sense it is before the simpleInitApp(). All the calls to getMonitors() and such is "AFTER" the fact. It only use is for programmers to get a list of monitors if they wish to use it in "settings" gui to list the monitor to use on "Next" launch. I have no problem with you making suggestions or even demands to fit JME. I don't write JME and don't know the true layout. So I can change Calling AppSettings::setMonitor() updates the "App Ini settings". The game could then capture the close and write out ini so it is updated, or could write the ini in the gui control when it updates the monitor. |
|
The objects returned from getMonitors/Displays/Whatever should be immutable objects with no setters on them. There should be no Monitors class as it is unnecessary. A list is fine. "Monitor" is also a limiting name since a Monitor is a Display but not all Displays are "monitors". We should find a way to fix JME so that we can make platform specific queries before starting JME. Else we end up with cases where the user never gets to see the app because the OS chose to put it on a display that isn't even turned on. Setting which display to use should be done on AppSettings. That is the "JME way", I guess... but we need a platform-independent way of reliably querying the available displays (and even the available resolutions... which we currently do not have either). That last bit is kind of a larger issue with how JME is setup. |
|
I used "Monitor" because that is the reference inside OPENGL. It is called a monitor in the code. I could change everything related to "Monitor" to "Display" to fix that idea of "Not a Monitor" I have a check inside jme3-lwjgl3::LwjglWindow::createContext(). it checks if the number of the display doesn't show up in the list of available displays then use the "primaryMonitor" instead. Now since OPENGL lwjgl3 call to get monitors sends a new number each time that application is launched and programmers will never be able to guarantee it is the same monitor. So if a person changed their primary monitor. Then next time, it will open on another monitor. We only get a list of monitor and the ID changes all the time. So we can't determine which monitor it is. |
|
Probably even OpenGL doesn't call it a monitor on mobile devices... but I haven't looked. Is the last paragraph new information or in response to something we were discussing? |
|
OpenGL does call it monitors.
|
|
The comment is related to you comments about a window opening up on a monitor that is not there anymore or turned on. The best we could do is if you select monitor 1 (0-1) and when game launches, we could default it to 0. But if you have 3 monitors 0-2 and you select 1. Then remove your 2nd monitor, so the 3rd monitor becomes the 2nd monitor. There is no way in code, to determine that is a different monitor now. |
|
GLFW = "for windows" not mobile. So is still going to be very PC-monitor specific. FWIW, OpenGL-ES calls it a "surface". |
|
Interesting, but this is not for Android. The only support is for PC. I do not code for mobile and don't know enough to write something that would support mobile. But again, I do not see support for multiple monitors in mobiles. So this feature is basically for PC not other devices. That is why if you call monitor functions, you get either NULL or 0, because it is not supported. |
|
Sorry, been away for sometime. Work and the farm taking up all the spare time. I just uploaded the changes to "Devices". |
…bility for other devices." This reverts commit 8594c1d.
|
I didn't mean to close this. |
|
My branch was old, and merged it to current, but not sure if I missed something. Please let me know, and I'll fix it. |
…and broke the lwjgl3.
# Conflicts: # jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglDisplay.java
|
What is the difference between Display and Monitor? They look copy paste to me. |
|
A Display is something capable of displaying graphics. A Monitor is the big giant display sitting on your desk... which is by definition a subset of Display. Was this about code or the naming conversation? |
|
Did github autoformat everything again? Going to take some extra time to review with all of the non-changes mixed in. |
Sorry, I'll try to be more specific. My question is that why both Java code classes are needed, What is the difference between the two in that context? They look exactly the same to me. If both classes and terms are needed, should there be some interface/abstract class rather than just copy paste code. Design-wise. |
Looks that way, there are two separate auto-format commits. |
|
Sorry about this. When we decided to change from Monitor to Display the 2 old files related to Monitors was not deleted. I removed them. |
|
Looks ok to me. We should finally either accept, refuse or otherwise make this go forward. It is not fair to keep this hanging for so long, rather trivial addition too. |
@tonihele We have a new release manager in training. Please be patient. |
|
@pspeed42 @stephengold @tonihele |
| * | ||
| * @author Kevin Bales | ||
| */ | ||
| public class DisplayInfo { |
There was a problem hiding this comment.
is it correct that the variables of the DisplayInfo class all have the public access modifier? I would like to know what the core members think about this?
There was a problem hiding this comment.
For effective encapsulation, public fields should be avoided.
There was a problem hiding this comment.
It really comes down to the style of code, and this style is almost the same as the C-struct-style. While, jMonkeyEngine and Java style doesn't use public fields as outside accessors, I recommend to stick to the style too. I am really now bothered more with thread-safety.
EDIT:
Also, maybe we should consider using a record instead, I guess this might be better for thread-safety, this object should be immutable by itself.
There was a problem hiding this comment.
I would suggest privatizing the variables and adding getter/setter methods. Don't use 'record' otherwise you break the engine's compatibility with java 8 and all applications currently out there.
If you think the values of the variables should be immutable, then I would suggest writing the class like this:
public class DisplayInfo {
private long displayID = 0;
private int width = 1080;
private int height = 1920;
private int rate = 60;
private boolean primary = false;
private String name = "Generic Monitor";
public DisplayInfo() {}
public DisplayInfo(long displayID, int width, int height, int rate, boolean primary, String name) {
this.displayID = displayID;
this.width = width;
this.height = height;
this.rate = rate;
this.primary = primary;
this.name = name;
}
public long getDisplayID() {
return displayID;
}
public int getWidth() {
return width;
}
public int getHeight() {
return height;
}
public int getRate() {
return rate;
}
public boolean isPrimary() {
return primary;
}
public String getName() {
return name;
}
}There was a problem hiding this comment.
I agree with this. I propose to add a final keyword to all the fields and to the class definition, as well. By doing this, we can immitate a record or an immutable data structure anyway, it's also best to try to privatize the constructor, so only the Displays utility can instantiate it to be used as only an immutable data structure.
EDIT:
It's not applicable here to add final to the fields as they have default values, but anyway my point is to build a fully immutable data structure.
Add support for multiple monitors.
Add a feature so that when a "Full Screen" window is created, that you can tell it which monitor to create the window on.
Add a feature so that the application can call context to get a list of monitors that OPENGL found. It returns them in an ArrayList so that the programmers can select a monitor from the list. JME will take the pos of the monitor from the arraylist to get its handle. So if you have 2 monitors, you will have 2 in the list. So to tell JME which monitor to create the window on it would be 0 or 1. The array position in the list.
The thought behind this is the program gets a list of monitors and then they can use that list in their settings for the user to select which monitor to us.
Since the ID of the monitor changes between each launch, I went with the position from the arraylist that it returned. So many if user changes the order of the monitors then the program will launch on a different screen. Minor.
Added in AppSettings a way to get/set Monitor. Monitor value is used only when creating a Full Screen.