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

Updated Sniper docs, config.map.example and config.example files. #5554

Merged
merged 6 commits into from
Sep 22, 2016

Conversation

YvesHenri
Copy link

Minor Sniper fixes (including docs).

@mention-bot
Copy link

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

"Dragonite": {},
"Snorlax": {},
"// Mew evolves to Mewtwo": {},
"Mew": {},
Copy link
Contributor

@Gobberwart Gobberwart Sep 20, 2016

Choose a reason for hiding this comment

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

Why removing these (Mew, Articuno, Zapdos, Moltres)? I know nobody's ever seen them, but what if one day... and the bot fails to snipe them :)

Copy link
Author

Choose a reason for hiding this comment

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

Those just appear in game events, which will me announced. Untill then, people are just sending fake reports to the broker, which triggers the sniper and waste a teleport.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... not sure I agree with this approach. Those are definitely VIP pokemon and if an event happens while I'm not paying attention, I'd like sniper to catch them. Also, things other than Sniper use vip pokemon list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @Gobberwart

Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is the only thing holding this PR up now? Bugger. I'll approve it if you change it back, otherwise... someone else will have to make the call.

Copy link
Contributor

@Gobberwart Gobberwart left a comment

Choose a reason for hiding this comment

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

All good with exception of removing Articuno, Zapdos etc. from vips.

@Gobberwart
Copy link
Contributor

good idea @cbogithub, but after the url not before it. That would let us keep semi-complicated configs in the file without having to delete them to disable.

@Gobberwart
Copy link
Contributor

Sorry about the conflicts @YvesHenri. I fixed that vip thing which seems to have upset this.

@Gobberwart
Copy link
Contributor

@cbogithub In the interest of letting this PR get merged without too much complication, I recommend you open a new issue as a feature request with this stuff in it.

"homing_shots": true,
"special_iv": 100,
"order": ["missing", "iv", "priority", "vip", "expiration_timestamp_ms"],
"order": ["missing", "vip", "priority"],
"sources": [
{
"url": "http://localhost:5000/raw_data",
"key": "pokemons",
Copy link
Contributor

@DBa2016 DBa2016 Sep 20, 2016

Choose a reason for hiding this comment

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

Please add "enabled": false

I suggest disabling all sources by default, allowing user to enable exactly what he wants to

Copy link
Author

Choose a reason for hiding this comment

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

Added.

"id": { "param": "pokemon_id" },
"name": { "param": "pokemon_name" },
"latitude": { "param": "latitude" },
"longitude": { "param": "longitude" },
"expiration": { "param": "disappear_time", "format": "milliseconds" }
}
},
{
"url": "https://pokewatchers.com/grab/",
Copy link
Contributor

Choose a reason for hiding this comment

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

add "enabled": false, here, same reason

}
},
{
"url": "http://pokesnipers.com/api/v1/pokemon.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

add "enabled": false, here, same reason

@YvesHenri
Copy link
Author

At everyone requesting changes: this PR is about updating the docs/configs ONLY, since many people have been asking on slack about it. Create an issue and request features instead.

@cbogithub No. If relying solely on me, I will not do this. Like I said before, I want to keep the configs clean and simple. Setting timeout/fetch times will only trigger the possibility of more issues/nightmares. All the default times are good enough. I came to this conclusion after A LOT of tests. You are supposed to find those sources yourself and set them up, instead of relying on me. That's why this sniper has been made for.

@YvesHenri
Copy link
Author

Issues are already coming up: #5560.

Fixed the bug where some pokemons were being classified as VIPs when they really werent (Gobb pushed a fix for this, but it is 'wrong' and was just a temporary one).
Increased the validation timeout from 10 to 7: this IS NOT going to be customizable!!!
Added the "enabled" flag for sources.
Sniper's config validation wont be triggered if it is disabled.
Updated the docs/configs accordingly.
@YvesHenri
Copy link
Author

YvesHenri commented Sep 20, 2016

Couple of updates:

  1. Fixed the potential bug with homing shots under social mode
  2. Fixed the bug where some pokemons were being classified as VIPs when they really arent (Gobb pushed a fix for this, but it is 'wrong' and was just a temporary one)
  3. Increased the validation timeout from 10 to 7: this IS NOT going to be customizable!!!
  4. Added the "enabled" flag for sources
  5. Sniper's config validation wont be triggered if it is disabled

@YvesHenri
Copy link
Author

Conflicts resolved.

Copy link
Contributor

@DBa2016 DBa2016 left a comment

Choose a reason for hiding this comment

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

Okay for me now, except the change requested by @Gobberwart

@@ -735,21 +735,25 @@ This task is an upgrade version of the MoveToMapPokemon task. It will fetch poke
### Options
[[back to top](#table-of-contents)]

* `sources` - Defines whether the **WHOLE** task is enabled or not. Please bear in mind that even if the task is enabled, all or any of its sources can be disabled. (default: false)

Choose a reason for hiding this comment

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

Should this be enabled instead of sources?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good pickup

Copy link
Author

@YvesHenri YvesHenri Sep 21, 2016

Choose a reason for hiding this comment

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

"sources - Defines whether the WHOLE task is enabled or not. Please bear in mind that even if the task is enabled, all or any of its sources can be disabled. (default: false)" and "sources.enabled - Defines whether this source is enabled or not. This has nothing to do with the task's enabled**"**are two things completely different. Does this really sound confusing to you? Really?

Choose a reason for hiding this comment

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

It does, with line 752, not 755.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've missed the point @YvesHenri . There is a line in configuration_files.md which reads 'sources' and it should read 'enabled'.

Copy link
Author

Choose a reason for hiding this comment

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

Finally got it. Sorry

@YvesHenri
Copy link
Author

YvesHenri commented Sep 21, 2016

Like I said before, those legendaries will not appear in game anytime soon. They're only catchable with masterballs and those dont even exist in the game yet (acquirable). It doesnt make any sense to have them on VIP while people can fuzz any of the sniping services out there (mqtt/broker, pokesnipers, slack/discord chats etc) like I've seen many times. Encounter/catch that use this fake data can easily lead users to a ban. I dont agree that these changes should hang this PR when there is clearly a potential bug regarding this task and social mode and many other fixes/enhancements done so far. I dont agree either with the requested changes.

I will not make anymore changes, not even the ones requested by @Gobberwart. You can either accept this and make another PR customizing the config file the way you find correct (and so other people can put their thoughts exclusively on it) or you can talk to @solderzzc about handling wrong data on the mqtt/broker side (might as well talk to all the other sniping source providers).

henrimeep added 3 commits September 21, 2016 11:23
Added a default expiration time (5 minutes) for sniping data that does not have an expiration value. This was due I found some source that did not provide this info, although, after some testings, all of its reports were invalid data.
@@ -382,16 +450,10 @@
"Any pokemon put here directly force to use Berry & Best Ball to capture, to secure the capture rate": {},
"any": {"catch_above_cp": 1200, "catch_above_iv": 0.9, "logic": "or" },
"Lapras": {},
"Moltres": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt anything to place these inside an example file. So we should better keep it as it.

@Gobberwart
Copy link
Contributor

Approving/merging as-is since we can't reach agreement on vip list and that's the only thing holding this up. New PR to follow restoring vip list to how it currently is.

@Gobberwart Gobberwart merged commit a8d7a22 into PokemonGoF:dev Sep 22, 2016
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.

6 participants