minetest / minetest_game

Minetest Game - A lightweight and well-maintained base for modding [https://github.com/minetest/minetest/]
http://minetest.net/
Other
1.42k stars 577 forks source link

Mods cannot robustly enabled radar view in survival mode #2802

Open Oblomov opened 3 years ago

Oblomov commented 3 years ago

I have created a mod that enables radar view in survival mode provided the user has a specific item, but I noticed that this cannot be done in a robust manner with the mapping kit API as it is now.

Currently, mapping is enabled through a periodic check to verify that the player has either creative permissions (in which case both standard and radar view are enabled) or the mapping kit (in which case, the standard view is enabled, and the radar view is disabled).

A mod can change the logic by overriding the map.update_hud_flags function. In my experience, this needs to be done in a minetest.register_on_mods_loaded call, but that's not the worst of it.

The “clean” way to do the overriding would be to store the previous definition of map.update_hud_flags, call that, and then further update the flags depending on additional conditions. Example:

minetest.register_on_mods_loaded(function()
    -- override map.update_hud_flags, extending it to also check for prospecting kit
    local old_update_hud_flags = map.update_hud_flags

    function map.update_hud_flags(player)
        old_update_hud_flags(player)
        local flags = player:hud_get_flags()

        local inv = player:get_inventory()
        local has_kit = inv:contains_item("main", "prospector:prospecting_kit")

        player:hud_set_flags({
            minimap = flags.minimap or has_kit,
            minimap_radar = flags.minimap_radar or has_kit
        })
    end
end)

This allows multiple mods to extend the logic according to their own preference, BUT it has a huge downside: if any of the nested calls sets either flag to false, the UI will get updated accordingly and not restored: e.g. in my case, even if the user has the prospecting kit, on the next timer call the default logic will disable radar view, and even when it gets re-enabled by the subsequent adjustment, the user must manually switch to it again, even if it was the current mode before the time triggered.

So currently a mod that that wants to override the minimap flags without having the rug pull from under its feet needs to override map.update_hud_flags replicating the logic present in default. Which creates a problem if multiple mods try to do the same.

A simple solution would be to actually provide an in-game item to enable survival mode radar view (I have a recipe that requires 8 gold, 8 diamond and 2 mese crystals if anybody is interested), but I think a better solution would be to provide an extensible API for the minimap logic. This could be as simple as having update_hud_flags take a second, optional parameter that holds the minimap flags from the other mods, so that the logic can become something like this:

minetest.register_on_mods_loaded(function()
    -- override map.update_hud_flags, extending it to also check for prospecting kit
    local old_update_hud_flags = map.update_hud_flags

    function map.update_hud_flags(player, o)
        local override = o or {}
        local inv = player:get_inventory()
        local has_kit = inv:contains_item("main", "prospector:prospecting_kit")

        old_update_hud_flags({
            minimap = override.minimap or has_kit,
            minimap_radar = override.minimap_radar or has_kit
        })
    end
end)

and the default one doing the actual setting or the flags while still respecting overrides. This would allow robust mod chaining of the overrides.

(Of course more complex APIs are possible, e.g. by providing some kind of tables of functions with associated priorities, allowing mods to insert them in more arbitrary places in the call chain, and thus e.g. force-disable some flags if the user is near a specific block or whatever. But maybe that's something that can be thought about later.)

Oblomov commented 3 years ago

Hm I think binoculars have the same issue.

sfan5 commented 3 years ago

Hm, I'm not sure if this should be categorized as a "bug" or "feature request". But it's indeed correct that some MTG functionality assumes exclusive control over certain API features without any way to interoperate with mods.

Oblomov commented 3 years ago

Well, the map object and its methods are exposed specifically to allow mods to override the default behavior, so in some sense it does allow the game to interoperate with mods: the issue in this case is that the only interaction it's designed for is a single-shot override, while for something more robust one would need some kind of extensibility.

I have changed the logic in my mod so that the override is a bit more robust:

    -- override map.update_hud_flags to also check for prospecting kit
    local old_map_update_func = map.update_hud_flags

    function map.update_hud_flags(player)
        local flags = {}
        if player:get_inventory():contains_item("main", "prospector:prospecting_kit") then
            flags.minimap = true
            flags.minimap_radar = true
            player:hud_set_flags(flags)
        else
            old_map_update_func(player)
        end
    end

(and ditto for the binoculars), but I'm still not sure how well it would cooperate with other mods trying to do the same.

sfan5 commented 3 years ago

Game. The engine makes zero restrictions here, games/mods can freely change the state of the minimap. (unless the game messes it up like here)

ghost commented 3 years ago

Yeah, I found the void game and tested it. Too many "play like I do" design bugs in the MC clones. :(