godot
godot copied to clipboard
godotengine
Add DisplayServer Window Center functions
Re-Adds an equivalent to 3.x's OS.center_window(), which was lost in the DisplayServer refactoring.
Testing is required for Windows and Linux, I only have a Mac.
Issue: Dragging the window to a different monitor then centering it places the window always on my second (bigger) monitor, but uses the size of whichever one the window was on for centering. I am unsure if this is caused by my function having an error, or a quirk of how DisplayServer.window_set_position() works. I attempted to set the window's current screen to be the screen it is on prior to setting the position, but this had no effect so I removed it.
Test project, press space to center the window. DisplayServer Center.zip
Closes https://github.com/godotengine/godot-proposals/issues/5232.
Shouldn't there be a way to just tell it to center the window on the current screen as well? Having to do DisplayServer.window_set_centered(DisplayServer.window_get_screen(),0) to do that seems janky.
@bruvzg Just to make sure I understand, Windows and MacOS also need the exact same virtual desktop coordinate addition, correct? And I should just go ahead and make it include a screen argument that defaults to 0? If so, should the window ID or the screen ID come first in the function? Just want to make sure I do this correctly.
I believe that I implemented the feature correctly now, anybody willing to check these on Linux and Windows?
I believe that I implemented the feature correctly now, anybody willing to check these on Linux and Windows?
Tested on Windows. It works. However I have a two-monitor setup and I had the editor screen on the left side when running and when centering the window it automatically always centered it on the right screen
it automatically always centered it on the right screen
You have to select which monitor to center the window on. I'm not entirely sure about this being the way that it works, but that's what @bruvzg said to do. The way to center it on the current screen would be to pass it DisplayServer.window_get_screen()as the screen to center the window within.
I feel like this is an error many people will end up making, especially devs who do not have more than one monitor to catch this. It feels like it could be a common "glitch" on Godot games due to being kind of unintuitive and I'm not sure how I feel about the concept. Even if a note pointing this out specifically was added in the docs, let's be honest, most people aren't going to read it.
~~Okay this seems to work fine for both monitors. I am just confused that p_screen on 0 is my right screen and 1 is my left screen when my system settings are like this:~~
Never mind, -1 as argument for the main window solves this. It recognized which screen the window is mainly on I guess and centers it on this screen then.
When writing the 3.x -> 4.0 migration guide, it must be noted that this new function does not operate as it did in 3.x, in which it centered the window on its current screen.
I still am concerned about how unintuitive this can be, especially for developers with only one monitor. I'm not sure why someone would want the default behavior to be for the function to cause the window to jump monitors. Usually this seemed to be used for resizing the window in game settings in 3.x and keeping it centered, which shouldn't care which monitor the user drags the window onto.
Fair enough on is_centered.
Just had a very strange error.
Github began claiming that platform/linuxbsd/display_server_x11.cpp and platform/linuxbsd/display_server_x11.h had merge conflicts with master, but Git did not. Trying to rebase to origin/master using Git on my computer was causing an issue where it wouldn't properly sync with master and forcing it to would amend some unrelated commits to say they were done by me. After me retrying about 4 times it randomly corrected itself but I started getting "unicorn errors" while trying to view this page or anything related to this commit.
Hopefully nothing is permanently messed up here and it's just Github's servers being rocky.
Tested locally rebased against master, it works (including after resizing the window). Nothing occurs when the window is maximized or fullscreen, as expected. I'm using Linux and a single-monitor setup.
The name DisplayServer.window_set_centered(...) feels odd to me, as you're not setting a boolean value. I think DisplayServer.window_center(...) would make more sense.
The name
DisplayServer.window_set_centered(...)feels odd to me, as you're not setting a boolean value. I thinkDisplayServer.window_center(...)would make more sense.
Yeah, in retrospect it is pretty weird. I guess it made more sense when it had a checker (window_is_centered) as an alternative. I’ll alter that soon.
I am still concerned about the “default” behavior as directed in which executing this function centers the window on the primary monitor of the computer, vs centering it on whichever monitor the window is currently open on.
I would imagine the most common use case of this would be for centering the window while resizing it in a settings menu, and it would be annoying if the user had dragged the window elsewhere and it kept snapping back to Display 1.
I am still concerned about the “default” behavior as directed in which executing this function centers the window on the primary monitor of the computer, vs centering it on whichever monitor the window is currently open on.
I think the default should be to center on the current monitor, not the primary one. You could compare this to maximizing a window or making it fullscreen: it won't change monitors when you do this.
I've pushed the changes, awaiting the checks to complete. Here is the updated test project. Press Space to center.
Will this be in a 4.0.x or will it be held until 4.1? Not sure how changes like this are being handled now that 4.0 is actually out.
@ztc0611 Most new features will only go in the next minor release. The cherry-picks to stable branches are for bug fixes and other minor things. However, the plan is for 4.1 to be out within a few months, so it will be in a stable version soon if it's accepted in time.
window_center is confusing, as "center" can be interpreted as a noun. Since center_window would go against the convention of using window_ as a prefix, how about window_center_to_screen (used in WinForms) or window_move_to_center.
Changes pushed.
Edit: Why is the spell check failing for code unrelated to what I added? I'm caught up to master.
Edit: Why is the spell check failing for code unrelated to what I added? I'm caught up to master.
It's now fixed in master, rebasing should solve it.
Code seems fine. But I'm not sure if there is any point to have a function that can be replicated with a few lines of GDScript.
If that is the case then that probably should have been mentioned before time and effort was spent on the pr from @ztc0611. I cant imagine it brings confidence in wanting to do more pr's in the future.
This function was present in 3.x so I think most people would expect it to be in 4.x. Especially if they are coming from 3.x.
Almost every game that allows the resolution to be changed whilst not in full screen will use this function.
The more friction put between basic function and what people will want to do, the less likely it is that it will be implemented. If everyone has to do this manually, I predict many games that allow resolution changing will not center the window. "Bugs" like this, no matter how unfair it is, will not reflect positively on the engine, in my opinion.
All that being said, I do agree with @produno that if you never wanted the function to reappear in 4.x I wish you would have said so before asking for numerous revisions. They weren't exceptionally time consuming, but it's the principle of the matter.
All that being said, I do agree with @produno that if you never wanted the function to reappear in 4.x I wish you would have said so before asking for numerous revisions. They weren't exceptionally time consuming, but it's the principle of the matter.
To be clear, I'm not against merging it, just pointing out that this function is not strictly necessary.
This discussion is very strange to me. Why do you need this on DisplayServer?
- It means adding code to every single platform for something that is not strictly platform specific.
- Users would not be able to use it anyway since in Godot 4 you use Window instead.
Does it not make more sense to just add the function to Window?
Okay I think there is a very big misunderstanding here, so I apologize.
To clarify, in Godot 4, DisplayServer is not meant to be used directly. You have to do window management in the Window class. This is a failure of documentation I guess as many Godot 3 users try to migrate to use DisplayServer directly and this is not ideal. The fact that many contributors in this thread were not aware of this either shows that from the project we did a bad job at communicating this change.
My suggestion is to add this feature on Window and use this instead, on a separate PR, and to also document on DisplayServer that these functions should not be used directly and suggesting to use Window functions (which you can access with get_root() or get_window() depending on the context.
@ztc0611 Sorry also that you lost so much time with this, It looks like we should have documented this better and you had the misfortune of bumping into some area of the engine where there was not much clarity for those involved, so many of us did not quite understood the purpose of this PR nor were able to give good feedback as a result.
Okay I think there is a very big misunderstanding here, so I apologize.
To clarify, in Godot 4, DisplayServer is not meant to be used directly. You have to do window management in the Window class. This is a failure of documentation I guess as many Godot 3 users try to migrate to use DisplayServer directly and this is not ideal. The fact that many contributors in this thread were not aware of this either shows that from the project we did a bad job at communicating this change.
Yep, i never knew this and all searches seem to point to using DisplayServer, so it seems to be a very common issue.
Does this also mean mentions of it in the documentation like Note: This property is only read when the project starts. To change the V-Sync mode at runtime, call DisplayServer.window_set_vsync_mode() instead. are also incorrect?
@produno Probably that is OK to use on DisplayServer, I meant more anything regarding to window management.