Improvements to step walker

[alexyaoyang]

Improvements to step walker:

Continued from #4025

Improvements:

• Simplify code for randomization
• Handles corner cases such as walk_min>walk_max (by uniform()).
• If walking speed is 0, throw an exception telling users that walking speed should be more than 1.

[mention-bot]

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

[mjmadsen]

I suppose we might have folks who sometimes want to have a speed of 0.

[alexyaoyang]

@douglascamata Your input please, why is the bump needed/necessary?

[TheSavior]

I think the original idea was that if walk was 0, then we should teleport straight to our destination. Aka, do it in a single step. I'm not sure that is actually possible anymore with the code that is on line 26. We should make sure we fix that functionality.

[mjmadsen]

Oh I see it now. With the speed bump, we're setting steps = 1 so line 38 still evaluates as true and thus teleports.

[alexyaoyang]

@TheSavior I see what you mean. I will fix it. Just making sure, by fix I assume fix to remove? According to @douglascamata, we should have it removed asap.

[mjmadsen]

I'd say remove the whole teleport business. If that's a thing we want to have, it should be configurable - but I don't think most people even know it's currently a thing.

[mjmadsen]

Bit off topic (still related to walker), but we could probably swap this line:
self.speed = self.bot.config.walk_max - random() * (self.bot.config.walk_max - self.bot.config.walk_min)

With this:
self.speed = uniform(self.bot.config_min, self.bot.config.walk_max)

Probably should have done this originally, but I'm still learning python.

[alexyaoyang]

@mjmadsen Thanks for pointing that out. I can make that change later, no problem. Let's see what @douglascamata says with regards to your comment above and I'll make the changes all together.

[alexyaoyang]

@TheSavior Ok, removed teleportation from step walker and simplified the code too.

[BriceSD]

Maybe someone wants to set walk_min and walk_max to 0. Maybe today it makes no sense, but it could be useful later on.
It’s better imo to set a default value to walk_min and walk_max when retrieving them from the config file, if they are not in it.
Kinda like

        self.walk_min = self.config.get("walk_min", 3)
        self.walk_max = self.config.get("walk_max", 5)

[alexyaoyang]

@BriceSD We are already retrieving from those config fields prior to calculating the speed here. I guess we should make a decision here whether to bump up the speed or not when the speed is 0, as @douglascamata mentioned in #4025.

[BriceSD]

As I understand it, he wants to remove the teleport thing. Currently it teleports when the speed (so walk_min/max) is equal to 0.
I’m suggesting to remove this as well (so no more self.steps = 1) and add the possibility to set walk_min/max to 0. This way you can force your bot to stay still (between 6 lured forts for example) and catch pokemons (currently catching pokemon would make you move).
Also having a safe default choice is a must have. With the 2 lines I’ve shown you, you got it as well (ofc you have to place them before computing the speed).

[alexyaoyang]

@BriceSD Rest assure that I fully understand your point, having a default is always good. I also agree with you that settings the walk speed to 0 is a easy method of making the bot stand still.

On top of removing teleport, he also wants to bump up any calculated walking speed of 0 to 4.16 ( #4025 ), which is exactly what you don't want. I think we should have more suggestions before I make more commits. Inputs needed @TheSavior @mjmadsen @douglascamata

I'll be happy to make the edits after we reach an agreement as there seem to be some discrepancy in this matter.

On another slightly related matter, to make the bot stand still, I think a clearer way is to not have any Move or Follow tasks. Allowing the bot to move during CatchPokemon task doesn't sound right to me at all. What do you think?

[BriceSD]

Nvm it doesn’t move, only MoveToMapPokemon does.
Maybe I misunderstood what he said. Oh well, he’s the the most qualified person for that one. Better wait him.

[mjmadsen]

We apply default values if either walk_min or walk_max are not specified in the config.

As far as I can tell, @douglascamata wanted to bump the speed up so that we wouldn't fall into teleporting. But if that is removed, I don't see much of a reason to bump the calculated speed up. I personally like the idea of being able to set the range from 0 to X, as a real player might not always be moving when not catching, getting a stop, etc. There may be another reason we want to prevent this that I am not aware of.

We can remove the import on random now since we're only using uniform.

[alexyaoyang]

@mjmadsen @BriceSD Ok I've pushed in the edits. We now allow max_walk and min_walk of 0. Note that the bot will still walk very very very slowly, since division by 0 is not possible. The best way to make the bot stand still is to not have any Move or Follow task 👍

[TheSavior]

I'm confused. Historically, walk being 0 means that it jumps straight to the destination. It does not mean "don't walk".

[mjmadsen]

The logic to teleport in case of speed = 0 was removed from this branch.

[alexyaoyang]

@TheSavior A decision was made to remove teleportation here because it results in super fast ban, refer to #4025 for earlier conversation. Do you have other opinions?

[TheSavior]

Okay, that seems fine to remove teleportation.

Why would anyone want to set walk to 0? Why would cause them to do that over just removing the move tasks from their config?

If there aren't good reasons why someone would want to keep the move tasks and have a walk of 0, then I'd rather we just raise an exception if people set walk to 0. Using some min or max python number seems pretty sketchy. And since it is exact opposite behavior from what was there before, it seems more reasonable to throw so that people know we are removing that functionality.

[mjmadsen]

@TheSavior My thought was that users may want to add in this movement variance. Someone playing the app might sometimes take a step and stop, or stand in one place for a moment, so forth. Such users can then set min to 0. Anyone who doesn't want such behavior can use a higher minimum. Sorry if I gave advice that contradicts our goals. I just don't quite see the need to prohibit such variance. Anyone whom enters 0 as a minimum would probably expect they will sometimes barely move.

[TheSavior]

The problem is that if you are randomly choosing between 0 (or near zero in this case) and something else, if 0 is selected, the whole bot will essentially stop working. maxint on a 32 bit system will be 2147483647. That means if it happens to select that, it will take 2,147,483,647 steps to reach their destination. Effectively meaning the bot will never get to its destination. Even if it runs for it will never happen. If we tick once every 3 seconds, it would take the bot 74 days for it to hit its destination. I don't think anyone actually wants that behavior to randomly occur.

If we don't want to throw, I propose that we take max(config_min, 0.1), the max of their min configured speed and some normally small number, like .1. I think since we are changing the value of what the user entered, we should also put in a log message saying that their config walk min should be a positive number.

[mjmadsen]

@TheSavior Oh! step is called recursively? So if we had a very low speed generated, it would be stuck in the call? Sorry to keep you vested in such a small decision.

@alexyaoyang How about we forgo the whole mess and just throw an exception as @TheSavior suggested (if walk_min less than 1 or something in there).

[mjmadsen]

Added minimums #4269

[alexyaoyang]

@TheSavior @mjmadsen Sounds good! Throwing an exception when walk speed is 0 is logical. I've pushed in the changes.

[alexyaoyang]

@mjmadsen @TheSavior All set and ready!

[TheSavior]

Thanks guys

[MZorzy]

@TheSavior 74 day? fix speed fro a point to b point is not 'human'
and no stop is unre too x2