Fix mgValleys getSpawnLevelAtPoint() #7756

Treer · 2018-09-29T06:20:04Z

adjustedTerrainLevelFromNoise() will only add the effect of inter_valley_fill 3D noise when the noise is raising the terrain height, but sometimes the noise lowers the terrain height, and this can lower it below sea-level.

For example, on seed 1, calling minetest.get_spawn_level(-1400, 520) in Lua returns 4, but this location is ocean:

The code in this pull request is a C++ port of the fix made in Amidst, which had the results shown below, but I have not tested or even compiled this C++ code as I don't have a Minetest build environment set up (yay for Travis CI!).

(note I've assumed the terrain generation is authoritative, if the mgValleys author only intended 3D noise to raise the terrain height, then adjustedTerrainLevelFromNoise() might be correct and the terrain generation might be wrong)

adjustedTerrainLevelFromNoise() only adds the effect of inter_valley_fill 3D noise when the noise is raising the terrain height, but sometimes the noise lowers the terrain height, and this can lower it below sea-level.

paramat · 2018-09-29T06:58:30Z

@duane-r

Thanks. Mgvalleys does have an occasional spawn bug, which suggests you may be right.
I'll test this.

Good mapgen, but mgvalleys was added far too easily and the code was too messy when it was added, we've learned out lesson now and will be much more strict on new mapgens.
I still don't understand how the 3D noise is added in mgvalleys.

Implements minetest/minetest#7756

duane-r · 2018-09-29T08:32:29Z

Sorry about that, but I never intended to suggest that I understood how the algorithms worked. Gael de Sailly's theory is over my head. I just hoped to port it from lua to C++.

I tend to suspect that the terrain generation is in the right here. A lot of changes have been made to the spawn location code from the original lua, which used a very different method. But I wouldn't worry too much about it, since it's diverged quite a bit all around.

paramat · 2018-09-29T09:14:41Z

Duane, no problem, the mistake was ours and mostly mine for approving it without fully understanding the code first. I made the mistake of assuming you understood it fully and would maintain it.
We're in the weird situation of no-one fully understanding the code. It will be my task to do this (after 5.0.0).

Treer · 2018-09-29T09:44:06Z

I haven't learned the thinking behind the noise either, but I spent a lot of time staring at generateTerrain() trying to figure out why it deviates from adjustedTerrainLevelFromNoise(), and I can explain what's happening in the fix.

We can treat slope * fill as Gael de Sailly's back box :)

For every y value, generateTerrain() considers a node to be ground if slope * fill is greater than y - <preliminary_ground_height>, so if slope * fill is a negative number then nodes can go from being ground to being air/water when y has a lower value than the preliminary_ground_height.

OTOH adjustedTerrainLevelFromNoise() starts at y = preliminary_ground_height and scans up to 1000 nodes up, stopping as soon as it finds air.

The changed code scans up looking for air if the node at preliminary_ground_height is ground, and scans down looking for ground if if the node at preliminary_ground_height is air.

Reading through this old issue from 2016 suggests the behaviour may have been known, but in my mind fixing it should make it more easily able to find a spawn because adjustedTerrainLevelFromNoise() will often return lower values than it currently does, and the problem was finding locations lower than 16. There's some talk of rivers which I'm not clear about but those locations are eliminated by code in getSpawnLevelAtPoint().

paramat · 2018-09-29T19:02:18Z

src/mapgen/mapgen_valleys.cpp

+		bool was_ground = is_ground;
+		is_ground = fill * *tn->slope >= y - mount;
+		if (is_ground) result = y;
+		if (is_ground != was_ground) break;


For both these conditionals the action should be indented on the following line.

I'm not sure this will work for when the initial ground level is air, as result won't be reduced as this searches downwards. So when result is returned it will still be the initial level.

If the initial ground level is air, the result will be reduced upon finding the first ground block.

I'll clean these other things up.

Having a separate search loop for each direction and an if statement to select which one to use might have been a more easily readable way of writing it.

Yes just realised this will work. Is fine then.

paramat · 2018-09-29T19:03:08Z

src/mapgen/mapgen_valleys.cpp

@@ -393,18 +393,26 @@ float MapgenValleys::terrainLevelFromNoise(TerrainNoise *tn)
 float MapgenValleys::adjustedTerrainLevelFromNoise(TerrainNoise *tn)
 {
 	float mount = terrainLevelFromNoise(tn);
+	float result = mount;
 	s16 y_start = myround(mount);



No need for this empty line.

paramat · 2018-09-29T19:06:25Z

src/mapgen/mapgen_valleys.cpp

 	}

-	return mount;
+	return result;


result + 1 to account for biome dust that can be 1 node deep, other mapgens also + 1.

I've put the +1 in getSpawnLevelAtPoint() - it seemed appropriate there.

That's fine.

paramat · 2018-09-29T19:11:19Z

I can see you are correct about the cause of the bug.

The code of this mapgen is so overcomplex and confusing i might have to rewrite it after 5.0.0 so people have a chance to understand it :]

paramat · 2018-10-01T20:55:06Z

Code looks fine, will test.

paramat · 2018-10-02T23:16:04Z

👍 tests fine.

fix mgValleys getSpawnLevelAtPoint()

46653bb

adjustedTerrainLevelFromNoise() only adds the effect of inter_valley_fill 3D noise when the noise is raising the terrain height, but sometimes the noise lowers the terrain height, and this can lower it below sea-level.

paramat added @ Mapgen Bugfix 🐛 PRs that fix a bug labels Sep 29, 2018

Treer added a commit to Treer/Amidst-for-Minetest that referenced this pull request Sep 29, 2018

Fix terrain height bug in mgValleys

003705c

Implements minetest/minetest#7756

paramat added Blocker The issue needs to be addressed before the next release. High priority labels Sep 29, 2018

paramat added this to the 5.0.0 milestone Sep 29, 2018

paramat added this to PR in Minetest 5.0.0 blockers Sep 29, 2018

paramat reviewed Sep 29, 2018

View reviewed changes

Clean up formatting

8f75fd7

Treer changed the title ~~fix mgValleys getSpawnLevelAtPoint()~~ Fix mgValleys getSpawnLevelAtPoint() Sep 30, 2018

paramat added One approval ✅ ◻️ Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Oct 2, 2018

paramat merged commit 84a5fa0 into minetest:master Oct 2, 2018

paramat mentioned this pull request Oct 2, 2018

first spawn of player underground - valleys mapgen #3789

Closed

paramat removed this from PR in Minetest 5.0.0 blockers Oct 3, 2018

osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019

Fix Mapgen Valleys getSpawnLevelAtPoint() (minetest#7756)

f98513e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mgValleys getSpawnLevelAtPoint() #7756

Fix mgValleys getSpawnLevelAtPoint() #7756

Treer commented Sep 29, 2018 •

edited

Loading

paramat commented Sep 29, 2018 •

edited

Loading

duane-r commented Sep 29, 2018

paramat commented Sep 29, 2018 •

edited

Loading

Treer commented Sep 29, 2018 •

edited

Loading

paramat Sep 29, 2018

paramat Sep 29, 2018 •

edited

Loading

Treer Sep 30, 2018 •

edited

Loading

paramat Oct 1, 2018 •

edited

Loading

paramat Sep 29, 2018

paramat Sep 29, 2018

Treer Sep 30, 2018 •

edited

Loading

paramat Oct 1, 2018

paramat commented Sep 29, 2018 •

edited

Loading

paramat commented Oct 1, 2018

paramat commented Oct 2, 2018

Fix mgValleys getSpawnLevelAtPoint() #7756

Fix mgValleys getSpawnLevelAtPoint() #7756

Conversation

Treer commented Sep 29, 2018 • edited Loading

paramat commented Sep 29, 2018 • edited Loading

duane-r commented Sep 29, 2018

paramat commented Sep 29, 2018 • edited Loading

Treer commented Sep 29, 2018 • edited Loading

paramat Sep 29, 2018

Choose a reason for hiding this comment

paramat Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

Treer Sep 30, 2018 • edited Loading

Choose a reason for hiding this comment

paramat Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

paramat Sep 29, 2018

Choose a reason for hiding this comment

paramat Sep 29, 2018

Choose a reason for hiding this comment

Treer Sep 30, 2018 • edited Loading

Choose a reason for hiding this comment

paramat Oct 1, 2018

Choose a reason for hiding this comment

paramat commented Sep 29, 2018 • edited Loading

paramat commented Oct 1, 2018

paramat commented Oct 2, 2018

Treer commented Sep 29, 2018 •

edited

Loading

paramat commented Sep 29, 2018 •

edited

Loading

paramat commented Sep 29, 2018 •

edited

Loading

Treer commented Sep 29, 2018 •

edited

Loading

paramat Sep 29, 2018 •

edited

Loading

Treer Sep 30, 2018 •

edited

Loading

paramat Oct 1, 2018 •

edited

Loading

Treer Sep 30, 2018 •

edited

Loading

paramat commented Sep 29, 2018 •

edited

Loading