Add support for IV scanning and CP scanning #1980

krosenvold · 2017-04-21T21:17:17Z

Added two additional account pools, for a total of three pools. One regular pool, one for level 25 iv scanners and another for level 30 cp/iv scanners.

Added additional abstractions to handle the increased complexity.

To reduce overall risk, I did not introduce the new abstractions in the existing code. I believe the safe approach is to do this once the new feature is properly battle proven. Hence the impact on existing functionality if running without IV/CP scanning is practically nonexistant

Description

Added Account, AccountManager, Worker and WorkerManager to facilitiate multiple account pools and safe & simple IV/CP scanning.

Motivation and Context

Continued support of IV scanning as well as CP scanning.

How Has This Been Tested?

A superset of this code has been run on my 250km2 scanner rig. The CP scanning code has been running for 4 months

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ x] My code follows the code style of this project.
[x ] My change requires a change to the documentation.
I have updated the documentation accordingly.

Alderon86 · 2017-04-21T22:28:21Z

this will need a lot of testing, also i can see alot of unneeded changes in the code, prepare for adjustments :)

343334 · 2017-04-21T23:44:32Z

without looking at the code, is this designed to be where the iv/cp accts are restricted to a particular hex? or those who run bh -st 5 etc, will they be able to service all the hexes available ?

krosenvold · 2017-04-22T09:12:03Z

I'm not familiar with beehive, I use speed scan. Maybe someone who understands the entire code base can find out how this would work ? I suspect it will work. The workers will currently speed block to avoid high kph. I am planning to implement geo-localization of the workers, but might not include this in first version (The worker manager would take location as input and find the "closest" available free worker, worker still blocks to obey speed restrictions like today). Right now your IV checker pool needs to be sufficiently large to avoid too much travel-blocking issues (Currently it appears
something like 30-40 IV workers should be sufficient for a -st 33 area as long as you're not IV scanning the full pokedex)

343334 · 2017-04-22T10:33:26Z

well how --behive works with -speed is lets say i do -st 6 -wph 3 -w 108 that creates 36 st6 hexes - which if defined by hex would require 36 lvl 25 accts to handle it to keep one locked to each hex if needed. Your geo-loc would overcome that situation or that multi webhook as well that we talked about so the lvl 25 accts arent restricted to a certain region like those who are running multiple hives or bh. Though maybe the recommended way to run would be like -st10 wph 4 -wphiv 1 -w 144 etc so that in a st10 4 accts are normal and 1 is set as the iv checker but i think that would be way more work to go that route as many prefer the large st path.

bbdoc · 2017-04-22T12:07:20Z

Wonderful. I'll test this PR asap. Thankd for it.

Just one question : how does it work wrt webhooks ? As far as a "normal" account will probably find the pokemon before another account from the 25+ pool is sent for encountering, will it wait for sending the webhook until pokemon has been encountered or will it send 2 webhooks ?

Also will pool accounts be used for "normal" scanning as well or will they wait until they are needed for an encounter ?

And finally are you also sending CP to webhook ?

Thanks

krosenvold · 2017-04-22T20:38:45Z

All encountering is done before anything is sent to the webhook. cp is sent to webhook. cp & iv scanning pools are only used for cp & iv scanning

SiteEffect · 2017-04-23T23:56:51Z

Review inc. soon by me. It's a huge PR and my time feels like its reciprocal.

j16sdiz · 2017-04-24T02:30:29Z

pogom/models.py

@@ -1760,9 +1767,60 @@ def hex_bounds(center, steps=None, radius=None):
    return (n, e, s, w)


+def do_cp_iv_scan(cpWorkerManager, p, step_location):
+    worker = cpWorkerManager.get_worker_for_location(step_location)


put this line inside try block?

I am definitely not a python developer, but the try/finally block is not to guard against allocation failures (which should be handled inside get_worker_for_location), but from failures when invoking the actual logic inside the block. Please explain to me if python somehow works differently from other langs I'm used to :)

j16sdiz · 2017-04-24T02:33:15Z

config/config.ini.example

+# Search settings
+#################
+
+#china                          # Coordinates transformer for China.


All end-of-line changes?
Please reduce the noise

The line endings in this file are incorrect in the RocketMap repo, which means that any well behaved git client changing this file will appear to change the entire file. Someone with commit priviliges should really run dos2unix on this file and commit the change. I also see that the repository is missing a .gitattributes files, which is probably a very good idea to add. This would prevent future line ending issues., since .gitattributes dictate how the REPOSITORY wants things to be

walaoaaa1234 · 2017-04-24T07:26:16Z

accounts.txt

@@ -0,0 +1,308 @@
+ptc,irkopampcr0kysk,aaaaaaA1!


@krosenvold remove this file

bbdoc · 2017-04-24T13:00:29Z

Hello. Trying to test this PR, but I'm immediately getting an error when starting

InternalError: (1054, u"Unknown column 't1.cp' in 'field list'")

Looks like my DB was not updated as it should...

Any idea why ? What columns should I add to my DB ?

Also 2 more questions :

If providing only a CP scanning account file, will those accounts also be used for IV (I only want to add lvl30+ accounts, no specific lvl25+ for IV).
If providing no list of pokemons to scan for CP, will it by default scan for CP all pokemons that are in my encounter list ? Again, if working only with 30+ accounts, I have no reason to specify different lists, so everything that's in my encounter whitelist (or not in my encounter blacklist) should be scanned for both IV and CP using my 30+ accounts.

Thanks

Bart274 · 2017-04-24T13:10:47Z

A group of us have been testing for 6 hours yesterday and noticed the CP was different with the same spawn for all their levels and they're all lvl30+ trainers? They think the CP is now depended for each individual trainer level and is no longer the same for evere trainer level from lvl 30?

SiteEffect · 2017-04-24T13:12:04Z

@bbdoc Happy to see people in GitHub, fiddling around PRs etc. but you probably see reasons for direct errors pretty fast. So, in this PR the cp column was not integrated into the DB as well as there is a wrong DB version and migration only for level implemented. Please keep in mind, that this is not support forum for PRs but constructive Feedback. If you are really comfortable with the code-base, please ask the PR creator directly via Discord e. g. when you can't figure out an error at all and it is for sure related to the PR. In your case, how about adding manually a cp column with the correct type seen in models.py Class Pokemon regarding CP?

bbdoc · 2017-04-24T13:41:19Z

@aRengo Yes of course adding CP column would be a quick fix, but is it really a good solution.

I do see indeed that the code still mention db_schema_version = 16 which is the same as current dev version. Strange as I'm pretty sure I did see 17 at some point in time. Have to dig into latest commits to see where it was rolled back.

I still guess it's worth it mentionning to PR initiator that there is something wrong in his code resulting in db not being updated. I guess people who managed to get it working started with a blank db and thus didn't get this error.

SiteEffect

This was my first review regarding already existing RM code, I did not had a look into the new classes which demand extra time. I do like the abstraction in general but it'll need some more work. Additionally, please do not commit files like temp saved .gitignore~ or bad bad material we do not like here in the repo (psst, sprites).

SiteEffect · 2017-04-24T13:13:47Z

pogom/models.py

@@ -2625,6 +2723,12 @@ def database_migrate(db, old_ver):

        db.drop_tables([ScanSpawnPoint])

+    if old_ver < 12:


Pretty sure it has to be 17.

SiteEffect · 2017-04-24T13:14:10Z

pogom/models.py

+    if old_ver < 12:
+        migrate(
+            migrator.add_column('pokemon', 'cp',
+                                IntegerField(null=True, default=0)),


SmallIntegerField(null=True)

SiteEffect · 2017-04-24T13:14:46Z

pogom/models.py

@@ -109,6 +114,8 @@ class Pokemon(BaseModel):
    gender = SmallIntegerField(null=True)
    last_modified = DateTimeField(
        null=True, index=True, default=datetime.utcnow)
+    cp = IntegerField(null=True)


SmallIntegerField(null=True)

This is valid until we reach Pokémon with CP ~16.000 and can be extended by PositiveSmallIntegerField to ~32.000. The we can still switch IntegerField if this will expand further, what I do not believe.

SiteEffect · 2017-04-24T13:17:53Z

pogom/models.py

@@ -2667,6 +2771,7 @@ def database_migrate(db, old_ver):
                'NULL DEFAULT NULL,'
                'MODIFY COLUMN `move_1` SMALLINT NULL DEFAULT NULL,'
                'MODIFY COLUMN `move_2` SMALLINT NULL DEFAULT NULL;'
+                'MODIFY COLUMN `cp` SMALLINT NULL DEFAULT NULL;'


Remove this, we are at

Versions.schema_version = 16

SiteEffect · 2017-04-24T13:19:14Z

pogom/models.py

@@ -2738,4 +2843,9 @@ def database_migrate(db, old_ver):
            migrate(
                migrator.add_index('pokestop', ('last_updated',), False)
            )
+    if old_ver < 19:


Add this to the

if old_ver < 17

SiteEffect · 2017-04-24T13:32:16Z

pogom/models.py

                'height': None,
                'weight': None,
                'gender': None
            }

+            pokemon_info = None
+            cp = 0
+            pokemon_level = 0
            if (encounter_result is not None and 'wild_pokemon'


Instead of a do_iv_scan, do_cp_scan think about abstracting any encounter_result into one function definition def get_encounter_response where you request accordingly to given args for distinguishing the config of the user.

Style-wise I agree. But I also believe the current encounter logic is entirely useless ? I'd really enjoy looking at all of these together, so I'm sort-of awaiting the "encounter" issue to clear out.

SiteEffect · 2017-04-24T13:35:13Z

pogom/models.py

+                            pokemon_info = do_cp_iv_scan(cpWorkers, p,
+                                                         step_location)
+                            cp = pokemon_info.get('cp', 0)
+                            pokemon_level = pokemon_info.get('level', 0)


Afaik a level is not submitted, only the corresponding cp_multiplier which translates into a level with a table (I think I saw this table once in your fork)

SiteEffect · 2017-04-24T13:37:10Z

pogom/models.py

@@ -1968,47 +2027,86 @@ def parse_map(args, map_dict, step_location, db_update_queue, wh_update_queue,
                'individual_stamina': None,
                'move_1': None,
                'move_2': None,
+                'cp': None,
+                'level': None,


Until now we only store direct API values in the database - so this should be cp_multiplier and the conversion should be done somewhere after the DB instead of in the back-end

SiteEffect · 2017-04-24T13:39:15Z

pogom/models.py

+                    height = pokemon_info['height_m']
+                    weight = pokemon_info['weight_kg']
+                    gender = pokemon_info['pokemon_display']['gender']
+                    if pokemon_info['pokemon_id'] == 201:


Thanks for being so kind, addressing #1914 directly. While this is in general a good move to finally include it into RM, leave it out for lowering complexity since whatever will be merged first, the other one has to adjust.

SiteEffect · 2017-04-24T13:41:35Z

pogom/models.py

+                            cp = pokemon_info.get('cp', 0)
+                            pokemon_level = pokemon_info.get('level', 0)
+
+                if pokemon_info is not None:


Having a general dict update style across all possibilities imo it would be logically the best to get away from standard encounter, check if encounter lvl 25+ or lvl 30+, adding all values of old encounter by a first dict.update and then adding the last two values regarding cp.

I'm not entirely sure I understand this comment

krosenvold · 2017-04-24T17:24:58Z

Note that there was a bug I fixed this morning: For pokemons that were NEITHER CP or IV scanned, the "cp" would slip through from the original map worker. This was fixed about 5 hours ago

krosenvold · 2017-04-26T05:39:26Z

I have rebased this PR, but I am now not squashing to make it easier to follow the changes done in the review process.

walaoaaa1234 · 2017-04-26T06:35:43Z

@krosenvold you should remove copyrighted content

bbdoc · 2017-04-26T09:36:33Z

pogom/utils.py

+    parser.add_argument('--scan-iv', action='append', default=[],
+                        help='List of pokemon to check IV and '
+                             'moveset with level 25+ account')
+


I see a --scan-iv parameter on command line but no equivalent in config.ini.example ? Did I miss it ? I also think it would be good to have both whitelist and blacklist for this parameter as it does exist for encounter, and I guess if you use an encounter blacklist you also want to use the same blacklist for scanning IV with a lvl 25+ account. Quid if you only have -cpac ? Will those accounts be used for IV scanning as well (as if they are 30+ they are also 25+) ?

Running on my map right now and all looking fine, but only scanning IV right now. My IV scanning accounts are lvl30+ so I think when sending an IV scanning account to a spawn and it is lvl30+, it should automatically check cp as well as it doesn't cost any additional request.

sebastienvercammen · 2017-04-26T18:19:22Z

PR #1981 is being tested for merge rather than this PR.

Relaying feedback as mentioned in our Discord's #pr:

The PR tries to do too much simultaneously. Abstractions are good, but unfinished abstractions are not. More about this in the other points.
The first thing to do is to release a working implementation for our users that doesn't change the old way of working, or that doesn't break anything. The users need to be given time to adjust to major reworks.
Careful for Travis. Run a linter.
New abstractions try to incorporate changes to structure. This would be encouraged, if it was done properly. When writing abstractions, keep in mind the separation of responsibility between objects. As it stands, the Account is responsible for the Search (search & rest intervals, scan timing), for the API objects and requests, the hashing keys, the proxies and failure handling. This is too much.
You've moved search_interval and rest_interval into Account but you haven't implemented it.
A PR was recently merged to reduce memory usage, introduced some changes in how we should handle objects and requests. You've ignored these changes and went back to an older way of doing things that unnecessarily increases memory usage.
You've redefined equi_rect_distance in Account, which is already in utils.py.
You've mixed naming styles and you've ignored consistency with previous code: in naming, in comments, in structure.
We're not going to say much about __do_with_backoff, you probably know what we mean. At least it has a comment that says it needs reworking, that's a plus.
The amount of new config items you've added is enormous. While trying to copy the contents over to Discord, we received an error that the message we were trying to send was over the 2000 character limit of Discord. Here's a list of the config items you've added: -ivac, -cpac, --wh-iv-filter-whitelist, --scan-cpall, --scan-cp100, --scan-cp95, --scan-cp90, --scan-cp80, --scan-iv, --iv-auth-service, --iv-username, --iv-password, --cp-auth-service, --cp-username, --cp-password.

Arguably, you've taken time to work on things that absolutely needed to be reworked, but then you don't seem to have taken the time to wonder how the abstractions could be written properly. And then you don't seem to have talked to anyone about what you were doing, so there was no code feedback.

With the PR's size, there's no time for us to review and rework all points properly, because that would involve rewriting account handling as there's an important part in the legacy code that really needs to be reworked.

In the meantime, PR #1981 will serve as the initial implementation, and we'll rework the rest as we go. PR 1981 separates the new account pools from the old code so we can more easily rewrite account handling and the other TODOs without having to undo a new mess that a larger PR like this one would have created.

I've also been PM'ed that you've been inviting people to a new Discord of yours called "FreeSpeechRocket". I can appreciate a good joke, but not a bad attitude. I don't know you and I've never worked with you, so if there's already a problem I'd love to hear about it so I can make sure someone else talks to you instead in the future. That someone PM's me about it is already too much: I'm not sure how you consider that kind of attitude constructive or productive.

j16sdiz reviewed Apr 24, 2017

View reviewed changes

walaoaaa1234 reviewed Apr 24, 2017

View reviewed changes

SiteEffect suggested changes Apr 24, 2017

View reviewed changes

Qualimiox mentioned this pull request Apr 25, 2017

Configurable Beehive #1819

Open

6 tasks

bbdoc reviewed Apr 26, 2017

View reviewed changes

closed

d8c3064

krosenvold closed this Apr 28, 2017

krosenvold deleted the cpIvScanning branch April 28, 2017 11:38

		@@ -2625,6 +2723,12 @@ def database_migrate(db, old_ver):

		db.drop_tables([ScanSpawnPoint])

		if old_ver < 12:

Add support for IV scanning and CP scanning #1980

Add support for IV scanning and CP scanning #1980

Conversation

krosenvold commented Apr 21, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Alderon86 commented Apr 21, 2017

343334 commented Apr 21, 2017

krosenvold commented Apr 22, 2017 • edited Loading

343334 commented Apr 22, 2017

bbdoc commented Apr 22, 2017

krosenvold commented Apr 22, 2017

SiteEffect commented Apr 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krosenvold Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbdoc commented Apr 24, 2017 • edited Loading

Bart274 commented Apr 24, 2017

SiteEffect commented Apr 24, 2017

bbdoc commented Apr 24, 2017

SiteEffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krosenvold commented Apr 24, 2017

krosenvold commented Apr 26, 2017

walaoaaa1234 commented Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvercammen commented Apr 26, 2017 • edited Loading

krosenvold commented Apr 21, 2017 •

edited

Loading

krosenvold commented Apr 22, 2017 •

edited

Loading

krosenvold Apr 24, 2017 •

edited

Loading

bbdoc commented Apr 24, 2017 •

edited

Loading

walaoaaa1234 commented Apr 26, 2017 •

edited

Loading

sebastienvercammen commented Apr 26, 2017 •

edited

Loading