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 the casing ejection behavior of The Disorder sledgehammer #59323

Merged
merged 3 commits into from
Jul 24, 2022

Conversation

catdach
Copy link
Contributor

@catdach catdach commented Jul 16, 2022

Summary

Bugfixes "Fix the casing ejection behavior of The Disorder sledgehammer"

Purpose of change

Fixes #57683

Describe the solution

Adjusted casings_handle() to work on tools, instead of only working on guns.
Changed the item type of Disorder to "tool" instead of "generic"

Describe alternatives you've considered

Removing the check that makes the casing handler only work on guns. Doing this seems like it would almost certainly break something, so I'd prefer not to do that.

Changed the item type of Disorder to "gun." Doing this requires adding a whole bunch of gun data that would basically turn it into a ranged weapon, which is not what I want to do.

Testing

I've been having some issues with visual studio, so I haven't been able to properly test it myself yet. Once I can, the steps will be:

  1. spawn in the Disorder, some 22 blanks, and some crawling zombies. (also spawn and put on a backpack)
  2. load the Disorder with a 22 blank.
  3. hit crawling zombies with the Disorder until the special technique procs.
    The Disorder should now contain an empty casing for the 22 blank.
  4. (R)eload the Disorder.
    The empty casing should now be in your backpack or some other place in your inventory.
  5. Success!

Edit: testing is done, everything works correctly.

Additional context

Note that trying to pick up an item between steps 3 and 4 will cause the empty casings to fall on the ground. This happens with other weapons too, so it seems like a separate issue.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Items: Ammo / Guns Ammunition for all kinds of weapons and these weapons themselves <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 Jul 16, 2022
@catdach catdach marked this pull request as ready for review July 16, 2022 21:35
@catdach
Copy link
Contributor Author

catdach commented Jul 16, 2022

Ok, I got visual studio to behave for a while. Unfortunately it seems that the disorder refuses to expend its ammo at all. I also tried using it on the master branch and sure enough; I can't get it to expend its ammo, even when the technique procs.

So it's not a result of my PR, but I'm not even able to replicate the bug that this PR is meant to fix. I swear it worked fine a couple months ago.

so...yeah 🤷

@bombasticSlacks
Copy link
Contributor

Hmmm yeah it worked when I originally PRd it 🤔

If this doesn't solve the issue can you mark it as draft so no one yolo merges it? I'm not apposed to this being fixed but if you've tested this and it doesn't solve it then yah know.

@catdach catdach marked this pull request as draft July 18, 2022 15:36
@catdach
Copy link
Contributor Author

catdach commented Jul 18, 2022

Just to clarify; this PR should fix the casings ejection issue, but it's impossible to test unless the weapon actually produces casings.

@bombasticSlacks
Copy link
Contributor

bombasticSlacks commented Jul 18, 2022

https://github.com/CleverRaven/Cataclysm-DDA/blame/master/src/melee.cpp#L1638 I think it's this line that broke it, thanks for letting me know, I'm gonna try and write a simple unit test tonight for it expending ammo 😄

@catdach
Copy link
Contributor Author

catdach commented Jul 24, 2022

Finished testing. After #59474 everything seems to work correctly 👍

@catdach catdach marked this pull request as ready for review July 24, 2022 01:42
@bombasticSlacks bombasticSlacks merged commit c2b61d6 into CleverRaven:master Jul 24, 2022
@catdach catdach deleted the Disorder_ejection_fix branch July 25, 2022 16:22
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` Items: Ammo / Guns Ammunition for all kinds of weapons and these weapons themselves [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disorder sledgehammer reload_eject flag not working correctly
2 participants