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

Add CampFort task #5105

Merged
merged 2 commits into from
Sep 3, 2016
Merged

Add CampFort task #5105

merged 2 commits into from
Sep 3, 2016

Conversation

julienlavergne
Copy link
Contributor

@julienlavergne julienlavergne commented Sep 2, 2016

New task that walk between fort clusters and stay there for the time configured.

max_distance is radius in meters of the circle around starting position to limit movements of the bot.
min_forts_count is the minimum number of forts to be considered a cluster.
min_lured_forts_count is the minimum number of lured forts to be considered a cluster.
camping_time is the minimum time to stay at a cluster. A cluster can be selected again after if it is still the best place to be.

To be place before MoveToFort

{
    "type": "CampFort",
    "config": {
        "max_distance": 2000,
        "min_forts_count": 2,
        "min_lured_forts_count": 1,
        "camping_time": 1800,
        "moving_time": 600
    }
},

@mention-bot
Copy link

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

@solderzzc solderzzc closed this Sep 2, 2016
@solderzzc solderzzc reopened this Sep 2, 2016
self.config_max_distance = self.config.get("max_distance", 2000)
self.config_min_forts_count = self.config.get("min_forts_count", 2)
self.config_min_lured_forts_count = self.config.get("radius", 1)
self.config_camping_time = self.config.get("radius", 1800)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo? Shouldn't this be "camping_time" instead of "radius"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes typo. thanks i will fix

@solderzzc
Copy link
Contributor

This task includes many logic task here. Is there better way to struct it?

@julienlavergne
Copy link
Contributor Author

What logic are you referring to ?

alt = self.walker.pol_alt + random_alt_delta() / 2
self.bot.api.set_position(lat, lon, alt)
self.last_position_update = now
self.bot.heartbeat()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heartbeat should be call in tick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it. It is not needed anymore since commit 5b3cfbb.

Otherwise, if heartbeat should be called in tick, why isn't it ?

@solderzzc
Copy link
Contributor

@anakin5 I have two inline comments

@julienlavergne
Copy link
Contributor Author

I added moving_time parameter to forward movement to next task during that time.

@BreezeRo
Copy link
Contributor

BreezeRo commented Sep 3, 2016

I believe @solderzzc is saying that it is not advised to use info from workers in this way as there are cases where "running" information would not be ready to read depending on which order it were in in the task list - and he also said

but that's not related in this PR imo

this look good to me 💃

@solderzzc
Copy link
Contributor

Lol @BreezeRo has much better English language ability.

@solderzzc
Copy link
Contributor

solderzzc commented Sep 3, 2016

👍

Approved with PullApprove

@solderzzc solderzzc merged commit d67de1a into PokemonGoF:dev Sep 3, 2016
@th3w4y
Copy link
Contributor

th3w4y commented Sep 4, 2016

What tasks can this be combined with? What is you config for tasks #5169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants