Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kubasobon
Copy link

@kubasobon kubasobon commented Jan 15, 2024

Towards godotengine/godot-proposals#8788

This PR adds a new signal to the Window class called window_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 either Window.ContentScaleMode.CONTENT_SCALE_MODE_VIEWPORT or Window.ContentScaleMode.CONTENT_SCALE_MODE_CANVAS_ITEMS. The Viewport.size_changed signal, which Window 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 the NOTIFICATION_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:

enum ContentScaleMode Window.size_changed emitted Window.window_size_changed emitted - added by this PR
0 CONTENT_SCALE_MODE_DISABLED yes 💚 yes 💚
1 CONTENT_SCALE_MODE_CANVAS_ITEMS no ❌ yes 💚
2 CONTENT_SCALE_MODE_VIEWPORT no ❌ yes 💚

Bugsquad edit:

@kubasobon kubasobon requested review from a team as code owners January 15, 2024 11:09
@kubasobon
Copy link
Author

I acknowledge Window.window_size_changed vs. Window.size_changed is an unfortunate naming convention. Please feel free to suggest a way to make the names better and/or more self-explanatory. Window.resized perhaps?

@kubasobon
Copy link
Author

cc: @Gnumaru This PR aims at solving your original complaint in #79336. I'd love for you to weigh in.

@Gnumaru
Copy link
Contributor

Gnumaru commented Jan 15, 2024

cc: @Gnumaru This PR aims at solving your original complaint in #79336. I'd love for you to weigh in.

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.

@kubasobon
Copy link
Author

kubasobon commented Jan 15, 2024

I agree that while this PR adds a signal which works for window size change, regardless of scaling mode, it leaves the case of Viewport.size_changed unsolved.
Window inherits Viewport, so it will expose both size_changed (works only without scaling) and window_size_changed (new; works always).

AFAIK there is no way to overload signals in Godot's core, since the ADD_SIGNAL method prevents this.

Renaming Viewport.size_changed to Viewport.viewport_size_changed would make it clearer, but constitutes a big breaking change IMHO. Would improving documentation and elaborating on when to use which signal help?

@AThousandShips
Copy link
Member

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

@kubasobon
Copy link
Author

kubasobon commented Jan 15, 2024

@AThousandShips thank you for explaining this. My main goal for this PR is to solve the case where size_changed signal does not get emitted if any kind of Content Scaling is enabled. Currently there is no other way of knowing the window has been resized, than subscribing to the notification by patching the root window object. This might be confusing to other users, as it was for me, since size_changed signal just stops working.

If there is a better way of getting notified every time a window is resized, please let me know.

@Gnumaru
Copy link
Contributor

Gnumaru commented Jan 15, 2024

Renaming Viewport.size_changed to Viewport.viewport_size_changed would make it clearer, but constitutes a big breaking change IMHO. Would improving documentation and elaborating on when to use which signal help?

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

@kubasobon
Copy link
Author

@Gnumaru thank you for your remarks.

My main concern and vision for the scope of the PR is either fixing Window.size_changed with content scaling enabled (difficult, since it is inherited from Viewport, which in fact does not change size with scaling) or introducing a solution which enables users to track window size changes with similar effort/complexity - e.g. a signal.

@wgetJane
Copy link

i doubt that there is a valid performance concern with emitting one signal every time a window is resized (which happens very occasionally anyway)

@Gnumaru
Copy link
Contributor

Gnumaru commented Jan 27, 2024

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.

@kubasobon
Copy link
Author

@Gnumaru Please correct me if I'm wrong, but isn't that true of the Notification as well?

@Gnumaru
Copy link
Contributor

Gnumaru commented Jan 30, 2024

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
the user clicks and hold the window border in order to resize the window, or at least while holding the mouse button down and moving the mouse (thus actually resizing the window).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notify users about window size changes
4 participants