Recycle Items schedule/force #4513

Gobberwart · 2016-08-22T00:58:53Z

Short Description:

Added functionality to the Item recycler (recycle_items.py) which allows users to schedule regular recycles regardless of min_empty_space setting.

Fixes/Resolves/Closes

-Fixes #4417
-Fixes #4337
-Fixes issue caused by #3948

Added ability to schedule a forced item recycle regardless of
min_empty_space setting. Allows individual items to be recycled based on
"keep" settings, even when bag is not nearing capacity.
Added inventory refresh to bot tick (init.py) to ensure web
displays information correctly. UPDATE: Now uses cached inventory instead of api call.

- Added ability to schedule a forced item recycle regardless of min_empty_space setting. Allows individual items to be recycled based on "keep" settings, even when bag is not nearing capacity. - Added inventory refresh to bot tick (__init.py__) to ensure web displays information correctly.

Updated documentation and example config files to show new RecycleItems options.

solderzzc · 2016-08-22T03:14:32Z

pokemongo_bot/__init__.py

@@ -523,6 +532,7 @@ def _register_events(self):
    def tick(self):
        self.health_record.heartbeat()
        self.cell = self.get_meta_cell()
+        inventory.refresh_inventory()


Refresh_inventory will trigger packet sent to server which is not the same behavior as original.

Agree this isn't desirable. See my comment below.

BriceSD · 2016-08-22T09:24:15Z

pokemongo_bot/cell_workers/recycle_items.py

                if self.item_should_be_recycled(item_in_inventory):
                    # Make the bot appears more human
                    action_delay(self.recycle_wait_min, self.recycle_wait_max)
                    # If at any recycling process call we got an error, we consider that the result of this task is error too.
                    if ItemRecycler(self.bot, item_in_inventory, self.get_amount_to_recycle(item_in_inventory)).work() == WorkerResult.ERROR:
                        worker_result = WorkerResult.ERROR
+
+            if not (self.max_balls_keep is None):


Why did you put the recycling based on type after the recycling based on item amount ? The bot is discarding high balls before looking for lower balls to discard instead of highers one. You’re making the recycling based on type useless for people using both of them (with a well configured task).

What do you think @developerllazy ?

Gotcha. Didn't see that side of things before, but you're right. Happy to switch them back again.

Also happy to put back the inventory refresh (and remove it from bot tick) if this api call is likely to cause issues. Still just a temp fix, as the bot should be able to export inventory to web from its own internal cache without needing to call the api every time. If the current web interface is likely to disappear soon, then that should work for now.

Both issues fixed in the latest commit:

Changed the recycling order back to type first, then item amount

Web inventory now updates from cached inventory rather than api call

How's that looking?

I still don’t get it about the force inventory feature. Apart from that seems promising.

BriceSD · 2016-08-22T09:31:31Z

pokemongo_bot/cell_workers/recycle_items.py

+            if not (self.max_balls_keep is None):
+                this_worker_result = self.recycle_excess_category_max(self.max_balls_keep, [1,2,3,4])
+                if this_worker_result <> WorkerResult.SUCCESS:
+                    worker_result = this_worker_result


If the first returns error and the second running, what do we keep ? Looks like running, while we want to return error. For now we want to return error if one of them returns error. That’s it. Don’t make it harder than it needs to be with some boolean complementation.

One or two weeks ago we had no error state. This bot is moving fast, you can’t predict what will be the fourth state, thus you can’t predict if it should override the error state or not.
Don’t make it harder to spot bugs when something has changed somewhere else.

As it stands, the function will return "success" provided the final task is successful. That's inaccurate.

If any task returns something other than "success", the function should be able to report it accurately, so yes if the first is "running" and the second is "success", then overall the result is "running".

As it stands, the function will return "success" provided the final task is successful. That's inaccurate.
As I already agreed, this is unwanted.

If any task returns something other than "success", the function should be able to report it accurately, so yes if the first is "running" and the second is "success", then overall the result is "running".
Agreed. But you missed my point. Returning running over success is wanted, but not returning running over error.

Ah ok I see where you're coming from. However, given that none of the tasks called by the recycler actually return "running", is it something worth worrying about?

If it is important, I'm happy to take a look, but I can't currently see any possible condition where "RUNNING" will be returned.

Running isn’t returned. That’s why you don’t see it here

# If at any recycling process call we got an error, we consider that the result of this task is error too. if ItemRecycler(self.bot, item_in_inventory, self.get_amount_to_recycle(item_in_inventory)).work() == WorkerResult.ERROR: worker_result = WorkerResult.ERROR

But what about the fourth state. The day someone add it, he won’t know he added a bug by overwriting the error result. It’ll be an annoying bug to catch, hard to reproduce. Why not simply avoiding it by not allowing it to happen right now ?

BriceSD · 2016-08-22T09:38:40Z

Why would we want this ? To me forcing to recycle items looks like a hackish way to bypass a bad configuration, and make it overall harder to properly configure the bot for people who are just starting to use it.

Maybe I’m missing the point. Please develop why this is wanted.

Gobberwart · 2016-08-22T10:40:12Z

I don't understand why you think the configuration can take care of this. As it currently stands, there is no way that the bot will ever recycle until min_empty_space is reached. That results in very poorly balanced inventory.

Unless you can explain how a less "bad configuration" can correct this issue, then why would we not want this? Nothing "hackish" about it, but thanks for the feedback.

BriceSD · 2016-08-22T10:59:27Z

2016-08-22 12:34:14,106 [ItemRecycler] [INFO] [item_discarded] Discarded 40x Pokeball.
2016-08-22 12:34:16,877 [ItemRecycler] [INFO] [item_discarded] Discarded 7x Greatball.
2016-08-22 12:34:20,142 [ItemRecycler] [INFO] [item_discarded] Discarded 12x Potion.
2016-08-22 12:34:22,277 [ItemRecycler] [INFO] [item_discarded] Discarded 4x Super Potion.
2016-08-22 12:34:26,195 [ItemRecycler] [INFO] [item_discarded] Discarded 6x Hyper Potion.
2016-08-22 12:34:28,416 [ItemRecycler] [INFO] [item_discarded] Discarded 6x Razz Berry.
2016-08-22 12:34:31,576 [ItemRecycler] [INFO] [item_discarded] Discarded 2x Revive.

Then 5 minutes later :

2016-08-22 12:39:30,411 [ItemRecycler] [INFO] [item_discarded] Discarded 36x Pokeball.
2016-08-22 12:39:31,995 [ItemRecycler] [INFO] [item_discarded] Discarded 7x Greatball.
2016-08-22 12:39:34,380 [ItemRecycler] [INFO] [item_discarded] Discarded 2x Ultraball.
2016-08-22 12:39:38,228 [ItemRecycler] [INFO] [item_discarded] Discarded 17x Potion.
2016-08-22 12:39:40,052 [ItemRecycler] [INFO] [item_discarded] Discarded 3x Super Potion.
2016-08-22 12:39:42,825 [ItemRecycler] [INFO] [item_discarded] Discarded 3x Hyper Potion.
2016-08-22 12:39:45,322 [ItemRecycler] [INFO] [item_discarded] Discarded 4x Razz Berry.
2016-08-22 12:39:46,716 [ItemRecycler] [INFO] [item_discarded] Discarded 1x Revive.

If you want to discard items more often you can reduce the min_empty_space, as well as max amount to keep in inventory. Or improve your loot rate. Or something in between.

I don’t want this because you add nothing that can’t be done. Of course you’ve to take some time to make a configuration which meets your needs.

Gobberwart · 2016-08-22T11:46:45Z

I want to recycle every few minutes and keep exactly what I specify in my config. Changing min_empty_space or increasing max amount (of a given category) to keep doesn't achieve this result.

The bot previously did exactly what I wanted, and now it doesn't. I've come up with a solution that can work for people who do want it and those who don't (setting force_recycle: false or omitting force_recycle from config altogether disables it).

If you don't think the solution is suitable for inclusion, then feel free to disregard it. It works for me, so I'm happy, just thought I'd share.

solderzzc · 2016-08-22T14:28:54Z

People don't need it just set to False and default is False, so I think this is good to have.
But line 153 + inventory.refresh_inventory() will caused mass api call to server, that's not good.

Gobberwart · 2016-08-22T21:37:17Z

@solderzzc You're right. That's excessive.

Currently inventory refresh is performed during the recycle items task (about half-way through on line 103 of recycle_items.py).

Leaving it like that would mean the counts shown in web interface are almost certain to be inaccurate most of the time.

Ideally the bot should be able to dump its cached inventory to the web, but that requires a bit more work (if nobody's working on that, I'm happy to take a look).

Suggest moving inventory_refresh back to recycle_items.py for now, however for the recycler to work correctly, it needs to update the inventory at least twice (once at the beginning, and between the two different recycle types), and preferably a third time at the end to update the web interface after recycling finishes.

The result would be 2-3 API calls every n minutes (depending on config), rather than one every tick. Still not ideal but better than it is currently.

Any other thoughts?

Added "update web inventory" to inventory.py which dumps json to web/inventory-USERNAME.json from the cached inventory instead of calling api. Runs every tick to ensure values are as up-to-date as possible.

Gobberwart · 2016-08-22T22:53:36Z

@solderzzc I've just added a new commit which replaces inventory.refresh with a new function, inventory.update_web_inventory. This dumps the current cached inventory to the web folder, instead of calling the api.

I'm a bit suspect about how accurately the bot maintains its cached inventory, so not sure if this is 100%, but it does seem to work correctly. Items/pokemons/pokedex etc. are all being updated on every bot tick without calling the api.

Does this meet the requirements?

Any feedback appreciated.

Updated the comment block at the top of the file to explain/show new settings

Previous commit missed player stats. Fixed.

Gobberwart · 2016-08-23T02:00:39Z

I've just noticed that player_stats aren't being updated in web output after my update. Working on a solution that does not require another api call.

Gobberwart · 2016-08-23T03:36:54Z

OK that's a much bigger piece of work than I expected. Currently, every time player stats are handled anywhere in the code, there's an api call, and the results aren't stored anywhere that I can re-use them here (if they are, I can't find them). Therefore, a new api call is needed, and that's no good.

To properly fix this would require:

New object to store player stats
Method to update stats from api
Additions to multiple code locations to update stats without calling api (eg. km walked, xp, level etc.)

As far as I can tell, this isn't currently working anyway, so should we just leave it broken for now, pending implementation of a better way to track player stats?

Thoughts? @solderzzc @BriceSD

mjmadsen · 2016-08-23T04:56:30Z

In the future, try putting up more smaller PRs. Folks are more likely to ok small changes.

Removed api call for player_stats, and moved output of player_stats to web to update_live_stats.py

Gobberwart · 2016-08-23T08:33:51Z

@mjmadsen I'll bear that in mind. Unfortunately this "simple" fix showed up a few other things that were related and broken, so I've taken the opportunity to correct those as well.

And I've just made it slightly bigger in the interest of removing all api calls that were used here. Sorry :)

Correct error that would result in player_data being written multiple times. Something in the bot is still doing that somewhere, but definitely not in this new code.

solderzzc · 2016-08-23T14:36:23Z

pokemongo_bot/__init__.py

@@ -548,6 +557,7 @@ def _register_events(self):
    def tick(self):
        self.health_record.heartbeat()
        self.cell = self.get_meta_cell()
+        inventory.update_web_inventory()


No, we can't do it like this.

Care to elaborate? 1) Why not (although I think I know why), 2) Any suggestions what to do instead?

Would it be better to create a cell_worker and add it to the config? I'm just trying to keep the number of changes to a minimum, but I can do that instead if that's preferable?

I've changed this in a recent commit. Added a cell_worker to perform the function and removed it from init.py. Let me know what you think.

BriceSD · 2016-08-23T16:20:18Z

Yes the player stats is a big piece. Doing it in this PR would be to much. I don’t have enough time these days to do it, but metrics should change imo, and become a bit like inventory, but for stats and without tracking them, just calling the api from time to time.

Glad you choose to add a method to jsonify the inventory, it’s the right way to update the web inventory imo. That being said we may need to use the inventory in json somewhere else. Do you mind extracting the part which does it in an other method ?

Gobberwart · 2016-08-23T21:19:37Z

@BriceSD The method is in the Inventory class, so should be usable anywhere using inventory.update_web_inventory(). I think that's probably the best place for it.

BriceSD · 2016-08-23T21:30:51Z

Sure, keep it there.
I mean you can split the method in two methods. One which jsonify the inventory (like a method to_json(), and the other one is exactly update_web_inventory(). But instead of duplicate the code of to_json() you call it.

Json is serializable, can be pretty handy to have it ready to use. Easier to store in db for example.

Gobberwart · 2016-08-23T23:11:45Z

@BriceSD Something like this (tested working)?

    def update_web_inventory(self):
        web_inventory = os.path.join(_base_dir, "web", "inventory-%s.json" % self.bot.config.username)

        with open(web_inventory, "r") as infile:
            json_inventory = json.load(infile)

        json_inventory = [x for x in json_inventory if not x.get("inventory_item_data", {}).get("pokedex_entry", None)]
        json_inventory = [x for x in json_inventory if not x.get("inventory_item_data", {}).get("candy", None)]
        json_inventory = [x for x in json_inventory if not x.get("inventory_item_data", {}).get("item", None)]
        json_inventory = [x for x in json_inventory if not x.get("inventory_item_data", {}).get("pokemon_data", None)]

        json_inventory = json_inventory + self.jsonify_inventory()

        with open(web_inventory, "w") as outfile:
            json.dump(json_inventory, outfile)

    def jsonify_inventory(self):
        json_inventory = []

        for pokedex in self.pokedex.all():
            json_inventory.append({"inventory_item_data": {"pokedex_entry": pokedex}})

        for family_id, candy in self.candy._data.items():
            json_inventory.append({"inventory_item_data": {"candy": {"family_id": family_id, "candy": candy.quantity}}})

        for item_id, item in self.items._data.items():
            json_inventory.append({"inventory_item_data": {"item": {"item_id": item_id, "count": item.count}}})

        for pokemon in self.pokemons.all():
            json_inventory.append({"inventory_item_data": {"pokemon_data": pokemon._data}})

        return json_inventory

Removed update_web_inventory from init.py and created a new worker UpdateLiveInventory in light of feeback from solderzzc.

@BriceSD

As per feedback from @BriceSD

Gobberwart · 2016-08-23T23:40:26Z

@BriceSD I've split the two methods apart in latest commit. Tested working correctly. Theoretically, inventory.jsonify_inventory can now be used from anywhere to get the current inventory as a json-style list object without necessarily updating the web output. Let me know what you think.

Gobberwart · 2016-08-24T01:43:56Z

I think I've addressed all concerns now. What are the chances we can get this approved for inclusion in dev branch?

mjmadsen · 2016-08-24T01:45:11Z

@solderzzc Yes?

mjmadsen · 2016-08-24T02:33:19Z

Sorry @Gobberwart I don't know this bit well enough to merge myself. Waiting for some of the current commenters to check it out.

Please resolve the branch conflicts present.

Gobberwart · 2016-08-24T02:35:46Z

No worries!

MartinTerp · 2016-08-24T13:57:24Z

Hi

So for the bot to update the inventory file, with bag and pokemon list, UpdateLiveStats needs to be enabled? Or how is it triggered?

Also, if it uses UpdateLiveStats, wont that make API calls?

Gobberwart · 2016-08-24T20:49:41Z

@MartinTerp

The web inventory (everything but player stats) updates every bot tick, using the new UpdateWebInventory worker. No api calls are used for this at all.
Player stats are exported to the web when UpdateLiveStats runs. No ADDITIONAL api calls are used to do this. UpdateLiveStats makes a call, but that's not new.

solderzzc · 2016-08-25T05:44:01Z

@Gobberwart Thanks a lot for this PR.

alexyaoyang · 2016-08-25T17:23:04Z

@Gobberwart I think this is a great way to have a balanced inventory! Thank you!

Just one small thing: in all the sample config files you edited, you have the recycle_force_min set to 00:00:00, recycle_force_max set to 00:01:00 and recycle_force is set to True. Were those values supposed to be larger than that?

Fix No such file or directory caused by #4513

Gobberwart · 2016-08-25T21:26:13Z

Good pickup @alexyaoyang. I'll put through another PR with larger values.

alexyaoyang · 2016-08-25T21:29:24Z

@Gobberwart That's great! I was just thinking of new users who duplicated the sample config having a recycle every too often! 👍

Gobberwart · 2016-08-25T21:32:41Z

@alexyaoyang I'm not actually sure how that happened, should be 00:01:00 and 00:05:00 but I think while I was testing I managed to copy/paste my test values to all examples. New PR is ready for review.

daemonsyn · 2016-08-29T03:11:52Z

Possible add total number of Stardust?

Gobberwart · 2016-08-29T03:49:35Z

@daemonsyn this PR is now closed. Suggest create a new issue with your requested feature.

Gobberwart mentioned this pull request Aug 22, 2016

RecycleItems task not running frequently enough #4417

Closed

Gobberwart added 2 commits August 22, 2016 11:27

Updated docs/configs

80acac7

Updated documentation and example config files to show new RecycleItems options.

Merge remote-tracking branch 'PokemonGoF/dev' into dev

37aac25

solderzzc reviewed Aug 22, 2016
View reviewed changes

Merge remote-tracking branch 'PokemonGoF/dev' into dev

037462d

BriceSD reviewed Aug 22, 2016
View reviewed changes

BriceSD mentioned this pull request Aug 22, 2016

Update web inventory at every tick #4507

Closed

BriceSD reviewed Aug 22, 2016
View reviewed changes

Update web inventory from cache

bc58625

Added "update web inventory" to inventory.py which dumps json to web/inventory-USERNAME.json from the cached inventory instead of calling api. Runs every tick to ensure values are as up-to-date as possible.

Gobberwart added 3 commits August 23, 2016 09:43

Updated comments to include new config settings

62a5ad0

Updated the comment block at the top of the file to explain/show new settings

Merge remote-tracking branch 'PokemonGoF/dev' into dev

08f0f40

Added player stats to web output

2a9f345

Previous commit missed player stats. Fixed.

mjmadsen added the Second Review Required label Aug 23, 2016

Removed api call for player_stats

32f0c5d

Removed api call for player_stats, and moved output of player_stats to web to update_live_stats.py

Slight logic error

b8bb2d1

Correct error that would result in player_data being written multiple times. Something in the bot is still doing that somewhere, but definitely not in this new code.

solderzzc reviewed Aug 23, 2016
View reviewed changes

Gobberwart added 2 commits August 24, 2016 09:36

Added cell_worker for inventory update

57896ad

Removed update_web_inventory from init.py and created a new worker UpdateLiveInventory in light of feeback from solderzzc.

Separated update_web_inventory from jsonify_inventory

7d561a9

As per feedback from @BriceSD

Merge remote-tracking branch 'PokemonGoF/dev' into dev

bcbec6e

Gobberwart mentioned this pull request Aug 24, 2016

Web inventory doesn't get updated #4337

Closed

Merge remote-tracking branch 'PokemonGoF/dev' into dev

49344e9

mjmadsen added the Approved label Aug 24, 2016

Resolved conflicts

ff03f21

solderzzc merged commit 9605728 into PokemonGoF:dev Aug 25, 2016

solderzzc added a commit that referenced this pull request Aug 25, 2016

Merge pull request #4720 from cmezh/web_inventory

54f78b8

Fix No such file or directory caused by #4513

Gobberwart mentioned this pull request Aug 25, 2016

Larger times for recycling in example configs #4737

Merged

Recycle Items schedule/force #4513

Recycle Items schedule/force #4513

Conversation

Gobberwart commented Aug 22, 2016 • edited Loading

Short Description:

Fixes/Resolves/Closes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BriceSD Aug 22, 2016 • edited Loading

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

BriceSD Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BriceSD commented Aug 22, 2016

Gobberwart commented Aug 22, 2016 • edited Loading

BriceSD commented Aug 22, 2016 • edited Loading

Gobberwart commented Aug 22, 2016

solderzzc commented Aug 22, 2016 • edited Loading

Gobberwart commented Aug 22, 2016 • edited Loading

Gobberwart commented Aug 22, 2016 • edited Loading

Gobberwart commented Aug 23, 2016

Gobberwart commented Aug 23, 2016 • edited Loading

mjmadsen commented Aug 23, 2016

Gobberwart commented Aug 23, 2016

Choose a reason for hiding this comment

Gobberwart Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BriceSD commented Aug 23, 2016 • edited Loading

Gobberwart commented Aug 23, 2016

BriceSD commented Aug 23, 2016

Gobberwart commented Aug 23, 2016 • edited Loading

Gobberwart commented Aug 23, 2016 • edited Loading

Gobberwart commented Aug 24, 2016

mjmadsen commented Aug 24, 2016

mjmadsen commented Aug 24, 2016 • edited Loading

Gobberwart commented Aug 24, 2016

MartinTerp commented Aug 24, 2016

Gobberwart commented Aug 24, 2016 • edited Loading

solderzzc commented Aug 25, 2016

alexyaoyang commented Aug 25, 2016

Gobberwart commented Aug 25, 2016

alexyaoyang commented Aug 25, 2016

Gobberwart commented Aug 25, 2016

daemonsyn commented Aug 29, 2016

Gobberwart commented Aug 29, 2016

Gobberwart commented Aug 22, 2016 •

edited

Loading

BriceSD Aug 22, 2016 •

edited

Loading

BriceSD Aug 22, 2016 •

edited

Loading

Gobberwart commented Aug 22, 2016 •

edited

Loading

BriceSD commented Aug 22, 2016 •

edited

Loading

solderzzc commented Aug 22, 2016 •

edited

Loading

Gobberwart commented Aug 22, 2016 •

edited

Loading

Gobberwart commented Aug 22, 2016 •

edited

Loading

Gobberwart commented Aug 23, 2016 •

edited

Loading

Gobberwart Aug 23, 2016 •

edited

Loading

BriceSD commented Aug 23, 2016 •

edited

Loading

Gobberwart commented Aug 23, 2016 •

edited

Loading

Gobberwart commented Aug 23, 2016 •

edited

Loading

mjmadsen commented Aug 24, 2016 •

edited

Loading

Gobberwart commented Aug 24, 2016 •

edited

Loading