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

[th3w4y]

Below proposal is related to all tasks that are changing bot.position [via StepWalker]

Currently we achieve already a great deal (almost everywhere) to standardise the bot.position changes to only use the StepWalker.step() [be it the PolylineWalker(StepWalker)]

Assumptions:

• 90% of users don't deviate much from the documented config.json.example
• We expect all the walking Tasks to befriend each other!
• Tasks have a .work() they calculate distances and do stuff here, they create a step_walker = StepWalker() object and when they call step_walker.step() they expect True if bot has reached the requested destination of False if walk is in progress...
• We don't want to spend time explaining the users what are valid walking task combinations and what not! or to explain them in what order the tasks should be configured....
• Things should just work!

Identified Issues:

• Different Tasks Concurrently request different Destinations [Classic example FollowSpiral conflicting with MoveToFort (well actually conflicting with anything)]
• Certain tasks want the bot.position to not change [loitering for example] and others want it to change.
• Tasks that use PolylineWalker are conflicting with task that don't use it

Reason for the proposal against a refactoring alternative:

• To much refactoring would be needed to solve above mentions points, in too many Tasks..., Walkers... and so on... and so on...
• If i fuck up something the revert is to just undecorate the step() method in StepWalker

[th3w4y]

Metaphor:
I propose the tasks to befriend each other and share the "Rickshaw" of the task that that drives to the closest destination.

Implementation details:
Proposal for decorator that stores all tasks destinations and returns them to the Task call only in a smarter way... based on a distance cost calculation

Assumming bot.position (0,0)

Will check:

• When two tasks are requesting different destinations Task1(0,0), (5, 5)): Task2((0,0), (6, 6))
  We identify that the cost of reaching dest (5,5) is small than reaching dest (6, 6)
  cost(Task1) < cost(Task2)
  Thus:
  a) When Task2.work() calls walker.step() step() will NOT be executed but will return False immediately
  b) When Task1.work() calls walker.step() step() will be executed method will change the bot.position to the calculated new value and return whatever is the case according to speed (True if reached False if not reached)

• When two task have same destination Will check if one of them is a PolylineWalker and if that is the case
  will execute the .step() of the identified PolylineWalker object also for the Task that requested StepWalker!!!

• Other things to check (and possibly contradicting some logic from above)

  - Possibly check the priority based on the order of the tasks in config.json !!!
  

import inspect

class ConcurrentWalking(object):
    # this will store all previously seen walker object and validate/ invalidate them
    _WALKER_CONTAINER = data_container_for_all_StepWalker objects # [] or {} - to be defined!!!

    def _step(foo):
        def magic(self):
            caller_stack = inspect.stack()[1][0]
            if 'self' in caller_stack.f_locals.keys():
                # We identified the callerTask
                callerTask = caller_stack.f_locals['self'].__class__.__name__
                #####################################################
                #   is this the same callerTask object with same destination validate invalidate 
                # cache..
                #                               ... main logic to be defined....
                #
                #     Here we'd also look up the existing walker in the _WALKER_CONTAINER
                #               and will identify their costs
                # ####################################################

                if cost(callerTask) == smallestCost:
                    foo(self)
                else:
                    return False
            else:
                # This is a pure new StepWalker().step() thus we return unmodified!
                foo(self)
        return magic
    @_step
    def step(self):
        print('True')
    _step = staticmethod(_step)

[th3w4y]

And (this with all the cost calculations explained above for every tasks...) would be activated easily by decorating the step() in the StepWalker
[the only change to existing code]

class StepWalker(object):
    ...
    # The only change in StepWalker would be decorating the step() function!!!!
    @ConcurrentWalking._step
    def step(self, speed=1):
    ...

[th3w4y]

I would want your input:

• should I start implement this proposal (Should be quite simple to do..)
• what I should take into consideration in the cost functions:

For the moment i will make two big assumptions:

• Cost of staying put is 0
• Cost of teleporting is -float("inf")
  • for the move to map for example [StepWalker().step(speed=float"inf")]

[sohje]

Just sleepy thoughts: why not just implement some sort of priority queue with some kind of limit over Walker; on step() method we will extract and calculate less cost position and move to that coords, etc?

[th3w4y]

@sohje I am afraid that might not fix everything...

• [that will not allow to pass objects around...and replace one object type with another object type (PolylineWalker <==>StepWalker...)]
• currently the Tasks create stepper objects in each .work and they don't persist the Stepper, thus could be strange to have queue of volatile objects...

And anyway there is not much difference between your suggestion and what i had in mind..(functionally).

I just believe the decorator approach would prevent us from adding this TrafficControlor logic to the StepWalker class and thus keep that class readability, plus that storing those object in a different class the StepWalker will also solve some persistency.

Second part is that pure StepWalker objects are differentiated from the StepWalker object leaving inside a different Class (taskBase type).

Thus we'd still be able to test StepWalker logic in unit test without looking at the queue part with is not needed for the pure StepWalker class. (and we'll create separate test for the TrafficController)

[mjmadsen]

I've been thinking about if our destination picking tasks (movetofort, catchpokemon, etc) help build a list of possible destinations (from all read cells). We then run an algorithm where we map out our next and future destinations - updating our path as we pick up new stops, new pokemon, etc. Maybe use a+ plus some priority bits based on config settings (rarer pokemon, etc), estimated exp gain by heading towards a larger group of lured stops, etc. Then our fort spinner and pokemon catching tasks don't call our stepper - instead they only do those things and our walker task is the only thing directly using stepwalker.

As an added bit, we could have our map show your planned route. To go a step further, we could use our starting position and a new config setting for max distance from the starting position - anything, except for VIP pokemon, outside of that range is ignored. That would solve a very old FR.

I know this isn't quite what you're looking for as far as input, but I hope it can be helpful!

[th3w4y]

@mjmadsen yes i think i can solve all this with my above proposal...
The decorator i am proposing can act like Rickshaw driver/ TrafficController...
[This is actually want i am trying to achieve..]
It would be aware of all workers... and were they want to go, do the routing and tell which stepper to take

[th3w4y]

I don't have a fixation on decorator approach [some may be scared only when they read this word] but i see advantage:

• code readability
• separation of concerns
• being able to have 2 distinct behaviours for same class.method() decorated/undecorated

[sohje]

If i understand you correctly, we can make a singlton class and every task will get the same object and operate on it.
Also, decorator can implement call method instead ugly classmethod like @ConcurrentWalking._step, but its not about implementation, not about this concept in mention in this issue.

[th3w4y]

@sohje every task will get "an" object, the coordinated by logic one... in order to precompute paths and so on... it cannot be the same one [new object could also be created if needed based on ALL the Tasks destinations combined and could be called from here.]

(The uglyness was only for code exemplification... to show that it become a static method)

Also, decorator can implement call method instead ugly classmethod like @ConcurrentWalking._step, but its not about implementation, not about this concept in mention in this issue.

[th3w4y]

To simplify a bit currently:

Phase 0: # current implementation

• We have different mover Tasks - they currently each call StepWalker.step()
• StepWalker handles next_point calculation bot.position setting and returns True or False.
• workers do 3 things...
  • identify one destination
  • wait for the destination to be reached
  • do their work

Phase 1: Could be skipped if we agree to go for phase 2 directly
Without refactoring all workers:

• we keep the StepWalker as is
• workers still expect a True or False from the .step() method but that is only to tell them if they reached a destination (and do their work accordingly...)
• We add a new coordinating class for walking directions to handle what the bot should do with the conflicting destinations requests or to generate the best path between the workers destinations requests. (if that would decorates .step() then would mean less refactoring...)
• use of inspect module in that coordinating class.. ensure we know everything about that Task we can identify the caller and thus we can create a distance/task based queuing/coordinating logic {'MoveToFort':<StepWalker(12.3,12,4) _q36d74_>, 'FollowPath':<StepWalker(12.4,12,4) _au4g5_>}

Phase 2:

• we modify the workers to not send only one destination to StepWalker, but to possibly instantiate their step_walker differently:
  step_walker = StepWalker([(12.2, 12.2),(12.3, 12.2),(12.3, 12.3),(12.3, 12.4),(12.4, 12.4),(12.5, 12.5)]) but they call it not to walk but specify intent to reach those destinations..
• workers do 3 things...
  • identify destinations and send the as a list of tuples to StepWalker/CoordinatingClass (depending on implementation)
  • wait for the "a" destination to be reached (this will be the closest destination that was reached)
  • do their work
• coordinating class fetches those destinations and creates the walking route based on all the combined destination requests.
• Computed path could be displayed on the web GUI!

[julienlavergne]

Identified Issues:

• Different Tasks Concurrently request different Destinations [Classic example FollowSpiral conflicting with MoveToFort (well actually conflicting with anything)]
• Certain tasks want the bot.position to not change [loitering for example] and others want it to change.
• Tasks that use PolylineWalker are conflicting with task that don't use it

I don't understand this "conflict" issue. There is bug in MoveToFort line 112, it is returning SUCCESS instead of RUNNING. You change that single line and all the issues you mention above are gone.
Also my comments on each point:

• This is only true if one of the moving tasks wrongly return SUCCESS instead of RUNNING. If this condition is true, there is no two tasks giving different positions.
• Again, it is related to SUCCESS and RUNNING status.
• I do not have that issue, I use Polyline on one of my task and StepWalker in another and they go well with each other. What is the scenario ?

[th3w4y]

@anakin5 I suggest please take the time and read completely this proposal, The examples I gave as issues were mere examples don't focus on those per say (are the first verified identified issues that pop to mind)
(One more example of conflicting direction if you want #5186 (comment))

I don't understand this "conflict" issue. There is bug in MoveToFort line 112, it is returning SUCCESS instead of RUNNING. You change that single line and all the issues you mention above are gone.

Your comments are valid in .tick() context not at a higher level (end result achieved)

Also my comments on each point:

• This is only true if one of the moving tasks wrongly return SUCCESS instead of RUNNING. If this condition is true, there is no two tasks giving different positions.
• Again, it is related to SUCCESS and RUNNING status.
• I do not have that issue, I use Polyline on one of my task and StepWalker in another and they go well with each other. What is the scenario ?

As long as people write new Tasks new such issues will arise...
(Or even that one single line that you were mentioning #5382 is changing the behaviour in more then that one Task that you modified [is indirectly changing behaviour for PokemonCatch for example] depending on the order the tasks are configured...)

This proposal focuses also on future.. Is not limited at the currently identified issues....

I was mentioning in Assumptions:

• We don't want to spend time explaining the users what are valid walking task combinations and what not! or to explain them in what order the tasks should be configured....
• We expect all the walking Tasks to befriend each other!

I am proposing a sort of TrafficController to "rule them all" (in the decorator form - but hey forget implementation details for now!) and to do future destinations path routing...

...and you are saying that the exampled i gave are fixed...

Are you saying we don't need such a Traffic Controller?

[julienlavergne]

Are you saying we don't need such a Traffic Controller?

Yes, maybe we just make sure 2 tasks cannot drive the position in same time. I proposed something simple as a comment in my PR:

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>

You can do that with decorator as well, but that is an implementation detail

[th3w4y]

@anakin5 what about conflicting destination how do you solve that? because you just locked the first one you received!

What about calculating best route?

[julienlavergne]

There is only one task giving destination, all others are silent. Priority is handled with order in the task list, just like for other tasks.

[th3w4y]

I think you ignore this part

@th3w4y
What about calculating best route?

@anakin5 we collaborated in few PRs and issues before and I believe we're both quick to contradict each other at times... [but thats's usually because we provide close to real-time feedbacks to each other]
But i tend to believe we still did generally found value in each other's comments.
(Once we get to make our point understood by the other person...)

Please take the time to also check if there is a positive value in this proposal not only contradict.

So far it looks to me that your focus was only on the identifies issues examples.

[mjmadsen]

I will say I think you guys work well together after you both get on the same page. The walker updates you guys put in are great. Keep up the great work! We should try to use our chat more often (including private chats) so establish our thoughts. We all have the same goal of making an awesome bot. You're both very good programmers and have a great deal of insight. 👍

[th3w4y]

I have to mention that I like that @anakin5 is usually able to come up with a simpler solution...

But i still see dead peoples here:
@anakin5 how does MoveToFort play together with FollowPath? imagine them in whatever order you want (then revert the order) and think what results you'd get even with lock...

• do you still follow the points in FollowPath?
• do you still move to forts along the Path?

One high level result I would expect as a user is to go from A..B...C...Z (from my path file) but also pick all the pokestops in the way... in both cases.

[julienlavergne]

I guess people use FollowPath because they want the bot to stick to a route. FollowPath is also spinning Forts, so there is no need to especially move to a fort.

Let's say you still want to mix them to achieve something. There is a of issues in front.
What forts are you allowed to visit while following the path ? Is it a distance from the path ? How do you compute distance from the path with a Polyline anyway ?

We are only moving to spin fort and hatch eggs. It does not matter much whether you were able to find the shortest way to spin the 250 forts of your area.
Even if you imagine that you know in advance the location of the Pokemon, you will just find a way to catch all the ones you are interested in and disregard the forts.

I am just feeling this is going to take a very long time to get something working. And then another very long time to fix bugs. And then everything is going to be more complicated to implement.

But I mean, you asked for thoughts :), I am OK if you do something and it works as well. I just don't feel that work is going to be used much.

[th3w4y]

@anakin5 Thanks! feedback appreciated... this one i found to be more constructive.

one of the side advantage to my proposal would be that we could possibly show the computed path in the web GUI, that would be a candy future to have...

[th3w4y]

I plan to do some changes but not push yet to upstream branches yet that would modify the Polyline object to support base line segments operations... #5374 #5379 to minimise the nr. of API calls and to use Polyline.get_total_distance() for distance calculations.

How do you compute distance from the path with a Polyline anyway ?

[th3w4y]

lets just also add that i would consume less API calls if i know more destinations in advance...
I can send up to 10 waypoints in one API call to google and google will return directions back in the order according to the best path computed by them (then i would create either one Polyline with best route... or 10 small polylines out of one(1) API call - depending on the use case)

Thus much of the computation would be done indirectly on the Google side.