Add in-game configuration inventory tab

[Ekdohibs]

For now, only the options enable_tnt, enable_fire, and flame_sound are in it; I'm open for more suggestions.
Still need to do:

• Split the setting page into several ones if too long
• Allow "number" setting type
• API doc

This pull request enables both TNT and fire by default, even in multiplayer. However, it is now very easy to disable them.

[benrob0329]

+1

[sofar]

It's nice to see an idea take shape this quickly.

• indentation is off for a few lines (too many tabs)
• should probably squash the config mod to one commit, but make a separate commit for enabling the use in tnt and fire
• separate luacheck commit (and merge it asap, since trivial)

[Ekdohibs]

@sofar this should be fixed now.

[Fixer-007]

Is there any useful options that can be added to those?

[Ekdohibs]

@Fixer-007 probably, but I am not sure which options for now. However, with the config API, it will be very easy to add them anyway if we see we missed an option, so it can just go in a later PR.

[paramat]

Is this being done for good reasons other than the recent controversy? Yet another settings method needs a good reason, we already have 2 levels of settings.
Game settings are already settable in advanced settings and they are not hard to find.
The issue about tnt and fire options is not about ease of setting, but of warning people about them being default-enabled.
This should be done for reasons other than the recent controversy, otherwise it's a hasty reaction.

[sofar]

This solves several problems at once:

• per-world settings instead of settings for all worlds are possible in this matter
• settings have a chance of taking effect immediately
• easier to check and see your existing settings
• works with remote servers without console access (yes, this is actually a problem for some) or people on a terminal-unfriendly platform (windblows?)
• changing settings can be done by anyone with server permissions, not just the local admin

I think this is a good step forward to making working with servers a lot more simple.

[davisonio]

I'm open for more suggestions.

One for disabling/enabling lava cooling would be great. I have to delete the chunk of code every time I update for my server.

[paramat]

disabling/enabling lava cooling

That's probably too specialist. Hopefully we will replace that ABM with something less intensive soon.

[SmallJoker]

This topic looks somewhat dry, here an image to visualize how it will look like:

+1 for the concept; haven't looked at the code yet.

[rubenwardy]

I think it would be better to:

• Implement set_setting and /set callbacks (minetest.register_on_set_setting)
• Register mod settings using one of:
  • config.register_setting wouldn't be too bad
  • minetest.setting_register(name, type, default, supports_runtime_change)
  • settingtypes.txt for mods
• Implement this mod by looking for all registered settings for mods, and minetest.setting_set

[Ezhh]

I'd also love an easy option to disable lava cooling (or any replacement for it). It was the second thing I tried to work out after setting up my first server, right after stopping fire from spreading, so I don't think it's too specialist to add.

[tenplus1]

Why are we adding another settings page when one already exists in the main menu ? This is not needed and just overcomplicates mods relying on checking it's settings.

[paramat]

Perhaps i do not realise something, why is switching lavacooling important to server admins?

Fire sounds are not important enough to be here, that is a specialist setting for those who write mods to provide their own sounds.

[Fixer-007]

Maybe to prevent griefing via lava-water interaction and control cobble generators.

[sofar]

Why are we adding another settings page when one already exists in the main menu ? This is not needed and just overcomplicates mods relying on checking it's settings.

@tenplus1 because these settings are properly per world and not for all worlds.

[sofar]

@rubenwardy I can see your ideas, but I really don't think a minetest namespace is needed, or even wanted.

I actually like that this is a separate namespace and can live entirely outside of any mod or subgame by itself, just as it should be.

Implement this mod by looking for all registered settings for mods, and minetest.setting_set

That' is just a terrible idea. The idea of making this in any way backwards compatible makes me more uncomfortable than a parent with a kid with a balloon in Jushua Tree National park.

[rubenwardy]

The current solution only works for mods you add support for it for, just like my suggested solution. Mods would opt in in a similar way.

This solution feels like a "hack" to me, when proper world and setting change support should be in the engine - where the setting API is

[ghost]

This configuration tab with api would be very useful for mods to get per player in game settings in one tidy spot. I have a mod that resorts to overriding the crafting page to add a checkbox for a setting. Having a uniform location of all mods to hook in to would be excellent.
Seems a waste of space if this tab is for server priv use only.

[paramat]

The config API allows accessing and setting per-world configuration options that can be
changed in multiplayer games by players having the server privilege using the configuation tab.

This solves several problems at once:
per-world settings instead of settings for all worlds are possible in this matter
settings have a chance of taking effect immediately
easier to check and see your existing settings
works with remote servers without console access (yes, this is actually a problem for some) or people on a terminal-unfriendly platform (windblows?)
changing settings can be done by anyone with server permissions, not just the local admin

The intention seems fine, but this sort of significant configuration feature should be in the engine, otherwise every subgame will then need to duplicate code.
The last thing we need is lots of unnecessary code added to MTG.

The fire controversy, although the discussion was er, lively, is not important enough to be a primary justification fo this, this feature should be considered mainly for general reasons. It still seems this is a rushed 'kneejerk reaction' to that.

I feel this needs to be in the engine and reconsidered. I'm concerned having about another settings implementation, perhaps this can be integrated more into what we aread have?
👎

[paramat]

We already have a setting architecture and this adds extra 'register setting' code that has to be added to mods, it is messy.
Concerning the intent to make changing subgame settings more easily, i can imagine a better approach in the engine:
Use the existing 'settingtypes.txt' feature and add a field that flags a setting for inclusion in a new 'Subgame basic settings' menu.

Concerning 'per-world configuration', this is a powerful feature that should obviously be in the engine, it would probably have been added to the engine anyway without the fire and TNT controversy.

The fire and TNT issues should be forgotten here, this PR somewhat misses the point.
The actual fire and TNT issues are making server admin aware of the dangers, aware of the settings, and choosing how the defaults are set dependent on singleplayer / multiplayer.
Ease of changing the settings is not a big issue as subgame settings are easily found in the subgame section in advanced settings menu. A server admin will usually have enough ability to find and change settings in that menu.

In terms of the fire and TNT issues i would even prefer #1694 "TNT: do not change behavior based on singleplayer gameplay" to this PR. At least 1694 is simple. I do support 1693 but with 3 MTG devs against that is pretty much blocked.

All that has been added to this PR is fire and TNT settings, this suggests this is indeed a hasty and hacky reaction to that controversy and does not have much use outside those settings.

[sofar]

We already have a setting architecture and this adds extra 'register setting' code that has to be added to mods, it is messy. Concerning the intent to make changing subgame settings more easily, i can imagine a better approach in the engine: Use the existing 'settingtypes.txt' feature and add a field that flags a setting for inclusion in a new 'Subgame basic settings' menu. Concerning 'per-world configuration', this is a powerful feature that should obviously be in the engine, it would probably have been added to the engine anyway without the fire and TNT controversy. The fire and TNT issues should be forgotten here, this PR somewhat misses the point. The actual fire and TNT issues are making server admin aware of the dangers, aware of the settings, and choosing how the defaults are set dependent on singleplayer / multiplayer. Ease of changing the settings is not a big issue as subgame settings are easily found in the subgame section in advanced settings menu. A server admin will usually have enough ability to find and change settings in that menu. In terms of the fire and TNT issues i would even prefer #1694 "TNT: do not change behavior based on `singleplayer` gameplay" to this PR. At least 1694 is simple. I do support 1693 but with 3 MTG devs against that is pretty much blocked. All that has been added to this PR is fire and TNT settings, this suggests this is indeed a hasty and hacky reaction to that controversy and does not have much use outside those settings.

You know, I'm reading this and I'm just shaking my head. The engine already supports a special API especially made for this, and this pr actually uses it. You can't register a LuA callback from a txt file, either. The whole discussion also started because it evidently *is hard to change* these settings because otherwise nobody would have asked to disable fire and TNT in the first place. And now it is irrelevant? Geesh

[Ekdohibs]

@paramat As sofar said, you can't register a callback from a text file. Moreover, these settings should clearly be per-world but they are not right now, and the boilerplate code for doing per-world settings makes that few modders do it.
I would agree with moving this to core (well, builtin), but it means moving sfinv there as well: do we really want that?

[Ferk]

I would agree with moving this to core (well, builtin), but it means moving sfinv there as well: do we really want that?

You could add a per-world settings API to the engine but leave the logic that creates the inventory tab to MTG.

Imho, the admin inventory tab is a good idea. And I think it would be more powerful if the per-world settings were used by the engine as well.

Correct me if I'm wrong, but I don't think it's currently possible from a mod to change engine settings for a world (eg: "time_speed", "item_entity_ttl", "movement_speed_walk"...), without writing into the global minetest.conf which would affect every single world.

Some of that kind of settings might also be interesting for the admin window, imho.

Subgames can do this because there's a per-subgame minetest.conf.
Imho, there should be a per-world minetest.conf in the same way, and mods should be able to decide to write into the per world one instead of in the global one when using minetest.setting_set.

[paramat]

The engine already supports a special API especially made for this, and this pr actually uses it.

Ok, it must be a small part of this as most of this is setting architecture added to a subgame, which is not good.

You can't register a LuA callback from a txt file, either.

What i mean is:
Currently engine code reads settingtypes.txt and constructs an advanced setting menu from that. An extra text field could be added to settingtypes.txt like " basic" which when read makes that setting also appear in a new 'basic subgame settings' menu.
Although i'm not necessarily supporting this as i'm unsure it is needed.

The whole discussion also started because it evidently is hard to change these settings

Some said that but i did not and disagree that that is the issue.
The actual isses are: making server admin aware of the dangers, and, making them aware of the settings.
If these are not done it doesn't matter how easy it is to change the settings, because they will not be aware of the danger or the existence of the settings.
At the same time you make them aware of the setting you can direct them to the settings in advanced settings menu, with direction they are not at all hard to find, so then you also don't need to add 'easy to change settings'.

[paramat]

Ekdohibs, this will sound odd but, where is the code for in-game standard inventory? Is it in creative mod or in builtin? I can't find it yet. It seems sfinv should be wherever inventory code is.
If sfinv needs to be in MTG then that should not stop this feature moving to engine, and because fire and TNT issues should be removed from this PR then there is even less reason this should be in MTG.

This solves several problems at once:
per-world settings instead of settings for all worlds are possible in this matter
settings have a chance of taking effect immediately
easier to check and see your existing settings
works with remote servers without console access (yes, this is actually a problem for some) or people on a terminal-unfriendly platform (windblows?)
changing settings can be done by anyone with server permissions, not just the local admin

^ This is fundamental setting stuff that should be in buitin, otherwise every subgame needs to duplicate it.

[paramat]

I'd also love an easy option to disable lava cooling (or any replacement for it).

I'm ok with that, but it should just be another normal configuration settting in MTG. I guess it's to make cleanup easier by avoiding stone being created.
Any more support for this? (2 server admin so far).

[rubenwardy]

I like the idea of this, just not the current implementation :P

[Fixer-007]

I would like to see per world settings, I'm tired of my minetest.conf polluted with dreambuilder settings and other.

[rubenwardy]

I think settings set using minetest.setting should be default per world. I can't see any use cases for global settings that aren't abuses.

[paramat]

@davisonio #1726

[sofar]

I think settings set using minetest.setting should be default per world.

This is never going to get fixed unless someone rewrites this entire API/code block.

[paramat]

This does not need doing for release, and is still very controversial.

[paramat]

TNT issue is now settled, so that needs removing from this PR.
Please remove the flame sound option from this too, there's no need for yet another setting method for that.
👎 I'm still opposed for reasons explained above.
If this is actually justified (which i'm unsure about) why not move sfinv and this to builtin? It is useful for all subgames and should not be duplicated in every subgame.

per-world settings instead of settings for all worlds are possible in this matter

Per-world settings are useful, but should be implemented in builtin, obviously.

Probably needs adoption as @Ekdohibs is absent.

[ghost]

I don't understand why I would want settings on my inventory but okay. Why not just make a /settings command?

[rubenwardy]

👎 in its current form. This needs engine callbacks for setting changes.

[paramat]

sofar has stated he's not interested in MTG anymore, PR has no activity and and this has 2 disapprovals.
Feel free to continue discussion here but closing.