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

Make settings files per-world. #6982

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
5 participants
@red-001
Copy link
Contributor

commented Jan 28, 2018

Core changes

Settings are now loaded from world.mt on server start up, with minetest.conf settings being used as defaults.

Main menu Changes

The biggest change is a new menu based on the advanced settings menu that can be used to set per-world settings. A few small changes were also made to the "start game" tab to better support then new way that world.mt is used.

This PR replaces #6528 and is a simpler implementation, it still allows for separating client and server settings, WIP as it needs testing and some input on which client settings should be always global. It also closes #6966.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

Nice, much simpler than the previous implementation. I'd like world.mt to be renamed to minetest.conf, but that can be a different PR if you like

@red-001 red-001 changed the title [WIP] Make settings files per-world. Make settings files per-world. Jan 30, 2018

@paramat paramat added Testing needed and removed WIP labels Jan 30, 2018

@red-001 red-001 force-pushed the red-001:per_world_settings branch from aa548cc to 475fa33 Feb 2, 2018

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2018

Testing

General

  • /set and mods write to new config file.
  • Settings in minetest.conf are correctly used as a fallback.
  • Main menu changes new settings correctly.
  • g_settings is reset to minetest.conf settings on server shutdown.

World Creation

  • Settings from subgame/minetest.conf are copied to world.mt
  • enable_damage, player_backend,backend and creative_mode are copied from main settings unless the subgame overrides them.
@paramat

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

Testing:

Clicking on 'world settings' with no worlds present causes an error.

2018-02-13 23:09:16: ERROR[Main]: Main menu error: Runtime error from mod '' in callback handleMainMenuButtons(): ...er/bin/../builtin/mainmenu/dlg_settings_advanced.lua:484: attempt to index local 'conf' (a nil value)

I created world X and entered 'world settings', this still has 'return to settings page' as a button (wrong label).
I tried to edit 'mapgen flags', this caused an error.

2018-02-13 23:09:42: ERROR[Main]: Main menu error: Runtime error from mod '' in callback handleMainMenuButtons(): ...er/bin/../builtin/mainmenu/dlg_settings_advanced.lua:484: attempt to index local 'setting' (a nil value)

I made 'water level' 100 and saved this.
I entered 'advanced settings' (which are now default world settings) and set 'water level' to -100.

Started world X, water level is normal.
Looking in the world X folder there is both world.mt and map_meta.txt.
world.mt has 'water_level = 100' and map_meta.txt has 'water_level = 1' which is incorrect.

Started a second world Y to use the default settings in advanced settings menu, water level is -100 as it should be.
World Y also has both world.mt and map_meta.txt.
In map_meta txt ther is 'water_level = -100' which is correct.

'water_level = -100' also appears in minetest.conf, sems correct as that is a default set in advanced settings.

@paramat paramat added the WIP label Feb 13, 2018

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

IRC discussion http://irc.minetest.net/minetest-dev/2018-02-13#i_5230374
Explains many of the bugs in testing.

22:19 red-001 well thought I would ask you since I'm considering also merging map_meta.txt with world.mt, and mapgen is kinda your field
22:35 paramat ah hmm thanks
22:36 paramat oh it's more radical than i realised, will look over it
22:38 paramat what is the reason for merging those 2 files?
22:39 red-001 most of the C++ changes are just to avoid client settings like view range being saved in the world config
22:39 red-001 paramat, to make experimenting with mapgen flags easier
22:41 red-001 also makes it easier for someone to mess up thier world which is why I haven't implemented it
22:43 paramat will test 6982
22:45 paramat 6982 is big so needs lots of attention
22:51 paramat so i create a world, highlight it, then click 'world settings'?
22:52 red-001 that should open a modified version of advance settings with anything thats client only removed
22:53 paramat ok, it's very buggy, will reply in thread
23:14 red-001 paramat, like I said I haven't merged map_meta.txt with settings, just was considering doing so
23:17 paramat ok
23:17 paramat oh yeah misread
23:18 paramat is this why altering mapgen parameters doesn't work?
23:18 red-001 yes
23:18 paramat ok
23:18 red-001 fixed the two other issues
23:20 paramat map meta txt is already per world so setting mapgen parameters in world settings may be irrelevant, of course, duh
23:21 paramat but then you'd need to disallow changing some mapgen params in world creation i guess if they aren't going to work
23:22 red-001 well since mapgen now seems to be it's own top-level settings group I could just filter it out easily
23:23 paramat i think at the moment i prefer map meta txt stay separate and contain stuff that is already per-world, and therefore not be settable in world settings
23:23 red-001 done
23:23 paramat mapgen params would then be set in advanced settings as before

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

I see you have removed mapgen settings from the per-world settings menu but how do the settings APIs work now? I assume setting APIs act on the per-world settings (with 'get' falling back to .conf if the setting is not found in world.mt)?
But how do the mapgen setting APIs act?

We need to be very careful with this, settings behaviour is complex, subtle and a lot could break.
Mods that set settings and mapgen settings need to be tested.

Once tested this will need some docs to clarify how things work.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Screenshots?

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@paramat the PR is fairly simple, most of the C++ changes are either there to make sure that settings that should be global (e.g. keymappings) stay that way or to avoid having other code directly read or write to world.mt rewriting it to use g_settings instead. The part that settings up g_settings is pretty short and fairly self-explanatory, see https://github.com/minetest/minetest/pull/6982/files#diff-ad60d65b34e16a3319296bb5d683acd6R171

world_conf = core.settings
end

return world_conf:get(setting) or core.settings:get(setting)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Simpler: return (world_conf and world_conf:get(setting)) or core.settings:get(setting)


function set_world_setting(selected, setting, value)
local world_conf = get_world_config(selected)
if not world_conf or not value then return end

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Please use multiple lines.

for _, entry in ipairs(full_settings) do
if entry.type == "category" and entry.level == 0 then
ignore_top_level_category = true
for _, allowed_catergory in ipairs(world_settings_categories) do

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Typo: allowed_category

@@ -854,7 +859,28 @@ local function handle_change_setting_buttons(this, fields)
return false
end

local function create_settings_formspec(tabview, name, tabdata)
local function filter_world_settings()
local current_level = 0

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Unused variable

@@ -979,11 +1018,11 @@ local function handle_settings_buttons(this, fields, tabname, tabdata)
if setting and setting.type ~= "category" then
if setting.type == "noise_params_2d"
or setting.type == "noise_params_3d" then
core.settings:set_np_group(setting.name, setting.default_table)
conf:set(setting.name, setting.default)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Missing indent. Why not set_np_group?

This comment has been minimized.

Copy link
@red-001

red-001 Jun 21, 2018

Author Contributor

I think that was just an oversight.

@@ -280,6 +274,21 @@ local function main_button_handler(this, fields, name, tabdata)

return true
end

if fields["world_settings"] ~= nil then

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Superfluous ~= nil. It's never false, i.e. always either a string or nil.

if (!world_mt.readConfigFile(world_mt_path.c_str())) {
errorstream << "Cannot read world.mt!" << std::endl;
return false;
}
g_settings = &world_mt;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Pointer assigned to address of local variable. If you want to keep this, please ensure g_setting points to valid data when the function returns.

This comment has been minimized.

Copy link
@rubenwardy
set_default_settings(m_conf);
override_default_settings(m_conf, g_main_settings);
m_settings_path = path_world + DIR_DELIM + "world.mt";
if(!m_conf->readConfigFile(m_settings_path.c_str()))

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Missing space after if

if (!g_settings->getBool("disable_escape_sequences")) {
error_message = std::wstring(L"\x1b(c@red)").append(error_message)
.append(L"\x1b(c@white)");
}

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 1, 2018

Member

Is this change intended?

This comment has been minimized.

Copy link
@red-001

red-001 Jun 21, 2018

Author Contributor

no sorry that was meant for/should be in another PR.

red-001 added some commits Jan 30, 2018

Make settings files per-world.
Core changes

Settings are now loaded from `world.mt` on server start up, with `minetest.conf` settings being used as defaults.

Main menu changes

The biggest change is a new menu based on the advanced settings menu that can be used to set per-world settings. A few small changes were also made to the "start game" tab to better support then new way that `world.mt` is used.

@red-001 red-001 force-pushed the red-001:per_world_settings branch from 69f40b0 to bb8130d Jun 21, 2018

@rubenwardy
Copy link
Member

left a comment

What's the difference between g_main_settings and g_settings?

How have you tested this?

  • Have you verified that the priority order is world/world.mt, minetest.conf, game/minetest.conf?
  • Have you made sure that the only config that gets modified with in game settings is world/world.mt, eg: after /set
local function get_current_value(setting)
local value = core.settings:get(setting.name)
local function get_current_value(conf, setting)
local value = conf:get(setting.name)

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member
return conf:get(setting.name) or
		core.settings:get(setting.name) or
		setting.default
@@ -854,7 +859,27 @@ local function handle_change_setting_buttons(this, fields)
return false
end

local function create_settings_formspec(tabview, name, tabdata)
local function filter_world_settings()

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

Two things:

  • Please add a comment like -- Removes sections not applicable to world settings
  • Hard coding these sections isn't good given that mods can add their own sections. It should probably be a blacklist rather than a whitelist.

This comment has been minimized.

Copy link
@red-001

red-001 Jun 21, 2018

Author Contributor

Currently it only filters top-level catergories, aren't mod settings are all under the mods catergory? You are right it would be better to add this as an attribute to the settingtypes file.

@@ -95,12 +94,13 @@ local function get_formspec(tabview, name, tabdata)
retval = retval ..
"button[4,3.95;2.6,1;world_delete;".. fgettext("Delete") .. "]" ..
"button[6.5,3.95.15;2.8,1;world_create;".. fgettext("New") .. "]" ..
"button[9.2,3.95;2.5,1;world_configure;".. fgettext("Configure") .. "]" ..
"button[9.2,3.95;2.5,1;world_configure;".. fgettext("Configure Mods") .. "]" ..

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

I suggest calling it Mods

(we're probably limited on space here)

@@ -95,12 +94,13 @@ local function get_formspec(tabview, name, tabdata)
retval = retval ..
"button[4,3.95;2.6,1;world_delete;".. fgettext("Delete") .. "]" ..
"button[6.5,3.95.15;2.8,1;world_create;".. fgettext("New") .. "]" ..
"button[9.2,3.95;2.5,1;world_configure;".. fgettext("Configure") .. "]" ..
"button[9.2,3.95;2.5,1;world_configure;".. fgettext("Configure Mods") .. "]" ..
"button[4,5;2.6,0.5;world_settings;".. fgettext("World Settings") .. "]" ..

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

and this Settings

(we're probably limited on space here)

{
Settings conf;
if(!getGameConfig(game_path, conf) && !conf.exists("name"))
return "";

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

this fails if there is no conf and name in conf, whereas the rest of the code fallbacks to the folder name

infostream << "Initializing world at " << path << std::endl;

fs::CreateAllDirs(path);

Settings conf;
set_default_settings(&conf);
override_default_settings(&conf, g_settings);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

was this meant to change order?

conf.setBool("creative_mode", g_settings->getBool("creative_mode"));
conf.setBool("enable_damage", g_settings->getBool("enable_damage"));

SETUP_SETTING(backend);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

if the key isn't in conf, what value will this take?

This comment has been minimized.

Copy link
@red-001

red-001 Jun 21, 2018

Author Contributor

default


if (!conf.updateConfigFile(worldmt_path.c_str()))
return false;
} else {
// read settings for use by map_meta.txt
conf.readConfigFile(worldmt_path.c_str());

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jun 21, 2018

Member

conf isn't stored into g_settings after here

This comment has been minimized.

Copy link
@red-001

red-001 Jun 22, 2018

Author Contributor

that could result in g_settings getting accidentally modified by the main menu.

if (!world_mt.readConfigFile(world_mt_path.c_str())) {
errorstream << "Cannot read world.mt!" << std::endl;
return false;
}
g_settings = &world_mt;

This comment has been minimized.

Copy link
@rubenwardy
@paramat

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Seems neglected with requests unattended to. Will reopen on progress.

@paramat paramat closed this Nov 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.