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

big refactor to Stepper class #737

Merged
merged 7 commits into from
Jul 25, 2016
Merged

Conversation

douglascamata
Copy link
Member

@douglascamata douglascamata commented Jul 25, 2016

We now have a:

  • StepWalker class, responsible for walking a small distance towards a
    final destination.
  • SpiralNavigator class, which defines a route around an area in a spiral. It
    determines points to walk to and uses to StepWalker to slowly go from
    one point to another.

This architecture enables us to have different Navigators, although it
doesn’t implement a nice Navigator configuration YET.

Implementing #717

We now have a:

- StepWalker class, responsible for walking a small distance towards a
final destination.

- SpiralNavigator class, which around an area in a spiral. It
determines points to walk to and uses to StepWalker to slowly go from
one point to another.

This architecture enables us to have different Navigators, although it
doesn’t implement a nice Navigator configuration YET.
@jtdroste
Copy link
Contributor

Going to have to rebase and update your branch my friend :). Out of date with dev

self.stepper.take_step()
location = self.navigator.take_step()
cells = self.find_close_cells(*location)
self.work_on_cell(cells[0], location, False)
Copy link
Contributor

@elicwhite elicwhite Jul 25, 2016

Choose a reason for hiding this comment

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

What's the plan to have this look at more than just the first cell? Could we change find_close_cells to get_current_metacell or something and have that merge the cells?

@elicwhite
Copy link
Contributor

This looks right to me except for my comments

@douglascamata
Copy link
Member Author

@jtdroste yeah, I will. But I'm afraid of ugly merge conflict, so I'm taking a deep breath first.

@Nihisil
Copy link
Contributor

Nihisil commented Jul 25, 2016

👍

@douglascamata
Copy link
Member Author

Don't merge, I found problem.

@douglascamata
Copy link
Member Author

@jtdroste need help! My code is not doing small steps like I want it to, but I'm not a geometry guy and you are! :(

@elicwhite
Copy link
Contributor

Fixed the merge conflicts. Fixed the branch to successfully catch pokemon, spin forts, and walk around. It also doesn't spam the log with walking info.

@solderzzc
Copy link
Contributor

@tejado how do you think about this commit? Should we merge?

@solderzzc
Copy link
Contributor

Traceback (most recent call last): File "pokecli.py", line 222, in <module> main() File "pokecli.py", line 213, in main bot.take_step() File "pokemongo_bot/__init__.py", line 44, in take_step self.work_on_cell(cell, location) File "pokemongo_bot/__init__.py", line 133, in work_on_cell hack_chain = worker.work() File "pokemongo_bot/cell_workers/seen_fort_worker.py", line 109, in work raise RuntimeError(message) RuntimeError: Stopped at Pokestop and did not find experience, items or information about the stop cooldown. You are probably softbanned. Try to play on your phone, if pokemons always ran away and you find nothing in PokeStops you are indeed softbanned. Please try again in a few hours.

Works well in dev branch.

@@ -78,7 +112,7 @@ def work_on_cell(self, cell, position, include_fort_on_path):
if self.catch_pokemon(pokemon) == PokemonCatchWorker.NO_POKEBALLS:
break
if (self.config.mode == "all" or
self.config.mode == "farm") and include_fort_on_path:
self.config.mode == "farm"):
if 'forts' in cell:
# Only include those with a lat/long
forts = [fort
Copy link

@ProjectBarks ProjectBarks Jul 25, 2016

Choose a reason for hiding this comment

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

Am i missing something or is this missing a ]

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on the next line:

forts = [fort
                         for fort in cell['forts']
                         if 'latitude' in fort and 'type' in fort]

@ProjectBarks
Copy link

ProjectBarks commented Jul 25, 2016

@douglascamata I made a lot of comments but make sure you dont have two things primarily

  • Unused Variables
  • Unused Imports

Also in the SpiralWalker class could you explain why need position and x and y as two seperate things

status = map_objects.get('status', None)

map_cells = []
if status and status == 1:
Copy link

@ProjectBarks ProjectBarks Jul 25, 2016

Choose a reason for hiding this comment

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

just do map_objects.get('status') and if status == 1: the second change isnt needed

@MikeDX
Copy link
Contributor

MikeDX commented Jul 25, 2016

Looking forward to seeing this merged so I can have a go at implementing tsp for pokestops within the walk radius

@elicwhite
Copy link
Contributor

This seems to be structurally working now. I've been running through a bunch of the situations locally and it's been working great.

@elicwhite
Copy link
Contributor

elicwhite commented Jul 25, 2016

@ProjectBarks I'd like to fix most of the style errors for the stepper refactor in a follow up PR where we fix travis and can actually fix all these issues. And get the change in so we don't block all the other stepper changes that want to happen since this is a big PR that will be problematic with merge conflicts.

@ProjectBarks ProjectBarks merged commit 586a0dc into dev Jul 25, 2016
@elicwhite elicwhite deleted the feature/stepper_big_refactor branch July 25, 2016 05:01
@th3w4y
Copy link
Contributor

th3w4y commented Jul 25, 2016

I will need to rewrite some of the code that was merge in #721 because of this refactoring.

I had class PolylineStepper(Stepper): in polyline_stepper.py I need to rewrite it to a navigator.

@tejado
Copy link
Contributor

tejado commented Jul 25, 2016

@solderzzc do you mean me?

MFizz pushed a commit to MFizz/PokemonGo-Bot that referenced this pull request Jul 29, 2016
* big refactor to Stepper class

We now have a:

- StepWalker class, responsible for walking a small distance towards a
final destination.

- SpiralNavigator class, which around an area in a spiral. It
determines points to walk to and uses to StepWalker to slowly go from
one point to another.

This architecture enables us to have different Navigators, although it
doesn’t implement a nice Navigator configuration YET.

* small fixes to workers

* Merging

* Fixing arguments

* Moving the distance log line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants