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

Add snow option to Change Weather module #682

Merged
merged 5 commits into from
Oct 26, 2022
Merged

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Sep 4, 2022

When merged this pull request will:

Notes:

  • These changes might require some edits to the rain strength slider text, in order to be more clear:
    At the moment, there is "Rain" to set the rain strength and "Precipitation" to set the type of precipitation, which makes me believe it could lead to some confusion.
    Suggestions:

  • Previously: "Rain" -> Suggested: "Precipitation"

  • Previously: "Precipitation" -> Suggested: "Precipitation Type"

  • The rainParticles can be changed (color, drop speed, etc., see setRain, but I'm unsure what options should be added to the weather module. I didn't add any, as I felt the module might become too cluttered and maybe even complicated for some to use.
    If it is wanted, so that curators can change snow particle properties, making an entirely new module just for that might be necessary.

  • I haven't done much in terms of optimisation (e.g. not comparing the current precipitation state with the new one), but I'm not sure it's worth it.

  • I have tested it in SP. As it's relatively simple, I doubt there should be any issues in MP, but I haven't tested it, so I can't say for certain if it's MP ready.

@Kexanone Kexanone added the feature Adds a new feature label Sep 4, 2022
@Kexanone Kexanone added this to the 1.13.0 milestone Sep 4, 2022
@@ -44,20 +44,61 @@ if (isServer) then {
_unit allowFleeing 0;
}] call CBA_fnc_addEventHandler;

[QGVAR(applyWeatherSnow), {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably renamed to something like "toggle snow"

@Kexanone
Copy link
Member

Kexanone commented Sep 4, 2022

Works fine in MP so far.

@mharis001
Copy link
Member

I have not looked into it yet but would using BIS_fnc_setRain be better here? The wiki implies that it helps with rainParams synchronization in MP. I agree with changing the wording to "precipitation".

I am unsure if there is much value in making all rainParams options available via a module. Maybe just providing a small set of curated "precipitation types" (with values that work well together) would be more useful. Does not need to be a part of this PR of course and can be done later.

@Kexanone
Copy link
Member

Kexanone commented Sep 6, 2022

We gonna need to have a look on what BIS_fnc_setRain actually does. I tested changes with a second client and also with JIP and both worked.

- Used 'BIS_fnc_setRain' instead of CBA event
- When precipitations parameter of CBA event is default (-1), it doesn't do anything - previously it reset weather.
- Updated translations:
    - Removed translations lacking the word "precipitation" wherever necessary.
    - Changed "Rain" to "Precipitation" in some places
@johnb432
Copy link
Contributor Author

johnb432 commented Sep 6, 2022

We gonna need to have a look on what BIS_fnc_setRain actually does. I tested changes with a second client and also with JIP and both worked.

Looking at the code of the function, it should be MP and JIP compatible. I have only tested it the latest commit (c4f7531) in SP though.

@Kexanone
Copy link
Member

Kexanone commented Sep 8, 2022

Maybe my previous comment was confusing. What I meant is that I tested your initial PR and it worked fine on dedicated with a second client and JIP as well.

On a different note, we first need to know what BIS_fnc_setRain actually does before using it. Biki says you should execute it on the server, but at the moment you are executing it everywhere and on JIPs as well.

@Kexanone
Copy link
Member

Kexanone commented Sep 8, 2022

Just checked out BIS_fnc_setRain. The function is meant to be executed on the server only and basically just remote executes setRain with JIP. Hence, I suggest we revert to the original approach with directly using setRain.

@johnb432
Copy link
Contributor Author

johnb432 commented Sep 9, 2022

Maybe my previous comment was confusing. What I meant is that I tested your initial PR and it worked fine on dedicated with a second client and JIP as well.

No, no, I understood what you meant the first time. My communication abilities are poor, so I assume I didn't express myself well enough: I have tested both options in SP only (by options I mean either CBA Events or BIS_fnc_setRain).

On a different note, we first need to know what BIS_fnc_setRain actually does before using it. Biki says you should execute it on the server, but at the moment you are executing it everywhere and on JIPs as well.

I had looked into BIS_fnc_setRain (as in looked its code) before I used it and I know you need to execute on the server to get MP and JIP compatibility. At the moment, BIS_fnc_setRain is only executed on the server as can be seen here.

Just checked out BIS_fnc_setRain. The function is meant to be executed on the server only and basically just remote executes setRain with JIP. Hence, I suggest we revert to the original approach with directly using setRain.

Ultimately, I don't care, as both options work (well, in theory, I still haven't tested the option with BIS_fnc_setRain). Maybe the option using BIS_fnc_setRain is a bit more elegant, as requires less code written in ZEN, but on the other hand, there will be something with the JIP ID "BIS_fnc_setRain" in the JIP queue, as it's never cleared, only overwritten (at least as far as I could tell). I guess it's down to personal taste.

@Kexanone
Copy link
Member

Kexanone commented Sep 9, 2022

I had looked into BIS_fnc_setRain (as in looked its code) before I used it and I know you need to execute on the server to get MP and JIP compatibility. At the moment, BIS_fnc_setRain is only executed on the server as can be seen here.

Nvm, I somehow missed that you put it in the isServer part. We can leave it as it is then.

@johnb432
Copy link
Contributor Author

I have tested it with 2 clients, one acting as the server, and by the looks of it it seems to work (MP and JIP compatible).

On a different note: The particle arguments require some tweaking. For example: Wind doesn't seem to affect snow much, even if you set the windCoeff argument of the particles higher.
I'm not sure what parameters would be best, so I appreciate any feedback/ideas.

Co-authored-by: mharis001 <34453221+mharis001@users.noreply.github.com>
@mharis001 mharis001 changed the title Weather Module - Added Snow Option Add snow option to Change Weather module Oct 26, 2022
@mharis001 mharis001 merged commit d79fa3b into zen-mod:master Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants