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 OnPress callbacks #229

Merged
merged 10 commits into from
Aug 5, 2024
Merged

Add OnPress callbacks #229

merged 10 commits into from
Aug 5, 2024

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Aug 5, 2024

Part of #203
Pattern taken from @Freyja-moth. Thanks Freyja!

Alternative to the following:

handle_title_action should be done using observers, rather than a TitleAction component

Relevant discussion.
The TL;DR is that registering a similar OnPress<T: Event>(pub T), which would be need to trigger observers on presses, would require you to register your T in your menu in order to register the actual generic system for your T. Which seems like a weird extra abstraction to add and a potential source of errors, same a when a user forgets to do add_event for EventReaders and EventWriters.
That, or you setup your logic for triggering observers on presses separately for every menu, which seems like a lot of boilerplate for something as trivial as pressing on a button.
Additionally, adding observer structs introduces additional indirection where none is needed.

The ugly reflection stuff is due to bevyengine/bevy#14496.
The need for one-shot systems to be registered at the top of the function will be gone after #223

@janhohenheim janhohenheim marked this pull request as ready for review August 5, 2024 21:00
@janhohenheim janhohenheim changed the title Add OnPress systems Add OnPress callbacks Aug 5, 2024
@janhohenheim
Copy link
Member Author

I also want some feedback from @alice-i-cecile before merging since this is an alternative to her suggestion

@benfrankel
Copy link
Collaborator

benfrankel commented Aug 5, 2024

I like SystemId here because the API is a lot closer to On::<Press>::run from bevy_mod_picking, and the performance impact should be negligible for menu buttons.

@janhohenheim
Copy link
Member Author

Note that a possible extension of this could be an API like On::<Interaction::Press::run(system). I would not do that for the template, since it would be trivial to add for a user themselves and I don't think anything outside of Press will require a callback. Just wanted to point out the possibility.

@alice-i-cecile
Copy link
Member

IMO this is more likely to end up using observers, but this is fine until we get a first party solution.

@janhohenheim
Copy link
Member Author

janhohenheim commented Aug 5, 2024

@alice-i-cecile do you mean by that that a player will end up changing it to observers or that a first party solution will use observers? I assume the latter.

Copy link
Collaborator

@benfrankel benfrankel left a comment

Choose a reason for hiding this comment

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

IMO on_credits etc. names should instead be named based on the action they perform, like enter_credits or exit.

src/screens/credits.rs Outdated Show resolved Hide resolved
src/screens/title.rs Outdated Show resolved Hide resolved
src/ui/interaction.rs Show resolved Hide resolved
src/ui/interaction.rs Show resolved Hide resolved
src/ui/interaction.rs Outdated Show resolved Hide resolved
@janhohenheim janhohenheim enabled auto-merge (squash) August 5, 2024 22:39
@janhohenheim janhohenheim merged commit 01ebcc4 into main Aug 5, 2024
4 checks passed
@janhohenheim janhohenheim deleted the on-press branch August 5, 2024 22:42
@janhohenheim janhohenheim mentioned this pull request Aug 5, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants