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

Fix mapgen settings in minetest.conf being ignored #9737

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Apr 24, 2020

broken since e8a8185

To do

This PR is a Ready for Review.

How to test

#9296 (comment)

@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Apr 24, 2020
@sfan5 sfan5 requested a review from paramat April 24, 2020 10:32
@paramat
Copy link
Contributor

paramat commented Apr 24, 2020

Thanks will test.
Serious bug.

@SmallJoker SmallJoker self-requested a review April 24, 2020 17:53
@SmallJoker
Copy link
Member

SmallJoker commented Apr 24, 2020

#              v-- MGV7_MOUNTAINS | MGV7_RIDGES | MGV7_CAVERNS
# defaultsettings: mountains,ridges,nofloatlands,caverns
#  + Global .conf: nomountains, noridges, floatlands
#  + Game   .conf: nomountains, ridges, nocaverns
#
#          wanted: nomountains, ridges, floatlands, nocaverns
#
# ==> Actual map_meta.txt value:
# mgv7_spflags = nomountains, ridges, floatlands, nocaverns


# minetest.conf:
mgv7_spflags = nomountains, floatlands


# game/minetest.conf:
mgv7_spflags = nomountains, ridges, nocaverns

Works as expected. So far.

-- map_meta.txt: nomountains, ridges, floatlands, nocaverns

print(dump(minetest.get_mapgen_setting("mgv7_spflags")))
-- "nomountains, floatlands" (first join)
-- "nomountains, ridges, floatlands, nocaverns" (second join)

This is so confusing but works at least better now.

@paramat
Copy link
Contributor

paramat commented Apr 24, 2020

minetest.conf:
mgv7_spflags = nomountains, floatlands

Note that this is a discouraged and sloppy way to set flags in .conf, because every flag should always be stated, with or without a preceding 'no'. The presence, or lack of, a preceding 'no' is what sets the value, not whether the flag is present.
It is better that a user states all flags so that it is clear what they are choosing.
Also, when using 'All Settings' menu, the resulting setting in .conf states all the flags.

However, it seems that we now have the proper fallback logic that, if a flag is not stated, it is not altered from the default.

@paramat
Copy link
Contributor

paramat commented Apr 24, 2020

Confirmed the bug in master, surprised and ashamed i did not notice this bug earlier.
In master:
Global mapgen flags set in .conf work fine.
Mapgen-specific flags set in .conf are ignored.
Other mapgen-specific settings, like mgv7_cave_width set in .conf work fine.

@paramat
Copy link
Contributor

paramat commented Apr 24, 2020

Tested PR, mapgen-specific flags set in .conf are now applied.
I do not understand the code here, but if SmallJoker considers the code ok i think it should be merged.

@sfan5 sfan5 merged commit eca6ee9 into minetest:master Apr 26, 2020
@sfan5 sfan5 deleted the mgsettings_fix branch April 26, 2020 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants