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

Rename Project Settings, Display, Window width and height, and test_width and test_height settings to match their function #47522

Merged

Conversation

madmiraal
Copy link
Contributor

As originally identified by @leonkrause here, the Project Settings display/window/size/test_width and test_height should be renamed to window_width and window_height to reflect their function. To avoid confusion, this PR also renames display/window/size/width and height to viewport_width and viewport_height to reflect their function too.

Part of #16863

@reduz
Copy link
Member

reduz commented Mar 31, 2021

I would rename them from test to override, because otherwise its still confusing as users won't know which is the actual setting to use. In reality, your display size is not warranted and supplying a "real" window size is not portable (as it can't be ensured when you run on mobile, web, FS dedicated, etc), so other than users using this for scaling pixel art in desktop games it serves not a lot of purpose.

@madmiraal
Copy link
Contributor Author

madmiraal commented Mar 31, 2021

I don't think the word "override" reflects what it does. It doesn't override anything, it simply specifies the initial size of the window on desktop platforms. I've tried to make this clear in the documentation:

On desktop platforms, sets the game's initial window height.
Note: By default, or when set to 0, the initial window height is the viewport height. This setting is ignored on iOS, Android, and HTML5.

The window size can still be changed by the user (unless the Resizable property is disabled). How the viewport is displayed when these settings are used, or the window size is changed by the user, depends on the Stretch settings.

These settings are used to test what the viewport and stretch settings will look like on different mobile screen resolutions -- especially pixel art -- and I assume this is why it was called "test" initially, but ultimately it's the initial desktop window size.

Finally, the window_width and window_height are only available under Advanced Settings. They're not displayed by default; so it shouldn't confuse new users:
WindowSettings

@madmiraal madmiraal force-pushed the rename-test_width-test_height branch from a488631 to 12c316a Compare March 31, 2021 14:17
@smix8
Copy link
Contributor

smix8 commented Mar 31, 2021

In my mind for 3D desktop games it was always "Project Resolution" and "Launch Window Size".

As a Godot Beginner with advanced settings turned off I would immediately understand and know what to expect from "Project Resolution" but not "Viewport width/height" because I first need to know what is even a Viewport in Godot.

"Project Resolution" for authoring the projects user interface e.g. at 4k in the editor because you build your game interface at that size in the editor and this settings often affects how your 2d control node interface will look, behave and scale later. Changing this size later can end up terrible and destroy the layout of brittle control node scenes.

"Launch Window Size" because that is the size of the starting game window e.g. in a small launcher window before expending to user preference or platform resolution and window settings like fullscreen.

@madmiraal
Copy link
Contributor Author

@smix8 The problem with "Project Resolution" is that the published game's actual resolution (including the height to width ratio) depends on the device that it's played on, and that is outside of your control. It's only on desktop platforms, when the game is not run at Fullscreen and Resizable is disabled that the final resolution can be controlled (and then you need to keep it a minimum expected size). In all other circumstances you need to specify how your Viewport size is converted to the actual screen or window resolution; using the stretch modes. This is explained in the tutorials: https://docs.godotengine.org/en/stable/tutorials/viewports/multiple_resolutions.html. Ultimately, by the time you publish your game, you need to understand what the Viewport is and specify how it's size is converted to the actual screen or window size. And, yes, that requires that the layout of your Control nodes is not "brittle". Until then, and to help beginners, when running the game from the editor, the window size is simply the Viewport size.

"Launch Window Size" is what this PR is proposing calling window_width and window_height, and it's specific to desktop platforms.

doc/classes/ProjectSettings.xml Show resolved Hide resolved
doc/classes/ProjectSettings.xml Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@madmiraal madmiraal force-pushed the rename-test_width-test_height branch from 12c316a to 14108ab Compare July 8, 2021 09:22
@madmiraal
Copy link
Contributor Author

@LightningAA I've updated the documentation with slight modifications to your suggestions. Please let me know what you think.

@madmiraal madmiraal force-pushed the rename-test_width-test_height branch from 14108ab to 1a2603b Compare July 8, 2021 12:28
Copy link
Contributor

@AaronRecord AaronRecord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@reduz
Copy link
Member

reduz commented Jan 4, 2022

@madmiraal I see your point, but having as you suggest is ambiguous. Adding override makes it clearer where the order of importance is. Worst case this could be called "window width/height override"

@madmiraal madmiraal force-pushed the rename-test_width-test_height branch from 1a2603b to f107139 Compare January 4, 2022 14:07
@madmiraal madmiraal requested a review from a team as a code owner January 4, 2022 14:07
@madmiraal
Copy link
Contributor Author

Renamed window_width to window_width_override and window_height to window_height_override as suggested.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting and approved the renaming of display/window/size/test_{width,height} to display/window/size/window_{width,height}_override. That part can be merged already.

For the other part which renames display/window/size/{width,height} to display/window/size/viewport_{width,height}, we agreed that there is merit to it from a technical point of view (it's more accurate/descriptive), but are still unsure whether it's a good change from a UX point of view.
One other thing that could be worth looking into is maybe separating display/window parameters and display/viewport parameters, if we see that it makes sense for users.

So we suggest opening a proposal to discuss that specific part further and include more users in the discussion, including tutorial makers, to use the opportunity to figure out the most natural structure for these settings (if we're breaking compat, we can take this opportunity to revisit this).

@madmiraal
Copy link
Contributor Author

I've created godotengine/godot-proposals#3769.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following godotengine/godot-proposals#3769 there seems to be consensus that these names are good 👍

@akien-mga akien-mga merged commit 7c771ea into godotengine:master Jan 18, 2022
@akien-mga
Copy link
Member

Thanks!

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.

None yet

7 participants