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

Remove _pressed virtual method in BaseButton. #10767

Closed
P5ina opened this issue Sep 17, 2024 · 6 comments
Closed

Remove _pressed virtual method in BaseButton. #10767

P5ina opened this issue Sep 17, 2024 · 6 comments
Labels
archived breaks compat Proposal will inevitably break compatibility topic:gui

Comments

@P5ina
Copy link

P5ina commented Sep 17, 2024

Describe the project you are working on

A 2D topdown interactive story game

Describe the problem or limitation you are having in your project

When I saw the virtual method _pressed in BaseButton, I thought that the functionality for the button should be added by overriding this method. And then I didn't find the _area_entered method in Area2D. But both had a signal. Which raised the question of why the button has a virtual method _pressed at all. Or if it should be, then why doesn't Area2D have a similar method? I find this very confusing, especially for beginners.

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

Remove _pressed virtual method in BaseButton.

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

Without using _pressed it will look like this:

func _ready() -> void:
  button.pressed.connect(_on_button_pressed)

func _on_button_pressed() -> void:
  # Doing something here
  pass

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

It's very easy to work around this problem, by just not using _pressed.

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

It can't be done using an addon.

@P5ina
Copy link
Author

P5ina commented Sep 17, 2024

It's not only about the pressed method, but also _toggled

@dalexeev dalexeev added the breaks compat Proposal will inevitably break compatibility label Sep 17, 2024
@AThousandShips
Copy link
Member

The virtual methods are very useful for things like extensions, removing this would break compatibility and make it more annoying to work with buttons in extensions especially, having to swap to using signals, signals are also worse for performance than virtual methods

If anything I'd say we should add virtual methods in more places

@Mickeon Mickeon changed the title Confusing BaseButton virtual methods Remove _pressed virtual method in BaseButton. Sep 17, 2024
@Calinou
Copy link
Member

Calinou commented Sep 17, 2024

If anything, _pressed() is useful for prototyping when you can't be bothered to write all the signal connection boilerplate 🙂

@P5ina
Copy link
Author

P5ina commented Sep 17, 2024

I agree, my proposal was about making this things more consistent.

@dalexeev
Copy link
Member

Note that virtual methods differ from signal handlers functionally and semantically.

Signals can be disconnected (disconnect()) or signal emission can be blocked (set_block_signals()). While a virtual method will still be called, if all other things are equal.

In my opinion, virtual methods are better suited for overriding internal functionality, although you still sometimes have to use self-connection of a signal if the virtual method is not provided by the engine. While signals are primarily intended to notify external objects of an event.

Imagine that you are designing a Character class that can be inherited. You can add both a virtual method _take_damage() to customize the character's behavior in inherited classes, and a signal damage_taken, which is designed to react external objects to a damage event.

@P5ina
Copy link
Author

P5ina commented Oct 23, 2024

Closing proposal, considering the explanation of the existence of virtual methods for buttons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived breaks compat Proposal will inevitably break compatibility topic:gui
Projects
None yet
Development

No branches or pull requests

5 participants