minetest_game icon indicating copy to clipboard operation
minetest_game copied to clipboard

Implement API to disable weather

Open t-affeldt opened this issue 2 years ago • 5 comments

I implemented an API for the weather mod. This resolves #2913 .

This is not a new feature per se, it is designed to enable compatibility between MTG and weather mods that add other effects to the sky or lighting. See the respective issue for the discussion.

This also reduces the amount of flashing due to clouds updating. When a new player joins, the sky will no longer be immediately recalculated for everyone but only for the new player.

The new API methods are exposed through a global weather object and are as follows:

-- returns bool for whether weather is active
weather.is_enabled()

-- enables / disables weather globally.
-- Has no effect if enable_weather == false in minetest.conf
-- If disabled, completely stops update cycle and reset every player to default values
weather.set_enabled(enable)

-- Most important part of this PR
-- Allows for overriding this function
-- Passes player object and tables that are about to be updated
-- Return values determine actual update to player object
-- Returning nil values for a table prevents it from getting updated
weather.on_update = function(player, { clouds, lighting })

Edit: Updated this post in line with redesign. Previous iteration suggested a set_enable function per-player instead of the on_update.

Tested on Minetest 5.6.1 and 5.7.0 RC1

t-affeldt avatar Mar 10 '23 23:03 t-affeldt

I have now updated the code to use a proper set and to store players by name. Shadows should now get correctly updated in v6 and singlenode.

t-affeldt avatar Mar 11 '23 12:03 t-affeldt

Seems fine to me. Luacheck will probably complain because you're shadowing playerset though (even though IMO this shadowing is valid)...

Yes, I just got the email about the failed check. I thought shadowing is valid here because it's semantically the same. Just renamed it.

t-affeldt avatar Mar 11 '23 12:03 t-affeldt

The need to have weather.enable_on_join in addition to the API functions is pretty inconvenient. What about defining a public weather.update = function(player) that mods can override? This gives:

  • clean way for mods to disable weather, no need for synchronization on join
  • multiple mods can use this mechanism without interference (mostly)

It does not provide ability to "reset" the weather changes but that's a lost cause anyway since values set by mods cannot stack.

sfan5 avatar Mar 11 '23 12:03 sfan5

What about defining a public weather.update = function(player) that mods can override? This gives:

This would require a complete rewrite, so we should probably have this discussion before I do that.

The current implementation offers, in my opinion, the easiest way to toggle weather effects per player. The enable_on_join is sadly necessary to prevent new players from ever getting any effects in the first place.

In an ideal scenario, mod authors would not only be able to toggle the weather but also adjust it slightly without a rewrite. In order to do so, they would need to be handed the cloud and lighting tables before they get written to the player object. However, if disabling the mod is the only purpose then generating those tables would be unnecessary overhead.

So, an alternative suggestion would be the following:

weather.enabled = true
weather.on_update = function(player, { clouds, lighting })

The weather.enabled would simply toggle the weather globally.

The weather.on_update would be called after the tables have been generated and they would be passed as an argument. You could then override individual values and return the modified tables before they get written to the player object. Returning nil would instead prevent a table from getting written at all. If weather.enabled == false then this function would never be called.

Edit: It is possible that one mod wants to disable it completely and another one only temporarily (and then re-enable it). For those cases, I would note in the API doc to override the on_update method for temporary effects rather than the enabled. Alternatively, the on_update could be split into a before_update (to determine whether or not to generate the effects) and an after_update. I feel like a simple boolean is easier to understand, though.

t-affeldt avatar Mar 11 '23 13:03 t-affeldt

I have redesigned the API again. This iteration should be as flexible as possible. Overriding the on_update method can be used to disable functionality, filter by players, add additional effects, apply modifiers, etc. with not much of a hassle.

The set_enabled function can be used to completely disable the update cycle, improving performance when weather is unused.

See updated original post for details. I have also added the respective explanations to the game_api.txt

t-affeldt avatar Mar 11 '23 17:03 t-affeldt