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 signal/event systems #706

Closed
wants to merge 1 commit into from
Closed

add signal/event systems #706

wants to merge 1 commit into from

Conversation

namlehong
Copy link
Contributor

Short Description:
Implement signal system.

@@ -0,0 +1,35 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is meant to be a test, can you write this in a way that it will actually pass / fail on travis? Aka, make it a unit test instead of printing things and requiring manual work to verify that it is printing the right stuff.

Copy link
Contributor Author

@namlehong namlehong Jul 24, 2016

Choose a reason for hiding this comment

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

@TheSavior :( sorry about my laziness, but it also a usage example for barks.
@ProjectBarks may you be my reinforcement?

@elicwhite
Copy link
Contributor

I'm concerned that a dispatcher + event system will be very incompatible with the longer term approach of using behavior trees.

It might be compatible, but I'm not sure how we intend to use this / what we intend to dispatch.

I'll talk to @namlehong offline about this and see if I can understand better.

@elicwhite
Copy link
Contributor

elicwhite commented Jul 24, 2016

From @namlehong:

For example, a sign_in_signal

people can add ton of receiver like
- check inventory
- do transfer old pokemons

then 
pre_catch_pokemon:
- check pokeball

post_catch_pokemon:
- do transfer or not
- update pokeball count
One of the typical problems with hooks is that you end up having to have a bunch of logic to handle people who want to order hooks, or hooks that need to dispatch other things and thus call more hooks which call more hooks.

One of the other problems with hooks / eventing on a project like this is that you essentially end up with dispatching events between every two lines of code to support people's needs, which is how WordPress got to have the codebase they have.

I'm worried that this event system will actually cause more problems than it solves.

@elicwhite
Copy link
Contributor

elicwhite commented Jul 24, 2016

It has been mentioned in slack that the intention for the event system is for things like the logger. Which would provide plugin level support for something like the lcd display and the web ui.

I think that is a reasonable decision, but it becomes very important that we do not trigger bot behavior with events. For example, we should not have events like in the above snippet where we send an event on pre_catch that checks the pokemon inventory and transfers old pokemon. This type of logic should be handled by the behavior tree.

I think supporting the webui and loggers with events is a good solution. I'm very concerned that we will start merging PRs that abuse the event system for bot behavior. This is why I want to make sure we are all on the same page before starting to add event stuff into the codebase.

@ProjectBarks
Copy link

ProjectBarks commented Jul 25, 2016

You have the right implementation but this isn't completed so we can't really just merge it in right now. I will work on these changes later. Thanks for the help!

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