Remove charges from solid comestibles

[mqrause]

Summary

Infrastructure "Remove charges from solid comestibles"

Purpose of change

Starting to phase out count_by_charges.
Fixes #47842: In general count_by_charges items should never have pockets, because a hide bag (1) has the same amount of pocket space as a hide bag (2) anyway. Might be worth it to add a check for that.

Describe the solution

• exclude solid items from count_by_charges
• items that aren't count_by_charges anymore get migrated, unless they get added to the charge removal list
• recipe charges work both for items with and without count_by_charges
• iuse_transform can transform into a container containing multiple target items, not just count_by_charges items
• Because solid comistibles can't have charges anymore, default charges of comestibles is now 0. This means liquids/gases need to explicitly define charges. As a side effect, solid comestibles can't copy-from from liquid/gaseous comestibles anymore.
• automated json adjustments with minor manual adjustments to fix test failures

Possible todos/issues:

1. include inline item groups in automated adjustments
   • it doesn't cause direct issues to leave them as is, but spawn amounts are wrong since charges get ignored
   • they're sprinkled everywhere in all kinds of other json objects
2. automatically created groups that end in _inf
   • The intention of those groups is to spawn an item in a container, but the container is filled with a random number between min and filling up the container. This behavior currently isn't replicable with count instead of charges, so it either needs code support or explicitly calculated max amounts for these groups.
3. sealed container issues should be mostly fixed by Itemgroups can seal their containers #66971
   • Spawning sealed containers seems to depend on the container being completely filled, with a remaining volume of 0 ml. The charge removal and volume changes break that for some items.
   • for example 4 charges in 250ml put into a glass jar turns into 4 items of 62ml, so 248ml, which causes the glass jar to not be sealed anymore, because it's not full
4. tiny items <1ml
   • A few items had charges with a volume < 1ml. Since we don't support that they were rounded up to 1ml. I'll include a list of these items here once it's ready to merge.

Other observations:

• Things like "charges": [ 0, 2 ] can't actually spawn 0 charges from what I have seen. Since this got changed to "count": [ 0, 2 ], this can actually spawn nothing now. Because some of these cases got moved to their own item groups due to container issues, a count of 0 in those cases means an empty container is spawned. This may or may not be what people intended with these groups, but there's not much to do about it with just automated changes.

Describe alternatives you've considered

Also including liquids/gases in this, but I don't wanna imagine the kinds of things that would break.

Testing

Tests pass, and old games seem to load fine, but only minor testing was done for now.

Additional context

Gist with some data from the automated json changes:
https://gist.github.com/mqrause/dc234737bfd5c4309aa4dc496d17bd53

[anoobindisguise]

This is going to be horrible without some serious reworks to how non-charge items work.
For example, take caffeine pills. The recipe makes nearly 2000, and when this happens, you will only be able to store 4096 of them (less than 1 liter) in any given place. It will take over 2 hours to fully empty a bottle with 4096 pills, or to insert 4096 pills into a bottle. Then there is the issue of 4096 pills being extremely laggy, when there are so many items nearby it takes 10 seconds or more to eat a single item. This is easily observable when you have a gigantic stockpile of food and it takes ages to eat anything from it, whereas if you have a stack of a few thousand slow spoilage food items in consolidated stacks - for example 800 necco wafers, 800 chocolate candies, 800 dehydrated meats, and 800 hard cheeses, it takes less than a 5th of a second to eat something. Turn those into individual items and the time to eat something will increase 1000 fold. There is also the extreme time it takes to move items around and the sheer clunkiness of navigating a food menu that is 100 pages long that could have been literally 1 page long. And the lag so many items calculating spoilage will generate. If we do this then every single food stockpile is going to be like the awful MRE warehouse in the mil_base.
I don't think it's an exaggeration to say that this will make the game completely unplayable. It's probably less work to just fix the problems with charges than dealing with the madness that is uncharged comestibles. Individual tailoring patches are already bad enough, anyone who's tried to accumulate tailoring supplies currently can attest to that.

[mqrause]

the sheer clunkiness of navigating a food menu that is 100 pages long that could have been literally 1 page long.

Nothing will change in menus, except the number will be in the front instead of the back of the name.
The other issues need fixing anyway.

It's probably less work to just fix the problems with charges than dealing with the madness that is uncharged comestibles.

There is no "just fix the problems with charges", they're horrible from an infastructure and maintainability standpoint with special cases and separate logic at every corner.

[Zireael07]

@anoobindisguise however raises some valid points. There's already at least one open issue about many items being laggy (thread, rags). This looks likely to exacerbate this...

[mqrause]

It's not likely to, it absolutely will make these issues worse. They just need someone to work on them.

[Amoebka]

Alright, so perhaps this PR should be on hold until those issues are fixed then? Otherwise you are just making life miserable for players, and reducing the number of people playing the latest experimentals, on whom the project depends for reporting bugs.
Similar situations have happened before, and making existing issues worse most definitely did NOT make the fixes arrive faster.

[mqrause]

It's not for me to decide if and when this gets merged and it's kinda pointless to talk about it to this extent at this point in time.

[chrispikula]

So what will this do to items that have more than 4096 charges, which is the item stack? Just overflow in the transition? If so, my vehicle storage is going to delete many, many items, as overflow is limited to, iirc, 3 or 4 tiles.

[mqrause]

As it currently is, items are migrated to a single item with all the other charges as single items in its migration pocket, which isn't subject to the limit. This state will persist until you put the item in your inventory, where it will unload the migration pocket. I think currently you can even wield it without it unloading as long as you don't touch anything else in your inventory, so if you're smart about it, nothing will be lost. I still need to look at it later on because of the amount and nature of the items affected, since I'm not sure how it behaves when crafting/consuming. But that's just the finishing touches of the PR once everything else is in place.

[chrispikula]

Or if you put something in your medication storage trunk, right after the update? 'cause that'll update that tile, yeah?

If it does, that will cause spillage, and thus item deletion when it can't spill anything else. As it stands, my powdered food & adjacent goods storage has upwards of 82 thousand charges of items stored in it. Beside it is the chemical storage has 248k charges of items in it. Not including liquids. That'll end up being upwards of 85 hours of picking up items, one by one, second by second, in game and trying to find new places to store them, if they don't flow into the 'can't place item' error.

Side note, possible issue is that gunpowder and all of the items that copy-from it come in a count of 4540 charges when you get them. Will these just be expected to spill? It's also gonna feel kind of like a waste when you can't place more than a single jar of gunpowder into a cargo hold.

[mqrause]

Currently only things in your inventory get updated. Map/vehicle tiles don't do anything.

As long as gunpowder comes in a container, there is no issue whatsoever. Containers are not affected by the item limit. And only the container itself counts towards the item limit. And frankly if this update stops people from storing their foodstuffs, chemicals and powders of any kind directly on the floor, it's mostly a good thing.

[chrispikula]

Sure, yeah, if there was a way to do that without involving even more highly annoying, repetitive button pressing, I'd agree. But we currently can't put things into pre-existing containers while they are in storage yet, other than fluids.

How will this deal with merging multiple partially filled containers? Will it be rate-limited to the roughly 1 second per item transfer rate of normal items? Given a lack of bulk movement transfer, that is. Common cases of this would be all of the 100-units of salt you find in kitchens, that you'd probably want to put into a single container so as to not have half your storage wasted by small plastic bottles and vials.

Uncommon case would come from disassembling a car battery for lead, where you'd get 4 thousand or so. Picking that off the workbench beside you, and throwing that into a box would take some time, yeah?

[mqrause]

As said before, that's explicitly not a concern of this PR. There are issues for that kind of stuff already (and if not feel free to open one). If the main dev team deems those issues critical for this PR, it won't be merged until they're closed and that's that.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

[Maleclypse]

I'm guessing this is probably something going on with my local copy but this is the only errors I found and it was on start.

23:28:18.956 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 1
23:28:23.785 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 2
23:28:23.788 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 3
23:28:23.788 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 4
23:28:23.789 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 5
23:28:23.789 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 6
23:28:23.791 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 7
23:28:23.791 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 8
23:28:23.792 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 9
23:28:23.792 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 10
23:28:23.794 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 11
23:28:23.794 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 12
23:28:23.796 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 13
23:28:23.796 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 14
23:28:23.798 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 15
23:28:23.798 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 16
23:28:23.800 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 17
23:28:23.800 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 18
23:28:23.802 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 19
23:28:23.802 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 20
23:28:23.805 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 21
23:28:23.805 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 22
23:28:23.807 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 23
23:28:23.807 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 24
23:28:23.810 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 25
23:28:23.810 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 26
23:28:23.823 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 27
23:28:23.823 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 28
23:28:23.825 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 29
23:28:23.825 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 30
23:28:23.826 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 31
23:28:23.826 ERROR : C:\Users\colli\Documents\GitHub\Cataclysm-DDA\src\mission.cpp:300 [assign] strange: player is already assigned to mission 32

If you aren't experiencing this error on start let me know and I'll go ahead and merge. Ping me in discord to let me know.

[alp7292]

good now we have to wait 1 month (or more) untill all the performance issues and bugs fixed

[Zireael07]

It's sad that my concerns about performance were brushed aside only to be validated after this got merged :(

[SurFlurer]

Eventually when pocket contents' total weight and volume can be cached, there shouldn't be too much difference.

[mqrause]

It's sad that my concerns about performance were brushed aside only to be validated after this got merged :(

I'll say it again, it was obvious to me and everyone involved that performance would take a hit. The PR was around for almost a year and some things were already addressed. Some more things are already in the works or at least identified. And chances aren't too bad that in the end performance will be better than before.

[Maleclypse]

I'm locking conversation because the PR is closed and as I have been having to say quite a bit lately "Closed PRs are not the place to have conversations about the effects of PRs". There is zero visibility to anyone who isn't already in the PR conversation, any words said on closed PRs are words that are wasted.