-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
(WIP) Behavior Tree Proof of Concept #155
Conversation
tasks = [] | ||
|
||
for taskConfig in self.config.tasks: | ||
taskRef = globals()[taskConfig['type']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globals()
is really bad behaviour. Please encapsulate properly.
Also taskConfig
is CamelCase and we use snake_case for variables and functions. CamelCase only for class definitions. This is following Python standards, defined as in pep-8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with python, the thinking here is that we want to be able to instantiate these tasks by name. It's probably the right choice to just put a switch statement in for now. If we were trying to support a plugin system at this point, we would need to figure out some way to dynamically pull those in and instantiate by string name. Overengineering at this point though. :)
@TheSavior you tasks implementation looks a lot like our workers. Please keep them in mind when you do your work, because they are used already to process cells (which can be pokemons) and then action are taken in each of those cells by these workers. |
@douglascamata I agree that the tasks and workers seem very similar. I think the biggest difference is that I want to get these tasks more standalone than the workers are. Currently a lot of the logic is not actually in the worker. For example, the stepper essentially tells the worker which pokemon to catch. Since it iterates over each visible pokemon, if you fill up your bag while catching, the worker will be stuck. So in the case of the catch pokemon task, the goal would be for it to figure out what is visible, pick one, catch it, and return. |
@TheSavior that's expected, because the |
And then we can have many classes to decide where to go based on the current state. |
@douglascamata I messaged you on gitter. I'd love to chat this out, and make sure I'm heading in the right direction. One of the side effects of the problem I'm trying to solve in #142 would actually pull out that decision making logic from the stepper. Having it in the stepper makes it hard to extend, configure, and swap out. Take a look at the linked issue where I walk through it a bit more and see if it makes sense where I'm coming from. Edit, I'm now on the slack channel. |
@TheSavior we moved to slack. To get an invite, go here |
Closing the PR for now. After talking to @douglascamata it sounds like there are a bunch of bigger things that need to be worked on now that will cause problems in the short term for this refactor. We'll revisit this soon, so let's continue the conversation in #142 and try to get a good grasp on the approach and implication. It sounds like @douglascamata had something a tad different in mind, so it would be good to figure this stuff out in the mean time. |
added werkzeug version requirement
As talked about in #142, I want to explore the possibility of breaking the bot into extensible, possibly shared, configurable tasks. This is the first step in that direction and I would love any feedback people have on the implementation