Port over salvage-by-weight from DDA

[chaosvolt]

Co-Authored-By: Kevin Granade 860276+kevingranade@users.noreply.github.com

Summary

SUMMARY: Bugfixes "Port over DDA PR changing salvaging to go by weight instead of volume"

Purpose of change

It was pointed out that salvaging items is still wacky because it goes by volume, and there's a DDA PR that changes it to by-weight that we could port over.

Describe the solution

1. In iuse.cpp, ported over change removing minimal_volume_to_cut in with visit_salvage_products and minimal_weight_to_cut, updating salvage_actor::time_to_cut_up with the added weight stuff, the change to salvage_actor::valid_to_cut_up, and the updates to salvage_actor::try_to_cut_up and salvage_actor::cut_up that actually gets yield from weight.
2. Ported over the test from the source PR, with fixes thanks to advice from Coolthulhu and Olanti to get it properly compiling in BN. Also updated to not test migrated-out items, and excluding some items for which testing makes no sense.
3. Per feedback, fixed weight of dog armor raincoat being less than anything it could be salvaged into.
4. Also added NO_SALVAGE to wetsuit gloves and burette per suggestion.
5. Checked affected file with astyle.

Describe alternatives you've considered

Screaming.

Testing

1. Compiled and load tested.
2. Spawned in a knife, quadruped horse armor, and debugged in all skills.
3. Cut it up, obtained 83 leather patches and 83 rags.
4. Checked recipe, it requires 105 leather patches and 18 rags.
5. Repeated this test in a recent release, obtained 299 leather patches and 299 rags.
6. Spawned in and cut up a blanket in both compiled build and release.
7. Yield was 60 rags in release, 14 in compiled build.
8. Noted that recipe takes 35 rags.
9. Noted that 14 rags weigh 2.47 pounds, or ~99.6% of the 2.48 pounds a blanket weighs.

So now things tend to lean closer to what they're supposed to be, just needing weight adjustments of problematic items in a follow-up PR, though it still performs poorly for multi-material items.

Additional context

Source PR, "Proportional part salvaging" by @kevingranade: CleverRaven/Cataclysm-DDA#45779

Stuff not ported over:

1. The iuse_actor_test.cpp additions. Its current form does not actually compile, with the below errors:
   errors.txt
2. The removal of uncrafts, because blankets and likely the other items still have wonky item weights these still serve a purpose until the offending items have a weight fix. I can do that now or in a follow-up PR, this still gets us nudged in the direction of closer-to-sane results at least, and should reduce instances of material duping since component weight is more likely to be higher than item weight instead of lower.

I tested in DDA build 2022-10-27-0335 and found that blankets still have the same weight as the ones in BN, they return 198 cotton patches (or 24.75 patchwork cotton sheets), and require 33 cotton sheets to craft, for an effective yield of 75% (not counting lost thread). This is better than the yield from the above test (14/35, or 40%), but still a sign that we should keep special-case uncrafts until the weight gives results actually closer to what you'd rationally expect of them.

[Coolthulhu]

Automated testing will be needed here for sure. I won't merge it without it, manual testing just doesn't cut it.

[chaosvolt]

Hmm, I'll try to look athow to port over the rest stuff.

[chaosvolt]

Committed the current work to port over the test for now, still having trouble sorting it out plus been distracted with offline stuff a lot lately.

[olanti-p]

Error: iuse_actor_test.cpp:83:59: error: member access into incomplete type 'const material_type'
        if( const cata::optional<itype_id> item_id = mater->salvaged_into() ) {
                                                          ^
../src/type_id.h:91:7: note: forward declaration of 'material_type'
class material_type;
      ^

Compiler needs to know full definition (internal layout) of the class to allow accessing its members and methods. Here, the compiler only has access to declaration (it knows it's a class and how it's called, and nothing else), hence it complains about incomplete type.

You can fix it by finding which header has the definition (in Visual Studio and VSCode, go to that declaration and right-click -> "Go to definition") and then including that header at the top of iuse_actor_test.cpp.

[chaosvolt]

Ah, thanks. I'll try to check this when I get home.

[chaosvolt]

Okay, so now the test seems to work, I just gotta figure out what the implications are for it failing. I still don't even fully 100% understand what Kevin's test is expecting to actually determine.

iuse_actor_test.cpp:89: FAILED:
  REQUIRE( smallest_yield_mass != units::mass_max )
with expansion:
  9,223,372,036,854,775,807mg
  !=
  9,223,372,036,854,775,807mg
with message:
  target := "kevlar_harness"

This tells me it wants the smallest possible yield to be...anything BUT the maximum mass? Not >= or <= but specifically !=, I can't think of any sane reason for that.

EDIT: So evidently what it means is something about the code ported over from the PR is causing kevlar harnesses to return the absolute fucking cap for mass, stored as a 64-bit value. Holy shit HOW.

EDIT 2: Cause might be due to kevlar harnesses being a migrated item actually? Lemme look into this further.

[chaosvolt]

There we go, fixed the use of migrated dog armors. Also fixed the weird redundant use of two separate kevlar harnesses, I assume Kevin meant one of those to be the kevlar vest.

This leaves what seems to be some signs of test failures indicating that some small items can salvage into more material than they have.

[Coolthulhu]

Quickest fix would be to round their weights up to material weight.

[chaosvolt]

Ah, guess I can work on that. That said, I'm not even 100% sure what the item is testing for. It looks like the ones that're failing are for items that weigh less than the weight of a single patch item.

What is even the intended behavior in that instance? If it's just meant to fail to yield a patch, I assume most items that weigh less than a patch should probably be shifted to using NO_SALVAGE with a specific uncraft recipe (like how some underwear is uncrafted into just thread), and excluded from the test?

[Coolthulhu]

NO_SALVAGE by default, bump up mass if it makes sense.
For the 3 items that pop up:

• Rain dog armor is reversible, but weighs less than any of the components, so the item's mass is just clearly wrong
• Swimming gloves are tiny, uncraftable, and made of 3 materials, so NO_SALVAGE sounds best here
• Burette includes glass, so NO_SALVAGE

[chaosvolt]

Ah, nice. I'll fix that in a bit then.

[Coolthulhu]

Looks mostly good, but we'll still need a test case that goes over ALL items and checks if they are one of:

• Can't be cut up
• Can be cut up and doesn't blow up

You can get all item types with item_controller.all().

[chaosvolt]

I'll have to see if I can implement that when I get back home, hopefully works out.

[chaosvolt]

Didn't get the chance to work on it much yet, but did get to look into it a bit. The melee balance test seems to include a good example of grabbing all items using this method: https://github.com/cataclysmbnteam/Cataclysm-BN/blob/upload/tests/melee_balance_test.cpp#L24

Next step I assume would be only calling push_back if the item in question lacks the NO_SALVAGE flag and consists off salvagable materials, so the test filters out stuff where salvage-by-weight is irrelevant.

I assume then that the actual test would basically entail taking the current test from the source PR, and instead feeding it all the items that've been called up, checking that in all cases salvaged mass doesn't exceed the target item's mass.

[chaosvolt]

So I'd looked into it recently and main issue I found is that item_controller is a member of itype, while the salvage stuff called up in the test (in particular the is_reversible boolean I'd need to filter out invalid items) is a member of item

[Coolthulhu]

FFFF I accidentally wrecked your branch when pushing a tiny update (the "check all items" thing), I'll fix it.

[Coolthulhu]

Alright, fixed.

I implemented the whole "go over all items, check if they can be cut up properly or not at all" thing. Now it complains for some items.
Once those items are fixed, I think it's good to go.

[chaosvolt]

Oh nice, thank you. I'll check in a sec to see what items it complains about then tweak them, then.

EDIT: Nice, the buildbot isn't showing them because it's only focused on complaining about the scenario whitelist in DSA, so guess I'll try to remember how to test manually again.

EDIT: And having trouble getting any test results showing as failed for me to tell what items need updating.

[olanti-p]

The "check all" test is disabled by default, you have to run the test executable and supply it with the test name, e.g. for VS build it's like this:

Cataclysm-test-vcpkg-static-Release-x64.exe cut_up_yields_all

[scarf005]

build worked, tests passed, gameplay worked. LGTM