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

Add event for clicks #10141

Closed
wants to merge 38 commits into from
Closed

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Oct 16, 2023

Objective

Solution

  • Add API to send Click events when an Interaction goes from Interaction::Hovered to Interaction::Pressed.
  • Incorporate Click event sends into UiPlugin by default.
  • Refactor game_menu.rs example to use Click rather then manually checking the Interaction.

Changelog

Added

  • Click events when an Interaction goes from Interaction::Hovered to Interaction::Pressed.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more A-UI Graphical user interfaces, styles, layouts, and widgets X-Controversial There is active debate or serious implications around merging this PR labels Oct 16, 2023
@alice-i-cecile
Copy link
Member

Can you say more about why this is a better pattern? What problems is this solving, and why should we add another way to do this?

I think that our "UI interaction" model is very half-baked, but I'm not yet sold that this is the right fix.

@Shatur
Copy link
Contributor Author

Shatur commented Oct 16, 2023

Can you say more about why this is a better pattern? What problems is this solving, and why should we add another way to do this?

Interaction::Pressed is a button state. It's useful for changing button style, for example.
But a click in a UI is not a state, but an event where the button is released while the cursor is still on it.
So using Clicked event is not the same as checking for Interaction::Pressed. Without this PR implementing "conventional" clicks will require users to create their own component that tracks previous state and compare if state changed and the previous state was Interaction::Pressed. So it's a lot of boilerplate to do it without it.

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
///
/// Note click captures the full click/press-release action.
#[derive(Event)]
pub struct Clicked(pub Entity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure "clicked" conveys the right meaning, as the name doesn't imply it's on release. The Web API's click event is only emitted on release just like you implemented it here, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think clicks in any UI always works this way. But won't hurt to mention in the docs, of course!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, "click" is the right terminology here. We have "pressed" as a one-phase event, "just pressed" as a two-phase event, and "clicked" as a three-phase event.

Co-authored-by: Pascal Hertleif <killercup@gmail.com>
Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

This PR improves the existing implementation a bit, but in the long run this code needs to be completely replaced with a solution more similar to bevy_mod_picking.

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor Author

Shatur commented Oct 21, 2023

This PR improves the existing implementation a bit

I would like to clarify that this PR doesn't improve or change any behavior. It just adds an event for proper "clicks". Interaction::Pressed is not the same. I would use Interaction::Pressed for things like style change and click events for actual logic.

@rparrett
Copy link
Contributor

rparrett commented Oct 21, 2023

I just want to link another PR with a similar goal and different strategy. #8157 which adds to the Interaction enum. Not taking a position either way. edit: some great discussion in #7371 about yet another potential approach involving Interaction.

This is technically non-breaking but it is beginning a design for a very core aspect of bevy_ui, and it feels awkward in code to mix the two styles to me. It seems possible to implement in a plugin and is very likely to change significantly (see #9538).

So I think I am not in favor, but I don't feel strongly about that.

@rparrett rparrett requested a review from JoJoJet October 21, 2023 17:43
@Shatur
Copy link
Contributor Author

Shatur commented Oct 21, 2023

I just want to link another PR with a similar goal and different strategy. #8157 which adds to the Interaction enum.

it feels awkward in code to mix the two styles to me.

I saw it. But I think that "released" not a state, it's a state transition from Interaction::Pressed. It's a one-time thing, why change button to Interaction::Released for one frame?
Interaction best suited for changing button styles, clicking from this PR - for button logic. Otherwise what style should button have when Interaction::Released? If the same as Interaction::None, colors will flash for a single frame if you keep your cursor on the button. This is why I think that using event is a more ergonomic way.

It seems possible to implement in a plugin

Sure, but it requires only 6 new lines of code logic inside Bevy, this is why I decided to suggest it.

and is very likely to change significantly (see #9538).

Looks like logic will be event-driven, so this should play nicely with the upcoming rework.

@shanecelis
Copy link
Contributor

Hi, I got surprised by the pressed behavior on buttons not being click. I realized thanks to @Shatur's comment that click is a transition and instead of adding Released, we can just look for the transition from Pressed to Hovered. The code for that is short and simple.

/// Excerpt from this gist[1].
///
/// * * *
///
/// A trigger event that signifies a click on a button.
///
/// ```
/// # use bevy::prelude::*;
/// # use bevy_asky::view::click::{self, Click};
/// fn setup(mut commands: Commands) {
///     commands.spawn(ButtonBundle::default())
///         .observe(|trigger: Trigger<Click>|
///             eprintln!("Clicked on {}", trigger.entity()));
/// }
/// ```
/// [1]: https://gist.github.com/shanecelis/06b2d1a598e1e06d0a00671596e9f74f
#[derive(Event, Debug)]
pub struct Click;

/// This system looks at [Button] [Interaction] changes. If that state changes
/// from [Interaction::Pressed] to [Interaction::Hovered] then it will trigger a
/// [Click] event targeting the Entity with the [Interaction] component.
///
/// TODO: The `Local<EntityHashMap>` should be reset or drop elements that are stale
/// so it doesn't grow unbounded.
fn button_click(
    mut interaction_query: Query<(Entity, &Interaction), (Changed<Interaction>, With<Button>)>,
    mut last_state: Local<EntityHashMap<Interaction>>,
    mut commands: Commands,
) {
    for (id, interaction) in &mut interaction_query {
        let last = last_state.get(&id);
        if *interaction == Interaction::Hovered && matches!(last, Some(Interaction::Pressed)) {
            commands.trigger_targets(Click, id);
        }
        last_state.insert(id, *interaction);
    }
}

@alice-i-cecile alice-i-cecile added the A-Picking Pointing at and selecting objects of all sorts label Jul 20, 2024
@alice-i-cecile
Copy link
Member

Closing in favor of integrating this functionality into the now-upstreamed bevy_mod_picking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Picking Pointing at and selecting objects of all sorts A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants