Bug fix on FollowPath, MoveToFort and PolylineGenerator #5382

julienlavergne · 2016-09-11T02:16:18Z

Fix a crash with PolylineGenerator when speed is zero
Fix a bug in FollowPath that think destination is reached each time we walk one step of a Polyline
Fix FollowPath and MoveToFort tasks not returning RUNNING when they are actually the ones dictating the position.

Should fix issues users are having when they put FollowSpiral after a MoveToFort or a FollowPath. With this fix, the FollowSpiral will only be used if MoveToFort/FollowPath has nothing to do.

Fix #5362, #5333, #5186, #5367

mention-bot · 2016-09-11T02:16:19Z

@anakin5, thanks for your PR! By analyzing the annotation information on this pull request, we identified @janpascal, @TheSavior and @th3w4y to be potential reviewers

solderzzc · 2016-09-11T04:02:46Z

pokemongo_bot/cell_workers/move_to_fort.py

@@ -109,7 +109,7 @@ def work(self):
                    formatted='Arrived at fort.'
                )

-        return WorkerResult.SUCCESS


Why you need bypass the following tasks ?

Because you do not want another moving tasks to dictate the position. You are the one deciding at that point.

The other tasks could be caching pokemons! isn't that one of the purpose of loitering?
This makes an assumption on the order of the tasks, otherwise can lead to issues...!
(example..:

if you'd have FollowSpiral before FollowPath you will still move

if you'd have CatchPokemons after MovetoFort you'd not catch any pokemons)

This is what i would like to achieve in #5367 to make the order in which the tasks are specified not matter anymore but simply calculate cost (thus cost of staying put would be zero no other task would move from there until nobody requests to stay put anymore...)

@th3w4y Agreed.

There is no point to put your catch Pokemon task after the moving task. There is no point offering this kind of flexibility. People want the bot to work, they do not care about all the mighty features that they never asked for.
If you really want to remove the WorkerResult because you still want to allow people to put their moving tasks in the middle, why not just having a single variable that we use as a "lock" to decide who set the position ?

in MoveToFort

if self.bot.move_lock is not None and self.bot.move_lock != "MoveToFort": return self.bot.move_lock == "MoveToFort" <do my logic here>

in FollowPath

if self.bot.move_lock is not None and self.bot.move_lock != "FollowPath": return self.bot.move_lock == "FollowPath" <do my logic here>

That way you can put every move tasks where you want, and it only cost you 2 lines of code per walker.

There is no point but people will still do it

There is no point to put your catch Pokemon task after the moving task.

The point is less opened issues to spend our time with...
but if you want to "educate" users on how to configure the bot and what is the "supported" task order and look at all issues rising from this.. I'll guest you do have a lot of time to spare..

There is no point offering this kind of flexibility

But this is already a side discussion.. to be taken at some other point!

solderzzc · 2016-09-11T04:07:18Z

I see. current RUNNING status is not good. but still usable.

th3w4y · 2016-09-11T08:27:56Z

pokemongo_bot/walkers/polyline_generator.py

@@ -94,7 +94,7 @@ def __init__(self, origin, destination, speed, google_map_api_key=None):
        self._step_keys = sorted(self._step_dict.keys())
        self._last_step = 0

-        self._nr_samples = int(min(self.get_total_distance() / self.speed + 1, 512))
+        self._nr_samples = int(min(1 + self.get_total_distance() / max(self.speed, 1), 512))


Wait a second... if you hit the ZeroDivizion something is initializing PolylineWalker incorrectly!

There should be no reason why speed is set to zero during the init of Polyline to hit that case..

distance/speed + 1 is to get minim 2 samples [assumes you want to walk at least self.speed distance which is always the case cause there are no pokestops 2-3m only apart]

Looked a bit in depth... into the API min nr_sample needs to be 2 in order to have elevations returned. otherwise is an invalid response

The polyline is not initialized in the constructor of the Walker, it is initialized at the first step.

1 + self.get_total_distance() / max(self.speed, 1) also get you minimum 2 samples

I found the problem #5382 (comment)

We should not handle ZeroSpeed here otherwise we'll mask future incorrect initialisation cases..

It this PR would have gone through i would have probably not spotted the problem in PolylineObjecthandler... and you would get an invalid Polyline which would be cached!

resulting in best case in just 2 elevations along the origin-destination path

What do you mean ? In what case do you get an invalid Polyline ?

th3w4y · 2016-09-11T10:12:12Z

lets assume bot position is 0,0
It is not valid to do this:

step_walker = PolylineWalker(0,0)
step_walke.step(speed=0)

This is invalid because:

You will initialise a Polyline object with speed ZERO which makes no sense!
Purpose of PolylinebjectHandler is to give you back a valid reusable Pollyline with a valid destination not one initiated by step(speed=0)
You consume one API call for nothing...
there will be no directions or elevations returned distance is 0
Polyline._points = one point
encoded polyline has only one point...
no mater of the nr. of samples there will be no elevations returned

th3w4y · 2016-09-11T10:14:16Z

correct use case is
bot.position 0,0
You take a valid next destination AND stay put towards it!

step_walker = PolylineWalker(0.1,0.1)
step_walker.step(speed=0)

I am sorry if this is a problem but we'll will have to adopt this model.
There is nothing we can do about how the Google APIs responds to us when distance is "Zero"

th3w4y · 2016-09-11T10:32:12Z

Even when used correctly as per #5382 (comment)

There could still be a problem when the first call to PolylineObjectHandler is created by a step with speed zero.

https://github.com/PokemonGoF/PokemonGo-Bot/blob/dev/pokemongo_bot/walkers/polyline_generator.py#L52

For that solution is
speed = speed or FIXED_VALUE
before we call:

PolylineObjectHandler._cache = Polyline(origin, destination, speed, google_map_api_key)

julienlavergne · 2016-09-11T10:49:15Z

I am not sure what issue you are talking about. I do not want to initialize a Polyline with the current location.

th3w4y · 2016-09-11T10:50:24Z

Yes.. I realised... that is why i commented after...

still a corner case... in the correct usage at

https://github.com/PokemonGoF/PokemonGo-Bot/blob/dev/pokemongo_bot/walkers/polyline_generator.py#L52

I am not sure what issue you are talking about. I do not want to initialize a Polyline with the current location.

th3w4y · 2016-09-11T10:58:52Z

~~I have to think now what will happen when speed is float("inf")..~~

Handled that also!

julienlavergne · 2016-09-11T11:17:34Z

So why not 1 then ?

th3w4y · 2016-09-11T11:21:38Z

I see you understood.. 👍

So why not 1 then ?

will fetch too many elevations... much more calculations later on in .get_alt()

makes sense?

th3w4y · 2016-09-11T11:25:21Z

i could even standardise on a fixed speed no matter what... or not involve speed at all
.. it will not affect walking! only nr. of elevation samples

Shall i do that? say like pick 3.16 no matter what... override the passed argument...?

maybe comment on #5392 to not highjack this one...

julienlavergne · 2016-09-11T11:32:06Z

I would pick 1 each time it is possible (that is for every Polyline of 512 meters of less). And I would pick 2 for polyline between 512 and 1024 meters, and 3 for polyline between 1024 and 1536 etc...
The speed does not matter, we just need some elevation points, they don't need to be exact at each centimeters.

th3w4y · 2016-09-11T11:36:42Z

@anakin5 meaning you would always pick min(distance, 512) right?

julienlavergne · 2016-09-11T11:36:48Z

I convinced myself
self._nr_samples = 1 + int(math.ceil(self.get_total_distance() / 512))

th3w4y · 2016-09-11T11:42:51Z

Incorrect!

for distance = 510
1 + math.ceil(510/512) = 1
we need at lest 2!

we actually want
min(max(self.get_total_distance(), 2), 512)
if we don't want to involve speed!

julienlavergne · 2016-09-11T11:44:04Z

1 + int(math.ceil(510/512.0))
2

I will add the point to 512 eve if get_total_distance return a float, to be sure

th3w4y · 2016-09-11T11:44:47Z

and the you will have only 2 elevation samples!!!!!! for 510 meters....

I am repeating myself we need

min(max(int(self.get_total_distance()/3), 2), 512)

julienlavergne · 2016-09-11T11:46:22Z

You always want 512 points ?

th3w4y · 2016-09-11T11:47:53Z

That was why i was involving speed to get kinda 1 elevation sample at each step!

julienlavergne · 2016-09-11T11:48:55Z

ok for the min then

th3w4y · 2016-09-11T11:50:33Z

maybe min(max(int(self.get_total_distance()/3), 2), 512) to ease a big on the calculations... not 3 elevations per step...

julienlavergne · 2016-09-11T11:52:17Z

ok

th3w4y · 2016-09-11T12:00:42Z

~~one small problem still... get_total_distance returns float... i need to send int to the API!~~

Ohh you handled that already! Cool!

th3w4y · 2016-09-11T12:25:55Z

Regarding the Changes in the moving task .RUNNING <==> .SUCCES
My opinion is that the changes done here are 💯% in line with HOW we do the things NOW.

(of course we could do it differently in regards to the order of Tasks in the config file...
...but to ease the pressure on this PR i suggest we disregard those concerns for NOW)

th3w4y · 2016-09-11T13:14:49Z

Test cases for polyline_generator are breaking
you still call with ex_speed argument at
https://github.com/Anakin5/PokemonGo-Bot/blob/b31f5226586629dddc90779efc0a53316dd014da/pokemongo_bot/test/polyline_generator_test.py#L34

and ex_nr_samples should be changed to 64

th3w4y · 2016-09-11T15:28:41Z

pokemongo_bot/test/polyline_walker_test.py

@@ -16,12 +16,10 @@
 ex_total_distance = 194
 ex_resp_directions = 'example_directions.pickle'
 ex_resp_elevations = 'example_elevations.pickle'
-ex_enc_polyline = 'o_|~Gsl~r@??h@LVDf@LDcBFi@AUEUQg@EKCI?G?GBG@EBEJKNC??'
+ex_enc_polyline = 'o_%7C~Gsl~r@??h@LVDf@LDcBFi@AUEUQg@EKCI?G?GBG@EBEJKNC??'


you even took the time to url encode 👍

No need for speed in Polyline. One elevation every 3 meters.

julienlavergne · 2016-09-12T04:32:18Z

Can I merge ?

solderzzc · 2016-09-12T04:36:07Z

I think @th3w4y meant we can.

solderzzc · 2016-09-12T04:36:10Z

👍

solderzzc reviewed Sep 11, 2016
View reviewed changes

th3w4y reviewed Sep 11, 2016
View reviewed changes

th3w4y mentioned this pull request Sep 11, 2016

Proposal for ConcurentWalking "Rickshaw" sharing between the tasks that have to walk #5367

Closed

th3w4y mentioned this pull request Sep 11, 2016

standard average speed for Polyline initialization when step(speed=0) #5392

Closed

th3w4y reviewed Sep 11, 2016
View reviewed changes

Fix walkers.

3c1b0f5

No need for speed in Polyline. One elevation every 3 meters.

julienlavergne merged commit 8b2eb8c into PokemonGoF:dev Sep 12, 2016

julienlavergne deleted the fix_movers branch September 12, 2016 04:42

Bug fix on FollowPath, MoveToFort and PolylineGenerator #5382

Bug fix on FollowPath, MoveToFort and PolylineGenerator #5382

Conversation

julienlavergne commented Sep 11, 2016

mention-bot commented Sep 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3w4y Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3w4y Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solderzzc commented Sep 11, 2016

th3w4y Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

julienlavergne Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3w4y Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016

julienlavergne commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016 • edited Loading

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016

julienlavergne commented Sep 11, 2016

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

th3w4y commented Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

julienlavergne commented Sep 12, 2016

solderzzc commented Sep 12, 2016

solderzzc commented Sep 12, 2016 • edited by alexyaoyang Loading

th3w4y Sep 11, 2016 •

edited

Loading

th3w4y Sep 11, 2016 •

edited

Loading

th3w4y Sep 11, 2016 •

edited

Loading

julienlavergne Sep 11, 2016 •

edited

Loading

th3w4y Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

julienlavergne commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

th3w4y commented Sep 11, 2016 •

edited

Loading

solderzzc commented Sep 12, 2016 •

edited by alexyaoyang

Loading