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

Cavegen/Mgv5/Mgv7: Add optional giant caverns #5382

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@paramat
Copy link
Member

paramat commented Mar 12, 2017

Add to MapgenBasic for use by multiple mapgens.
Add to mgv5 and mgv7.
Similar to mgvalleys caverns but half the scale.
Parameters for upper y limit, distance caverns taper to full size, and
noise threshold (full cavern size).
As with mgvalleys caverns are generated first and classic caves are
disabled in any mapchunk containing a cavern, to avoid excessive
spreading volumes of liquids.
This also avoids floating blobs of liquid where a large classic cave
has overgenerated out into a neighbouring previously-generated mapchunk.
////////////////////////////////////////////////////////

screenshot_3524895488

^ Screenshot is from my 'subterrain' mod but structure is identical to this PR

See #5380
I have noticed that the 3D noise giant caverns in mgvalleys are very popular, this PR adds these to MapgenBasic for use in any mapgen.
In mgv7 there will be no performance problems because the 3D noises for mountains and rivers are not calculated underground, so i can add a cavern 3D noise.

These are scaled smaller than the caverns in mgvalleys, big is good but 768 nodes is just too big for a cavern, the scale has been halved. Otherwise identical.

There are parameters for the upper limit of caverns (default -256), taper distance to full size (default 256) and the noise threshold (size of caverns and what proportion of the underground volume they take up).

@@ -76,9 +79,14 @@ MapgenV7::MapgenV7(int mapgenid, MapgenV7Params *params, EmergeManager *emerge)
// 1-up 1-down overgeneration
noise_mountain = new Noise(&params->np_mountain, seed, csize.X, csize.Y + 2, csize.Z);
noise_ridge = new Noise(&params->np_ridge, seed, csize.X, csize.Y + 2, csize.Z);
// 1 down overgeneration
noise_cavern = new Noise(&params->np_cavern, seed, csize.X, csize.Y + 1, csize.Z);

This comment has been minimized.

Copy link
@Zeno-

Zeno- Mar 12, 2017

Contributor

Where is this memory freed/delete'd?

This comment has been minimized.

Copy link
@Zeno-

Zeno- Mar 12, 2017

Contributor

Also, does it even have to be allocated if that noise is not ever used (it's optional isn't it?)

This comment has been minimized.

Copy link
@paramat

paramat Mar 12, 2017

Author Member

Oh crap i forgot, thanks.

This comment has been minimized.

Copy link
@paramat

paramat Mar 12, 2017

Author Member

So you're suggesting the noise objects are created according to the options, i have no idea if that's ok, maybe.

This comment has been minimized.

Copy link
@Zeno-

Zeno- Mar 12, 2017

Contributor

Yeah I am :) If it's not used then there is no point is wasting RAM. Is there?

This comment has been minimized.

Copy link
@Zeno-

Zeno- Mar 24, 2017

Contributor

What do you mean edit an earlier commit? You already have by fixing the mem leak I noticed.

This comment has been minimized.

Copy link
@paramat

paramat Mar 24, 2017

Author Member

I mean if there are 2 commits i don't know how to edit commit 1, here i fixed stuff because there is only 1 commit and i used git commit --amend.

This comment has been minimized.

Copy link
@paramat

paramat Mar 24, 2017

Author Member

Since i will need to make noise objects optional in the other mapgens it's better i do them all in 1 PR separate from this.

This comment has been minimized.

Copy link
@Zeno-

Zeno- Mar 25, 2017

Contributor

Ok then. I don't think we should just allocate memory that is not used... that's not good practice. But a separate PR is fine

This comment has been minimized.

Copy link
@paramat

paramat Mar 26, 2017

Author Member

I agree, and will fix all mapgens.

@bigfoot547

This comment has been minimized.

Copy link
Contributor

bigfoot547 commented Mar 12, 2017

Issue: #5380

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 12, 2017

bigfoot547 the issue is linked in the first post.

@paramat paramat force-pushed the paramat:mgv7caverns branch from 46dec28 to d274a8d Mar 12, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 12, 2017

Now deletes the new noise, huge thanks to Zeno-.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

Wuzzy2 commented Mar 12, 2017

I love those huge valleys caverns, I am very happy to hear they may come to v7 as well. :-)

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

2 issues:
The sheer amount of liquids flowing into the caverns, too much for my taste, but if players are happy with that then i'm ok with it. Making liquid caves rarer would not be acceptable due to how that affects gameplay away from caverns.
More importantly, floating blobs of liquid where a large cave has overgenerated into a neighbouring mapchunk, this creates a pillar of liquid in the cavern.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

I might be able to solve the 2nd issue by disallowing the classic large caves from placing nodes in air nodes, it would be pointless to replace air with air, and replacing air with liquid may not be necessary.
Air and ignore are both is_ground_content = true,

@MarkuBu

This comment has been minimized.

Copy link
Contributor

MarkuBu commented Mar 13, 2017

Liquids can be a big problem in such huge caves. This "flood" is caused by a handful of lava sources

screenshot_20170313_173150

We should rethink the liquid system. One source could never flood such an area. In the Minetest system each flowing node is in fact also a source

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

Yeah. I might be able to generate the caverns first then supress liquid caves in a mapchunk if a cavern has generated there, mgvalleys does something similar https://github.com/minetest/minetest/blob/master/src/mapgen_valleys.cpp#L750

@paramat paramat force-pushed the paramat:mgv7caverns branch from d274a8d to e4b6ff6 Mar 13, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

New version as described above:

+	// Generate caves
+	if (flags & MG_CAVES) {
+		bool has_cavern = false;
+		// Generate caverns
+		if (spflags & MGV7_CAVERNS)
+			has_cavern = generateCaverns(stone_surface_max_y);
+		if (has_cavern)
+			// Disable large caves by setting 'large cave depth' to world base
+			// Avoids too much liquid in large caverns.
+			generateCaves(stone_surface_max_y, -MAX_MAP_GENERATION_LIMIT);
+		else
+			generateCaves(stone_surface_max_y, water_level);
+	}
@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

Tested. This works well, now only rarely will a liquid cave intersect a cavern, or a liquid flow down a tunnel into a cavern, when it does happen the nature of liquid spread means there will be plenty of liquid.

@paramat paramat removed the WIP label Mar 13, 2017

@paramat paramat force-pushed the paramat:mgv7caverns branch from e4b6ff6 to 5a135f1 Mar 13, 2017

@paramat paramat changed the title Mgv7: Add optional giant caverns Mgv7: Add optional giant caverns at depth Mar 13, 2017

@paramat paramat force-pushed the paramat:mgv7caverns branch from 5a135f1 to 0c07c0a Mar 13, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 13, 2017

Seems to be ready for merge.

@paramat paramat force-pushed the paramat:mgv7caverns branch from 0c07c0a to fe291dd Mar 15, 2017

@paramat paramat added the WIP label Mar 17, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 17, 2017

WIP i need to consider the default parameters, like what proportion of ground should be cavern.
How deep should these be? 512 seems reasonable and challenging but maybe 256 makes the world more interesting earlier for players?

@bigfoot547

This comment has been minimized.

Copy link
Contributor

bigfoot547 commented Mar 17, 2017

@paramat I like y: 256. I have no idea how people even get to ores.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 17, 2017

Just thought, i can have them atart at -256 then gradually grow in size to full size at -512 so there is some kind of progression.
With these 3D tunnels it's sometimes possible to walk to -512 it's not very difficult.

@paramat paramat force-pushed the paramat:mgv7caverns branch from fe291dd to 836e7ed Mar 17, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 17, 2017

Have done that, but also added a 'cavern_taper' parameter to control the distance they taper to full size.
Still need to tune the full size.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 17, 2017

ct0p8

^ Ths is a 1-node-high slice of full cavern size with threshold 0.8, covering 4000x4000 nodes.
I think more common is needed, underground caves need to be a little more common than a map visually suggests, because it's so easy to miss caves underground. I'll try 0.7.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 20, 2017

Another dev has that view too, so no mgflat for now.
I am updating the PR to add these to MapgenBasic for use in multiple mapgens: mgv5, mgv7, mgfractal.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 21, 2017

mapbnhuj

Threshold 0.65, i'm going with this. mgvalleys uses 0.6.

@paramat paramat force-pushed the paramat:mgv7caverns branch 2 times, most recently from 85e4281 to 9419599 Mar 21, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 21, 2017

Updated with cavern generation moved in to MapgenBasic, the place for code used by multiple mapgens.
Currently works in mgv7 only, other mapgens coming.
Default disabled.
Need to see if default enabled would add these to (and therefore break) existing worlds, i would prefer default enabled.
Docs come last just before merge.

@paramat paramat force-pushed the paramat:mgv7caverns branch from 9419599 to 6c71ea2 Mar 22, 2017

@paramat paramat changed the title Cavegen/MapgenBasic: Add optional giant caverns Cavegen/Mgv5/Mgv7: Add optional giant caverns Mar 22, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

Added to mgv5. For this commit i prefer to add to mgv5 and mgv7 only, i'm unsure about mgflat and mgfractal, those can be added to easily in the future.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

If this PR sets caverns to be enabled by default, a pre-existing world will have it's mgv7_spflags overriden and caverns will be enabled in that world, breaking that world.
This can be avoided only by setting:
mgv7_spflags = mountains,ridges,nofloatlands,nocaverns
before using the new engine version with this PR merged.

I would prefer to enable caverns by default, but even if we announce this in the forum it is very likely some will not use that setting to avoid breakage.

What do you think? Safer to keep these disabled by default, at least at first? Enabling them later has less risk of breaking worlds.

Currently this PR has caverns disabled by default in both mapgens.

WIP still needs docs but is ready for review.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

Wuzzy2 commented Mar 22, 2017

I actually don't have strong feelings about this and I am not sure what to say. Maybe you should ask the forums if you are not sure.

@MarkuBu

This comment has been minimized.

Copy link
Contributor

MarkuBu commented Mar 22, 2017

This won't break the existing world. Gigant caverns only can have plain walls on chunk borders. Define it as feature and everything is ok

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

By break i mean newly generated parts of a world will have caverns while older parts will not, some users may be ok with that but it's still breakage. There will be flat walls on chunk borders as these cross many borders.
But then, mgv5 and mgv7 are not officially stable and we recently had some straight biome borders appear.

@MarkuBu

This comment has been minimized.

Copy link
Contributor

MarkuBu commented Mar 22, 2017

But then a lot of people miss this new feature. Isn't it possible to enable at new generated maps and disable on existing?

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

I tried but in testing when a new flag is added and enabled by default, it is added as enabled to old worlds too.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Mar 22, 2017

Coming in a bit late but would still like to show my opinion here:

Overall, these cavers look nice and should definitely be enabled by default for new created worlds, disabled for existing ones. Using a threshold of 0.65 looks a too frequently to me. I'd prefer 0.7 or even slightly higher to make them 1) smaller and 2) more a joy when finding a cavern.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

should definitely be enabled by default for new created worlds, disabled for existing ones.

That's what i prefer, but it can't be done automatically. I'm thinking we could warn users to use the setting above #5382 (comment) before updating their engine.

I admit i have been unsure about 0.65 or 0.7 too, the maps are a 1 node slice, seen from the side the structures are squashed by a factor of 3 so are easier to find than the map suggests. I'll go with 0.7.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

mapct0p7

^ 4000x4000x1 nodes.
Generated another map of 0.7 to be sure, this is dense enough.

@paramat paramat force-pushed the paramat:mgv7caverns branch from 6c71ea2 to 4008282 Mar 22, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 22, 2017

Updated to 0.7 and caverns enbled by default.
Only docs to do, will add once this gets some approval.

@paramat paramat removed the WIP label Mar 24, 2017

@paramat paramat force-pushed the paramat:mgv7caverns branch from 4008282 to 70b4940 Mar 27, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Mar 30, 2017

@Zeno- are you able to approve?

@SmallJoker
Copy link
Member

SmallJoker left a comment

LGTM

Cavegen/Mgv5/Mgv7: Add optional giant caverns
Add to MapgenBasic for use by multiple mapgens.
Add to mgv5 and mgv7, enabled by default.

Similar to mgvalleys caverns but half the scale.
Parameters for upper y limit, distance caverns taper to full size, and
noise threshold (full cavern size).
As with mgvalleys caverns are generated first and classic caves are
disabled in any mapchunk containing a cavern, to avoid excessive
spreading volumes of liquids.
This also avoids floating blobs of liquid where a large classic cave
has overgenerated out into a neighbouring previously-generated mapchunk.

@paramat paramat force-pushed the paramat:mgv7caverns branch from 70b4940 to bfa113d Apr 3, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Apr 3, 2017

Docs added and advanced menu tested.

@paramat

This comment has been minimized.

Copy link
Member Author

paramat commented Apr 3, 2017

@paramat paramat closed this Apr 3, 2017

@paramat paramat deleted the paramat:mgv7caverns branch Apr 6, 2017

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.