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

Refactor Polyline to support speed variance. #5157

Merged
merged 3 commits into from
Sep 4, 2016
Merged

Refactor Polyline to support speed variance. #5157

merged 3 commits into from
Sep 4, 2016

Conversation

th3w4y
Copy link
Contributor

@th3w4y th3w4y commented Sep 4, 2016

Short Description:

Description will be updated soon

Fixes

  • adds support for speed variance
  • removes .pause() .unpause() methods..
  • removes all timebases internals..
  • some methods become private... they should not be used externally...
  • ...

@mention-bot
Copy link

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

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@BreezeRo @sohje @solderzzc @mjmadsen as discussed in #5147

Please peer review and add the Review and Help tags to this PR

Thanks...

@th3w4y th3w4y changed the title Reactor Polyline to support speed variance. Refactor Polyline to support speed variance. Sep 4, 2016
@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

This is the generated test line... without the noise..

You can see that the dots are not even separated because of changing speed between the steps...
used a big range of min 2.16 max 30 for illustration purpose

screen shot 2016-09-04 at 03 38 05

@mjmadsen
Copy link
Contributor

mjmadsen commented Sep 4, 2016

👍

Approved with PullApprove

@solderzzc solderzzc merged commit 1b6f43a into PokemonGoF:dev Sep 4, 2016
@julienlavergne
Copy link
Contributor

It seems there is still an issue :s. When the google api quota is reached, the MoveToFort task configured with PolylineWalker makes us jump from fort to fort.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 will try to see where it can go wrong...

@julienlavergne
Copy link
Contributor

The Polyline only contains starting point and destination. Before, StepWalker was taking care of doing small steps. But now we directly set the destination.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 it cannot be... we have the speed involved in the calculation...

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 did you check after this commit... #5162

This might also fix something..
I continue looking.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 I will add a test case for that see were it breaks..

I have managed to reproduce an object with API limit reached for directions..


In [5]: a._directions_response
Out[5]: 
{u'error_message': u'You have exceeded your rate-limit for this API.',
 u'routes': [],
 u'status': u'OVER_QUERY_LIMIT'}

In [6]: 
In [8]: a
Out[8]: <pokemongo_bot.walkers.polyline_generator.Polyline at 0x110beb1d0>

In [9]: a._points
Out[9]: [(47.1706378, 8.5167405), (47.1700271, 8.518072999999998)]

In [10]: a._step_dict
Out[10]: 
{121.47962242296278: ((47.1706378, 8.5167405),
  (47.1700271, 8.518072999999998))}

In [11]: a.get_alt()
Out[11]: 429.5892333984375

In [12]: a._last_pos
Out[12]: (47.1706378, 8.5167405)

In [13]: a._last_step
Out[13]: 0

In [14]: a._step_dict
Out[14]: 
{121.47962242296278: ((47.1706378, 8.5167405),
  (47.1700271, 8.518072999999998))}

In [15]: a.speed
Out[15]: 3.0

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

I am sorry but i cannot reproduce...

all .get_pos() returned the next calculated distance on the bearing between origin and destination

In [14]: a.get_alt()
Out[14]: 429.5892333984375

In [15]: a.get_pos()
Out[15]: (47.17062271583738, 8.51677331105169)

In [16]: a.get_pos()
Out[16]: (47.17060763167472, 8.516806122094092)

In [17]: a.get_pos()
Out[17]: (47.17059254751201, 8.516838933127204)

In [18]: a.get_pos()
Out[18]: (47.17057746334926, 8.516871744151027)

In [19]: a.get_pos()
Out[19]: (47.170562379186485, 8.51690455516556)

In [20]: a.get_pos()
Out[20]: (47.170547295023674, 8.516937366170804)

In [21]: a.get_pos()
Out[21]: (47.17053221086082, 8.516970177166757)

In [22]: 

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 i am not able to simulate the Elevation API limit reach but i doubt that would be cause of the problem...


In [29]: while True:
    ...:     a = Polyline(ex_orig, ex_dest, 3)
    ...:     print(a._directions_response['status'], a._elevation_response['status'])
    ...:     
    ...:     
    ...:     
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OK', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OVER_QUERY_LIMIT', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')
(u'OK', u'OK')

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 how many tasks do you have, which one are those,

and.... any 2 tasks use PolylineWalker in the same time?

@julienlavergne
Copy link
Contributor

It is daily limitation of the direction API. When it happen, there is only 2 steps: the origin and the destination.
get_pos() return the origin at first call and the destination at second call.
On second call, The stepWalker step do a get_new_pos that seems to return the final destination.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 this not able to reproduce... do you have origin and destination?

in my above example you can see i also had API limit reached and .get_pos() was returning correct:


In [15]: a.get_pos()
Out[15]: (47.17062271583738, 8.51677331105169)

In [16]: a.get_pos()
Out[16]: (47.17060763167472, 8.516806122094092)

In [17]: a.get_pos()
Out[17]: (47.17059254751201, 8.516838933127204)

In [18]: a.get_pos()
Out[18]: (47.17057746334926, 8.516871744151027)

In [19]: a.get_pos()
Out[19]: (47.170562379186485, 8.51690455516556)

In [20]: a.get_pos()
Out[20]: (47.170547295023674, 8.516937366170804)

In [21]: a.get_pos()
Out[21]: (47.17053221086082, 8.516970177166757)

In [22]: 

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 and btw... first .get_pos() should not return the origin... should return the position after self.speed seconds..

are you sure it returns the origin?
just to know where to look...

@julienlavergne
Copy link
Contributor

I was trying to print more logs to give you some info, but I found the issue. There is no sleep(1) in the whole bot loop when using PolylineWalker. It gives the impression we teleported while in fact we were setting the position very quickly.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

the Sleep is in StepWalker..

StepWalker sets the position then sleeps one second

@julienlavergne
Copy link
Contributor

In my cast, I see the steps of the MoveToFort task with Polyline walker are around 2.25 meters, which is below my speed (2.5). So we teleport there because of the first condition in StepWalker.step.

@julienlavergne
Copy link
Contributor

And there is no sleep there.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 you mean this ?:

if (self.dLat == 0 and self.dLng == 0) or self.dist < self.speed:

@julienlavergne
Copy link
Contributor

yes

@mjmadsen
Copy link
Contributor

mjmadsen commented Sep 4, 2016

Should we call heartbeat in that if?

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@mjmadsen yes that would be fine.. but we also have to sleep

@julienlavergne
Copy link
Contributor

I am testing removing that branch completely, I do no see why we need it now.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

for now we have this PR #5169

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 i also believe is not needed but if we take it out it will scare people

of course that is the only part that actually returns True... keep that in mind...

@th3w4y th3w4y deleted the refactor branch September 4, 2016 09:18
@julienlavergne
Copy link
Contributor

It scares me to have it. Seems it is fine without that piece of code. However need to pay attention to the last step. Does the new calculation always return the exact destination point ?

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 strictly from Polyline yes...
Is only the StepWalker that returns a indirect delta of default [-50cm, 50cm]

@julienlavergne
Copy link
Contributor

So let's remove that branch then.

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 not comfortable with that yet.. is the only branch that returns True..

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 and once again thanks for testing the PolylineWalker... you are usually first to report any bugs related to it. Thanks Thanks

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

just worth mentioning not only the Polyline walker would have been affected by this... but all tasks.. at the last step....

(polyline was affected on each step...when random speed fetch by StepWalker from 2.16 to 4.15 would have been smaller then the calculated next pos on the poyline)

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

@anakin5 i still think there might be an issue... not sure of the PolylineObjectHandler is doing it's job

or at least i cannot mock it properly

@th3w4y
Copy link
Contributor Author

th3w4y commented Sep 4, 2016

Seems to return something similar to api limit although is should not...

There should be more then orig and destination there...

In [32]: PolylineObjectHandler.cached_polyline(ex_orig, ex_dest, 3)._points
Out[32]: [(47.1706378, 8.5167405), (47.1700271, 8.518072999999998)]

Update:

It was the Mock()... was returning an mock object not None or API key..

All good!

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.

5 participants