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

Weather mod needs API to disable it #2913

Closed
Zughy opened this issue Jan 1, 2022 · 10 comments · Fixed by #3112
Closed

Weather mod needs API to disable it #2913

Zughy opened this issue Jan 1, 2022 · 10 comments · Fixed by #3112

Comments

@Zughy
Copy link
Member

Zughy commented Jan 1, 2022

Quoting from minetest/minetest#11915, talking about weather mod:

I had no idea such mod existed. I looked at the code and it's just all about cycling clouds, so "weather" is a bit of a stretch

Since the introduction of set_sky() etc. functions, having a mod that only cycles clouds feels useless in my opinion. There are no atmospheric agents implemented and I doubt any mod depends from something like this. Right now is only something that messes with the set_clouds() function, and it's not like it'll be expanded anyway (as MTG is in mainteinance mode only)

@sfan5
Copy link
Member

sfan5 commented Jan 1, 2022

👎 on removing it, but it should be ensured that an API is provided for other mods to inhibit this.

@Zughy
Copy link
Member Author

Zughy commented Jan 1, 2022

I disagree, as it'd mean that every mod should take in consideration MTG (also, more work to do here). I would have agreed if MTG hadn't been the default game experience, but that's not the case. What I personally see is an unfinished mod that went deprecated by the sky PR, causing more troubles than anything.

Disclaimer: it's not a big problem for me, as I only need the sky change for my server, where there are just a couple mods from MTG (meaning that it's bothering me only when testing locally), but this can turn into a problem for people creating singleplayer mods

@sfan5
Copy link
Member

sfan5 commented Jan 1, 2022

Uh yes? Mods are largely game specific so they obviously have to take the defaults and APIs provided by the games it intends to support into account. In the same fashion you could complain that player_api claims model and animation management for its own or that map claims the minimap modes and so on.

I would have agreed if MTG hadn't been the default game experience, but that's not the case.

I don't see how that's relevant here or why it changes anything.

What I personally see is an unfinished mod that went deprecated by the sky PR

What makes you think the mod is deprecated? It changes the cloud characteristics depending on some parameters including the biome a player is in and it does that well.

@Zughy
Copy link
Member Author

Zughy commented Jan 1, 2022

True, thanks for your time sfan5. Closing

@Zughy Zughy closed this as completed Jan 1, 2022
@sfan5
Copy link
Member

sfan5 commented Jan 3, 2022

Your need to disable this is valid though so I'll keep this issue open.

@sfan5 sfan5 reopened this Jan 3, 2022
@sfan5 sfan5 changed the title Remove weather mod Weather mod needs API to disable it Jan 3, 2022
@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 3, 2022

@paramat

@appgurueu
Copy link
Contributor

appgurueu commented Apr 4, 2022

@Zughy: Would you prefer disabling the weather mod entirely (globally, for all players) or rather a more fine-grained per-player control?

I suggest the following APIs:

  • weather.enabled: boolean indicating whether the weather mod is enabled
  • weather.get_enabled(): alternative way to obtain weather.enabled, see below
  • weather.set_enabled(enabled): sets the boolean, clears everything set by the weather mod

Using metatables the setter could even be removed and replaced with just variable access. Alternatively, if the variable is considered not clean (as it may be changed without notifying the weather mod, bypassing set_enabled), a getter may be used instead. To summarize, the options here are:

  • Full func style: Getter + setter (the way it's currently done by most engine APIs)
  • A little sugar: Variable + setter (somewhat inconsistent?)
  • All the sugar: Just variable (requires metatable)

For players, this would just be parameterized with another player param, possibly exposing a table of "enabled" bools.

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2022

It needs to be per-player.

@appgurueu
Copy link
Contributor

It needs to be per-player.

Then I suggest weather.get_enabled(player) (or weather.is_enabled(player)) as well as weather.set_enabled(player, enabled)

@t-affeldt
Copy link
Contributor

I would like two throw in my two cents here as well. The bundled "weather" mod is pretty much the main reason why I stopped active development on my Regional Weather mod.

Obviously weather mods have a strong need to tie cloud density, etc. to their own active weather effects. Rain would look pretty stupid otherwise. The inclusion of the "weather" mod breaks compatibility with no easy way to resolve it.
I keep telling people to deactivate the mod in the settings but they're very well hidden and many people just download stuff in-game without looking at a readme. I've seen others try to disable the setting programmatically but this depends on load order with no way to ensure that the own mod loads before the bundled "weather" mod.

Having some kind of API to disable it per-player would be nice to have but I very much prefer a global setting (or both).
Chances are, if you need to disable this mod then you already have made some kind of replacement and will no longer need it at all. Having to iterate through the players joining just adds an unnecessary work overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants