Deadrik / TFCraft

TerraFirmaCraft
GNU General Public License v3.0
204 stars 142 forks source link

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

Closed pgelinas closed 9 years ago

pgelinas commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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