-
Notifications
You must be signed in to change notification settings - Fork 156
Start using settingtypes.txt #665
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
base: master
Are you sure you want to change the base?
Conversation
@@ -7,3 +7,30 @@ | |||
# | |||
# Disabling this feature will sacrifice safety for convenience. | |||
technic_safe_chainsaw (Chainsaw safety feature) bool true | |||
|
|||
# Enables the mining drills. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable Mining Drill
is already shown in the settings. Players expect more information if a help icon is shown. If there is no additional information to show, then please remove these description lines.
-->
enable_mining_drill (Enable Mining Drill) bool true
enable_wind_mill (Enable Wind Mill) bool false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. I just wasn't sure what i should put there. The descriptions should be written by someone who understands technic more. I suppose I could remove them.
enable_mining_drill (Enable Mining Drill) bool true | ||
|
||
# Enables Wind Mills. | ||
enable_wind_mill (Enable Wind Mill) bool false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the technic_
prefix on all settings such that that their origin is clear. The minetest.conf file is already somewhat chaotic, thus let's not make it much worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Will do.
} | ||
|
||
for k, v in pairs(defaults) do | ||
if conf_table[k] == nil then | ||
technic.config:set(k, v) | ||
local minetest_val = minetest.settings:get(k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use technic.config:set(k, core.settings:get_bool(k, v))
where v
is the fallback. Requires Minetest 5.0.0 or newer. This is already a requirement, thus usable without further adjustments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't put the nil check for compatibility. If we call one of the core.settings functions and the setting is at it's default value then the function will return nil anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part would return nil?
Correction of my previous example:
technic.config:set(k_but_without_technic_prefix, tostring(core.settings:get_bool(k, v)))
-- can be either "true" or "false"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to recall that core.settings:get and core.settings:get_bool return nil when the setting hasn't been changed at least once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, without the 2nd argument it would indeed return nil
. However, providing the fallback argument will act as a default value.
This currently isn't finished but should be soon. I need help figuring out how to correctly generate the technic.config.