Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor salvage_actor::cut_up #58414

Merged
merged 27 commits into from
Jul 18, 2022
Merged

Conversation

Willenbrink
Copy link
Contributor

@Willenbrink Willenbrink commented Jun 14, 2022

Summary

Bugfixes "Refactor salvage_actor::cut_up and fix minor bugs"

Purpose of change

This PR fixes #54657 which pointed out an issue with the formula for the returned weight.
I found another issue with the item loss, due to low fabrication etc., being applied multiple times.

Describe the solution

While fixing the issue with multiple materials, I decided to refactor the function to reduce code duplication and make things clearer.
The changes are mostly:

Validation:

  • Remove try_to_cut_up and use only valid_to_cut_up so only one function is used for validation. Not perfectly happy with the solution but I still think two nearly identical functions will cause problems in the future.
  • Make cut_up private and wrap it with try_to_cut_up which does the validation itself. An external call without validation is no longer possible
  • Move remaining validation completely outside of cut_up instead of doing it twice.

Refactoring:

  • Rename some vars for clarity (it -> cutter/cut, remaining_weight -> efficiency, salvage_to -> salvage)
  • Extract find_uncraft_recipe to simplify cut_up. It attempts to find a crafting or disassembly recipe fitting some constraints.
  • Extract cut_up_component into a recursive lambda. Makes the order of traversal clearer and passing arguments simpler.

Changes

  • Lift the restriction on recipe weight. As the ingredients weigh more most of the time recipes were rarely used previously. Now efficiency is decreased while reversing the recipe.
  • Prefer disassembly recipes to normal ones. Previously cutting items up could give more than disassembling
  • Increase malus for low fab skill (1% -> 10%)
  • Discard unsalvageable or count_by_charges item instead of distributing their weight to their materials

Describe alternatives you've considered

Not refactoring stuff and leaving things a whole lot less clear. It would be simpler to review I guess. Unfortunately changes to the indentation make the side-by-side diff basically useless.

Testing

The tests succeed and I've tested it with some specific items:

Item Weight Original Weight Salvage
Leather Vest 1.00 0.96
Pair of Survivor Boots 1.33 0.22
Scavenger Gear 7.92 7.8
US Ballistic Vest 2.91 2.62

Note that the boots can be disassembled to obtain a rubber sole. As cut_up now prefers disassembly recipes when possible, a lot of weight is lost.

Additional context

I'm unsure about some potential problems and would like some feedback:

  • The penalty for cutting up is 0% (besides rounding) for high fab + dex characters.
    • Decrease efficiency to a flat 75% + additional modifiers for fab/dex.
  • Items with the NO_SALVAGE flag but with appropriate materials were still cut up.
    • This is unintuitive. The whole item is discarded now.
  • cut_up returns some weird values for the cost.
    • We could use time_to_cut_up for the cost instead. Seems more natural but I'm not sure what the value is exactly used for.

Ideally both fabrication and dexterity requirements would relate to the complexity of the item. Not sure how to do that. Do recipes exist for the majority of items? Then we could use their skill level to determine complexity. Could also use hardness of materials (and/or number of materials although that was just removed by this PR :) ). I think thats out-of-scope for this PR.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 14, 2022
Move mat_set outside of the loop
Invert if
Remove useless braces
Efficiency is calculated once and assigned to a const
The recipe is found in a separate function, more clearly separating that
functionality from what should be done if no recipe is found
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 14, 2022
@Willenbrink Willenbrink marked this pull request as draft June 20, 2022 20:04
@Willenbrink Willenbrink deleted the refactor_cut_up branch June 23, 2022 19:38
@Zireael07
Copy link
Contributor

Why close?

@Willenbrink Willenbrink restored the refactor_cut_up branch June 24, 2022 08:13
@Willenbrink
Copy link
Contributor Author

Oops, that was completely unintentional. Apparently I deleted the wrong branch and Github automatically closed the PR.

@Willenbrink Willenbrink reopened this Jun 24, 2022
@Willenbrink Willenbrink marked this pull request as ready for review June 27, 2022 16:16
src/iuse_actor.cpp Outdated Show resolved Hide resolved
src/iuse_actor.cpp Outdated Show resolved Hide resolved
src/iuse_actor.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 1, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 5, 2022
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 11, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 11, 2022
@dseguin dseguin merged commit dcfd78b into CleverRaven:master Jul 18, 2022
@Willenbrink Willenbrink deleted the refactor_cut_up branch July 19, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When cutting up items, you get 1/X original weight from items with X different materials
4 participants