-
Notifications
You must be signed in to change notification settings - Fork 26
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 Fullscreen shortcut #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Thank you for the contribution! I suggested some changes to make this work more in line with how other settings are implemented
scenes/Main.gd
Outdated
if Input.is_action_pressed("toggle_fullscreen"): | ||
# '%' is a unique name property of a node (since 4.0) | ||
var fullscreen = $CanvasLayer/Settings/%GraphicsSettings/%Fullscreen | ||
fullscreen.button_pressed = not GraphicsManager.fullscreen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be emitted via "GlobalMenuEvents" and then handled in GraphicsSettings, so that main does not need to reach into the graphics settings menu internals
@@ -16,6 +16,7 @@ var remappable_actions_str := [ | |||
"jump", | |||
"crouch", | |||
"interact", | |||
"toggle_fullscreen", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be rebindable as it's not really a game control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if more miscellaenous controls like zoom/pan, show fps, etc. will be added they should maybe have a special category in the control settings, I dont see why not to allow users to change these binds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit strange right now because it also offers the ability to toggle full screen via the joypad which isn't really needed. maybe can be reconsidered once there's more controls, like you say
ok this looks great, thank you! |
Well I figured since there is fullscreen button, it could be convenient to have a shortcut for it in the settings. The feature currently works even in the main menu before you start the game. I know this is a minor thing but I want to get used to this codebase and understand how things are done.
I am aware of the fact that I could have used another signal but then the button's state would have to be adjusted, like this its all done as it hooks to the existing code for the button press