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 furniture reloading with multiple ammo types #72743

Closed
wants to merge 2 commits into from

Conversation

nornagon
Copy link
Contributor

@nornagon nornagon commented Mar 31, 2024

Summary

Bugfixes "Fixed reloading of furniture that allow multiple ammo types"

Purpose of change

The reload_furniture action was written assuming each furniture only allowed
a single ammo type, which is no longer true, resulting in strange bugs when
mixing ammo types.

Fixes #68371.

Describe the solution

Refactored reload_furniture to enforce the invariant that only a single type
of ammo is present in the furniture at a time.

I also changed some pseudo crafting items to have the PSEUDO flag, which stops
the reload screen from showing "(0/2000)" when choosing ammo (even when there
is ammo present). The denominator there is also wrong for the forge as it
allows a different amount of max charges for coal vs charcoal.

And I fixed the ammo selection screen giving 2000 as the maximum amount of coal
to load at a time by changing item::reload_option::qty to check the ammo
remaining for the specific ammo type being loaded.

Describe alternatives you've considered

Perhaps it should be allowable to mix ammo types? I didn't take that approach
because it seemed like it would be messy on the ammo usage side.

Testing

I manually tested:

  • Loading an empty forge lets you choose either coal or charcoal
  • Once a forge has something in it, you can only reload that same item type
  • Loading coal lets you load up to 10000 at a time, vs charcoal with 2000

Additional context

@RenechCDDA
Copy link
Member

Alternative to #72703 I guess?

@nornagon
Copy link
Contributor Author

nornagon commented Mar 31, 2024 via email

@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` Player Faction Base / Camp All about the player faction base/camp/site json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Mar 31, 2024
@katemonster33
Copy link
Contributor

Looks like there's more here than what is in my branch, if @RenechCDDA wants to add their changes to this and merge this instead I'm fine with it

@nornagon
Copy link
Contributor Author

nornagon commented Mar 31, 2024 via email

@katemonster33
Copy link
Contributor

We might want to blend them a bit? Idk if it’s better to restrict to one
ammo like this does or allow mixing like yours does

Renech and I talked on this in discord, we actually agreed to restrict it to a single ammo type because weird issues were happening where you would hit the "cancel" button on one ammo and it would not allow you to reload the furniture. Also I believe there was reasons involving the other parts of the code that needed to change for the furniture to actually consume the non-default ammo

Copy link
Member

@RenechCDDA RenechCDDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't correctly consume charges for non default ammo.

Put 5 coal into a forge, craft crude glass lens. Examine forge. Notice that despite having done the recipe consuming 5 charges, there is still 5 coal to take out.

@RenechCDDA
Copy link
Member

The above behavior was rectified in katemonster33's PR via 5c9ed5f

@nornagon
Copy link
Contributor Author

nornagon commented Apr 1, 2024

Closing in favor of #72703

@nornagon nornagon closed this Apr 1, 2024
@nornagon nornagon deleted the fix-forge-ammo-reload branch April 1, 2024 18:47
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 <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Player Faction Base / Camp All about the player faction base/camp/site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clay kiln and rock forge makes coal (not charcoal) disappear
3 participants