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

Throw debugmsg when setting charges for items that cannot have charges #72780

Merged
merged 5 commits into from
Apr 7, 2024

Conversation

RenechCDDA
Copy link
Member

@RenechCDDA RenechCDDA commented Apr 1, 2024

Summary

Bugfixes "Butchery properly gives a scaling amount of butchery refuse, instead of just 1"

Purpose of change

@GuardianDll raised a question in the development discord about butchery only giving 1 butchery refuse. Some checks later, I found out that constructor naively sets charges regardless of whether the type is allowed to have them or not. This results in an item with a charges defined, but that is handled by our code as not being count_by_charges(), thus ignoring the value.

Describe the solution

Only set charges if type is allowed to have charges
Debugmsg if type is not allowed to have charges

Fix existing butchery call

Describe alternatives you've considered

Testing

Additional context

"Charges are the devil" -kevingranade

src/item.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the [C++] Changes (can be) made in C++. Previously named `Code` label Apr 1, 2024
@RenechCDDA
Copy link
Member Author

Oh yeah, the tests absolutely explode.

We have deeper problems than just butchery, don't we?

@mqrause
Copy link
Contributor

mqrause commented Apr 1, 2024

Chances are 99%+ of those errors set charges to 1. Because any value bigger than that should trigger charge removal after a save/load cycle, causing stuff to fall out once you interact with it. And I'd expect that would have been noticed already.

@RenechCDDA
Copy link
Member Author

Yeah that's what I'm seeing when running the tests locally, just lots of calls to the constructor with irrelevant quantities. I think I'm just going to paper over it by only throwing error on qty > 1

@mqrause
Copy link
Contributor

mqrause commented Apr 1, 2024

Chances are also good this is coming from limited sources like item group creation.

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Apr 2, 2024
@RenechCDDA RenechCDDA marked this pull request as draft April 2, 2024 00:29
@github-actions github-actions bot added Items: Food / Vitamins Comestibles and drinks Code: Tests Measurement, self-control, statistics, balancing. <Bugfix> This is a fix for a bug (or closes open issue) labels Apr 2, 2024
@RenechCDDA
Copy link
Member Author

Assuming this pass succeeds on GH I am going to consider removing the if qty > 1 requirement and eliminating the rest of the malcontents.

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Apr 2, 2024
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Apr 2, 2024
@PatrikLundell
Copy link
Contributor

If there are too many complaints I'd suggest checking for >1 as suggested and then initiate a series of cleanup PRs, ending with the removal of the incorrectness hiding check. Especially in the light of the recent "too large" PR rules.

@RenechCDDA RenechCDDA marked this pull request as ready for review April 2, 2024 13:53
@akrieger
Copy link
Member

akrieger commented Apr 7, 2024

IDK seems green.

@akrieger akrieger merged commit 84dae0b into CleverRaven:master Apr 7, 2024
26 checks passed
@RenechCDDA RenechCDDA deleted the catch_bad_charges branch April 7, 2024 00:52
@I-am-Erk I-am-Erk added the 0.H Backport PR to backport to the 0.H stable release canddiate label Apr 7, 2024
Procyonae pushed a commit to Procyonae/Cataclysm-DDA that referenced this pull request May 17, 2024
Throw debugmsg when setting charges for items that cannot have charges
@Procyonae Procyonae mentioned this pull request May 17, 2024
kevingranade pushed a commit that referenced this pull request May 17, 2024
Throw debugmsg when setting charges for items that cannot have charges

Co-authored-by: RenechCDDA <[email protected]>
@Procyonae Procyonae added 0.H Backported and removed 0.H Backport PR to backport to the 0.H stable release canddiate labels May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.H Backported 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. Items: Food / Vitamins Comestibles and drinks json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants