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

Fix overly-restrictive logic in check for ability to crush frozen liquids #63019

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

cake-pie
Copy link
Contributor

@cake-pie cake-pie commented Jan 7, 2023

Summary

Bugfixes "Fix overly-restrictive logic in check for ability to crush frozen liquids"

Purpose of change

Partially addresses #62952, specifically the part that is caused by flawed logic when checking whether the player has the tools required to crush frozen liquids.

The preexisting check was:

  • iterate through all items that the player has
    • if an item has screwdriver quality, put it in list screw_tools
    • if an item has hammering quality, put it in vector hammer_tools
  • iterate through hammer_tools
    • remove that item from screw_tools
  • check that there are still screw tools left

The intent behind this check is presumably to deal with the corner case where the player only has one tool that satisfies both hammer and screwdriver qualities -- that does not satisfy requirements because that one item cannot be used to hammer itself.

However, the logic is flawed and rejects some valid cases -- a tool that has both hammer and screwdriver qualities cannot be used to fulfill the screwdriver requirement (to be used in conjunction with another item that will serve as the hammer).

Describe the solution

Rewrite the logic. This required a bit of additional machinery in item class to get/check the qualities of an item itself excluding its contents, whereas preexisting code does these checks recursively, including any contents of the item

Describe alternatives you've considered

a)

Not adding the new quality checking functions and just use preexisting item::has_quality(), i.e. code as at af50aa3

This would have a different bug; an edge case due to the recursive nature of item::has_quality(). When the player only has one tool with both qualities they should fail the check. But if that item is in a container in inventory (e.g. a backpack) rather than wielded, then the container also reports having the required qualities. The container then counts as a second item in addition to the tool itself, and this situation would pass the test erroneously.

b)

Instead if implementing Item::get_quality_nonrecursive, change Item::get_quality so that it has an additional boolean flag parameter recursive which defaults to true. (And likewise for has_quality). This could have caused complications because there is already another optional boolean param strict_boiling. Opted for a separate function to minimize potential for mistakes.

Testing

  • set up a bucket with water and let it freeze
  • ensure character has absolutely no tools of any sort
  • try to g grab ice and note message indicating lack of tools for crushing
  • debug spawn a firearm repair kit small_repairkit and make sure to wield it
  • try to grab ice, ensure it fails
  • put the repair kit in a worn backpack or similar. examine backpack - it will list qualities from the repair kit
  • try to grab ice, ensure it fails
  • debug spawn a second firearm repair kit, make sure to pick it up
    • should now have both kits in inventory
  • try to grab ice, the message about lack of tools should be gone (don't grab the ice yet!)
  • disassemble one of the repair kits and discard all of its components except for the hammer
    • should now have one hammer and one intact firearm repair kit in inventory
  • try to grab ice, this should also work

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 7, 2023
@PatrikLundell
Copy link
Contributor

I don't know if there is such a case currently, but what about a "kit"/toolbox that contains two distinct tools that provide two different functionalities? The "kit" should be usable to perform functions requiring both functionalities because the tools providing the functionality are distinct.

My intuitive approach would be to use a "kit" if there are no need to look for conflicts, but look at container content and ignore the "kit" if there is such a need (or always ignore container items). This might cause a problem if the container somehow gets additional values on top of what the contents provide, though.

@cake-pie
Copy link
Contributor Author

cake-pie commented Jan 7, 2023

I don't know if there is such a case currently, but what about a "kit"/toolbox that contains two distinct tools that provide two different functionalities? The "kit" should be usable to perform functions requiring both functionalities because the tools providing the functionality are distinct.

Yes, that is also an issue. See #62952, which is a two-part issue; this PR only addresses one part of it: the broken logic specific to the frozen liquid crushing requirement check. The other part of the issue is precisely related to "toolkit" type that are a composite of their component tools, behaving as a "sum-of-parts" but being a single item for convenience of handling. There are several such items in the game and it would be better to have a separate PR to take care of whatever changes are needed for how those are handled.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 7, 2023
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` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants