godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Add a `grab_focus_no_signal` option to controls #10844

Closed Snowdrama closed 1 month ago

Snowdrama commented 1 month ago

Describe the project you are working on

I'm working on 3rd person adventure game with menus for inventory and upgrades.

Describe the problem or limitation you are having in your project

I am implementing animations and audio effects on my UI that are triggered on focus so they occur when moving the selection around on UI elements with a gamepad, however I don't want it to be called when opening a menu for the first time.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Occasionally you may want to grab focus of something without triggering the focus effects when the focus is given when a menu opens.

This would also work in harmony with this proposal https://github.com/godotengine/godot-proposals/issues/1472 for integrating audio directly into the theme, which would similarly need something like this if you wanted to grab a button's focus without triggering the audio when a menu opens.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

When a menu opens for the first time, the first button should be focused, but it should not trigger on focus effects: This is a menu I set up, the first element is always focused when the menu opens.

Here's a video with some example sounds, you can hear when the menu opens you hear the click due to it focusing the first element. You can also hear it when you switch between menus behind the "select" sound which is played on click it plays both because pressed is called, then focus is called on the first element of the next menu.

https://github.com/user-attachments/assets/2aa09832-f1bf-43b2-bab2-b61f3d5787d8

Here's a basic example of that menu. The menu controller opens each menu and then sets the target element as focused when the menu becomes visible

extends Node

@export var menu : Control
@export var control_to_focus_on_open : Control
@export var controls : Array[Control]
@export var sound_to_play_on_focus : AudioStreamPlayer

func _ready():
    menu.visibility_changed.connect(menu_visibility_changed)
    for c in controls:
        c.focus_entered.connect(play_sound)

func menu_visibility_changed():
    # if the menu is visible than we want to grab focus of the button
    if menu.is_visible_in_tree():
        # this will cause the sound to play
        control_to_focus_on_open.grab_focus()
        # The proposal introduces a variant of grab_focus
        # That doesn't trigger the focus_entered signal.
        control_to_focus_on_open.grab_focus_no_signal()
        # This would be similar to something like a slider
        # which can be updated without triggering
        # the value_changed signal:
        # var slider : HSlider
        # slider.set_value_no_signal()

func play_sound():
    # this sound should only be played when you focus it after it's been opened
    sound_to_play_on_focus.play()

func open_menu():
    menu.visible = true

func close_menu():
    menu.visible = false

This change would allow the first item to be selected without triggering the sound!

If this enhancement will not be used often, can it be worked around with a few lines of script?

I don't believe there is a reasonable workaround that I've been able to figure out, and I think this is something that makes sense to be added as a small quality of life feature for controls.

Is there a reason why this should be core and not an add-on in the asset library?

I don't know if it could be implemented with an asset library with like GDExtension, this adds functionality to the core control class.

KoBeWi commented 1 month ago
button.set_block_signals(true)
button.grab_focus()
button.set_block_signals(false)
Snowdrama commented 1 month ago

@KoBeWi Ohh interesting, it's nice that exists!

Snowdrama commented 1 month ago

I still think something like this would make sense for consistency with other controls, if the expectation is to use this, than it should be consistent across all controls.

hslider.set_block_signals(true)
hslider.set_value(0.5)
hslider.set_block_signals(false)

But I think that might be better for a bigger consistency pass including adding stuff like set_visibility_no_signal and such. I don't think this would inherently be a big change but for now, that workaround is good enough for my use case.

KoBeWi commented 1 month ago

The existing "no_signal" methods were actually added by accident, because set_block_signals() is too obscure and was overlooked. It was pointed out for grab_focus_no_signal() implementation: https://github.com/godotengine/godot/pull/76432#issuecomment-1671397857 We could mention it in the documentation probably.

The existing methods can't be removed due to compatibility. Their usage is also much more common, so they kind of make sense.

Snowdrama commented 1 month ago

Gotcha, yeah the only reason I went looking for a no_signal variant of grab_focus was because those other ones existed, hence my comments on consistency, I personally don't mind either way it just it makes sense to have them all behave the same way, but I get that the existing ones can't be removed. For me, at this point it makes sense to add the shortcut versions since you have some of them. If it's a mistake to have them, then it can be corrected in like a 5.X where breaking changes make sense, but for consistency of 4.X it might make sense to just add all the no_signal functions.