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

Fixes #81 - Aging Not Applying #82

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

doodlebunnyhops
Copy link
Contributor

@doodlebunnyhops doodlebunnyhops commented Jul 10, 2024

Fixes #81
Updated sections looking for produce.keg != null where it should be produce.kegType != null && options.aging != "None" to see if aging is applied.

@doodlebunnyhops doodlebunnyhops marked this pull request as ready for review July 10, 2024 13:37
@Doksperiments
Copy link

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.
On line 446 you changed the check to jarType but forgot to change the content of the statement. As you have it now it will return a string of the jarType instead of calculating the actual income.

A fixed version could look like this:
netIncome += itemsMade * (crop.produce.jarType != null ? options.skills.arti ? (crop.produce.price * 2 + 50) * 1.4 : crop.produce.price * 2 + 50 : 0);

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:
return crop.produce.kegType == "Wine" ? 3 : 2.25;

And the code for calculating the income and kegPrice would look like this:
netIncome += itemsMade * (crop.produce.keg != null ? crop.produce.keg * caskModifier : Math.floor(crop.produce.price * kegModifier) * caskModifier);
var kegPrice = d.produce.keg != null ? d.produce.keg * caskModifier : Math.floor(d.produce.price * kegModifier) * caskModifier;

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

@doodlebunnyhops
Copy link
Contributor Author

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 crop.produce.keg - being available to items that have a different base value other base*3.

I updated the formula to first not apply artisan buff till much later and have getKegModifier accounting for the edge case of Coffee and Tea, lastly getCaskModifier simplified to just aging.

Additionally I repurposed var kegPrice that was calculated in the drawing function to be set as profitData.kegPrice to reduce function calling on line 456.

Copy link

@Doksperiments Doksperiments left a 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

js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
@doodlebunnyhops
Copy link
Contributor Author

@Doksperiments - would appreciate your input on changes made.
@Thorinair you as well :D

  • crops.js: for produce that can be used in a keg/cask a boolean of ages has been added.
  • main.js
    • getKegModifier removed
    • getKegBasePrice added
    • kegPrice simplified to kegPrice = crop.produce.ages ? kegBasePrice * caskModifier : kegBasePrice;

Copy link

@Doksperiments Doksperiments left a 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

js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
@rangigo
Copy link

rangigo commented Aug 26, 2024

Hey I just realised this PR fixed the problem showing incorrect Keg price with caskModifier. Do you know when this can be merged? Does only @Thorinair have the merge access for this repo?

@Thorinair
Copy link
Owner

If others believe that it is ready for a merge, I could merge it this week!

@doodlebunnyhops
Copy link
Contributor Author

If others believe that it is ready for a merge, I could merge it this week!

I'm ready for it ☺️

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.

4 participants