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

Fixed crop growing time on other year length than 96 days. #786

Closed
wants to merge 1 commit into from

Conversation

pgelinas
Copy link

I've fixed the growth rate formula:
The formula was using both TFC_Time.timeRatio96 and a timeMultiplier (360/TFC_Time.yearLength), so a longer year was way too slow. For example, a year length of 192 should be taking twice as much days, but instead was taking four time as much. So I removed the timeMultiplier and replaced it with the value used in 96 year length: 3, since it was doing an integer division.
The other solution would be to fix timeMultiplier to a float division instead, remove the usage of TFC_Time.timeRatio96 and extends the growthTime of all CropIndex by 1.25 to counter the fix to timeMultiplier (3.75 instead of 3 on a 96 year length).

Also fixed nutrient draining:
Both SoilMax method and DrainNutrient was using a timeMultiplier based on a 360 year length. SoilMax was getting bigger with longer year, and DrainNutrient was getting slower with longer year. This made nutrient draining extremely slow on longer year and thus making crop growth slightly faster than it should be

Growing time on a 96 year length (default) remains unchanged with this fix and growth time properly scale with other year lenght: on a 192 year length, crops take twice as much time. For example, wheat take roughly 18 days on 96 and 36 ont 192.

Also, you can use the following gist to test crop growth: https://gist.github.com/pgelinas/7a6ac0c79d85554878a7

@CHeuberger
Copy link
Contributor

I believe there is a little (copy&paste?) error in your code: adding instead of multiplying by timeRatio in the following line seems to be incorrect?
TECrop:157: ...(crop.growthTime + TFC_Time.timeRatio96)...

@Kittychanley
Copy link
Collaborator

I've ran extensive testing on the original code, using a stripped down version as its own program to spit out data with no fluctuations in water or temperature. The entire issue was literally a missing f on 360 on line 51 of TECrop. If you want the spreadsheets of the data, I'll be happy to provide them, but this entire PR is unnecessary as it's much much simpler to just add that one missing character.

@CHeuberger
Copy link
Contributor

this is not the only/main problem.
In (actual) line 158 of TECrop:

float growthRate = ((((crop.numGrowthStages / (crop.growthTime * TFC_Time.timeRatio96)) + tempAdded) * nutriMult) * timeMultiplier) * TFCOptions.cropGrowthMultiplier;

the rate is divided by timeRaio96 (daysInYear/96) and multiplied by timeMultiplier (360/daysInYear) so it is being effectively divided by square of daysInYear - so timeMultiplier should be removed (or timeRatio96).
I checked this in game, different year length (96 and 192) same world, same location, same time, just removed the random from the growth timer. I planted 20 garlic near water on 1. June. They are mature at:
July 2nd@96 days/year versus August 5th@192 days/year
I believe they should be mature near the same time (July 2nd@96 the same as July 4th@192), but the intervals are almost exactly the double (1 month/2 months)

@Kittychanley
Copy link
Collaborator

You really can't trust data from testing in-game because there are so many variables at play. Here's the spreadsheet confirming crop growth working as expected after the recent fix:

https://www.dropbox.com/s/kd447mxwaa45nig/Crop%20Analysis.xlsx?dl=0

It is intentional that crop growth doesn't scale perfectly with year length.

@CHeuberger
Copy link
Contributor

well, I am playing with the game and not the spreadsheet... what you wrote is kind of the same as telling that weather forecasts are more precise than the real weather...
Anyway my results with the game are similar, if not the same as the spreadsheet, or is it the reverse? [:-)

I just didn't know that it was intentional

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.

None yet

3 participants