-
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
follow_* and walkers set_position through StepWalker #5038
Conversation
@mjmadsen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @douglascamata, @tstumm and @leanhdaovn to be potential reviewers |
Not entirely sure why the CI is crapping out. But I'm still not finished :) |
is it .get_position() or .position() ? I see some inconsistencies between: and |
cLng, | ||
cAlt | ||
) | ||
|
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.
@mjmadsen This is not going to change the bot position anymore...
since you replaced .set_position() with just an object initiation.
But there is nothing calling .step() on the new step_walker object
@mjmadsen I created a PR towards your fork https://github.com/mjmadsen/PokemonGo-Bot/pull/1
|
PolylineWalker no longer overrides super .step() method
@th3w4y Thank you! |
|
||
if dist <= 1 or (self.bot.config.walk_min > 0 and step_walker == None): | ||
if self.ptr + self.direction >= len(self.points) or self.ptr + self.direction <= -1: | ||
self.direction *= -1 |
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.
Am I breaking anything by removing this? It didn't seem to be applied last night when I was looking.
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.
@mjmadsen it is breaking indeed is returning always the first point only!...
you still need something from the if dist <=1
if dist <= 1 or step_walker == None:
if self.ptr + self.direction >= len(self.points) or self.ptr + self.direction <= -1:
self.direction *= -1
if len(self.points) != 1:
self.ptr += self.direction
else:
self.ptr = 0
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.
Bah, I was too sleepy when I change all of this! I'll try to put the corrections in this afternoon.
@solderzzc PolyWalker seems to not add in the human variance changes still. |
@mjmadsen what do you mean? in this commit it should... since it does not override the StepWalker.step() anymore... |
Should we make polywalker the default for movetofort? This honestly seems to be superior. It could use some tweaks (as it walks down the middle of the road). |
@mjmadsen you don't see any jitter to it from the StepWalker? from the random_lat_long_delta()? |
Very little compared to the other walkers (follow_*) |
@mjmadsen i though the random_lat_long_delta() would be enough... the random_lat_long_delta() should add something between [-2, +2] m variance.. |
Those random deltas are the only thing added if our noise is disabled right? The variance seemed quiet high with the followers and the polylinewalker seems to be very minimal. |
@mjmadsen wow i think i know why the follow ones have a bigger jitter it has to do with the magnitude self.magnitude way of calculation.. the higher the distance to walk the higher the jitter maybe... I have to do the math... |
Lol. I'll probably work on a few other tasks before returning to this. It's a bit of a minefield. |
follow_path is also affected. Or it's that spiral and the like give such a distant target, our step variance spikes. Would seem that logic needs toe be altered. |
This looks odd to me the method we calculate now.... made a small test with speed 3 trying to reproduce what step walker does now...
15 cm 1 step 2.6m walked
1.56 m 1 step 2.87m walked in 1sec
15.65m 5 steps 3.46m walked in 1sec
156m 52 steps 3.40m walked in 1sec
1.5km 521 steps 3.4m walked in 1sec
|
I have some big problems with this merge:
|
yes it jumps directly to destination.. but the destination is the .get_pos() of the cached polyline not final destination so i don't see an issue...
I agree that gps does not jump 5m every second but you kinda have exact opposite behaviour then @mjmadsen who is complaining of too little variance.. |
#5132 I have a few alterations to this. The first portion is not implemented yet. |
get_pos() return the final destination if Walker has never been paused. random_lat_long_delta is too big. When I look at my GPS position reported by my phone versus what the bot set in StepWalker, there is a very big difference. |
how big is the distance till final destination? it should return final destination only if enough time passes...
|
@anakin5
do you have an example of from... and final destination? |
I think the problem comes from the fact that step_walker.step return True at the first step of the PolylineWalker. That is because the first point of the Polyline is the current position, so the distance to step is zero, which translate to "we are arrived at destination". |
But my opinion is that the real evil there is in the fact that we do not reuse walkers. There is no concept of final destination and next point in the walker, we are expected to create a new object at each step. |
what are you combining the PolylineWalker with?
|
It return True whatever is the task I am using it with. Be it MoveToFort or CampFort. Both of them relying on the fact that it should return True only when arrived. |
The change was big... some issues slipped through...
|
What do you consider a normal range for variation?
|
Oh I see. random_lat_long_delta is supposed to make us look more human with wandering. What would be consider as a better range? Currently supposed to be -9.1 to 9.1m variance. Even assuming those comments are correct, we could move 9.1 x 9.1 in a single step (which is a lot) The gps noise bit is the one that is supposed to alter our coords to match phone's inaccuracies. @anakin5 I assume you have this bit disabled too? |
Doh! At least you found it 👍 |
I do not feel this is ready to merge. But I want to get testers.
Goals it to drive all set position api calls through StepWalker. That includes centralizing noise and humanizing to position to one location. Outputting position updates from one location.
Repeat, I'm not done. Do not merge.