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

Added multiple callback logic to event_listener #22

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

av1roytman
Copy link

@av1roytman av1roytman commented Mar 25, 2024

This is a work in progress, just wanted to get some of the code out there as we are working on it.

Closes #21

@@ -33,7 +49,7 @@ pub trait EntityEvent: Event + Clone {
pub struct On<E: EntityEvent> {
phantom: PhantomData<E>,
/// A function that is called when the event listener is triggered.

Choose a reason for hiding this comment

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

This comment is outdated.

handle
}

/// Create an empty event listener.

Choose a reason for hiding this comment

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

This comment implies that it returns Self, but instead the method mutates.

}

/// Create an empty event listener.
pub fn init_callbacks(&mut self) {

Choose a reason for hiding this comment

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

This should be a constructor.

@@ -22,6 +23,21 @@ pub trait EntityEvent: Event + Clone {
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]

Choose a reason for hiding this comment

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

These derives need to be manually implemented; phantom data doesn't play nice, since it looks at the bounds for E, even though PhantomData always fulfills the bounds.

mut func: impl 'static + Send + Sync + FnMut(&mut ListenerInput<E>, &mut Commands),
) -> Self {
Self::run(
) -> ListenerHandle<E> {

Choose a reason for hiding this comment

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

We should revert the changes to the method signatures from here down. Rather than mutating self, create a new empty event listener, mutate and then return it.

Self {
phantom: PhantomData,
callback: CallbackSystem::New(Box::new(IntoSystem::into_system(callback))),
pub fn run<Marker>(&mut self, callback: impl IntoSystem<(), (), Marker>) -> ListenerHandle<E> {

Choose a reason for hiding this comment

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

This should not take &mut self either: create a new empty event listener first.

@@ -22,6 +23,21 @@ pub trait EntityEvent: Event + Clone {
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct ListenerHandle<E: EntityEvent> {

Choose a reason for hiding this comment

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

ListenerId would be clearer.

Copy link

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

We need a way to get the callback handles out of the event listeners.

We should add:

  1. Methods to iterate over all the callbacks.
  2. Return the generated ids whenever callbacks are added. add_command does this, but others don't.

impl<E: EntityEvent> ListenerHandle<E> {
pub fn new() -> Self {
Self {
id: Uuid::new_v4(), // Generate a new unique identifier

Choose a reason for hiding this comment

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

Consider using an atomically incremented identifier here instead.


/*

QUESTION: Do we still need this???

/// Take the boxed system callback out of this listener, leaving an empty one behind.
pub(crate) fn take(&mut self) -> CallbackSystem {

Choose a reason for hiding this comment

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

This should now remove and return all stored systems and their IDs.


/*

QUESTION: Do we still need this???

/// Take the boxed system callback out of this listener, leaving an empty one behind.
pub(crate) fn take(&mut self) -> CallbackSystem {

Choose a reason for hiding this comment

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

This would be clearer if renamed to remove_all.

You can keep take around as a function, and add a deprecation warning.

@@ -22,6 +23,29 @@ pub trait EntityEvent: Event + Clone {
}
}

/// `ListenerHandle` is a struct that generates unique identifiers for event listeners.

Choose a reason for hiding this comment

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

ListenerHandle no longer exists :)

@aevyrie
Copy link
Owner

aevyrie commented Apr 17, 2024

Here is a sketch of what the API could look like:

// In the simplest case, you can add a listener with no callbacks in a bundle
commands.spawn((Name::new("Foo"), On::<Click>::default()));

// Option 2 is to create a listener with callbacks ahead of time, and insert that
// Create an empty listener
let mut bar_listener = On::<Click>::default();
// Add callbacks to it via `Commands`, and return the id of the callback you just added
let callback_1 = commands.add_callback(bar_listener, || info!("hello from a callback 1"));
// Repeat as many times as you'd like
let callback_2 = commands.add_callback(bar_listener, || info!("hello from a callback 2"));
// Add the listener, which now has two callbacks, to a new entity:
commands.spawn((Name::new("Bar"), bar_listener));

// Option 3 is to add to an existing listener which you got from a query or similar
// Create an empty listener
let mut baz_listener = my_query.single_mut();
// Same API as above: we have mutable access to the listener, so we can add callbacks to it
let callback_3 = commands.add_callback(baz_listener, || info!("hello from a callback 3"));

// Note that commands.add_callback() could be provided in a few ways:
// same as above:       Commands::add_callback(&mut self, listener: &mut On<E>) -> ListenerId
// on the listener:     On::<E>::add_callback(&mut self, &mut World) -> ListenerId

// Because `add_callback` returns the id, you can then use it to remove or replace callbacks
let callback_id = commands.add_callback(listener, || info!("hello world"));
commands.remove_callback(listener, callback_id); // also despawns the callback entity
commands.replace_callback(listener, callback_id, || info!("a new callback"));

@aevyrie
Copy link
Owner

aevyrie commented Apr 19, 2024

From discord discussion, a rough look at what the new method will look like:

#[derive(Component, Default)]
pub struct On<E: EntityEvent> {
    phantom: PhantomData<E>,
    /// A function that is called when the event listener is triggered.
    pub(crate) callbacks: Hashmap<ListenerId, CallbackSystem>,
}

struct ListenerId(Entity);

impl<E: EntityEvent> On<E> {
    fn add_callback(&mut self, commands: &mut Commands, callback: impl IntoSystem<(), (), Marker>) -> ListenerId {
        let id = ListenerId(commands.spawn_empty());
        self.callbacks.insert(id, CallbackSystem::New(Box::new(IntoSystem::into_system(callback))));
        id
    }
}

The goal is to just get this change in, and update the examples. You can simply comment out all the On::foo helpers that internally use run, which is replaced by this. If you have time left over after the examples are working, we can look at getting other things updated as well.

@musjj
Copy link

musjj commented Sep 16, 2024

Now that bevy_mod_picking is upstreamed, will work be continued on this PR or somewhere else?

@wgxh-cli
Copy link

I have a idea! Maybe we can save those callback functions as, let's say, entities, querying and executing them when needed. This aims to decouple the bindings between so-called Event and Callback. In this case, we can even be able to cache these callbacks and related events with bevy's ecs api.

Is this accpetable?

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.

Add the ability to have multiple callbacks in a single listener
6 participants