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

Gh 47 More Leaves when Raining #49

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

cjhetzle
Copy link

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

Adding ability for wind strength to affect the spawn rate of leaves.

Weather events in Wind can now also affect spawn rate in a compounded fashion.
Need to make sure no funny business happens with bad user input.
eclipse support
Adding ability for wind strength to affect the spawn rate of leaves.

Weather events in Wind can now also affect spawn rate in a compounded fashion.

Signed-off-by: Cameron Hetzler <[email protected]>
Need to make sure no funny business happens with bad user input.

Signed-off-by: Cameron Hetzler <[email protected]>
eclipse support

Signed-off-by: Cameron Hetzler <[email protected]>
@cjhetzle cjhetzle mentioned this pull request Mar 31, 2023
@Fourmisain
Copy link
Collaborator

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' 😉

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
Copy link
Author

cjhetzle commented Apr 1, 2023

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:

  • rename leafWeatherSpawnCoeficient -> leafRainSpawnCoefficient
  • have leafRainSpawnCoefficient dependent on world.getLevelProperties().isRaining()
  • change leafRainSpawnCoefficient to a float value from 1.0-3.0f possibly
  • lastly, take a closer look into a proper value range for leafWindySpawnCoefficient

…ounding.

Wind is now heavily weighted around a multiplier of 1. To turn off with effect, set to 0. To turn off rain effect, set to 1.
@cjhetzle
Copy link
Author

cjhetzle commented Apr 1, 2023

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

Fyoncle commented Jul 2, 2024

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

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