item: aggregate contents

[andrei8l]

Summary

None

Purpose of change

Too many restrictive checks are performed when expanding contents name so a lot of containers that should reasonably be named container > contents_name (X) are instead shown as container > X items. And we can do even better by showing more common stats when most contents are of some particular type.

• Fixes: Charge Removal Side effect #4: Inconsistent spoilage #67603
• Fixes: Automatic stacking of containers inside other containers obscures information #69959
• Fixes: Pointless and cumbersome nesting of soft containers should be avoided #68341
• Fixes: Damaged items don't stack despite being the same condition which bloats windows containing items #70332 (incidentally)
  Also fixes items with varying whitelists stacking when they shouldn't (incidentally).

Describe the solution

1. Break item::tname() into segments
2. Break item::stacks_with() into bits corresponding to tname segments
3. When determining item contents, aggregate all the stacking bits by type and category (ie. determine the common bits for each type and category)
4. Display all the common segments for the dominant item or category. An item/category is dominant if it covers more than half of the contents

I'm also getting rid of the hidden marker because the chevron already conveys that information. I wanted to remove it back in #56630 but the chevrons were too new back then.

Some examples with pictures:

A bag of salt in a bag of salt (#69959)

Some pepper has snuck into this bag but the salt still dominates

A lot of pepper has snuck into this bag and the salt doesn't dominate anymore. But the dominant category is FOOD

This bag holds several variants of the same item. Note that the common durability and poor fit are also mentioned

One of the masks got damaged and the dominant type doesn't have common durability anymore

Another clothing example - the common filthy state is also mentioned

Nested food of various type

Another FOOD example, showing aggregated perishability and freshness

Bag with various sawn-off guns, showing aggregated barrel mod

TODO:

• Write more tests
• Investigate performance impact. Caching item::get_category_of_contents() covered most of it. This definitely reverts Optimize item::stacks_with for further load time gains #70685 and I haven't been able to restore it.

Describe alternatives you've considered

Step 1 isn't strictly necessary but item::tname() was a monolithic monster. #67954 was the original attempt that didn't split tname and I didn't think was tenable. Similar for splitting item::stacks_with()

If there were some guarantee that insertion/removal/transformation had backlinks (ex: by using an item_location), there wouldn't be any need for a timestamp in the category cache.

Testing

Updated test units that cover the examples above. I don't expect to correctly cover all the cases in this PR, but I will address errors as they are discovered in gameplay.

There is a moderate performance impact. Timing for opening the pickup all in City of the Dalles.zip with a standard (-Os) release build

Before:

After:

Additional context

While this is a large PR, a big chunk of it is just moving code around (step 1 - fe3abb6). The second biggest chunk is mostly just not'ing boolean expressions and moving them to separate functions (step 2 - ec53b3e). The third biggest chunk is test code (3ba1213).

[GuardianDll]

Can you make category be not all caps?

[andrei8l]

Can you make category be not all caps?

Category names are defined in JSON (example) and I don't want to mangle them in code.

[akrieger]

This definitely reverts #70685 and I'm not sure if I can restore that.

;_; sad. It's even worse because it unconditionally runs through the entire stacks_with function for every bit for every item.

[PatrikLundell]

This looks like a significant improvement.

However, I'm not too fond of the second Clothing example, as the impression would be that it included only clothing, but it may be that it wouldn't be too hard to learn to look at the 4/8 part to realize there's other stuff in there too.

I don't think there's a good way to show if a mixed container of food is all perishable, partially perishable, or non perishable (so you'd know whether to toss it onto a shelf, into the cellar, or sort it out now).

[Qrox]

However, I'm not too fond of the second Clothing example, as the impression would be that it included only clothing, but it may be that it wouldn't be too hard to learn to look at the 4/8 part to realize there's other stuff in there too.

I have the same feeling about it. Perhaps it would be better to display it as xxx CLOTHING among yyy items/whatever description that fits all contained items?

[andrei8l]

I have the same feeling about it. Perhaps it would be better to display it as xxx CLOTHING among yyy items/whatever description that fits all contained items?

I'm a little confused. So is > 4/8 salt also bad? If the number/number is hard to read, I don't think a longer and wordier description is going to be easier.

I don't think there's a good way to show if a mixed container of food is all perishable, partially perishable, or non perishable (so you'd know whether to toss it onto a shelf, into the cellar, or sort it out now).

It was easy and had negligible perf impact so I'll add it next time I push. Doesn't work for nested food though.

[PatrikLundell]

The 4/8 salt case would mean you somehow had 4 salt bags with 1 salt each in the top bag, if I understand things correctly, and so would mean you've got only salt and containers in it, which I think is fine. If it would also cover the case of 3 bags of salt, one salt, and one unrelated item I wouldn't like it (and my understanding is that it would end up in the least common denominator category, and so wouldn't happen).

[andrei8l]

The 4/8 salt case would mean you somehow had 4 salt bags with 1 salt each in the top bag, if I understand things correctly, and so would mean you've got only salt and containers in it, which I think is fine. If it would also cover the case of 3 bags of salt, one salt, and one unrelated item I wouldn't like it (and my understanding is that it would end up in the least common denominator category, and so wouldn't happen).

Woops, that's completely wrong so this isn't working the way I thought. The intention was that > 4/8 salt means 4 items out of the 8 total contents are salt (and they are the majority type). I'll have to rethink this if the interpretation is so far off.

[PatrikLundell]

Don't accept my interpretation without checking it, as it's not unusual for me to be wrong...

However, 4/8 meaning 4 out of 8 items being salt would be correct, as the remaining objects would be plastic bags containing salt. If one of the bags was empty I'd expect the top bag to be considered a mixed container, not a salt containing one, i.e. containers would only "transform" to something else if it was a container for that something else.

[andrei8l]

Don't accept my interpretation without checking it, as it's not unusual for me to be wrong...

Quick poll on discord agrees with you and Qrox. What do you think about plastic bag > 4 salt / 8 items ?

[PatrikLundell]

That display would indicate (to me) there are 8 unspecified items inside, 4 of which are specified to be salt, which I assume is what you're after.

However, I think it's worse from an information perspective, as it really only is of help compared to the pure "8 items" situation if I'm interested to get the salt items, but I still have to open it to see what the other stuff is (I wouldn't want to overload me with 4 zombie corpses or even 4 dirty rags, let alone 4 poisonous mushrooms (reference to recent report about inadvertent usage of those in cooking)).

[andrei8l]

However, I think it's worse from an information perspective, as it really only is of help compared to the pure "8 items"

Well yes, that was the goal. When you have a container of mostly X item, I don't want the header to hide X just because some other items snuck in there as well. I believe that's a more general solution to #69959 than propagating the type upwards. Propagating upwards only covers exactly one case that can be probably solved better with tighter whitelists.

But I think this framework is flexible enough that we can change the display to something else if players prefer it.

[akrieger]

I'm currently looking at the perf and seeing if there's anything to be done or how bad it hurts.

[akrieger]

I've instrumented game load a few times and TBH this doesn't really hurt it much? So I'm not super concerned. I'll wait & see if people report slowdowns for live actions like crafting or walking or whatnot but at first blush this is at worst only like 3% net slower, which is almost within the level of noise for the profiler.

[akrieger]

I managed to find like a 6ish% win for windows game load from avoiding a couple more temporary item objects, which basically makes up for the equivalent slowdown I can measure from this, fwiw.

[akrieger]

@Qrox any final comments?

[Qrox]

Looks good to me.

[andrei8l]

Thanks for merging!