-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Misc repair kit draws ammo from nearby player #73129
Conversation
You are creating a pull request with the master branch as the head branch. This is likely a mistake unless you really know what you are doing. You may read https://docs.github.com/en/get-started/quickstart/contributing-to-projects#creating-a-branch-to-work-on for a typical workflow of contributing to a project on GitHub. |
114645b
to
fee3cb9
Compare
fee3cb9
to
f22709c
Compare
f22709c
to
6a31677
Compare
2d90a31
to
82d0942
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good at a glance overall.
82d0942
to
9351258
Compare
9351258
to
df6c8e3
Compare
@RonaldCoppieters |
@esotericist No. Just a wrong command. |
okay, good. this PR looked promising, so i was worried for a moment. |
cdef4bb
to
46b572d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built locally, didn't find any issues with it.
Sorry this ended up sitting so long. I've rebased it to fix the conflicts and hopefully it will pass tests and I didn't mess anything up. |
This looks good to me. I'm not super familiar with the items code so let's hope I haven't doomed us all. |
…_kit Backport #73129 for 0.H (misc repair kit reloading fix)
Summary
Bugfixes "Misc repair kit draws ammo from around player and inventory, opposed to an internal ammunition pocket."
Purpose of change
Closes issue #71090, following suggestions by AnotherSeawhite in #72774. Note that this PR does not yet allow for selecting between different ammo types or item types like the suggestion describes, but this could be tweaked in the future.
This behaviour could eventually be expanded to sewing kits and such. It only requires that the "USES_NEARBY_AMMO" flag is added and the pocket removed in the item's JSON.
Describe the solution
The magazine pocket has been removed from the item. The flag "USES_NEARBY_AMMO" was added that searches for the default ammo type within the player's crafting inventory.
Note: I'm tweaking the string that says "Charges: [ammo]/[capacity]" to say "Charges: [ammo]" using regex in(Edit: No longer the case)src/activity_handlers.cpp:2573
, because I didn't want to mess with the translations, but a cleaner edit would probably just be to add a translation for a string without the "/[capacity]" part.Describe alternatives you've considered
The whole "repair_item" logic could probably do with a rework where it no longer relies on the ammo system, but this PR is a band-aid solution to get the item working again at the very least.
Testing
Tested on the latest build if a damaged weapon can be repaired using nearby duct tape, if the duct tape was consumed, and that it still disallows repair if no duct tape is around. Behaviour could possibly be weird in cases where the ammo disappears from within the player's reach during the few seconds of repair, like when materials are stolen by an NPC and such. In this case it might still attempt a repair at no cost, but this hasn't been tested.
An old save was loaded. It will throw errors on existing repair kits that a pocket cannot be found for the duct tape and any duct tape inside existing repair kits will be lost as a result, but nothing major beyond that.
Additional context