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

Toggle fullscreen mode when pressing Alt + Enter by default #39708

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 20, 2020

Note: Technically cherry-pickable to 3.x, but existing projects may already have implemented a fullscreen toggle on their own. I think it's better not to cherry-pick this PR to avoid trouble.


Alt + Enter is a widely followed convention to toggle fullscreen. This adds support for it both in the editor and projects, as long as the window is configured to be resizable.

The rationale for providing this feature in all projects is that many developers don't bother about adding a fullscreen toggle, especially for smaller/gamejam games. Yet, this convention is followed by many popular games out there, including AAA games.

The shortcut can be modified or disabled by editing the Input Map in the Project Settings.

This closes godotengine/godot-proposals#1983. See discussion in godotengine/godot-proposals#1087.

@aaronfranke
Copy link
Member

In my experience, F11 is a more common keyboard shortcut for this (ex: web browsers, Minecraft). If this feature is added, F11 should also be a keybind to do this.

@@ -2295,6 +2279,20 @@ void Viewport::_gui_input_event(Ref<InputEvent> p_event) {
}

if (mm.is_null() && mb.is_null() && p_event->is_action_type()) {
const bool resize_allowed = !DisplayServer::get_singleton()->window_get_flag(DisplayServer::WINDOW_FLAG_RESIZE_DISABLED);

if (p_event->is_pressed() && p_event->is_action_pressed("toggle_fullscreen") && resize_allowed) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the check && resize_allowed is superfluous? Some people turn off window resizing but allow fullscreen mode.
And if someone doesn’t need either, then they can simply delete all events from the toogle_fullscreen action.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some people turn off window resizing but allow fullscreen mode.

That doesn't really make sense to me. If your window is set to be non-resizable, it's usually because your game isn't designed for multiple resolutions. Otherwise, there's no reason not to allow it 🙂

Copy link
Member

@dalexeev dalexeev Jun 23, 2020

Choose a reason for hiding this comment

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

If your window is set to be non-resizable, it's usually because your game isn't designed for multiple resolutions.

Absolutely true, but the game can still work in fullscreen. The resolution does not change, the picture simply stretches (viewport mode). This is usually for pixel games.

[display]

window/size/width=320
window/size/height=180
window/size/resizable=false
window/size/test_width=640
window/size/test_height=360
window/stretch/mode="viewport"
window/stretch/aspect="keep"

core/input/input_map.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member Author

Calinou commented Jun 21, 2020

@aaronfranke It makes sense to bind F11 as well, but it's more likely to conflict with a key already used in the project. That said, since it can easily be removed, it should be safe to do.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 24, 2020

@aaronfranke F11 is more common in applications (browsers, text editors etc.). Most games use Alt + Enter.

@akien-mga
Copy link
Member

Needs rebase.

@Calinou Calinou force-pushed the add-builtin-fullscreen-toggle branch 2 times, most recently from d7896f8 to a4cf4eb Compare November 20, 2020 23:22
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

When testing this, it seems that there is a naming mismatch between toggle_fullscreen and ui_toggle_fullscreen. In the project settings input map, it says toggle_fullscreen, but then when pressing the key, this error appears:

E 0:00:04:0057   event_get_action_status: Request for nonexistent InputMap action 'ui_toggle_fullscreen'.
  <C++ Error>    Condition "!E" is true. Returning: false
  <C++ Source>   core/input/input_map.cpp:194 @ event_get_action_status()

Also, it would be nice to get F11 bound:

key.instance();
key->set_keycode(KEY_ENTER);
key->set_alt(true);
action_add_event("ui_toggle_fullscreen", key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
action_add_event("ui_toggle_fullscreen", key);
action_add_event("ui_toggle_fullscreen", key);
key.instance();
key->set_keycode(KEY_F11);
action_add_event("ui_toggle_fullscreen", key);

key.instance();
key->set_keycode(KEY_ENTER);
key->set_alt(true);
events.push_back(key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
events.push_back(key);
events.push_back(key);
key.instance();
key->set_keycode(KEY_F11);
events.push_back(key);

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the new InputEventKey::create_instance( KEY_MASK_ALT | KEY_ENTER) here to keep it succinct.

@Calinou
Copy link
Member Author

Calinou commented Nov 21, 2020

@aaronfranke The issue with adding F11 as a shortcut is that it conflicts with the editor's distraction-free toggle, which defaults to Ctrl + Shift + F11. It no longer works if I add F11 as a key for ui_toggle_fullscreen.

Anyway, I fixed the issue with the ui_ prefix not being present for the default automatically-created input map.

@Calinou Calinou force-pushed the add-builtin-fullscreen-toggle branch from a4cf4eb to 592a5e7 Compare November 21, 2020 00:11
@aaronfranke
Copy link
Member

For F11: Understandable, if there's a reason to not have it then that's fine.

Tested this again. When exiting fullscreen on Linux, the window's title bar is gone.

This might be unrelated to this PR though, since this is just binding a key.

Comment on lines 2344 to 2346
const bool resize_allowed = !DisplayServer::get_singleton()->window_get_flag(DisplayServer::WINDOW_FLAG_RESIZE_DISABLED);

if (p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen") && resize_allowed) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const bool resize_allowed = !DisplayServer::get_singleton()->window_get_flag(DisplayServer::WINDOW_FLAG_RESIZE_DISABLED);
if (p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen") && resize_allowed) {
if (p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen")) {

See previous discussion. Let's not create artificial restrictions. If a person has window_resizable == false and they don't need fullscreen, then they can simply remove all events from the ui_toggle_fullscreen action.

Copy link
Member Author

@Calinou Calinou Nov 21, 2020

Choose a reason for hiding this comment

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

If the window is not resizable, then we should not allow toggling fullscreen to not break expectations (since making it fullscreen is almost guaranteed to resize it in one way or another).

If you really want to make a non-resizable window fullscreen, you can use a window manager that allows ignoring geometry restrictions like KWin. Either way, this is very much an edge case.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Calinou here, if resizing a window isn't allowed, it shouldn't be resized, whatever keys you press.

@Calinou
Copy link
Member Author

Calinou commented Nov 21, 2020

Tested this again. When exiting fullscreen on Linux, the window's title bar is gone.

I can reproduce this on Linux too. It's most likely a regression from DisplayServer.

@iwanPlays
Copy link

iwanPlays commented Dec 14, 2020

Absolutely do not use F11 for full screen toggle of games.

Definitely do use ALT+Enter for full screen toggle of games.

Or use both for the sake of people who learned gaming with minecraft or Roblox (Shift+F11) but ALT+Enter should be the priority for the sake of any other gamer.

If I'm not mistaken, there is conversation about having the same key combo for Godot editor and games made in Godot? If that is correct: Both F11 and ALT+Enter do nothing in Unity editor, neither should be expected to do anything. Full-screening a game and a game editor are entirely different things - like full-screening a video to watch, vs full-screening a video editor application.

If there is some need to fullscreen toggle godot editor, I'd suggest to use F11 (based on most applications that have fullscreen mode).

@Calinou
Copy link
Member Author

Calinou commented Dec 14, 2020

@iwanPlays The Godot editor already provides Shift + F11 to toggle fullscreen. We could perhaps change this to just F11 in a future pull request.

@dalexeev
Copy link
Member

Absolutely do not use F11 for full screen toggle of games.

@iwanPlays I think both should be enabled by default. Personally, I learned about the Alt + Enter combination much later than about the F11 key.

@aaronfranke
Copy link
Member

@Calinou Now that we have exact matching for input events (#44355), can we add F11 as a shortcut?

@YuriSizov YuriSizov requested a review from a team August 24, 2021 23:20
@groud
Copy link
Member

groud commented Sep 1, 2021

I think the feature and the implementation look good, but the PR needs a rebase.

@@ -2355,6 +2341,20 @@ void Viewport::_gui_input_event(Ref<InputEvent> p_event) {
}

if (mm.is_null() && mb.is_null() && p_event->is_action_type()) {
const bool resize_allowed = !DisplayServer::get_singleton()->window_get_flag(DisplayServer::WINDOW_FLAG_RESIZE_DISABLED);

if (p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen") && resize_allowed) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen") && resize_allowed) {
if (resize_allowed && p_event->is_pressed() && p_event->is_action_pressed("ui_toggle_fullscreen")) {

This way we can skip parsing input if resize is disabled.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Needs rebase.

@reduz
Copy link
Member

reduz commented Jul 28, 2022

Not necessarily against this, but you should check that the window is resizable/borderless before making this happen.
Also I do not think the implementation is good. The aciton is OK, but you should check/do/this at Window level and make this a Window property (disabled by default, and the one in root set by SceneTree by checking a project setting, which I guess its ok if its on by default).

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@Calinou
Copy link
Member Author

Calinou commented May 12, 2023

Rebased against master and reimplemented, but not functional yet.

The aciton is OK, but you should check/do/this at Window level and make this a Window property (disabled by default, and the one in root set by SceneTree by checking a project setting, which I guess its ok if its on by default).

I've looked into adding a flag to Window (also tried adding a property), but I don't know how I can avoid having to add code to the set_flag() override in each DisplayServer implementation. The shortcut does not need OS-specific handling after all.

Alt + Enter is a widely followed convention to toggle fullscreen.
This adds support for it both in the editor and projects, as long
as the window is configured to be resizable.

The rationale for providing this feature in all projects is that
many developers don't bother about adding a fullscreen toggle,
especially for smaller/gamejam games. Yet, this convention is
followed by many popular games out there, including AAA games.

The shortcut can be modified or disabled by editing the Input Map
in the Project Settings.
@Calinou Calinou force-pushed the add-builtin-fullscreen-toggle branch from 592a5e7 to 24e441c Compare May 12, 2023 01:48
@Calinou Calinou requested review from a team as code owners May 12, 2023 01:48
@Calinou Calinou marked this pull request as draft May 12, 2023 01:48
@m4gr3d
Copy link
Contributor

m4gr3d commented May 12, 2023

I'm guessing this is a no-op on Android?

@Calinou
Copy link
Member Author

Calinou commented May 12, 2023

I'm guessing this is a no-op on Android?

It could work if you have a hardware keyboard connected and floating windows are used, but otherwise I generally expect it to be a no-op on mobile platforms.

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.

By default, bind Alt + Enter to switch between fullscreen and windowed mode