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 crossbow gunmod compatibility #1526

Merged
merged 2 commits into from
May 13, 2022

Conversation

chaosvolt
Copy link
Member

Summary

SUMMARY: Bugfixes "Fix crossbows not counting as crossbows for the sake of gunmods, allow distinction between actual crossbows and pneumatic bolt driver"

Purpose of change

So recently I was poking through item.cpp and was reminded that the gunmod function that separates crossbows from other types of ranged weapon was never updated to account for crossbows using rifle skill, as it was switched from archery.

On looking at the function I decided that there were two ways to fix it. Either convert the "if ammo is bolt or it's a bullet crossbow, it's a crossbow" statement to not care about the archery skill, since it triggering would make the function return before it could grab the gun skill anyway. Or I could implement a flag check.

The main reason to do this is, aside from checking for specific ammo types and item IDs not being a good way to do this, there are going to be items that fire bolts but shouldn't count as a crossbow, example being the pneumatic bolt driver, plus you may have items that should be a crossbow but wouldn't be under that check. No vanilla examples of the former come to mind, though outside repo of course adds the example of wraithslayer crossbows in Arcana.

Describe the solution

  1. Updated item::gun_type to classify items as a crossbow if they have the CROSSBOW flag. This also meant removing the non-functional "return early if we see the archery skill" if statement since now if an item has the archery skill it tell it's a bow through the normal skill lookup.
  2. Added the relevant flag to flags.json.
  3. Added the flag to all of the crossbow items in vanilla. As mentioned above, the pneumatic bolt driver was deliberately excluded.
  4. Set the bow dampening kit to allow installing on bows as well as crossbows. Crossbows all have the dampening slot but could not actually install the only item that uses that slot.

Describe alternatives you've considered

As mentioned, we could keep the messy-looking check for specific ammo and item types, but then bolt drivers would count as crossbows and other weirdness as mentioned previously.

Testing

  1. Compiled and load tested.
  2. Confirmed that you can install a bow dampening kit on crossbows that have the flag.
  3. Also tested while the flag was not on all crossbows to confirm it wouldn't let me install a bow dampener on a crossbow that had the dampener slot but not the crossbow flag.
  4. Checked mods for any other crossbows to warrant the flag. Only really saw and example in the obsolete More Survival Tools, which reminds me I'll want to add it to the makeshift crossbow in MST Extra as a followup.
  5. Ran all JSON changes through the online linter.
  6. Ran item.cpp through astyle.

Additional context

Relevant PR that switched gun skill of crossbows, by @kevingranade: CleverRaven/Cataclysm-DDA#21545

I checked and confirmed the same issue is present in DDA: https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/item.cpp#L10122 so they may wish to be made aware of it for the purpose of porting over BN's fix for it.

chaosvolt added 2 commits May 9, 2022 20:13
So recently I was poking through item.cpp and was reminded that the gunmod function that separates crossbows from other types of ranged weapon was never updated to account for crossbows using rifle skill, as it was switched from archery by CleverRaven/Cataclysm-DDA#21545

On looking at the function I decided that there were two ways to fix it. Either convert the "if ammo is bolt or it's a bullet crossbow, it's a crossbow" statement to not care about the archery skill, since it triggering would make the function return before it could grab the gun skill anyway. Or I could implement a flag check.

The main reason to do this is, aside from checking for specific ammo types and item IDs not being a good way to do this, there are going to be items that fire bolts but shouldn't count as a crossbow, example being the pneumatic bolt driver, plus you may have items that should be a crossbow but wouldn't be under that check. No vanilla examples of the former come to mind, though outside repo of course adds the example of wraithslayer crossbows in Arcana.
@Coolthulhu Coolthulhu self-assigned this May 10, 2022
@Coolthulhu Coolthulhu merged commit 728f1da into cataclysmbnteam:upload May 13, 2022
@chaosvolt chaosvolt deleted the crossbow-fix branch May 13, 2022 17:40
scarf005 pushed a commit to scarf005/Cataclysm-BN that referenced this pull request May 13, 2022
* Fix crossbow gunmod compatibility

So recently I was poking through item.cpp and was reminded that the gunmod function that separates crossbows from other types of ranged weapon was never updated to account for crossbows using rifle skill, as it was switched from archery by CleverRaven/Cataclysm-DDA#21545

On looking at the function I decided that there were two ways to fix it. Either convert the "if ammo is bolt or it's a bullet crossbow, it's a crossbow" statement to not care about the archery skill, since it triggering would make the function return before it could grab the gun skill anyway. Or I could implement a flag check.

The main reason to do this is, aside from checking for specific ammo types and item IDs not being a good way to do this, there are going to be items that fire bolts but shouldn't count as a crossbow, example being the pneumatic bolt driver, plus you may have items that should be a crossbow but wouldn't be under that check. No vanilla examples of the former come to mind, though outside repo of course adds the example of wraithslayer crossbows in Arcana.

* Update item.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants