-
Notifications
You must be signed in to change notification settings - Fork 82
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
Fixes #81 - Aging Not Applying #82
base: dev
Are you sure you want to change the base?
Conversation
Hey i was having the same issue that was reported and ran into your pull request. I'm pretty sure the solutions you implemented won't work. A fixed version could look like this: In the solution for the kegs, the condition works only if the aging is null, otherwise the artisan bonus will still be counted twice (which was the original error causing incorrect values) and in the case of fixed price products (like beer), the price will be entirely incorrect (since you aren't using the produce.keg value anymore). One solution I tried to implement is to remove the artisan bonus calculations from getKegModifier entirely, since the function isn't ever called alone, and the artisan bonus is included in the caskModifier. The function getKegModifier could be simplified to: And the code for calculating the income and kegPrice would look like this: I tried to double check the new values with the wiki and ingame values and they seem to match, but there could be some edge cases i missed |
Good catch @Doksperiments - I updated jar to return 0 on type null. On your other point for keg/cask. I reevaluated the original formula here and I had an oversight on the use of I updated the formula to first not apply artisan buff till much later and have Additionally I repurposed |
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.
Figured a code review would be better to highlight specific parts of the code instead of writing a single comment
@Doksperiments - would appreciate your input on changes made.
|
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.
I tried a few of cases which were confirmed wrong before and the values seem to be all correct, but as always with cherry-picked tests there could be some cases I missed (which hopefully will be reported if they get found).
It would probably be useful to have a test suite to test against, but I feel no one would want to actually write it :D
Other than that it seems to me that it's working as intended now
Hey I just realised this PR fixed the problem showing incorrect Keg price with |
If others believe that it is ready for a merge, I could merge it this week! |
I'm ready for it |
Fixes #81
Updated sections looking for
produce.keg != null
where it should beproduce.kegType != null && options.aging != "None"
to see if aging is applied.