Thorinair / Stardew-Profits

A Stardew Valley crop profits calculator and visualizer. Link: https://thorinair.github.io/Stardew-Profits/
MIT License
164 stars 78 forks source link

Fixes #81 - Aging Not Applying #82

Open doodlebunnyhops opened 1 month ago

doodlebunnyhops commented 1 month ago

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.

Doksperiments commented 1 month ago

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 commented 1 month ago

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.

doodlebunnyhops commented 1 month ago

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

rangigo commented 1 week ago

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 commented 1 week ago

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

doodlebunnyhops commented 3 days ago

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

I'm ready for it ☺️