RandomMcSomethin / fallingleaves

MIT License
43 stars 20 forks source link

Gh 47 More Leaves when Raining #49

Open cjhetzle opened 1 year ago

cjhetzle commented 1 year ago

Added options for Wind and Weather to affect leaf spawn rate.

Fourmisain commented 1 year ago

Hm... I'm not yet sure if this is a good idea, at least the implementation leaves a lot to be desired.

First of all, it seems very confusing to have both leafWindySpawnCoeficient and leafWeatherSpawnCoeficient, where "weather" also includes "wind". It's just semantically unclear, making the descriptions/translations unclear as well.

Since the two wind states WINDY/STORMY already increase wind strength, it would IMO make more sense to have just a single coefficient (and maybe another one for rain, relating back to #47). Realistically speaking: Just because it's thundering, doesn't mean trees suddenly lose more leaves. It's the strength of the wind (and weight of the rain) that make/s more leaves drop. (Though I kinda get the idea of interpreting the wind states as "events" that suddenly change the visuals.)

Then there's another big issue: Since the leaf spawn rate gets directly multiplied by the coefficient, the average value of the coefficient matters a ton. The coefficient is proportional to the wind strength, hence we would first need to analize the average wind strength (actually, the distribution makes calculating the mean extremely easy) and tune the coefficient based on that.

From a quick look at the numbers, this PR would immediately bump leaf spawn rates by 10-40% in calm conditions and up to 480-880%(!) under extreme conditions, which is... just a tiny little bit too much.

Then finally: Coefficient with 2 'f' :wink:

All in all, I'm still pondering if the idea of "More leaves when wind" is actually good - it probably is!

I'm afraid I can't accept this PR as-is and I'd rather implement the suggested changes myself (since it requires a lot of finetuning and probably some new formulas).

Though I shouldn't have the final word on this, so @BrekiTomasson @RandomMcSomethin please, if you got some comments as well!

cjhetzle commented 1 year ago

Yeah, I can agree that my naming is messy and I didn't really put much thought behind the actual rates that were created. The amplitude of the foliage was really not "tuned" in any real sense of the term, but instead just based off what I thought looked right at the time, nothing correctly done, but it got the point across I think in a first attempt.

IMO make more sense to have just a single coefficient (and maybe another one for rain

So as of now, there is a coefficient affected by the level of wind and will rise as the "WIND" increases. (CALM, WINDY, STORMY)

So that's one. The other is almost based on the state of rain, but I see what you mean.

The second is currently based on this mod's interpretation of "weather" and not the worlds actual state of "isRaining". This could be changed and have the coefficient now read as leafRainSpawnCoefficient . This could be another float value from 1.0-3.0f.

You said that you may want to do this yourself, but I may go ahead out of boredom and redo this pull request with what you suggested and try to clean it up more.

I'd be looking to:

cjhetzle commented 1 year ago

@Fourmisain in this update, I've changed the "weather" variable to a more specific "rain" float multiplier with 2.0f being the default.

For the wind coefficient, I tried to make it less impactful by squeezing any possible range from 0.87->1.67.

These numbers are obtained from using the default "1.0f" weight for the wind. Typical wind speeds are around .3, but go as low as 0.05 and as high as 1.1.

Take X as wind speed and Y being the coefficient: Z = ( 10 * X ) ^ ( 0.2 * Y ) In this, Z will be used against the actual spawn rate and is weighted to be about 1 but can be configured for a bit heavier effect.

Please, let me know what you think of this :)

BrekiTomasson commented 1 year ago

I like the idea of this PR as well, wind should definitely affect the rate of leaves falling. Rain, however; not so much. However, as @Fourmisain already said, multipliers scale hard, so it's going to be a lot of trial and error to get the numbers just right.

Fourmisain commented 1 year ago

I quickly scanned over the changes and it looks a lot better already. The math will definitely still need some nitty gritty analysis and also play testing. Currently can't really test and play around with it much since I recently upgraded my OS and it's not yet "lived in" to the point of comfort. (I just only got IntelliJ back up running and it's got a new design to get used to as well.)

multipliers scale hard

This also reminds me of the "double autumn" issue: Some trees are "autumn" trees and thus have high spawn rates. With Fabric Seasons, you can have autumn, so what's an autmn tree in autumn? A double autmn tree with twice multiplied rates! With these changes, there's now another 2 multipliers on top of that.

We might need to rethink the way spawn rates are combined (e.g. by adding instead of multiplying) or add some sort of soft bound to the spawn rates or number of leaves - it's really not clear how exactly everything should look.

I don't expect this to get into a release version soon... 😅

Fyoncle commented 2 months ago

If that PR was merged it would be cool as hell.