godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Add a `manual_focused` signal for `Control` nodes #5699

Open juliohq opened 1 year ago

juliohq commented 1 year ago

Describe the project you are working on

A 2D RPG game with custom UI nodes that use Godot's built-in Control grab_focus feature.

Describe the problem or limitation you are having in your project

Every time I call grab_focus on a custom Control node (in the initial node I want to give focus to) with a sound attached it plays the sound when it is not intended to. I don't want to add a boolean flag because it would need to be done for every script, which isn't ideal as a signal like that would save a lot of time. Also, manual_focused would help a lot in other scenarios, as it would split between "general focus" and "player focus".

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

By adding a manual_focused signal to Control nodes, it would be easier to develop custom UI nodes without having to workaround simple things like that.

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

Simply connect manual_focused to the function that is responsible for playing that sound.

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

I don't think it can be worked around since my project is almost entirely based on, so adding a flag in every script would be a hassle.

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

This is about avoiding to workaround the own engine.

dalexeev commented 1 year ago

Every time I call grab_focus on a custom Control node (in the initial node I want to give focus to) with a sound attached it plays the sound when it is not intended to.

var _allow_focus_sfx := true

func grab_focus_no_sfx() -> void:
    _allow_focus_sfx = false
    grab_focus()
    _allow_focus_sfx = true

func _on_focus_entered() -> void:
    if _allow_focus_sfx:
        SFX.play('gui_focus.wav')

I don't want to add a boolean flag because it would need to be done for every script

Create your class (for example MyButton) and inherit other buttons from it.

juliohq commented 1 year ago

@dalexeev thank you for the help, but I think that wouldn't work as I have different types (sometimes HBoxContainer, Button, MarginContainer, etc.), that's why it would be handy to have manual_focused.

TheDuriel commented 1 year ago

There is an even simpler way. Which is to have your toggle in the SFX player, not the Control node. Thus there is no need to extend any of the Control classes.

Some Game Class

func something() -> void:
    SFXPlayer.skip_next_ui_sound = true
    some_control.grab_focus()

SFX Player

func play_UI_sfx(sound: Sound) -> void:
    if not skip_next_ui_sound:
        play_sfx(sound)
    skip_next_ui_sound = false
    return
juliohq commented 1 year ago

@TheDuriel that became too hard to debug why a sound wasn't played (or was played twice) that way, given all sounds are constantly switching that flag.

KoBeWi commented 1 year ago

Well, there is more robust way to achieve that:

Some Game Class

func something() -> void:
    SFXPlayer.manual_grab_focus()

SFX Player

func manual_grab_focus():
    skip_next_ui_sound = true
    grab_focus()
    skip_next_ui_sound = false

func play_UI_sfx(sound: Sound) -> void:
    if not skip_next_ui_sound:
        play_sfx(sound)
juliohq commented 1 year ago

Yes, that works for simple cases. However it doesn't scale well as you have more and more sounds being played at the same time and from different sources. That looks pretty much like workaround, instead of being an inherent feature of the engine. So I decided to learn a minimum of C++ to work on this feature myself (https://github.com/godotengine/godot/pull/76432).

KoBeWi commented 1 year ago

It's not a workaround for a missing feature, but for engine's design (if anything). Calling methods from code should never emit a signal, because it's redundant.

We do have 2 exceptions to this rule (BaseButton and Range), but they provide a way to bypass the signal (set_pressed_no_signal() and set_value_no_signal()). While this itself is a bit odd, your solution is even more inconsistent, because it adds a separate signal.

What might be acceptable and in-line with the usual signal design is set_focus_no_signal() method.

juliohq commented 1 year ago

Unfortunately that's exactly what grab_focus() does, it calls focus_entered. As for set_focus_no_signal(), that's exactly what I've implemented but with a different name.

KoBeWi commented 1 year ago

No, you implemented a signal (and a notification??), not a method. Your PR is overly complex.

juliohq commented 1 year ago

Okay, I just updated the code implementing set_focus_no_signal over the complex manual_focused signal.