-
-
Notifications
You must be signed in to change notification settings - Fork 21k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add window_size_changed
signal
#87210
base: master
Are you sure you want to change the base?
Conversation
I acknowledge |
Hi. Regarding performance, I have absolutely no idea what is more performant: many (say, a thousand) nodes connecting to a signal or the same nodes implementing _notification in gdscript just to receive a size changed notification. I have a hunch that implementing _notification performs the poorest since _notification is called for a lot of things and a size changed signal would be called only when changing the window size. Regarding usability, it is undoubtedly better to listen to signals than implementing _notification and matching an int to a constant Regarding names I think window_size_changed is better but please read to the end. I think the original size changed signal (and name) name is already misleading. In my own personal preference, what I reeeally would like is that we had two separate signals with specific purpose and names for 'real' and 'virtual' sizes, I mean, the actual window size as the operating system sees it and the underlying viewport size as godot renders it. My original real life problem when I opened #79336 is that I wanted to force the window size (the actual window, handled by the OS, not the underlying viewport) to an integer multiple of my game's base resolution. Nowadays godot have Window.ContentScaleStretch.CONTENT_SCALE_STRETCH_INTEGER but it is still not exactly what I would like since the actual window still can have a variable size, only the underlying viewport gets rendered to the integer multiple, centered in the window and surrounded with black bars. If we had a signal specific for notifying the value change of the actual window size (named window_size_changed while the other could be named viewport_size_changed) then I could simply connect to window_size_changed, check if the window is not maximized neither full screen (in which case I would leave as is) and, if the window size is not a multiple of my base resolution, I would resize the window again to the nearest multiple, like I'm already doing, but I'm attaching a script to the root window just for that, and I think that attaching scripts for engine objects like the root window or the scene tree is something that should really not be needed except for the most exceptional cases. |
I agree that while this PR adds a signal which works for window size change, regardless of scaling mode, it leaves the case of AFAIK there is no way to overload signals in Godot's core, since the Renaming |
Signals are not free and unlike notifications they are sent no matter if anyone listens to them, or rather the overhead doesn't go away, especially with a duplicate and potentially confusing signal |
@AThousandShips thank you for explaining this. My main goal for this PR is to solve the case where If there is a better way of getting notified every time a window is resized, please let me know. |
Indeed, renaming the size_changed signal is too big of a compat breaking change to happen out of the blue. Is it possible to create a "signal alias" or something like that? or maybe having two redundant signals (size_changed and viewport_size_changed) and mark size_changed as deprecated in the docs, saying that it will be removed in favor of viewport_size_changed in a future (but yet undetermined) version? The docs could be further improved but I think that if the cost is not high (both performance cost and man-hour coding cost), the best would be to have signals (or notifications) that handle the specific separate needs of knowing if the actual operating system window size changed OR the underlying viewport size changed (which would never happen when the scale mode is viewport unless it is possible to change it via script). Please note that I'm not asking for increasing the scope of this PR. I'm just throwing ideas. Having another signal (like the one in this PR) or having another notification that gets triggered in any condition is already the most beneficial change to the current state of things, regardless if the name of the signal is good or not. But I think it is good name since it would allow a future refactoring (godot 5?) renaming size_changed to viewport_size_changed and NOTIFICATION_WM_SIZE_CHANGED to NOTIFICATION_VIEWPORT_SIZE_CHANGED |
@Gnumaru thank you for your remarks. My main concern and vision for the scope of the PR is either fixing |
i doubt that there is a valid performance concern with emitting one signal every time a window is resized (which happens very occasionally anyway) |
Depending on the operating system and the window manager, while you resize a window by dragging the borders, the resize signal could be emitted every frame. But I do admit that very seldomly someone would be resizing a game window by dragging the borders. |
@Gnumaru Please correct me if I'm wrong, but isn't that true of the Notification as well? |
Indeed. Both the notification and signal should either be triggered every frame or one single time when the user let go the mouse click. On windows (since windows XP up to windows 11), there is a performance setting whose name I cannot remember right now that disables or enables drawing the window content when dragging or resizing the window. If this is set to render the window content (which is the default) then the notification and signal should trigger every frame while |
Towards godotengine/godot-proposals#8788
This PR adds a new signal to the
Window
class calledwindow_size_changed
. The signal is emitted whenever the window size is changed, either in code or using UI.The PR is inspired by #79336 and my issues with
Viewport.size_changed
signal while using eitherWindow.ContentScaleMode.CONTENT_SCALE_MODE_VIEWPORT
orWindow.ContentScaleMode.CONTENT_SCALE_MODE_CANVAS_ITEMS
. TheViewport.size_changed
signal, whichWindow
class inherits, is not emitted with the scaling mode set. It works as expected when scaling is disabled. This is not indicated in the engine documentation. There is no alternative to the signal, leaving users with only theNOTIFICATION_WM_SIZE_CHANGED
to subscribe to, and in a pretty obscure way as showcased in #79336 (comment) (patching the root node script).To better explain:
ContentScaleMode
Window.size_changed
emittedWindow.window_size_changed
emitted - added by this PRBugsquad edit: