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: Fix crash when getting biome outside mapchunk #7480

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Cavegen: Fix crash when getting biome outside mapchunk #7480

merged 1 commit into from
Jun 26, 2018

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Jun 24, 2018

Cavegen: Fix errors when getting biome outside mapchunk

Some cave segments are outside the mapchunk.

Previously, biome was being calculated by a function that uses the noise
maps. Points outside the mapchunk resulted in incorrect noise map indexes
that were sometimes outside the noise map size, causing a crash.

Use either noise maps or point noise calculations depending on point
location.
////////////////////////

Attends to #7272 (comment) and #7272 (comment)

Cavegen gets the biome for each cave segment in order to place biome-defined cave liquids.
See bb3baef , this caused the bug and the crashes. This PR is a partial revert of that.

calcBiomeAtPoint() was used originally, that function can be used for any point, inside or outside the currently generating mapchunk.

getBiomeAtPoint() is used in current master. It can only be used for X, Z values inside the currently generating mapchunk because it uses the heat and humidity values from the noise maps for the currently generating mapchunk. However it is faster as it does not calculate noise.

I just realised that cavegen generates out beyond the mapchunk borders so occasionally the biome is being 'got' for a point just outside the mapchunk.
getBiomeAtPoint() calculates the index for the noise maps:

Biome *BiomeGenOriginal::getBiomeAtPoint(v3s16 pos) const
{
	return getBiomeAtIndex(
		(pos.Z - m_pmin.Z) * m_csize.X + (pos.X - m_pmin.X),
		pos);
}

For a point outside the mapchunk this mostly results in a weird index value that accesses an incorrect position, but sometimes it can result in a negative index or an index larger than the noisemap size, that probably causes rare crashes and probably caused #7272 (comment)

This PR checks X, Z values and uses the fast function if inside the mapchunk and the slower one if outside.

@paramat paramat added Bug Issues that were confirmed to be a bug Blocker The issue needs to be addressed before the next release. @ Mapgen WIP High priority labels Jun 24, 2018
@paramat paramat added this to the 5.0.0 milestone Jun 24, 2018
@paramat paramat added Bugfix 🐛 PRs that fix a bug and removed Bug Issues that were confirmed to be a bug High priority labels Jun 24, 2018
Some cave segments are outside the mapchunk.

Previously, biome was being calculated by a function that uses the noise
maps. Points outside the mapchunk resulted in incorrect noise map indexes
that were sometimes outside the noise map size, causing a crash.

Use either noise maps or point noise calculations depending on point
location.
@paramat paramat added Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines and removed WIP labels Jun 26, 2018
@paramat
Copy link
Contributor Author

paramat commented Jun 26, 2018

Tested.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@paramat paramat merged commit 93661ca into minetest:master Jun 26, 2018
@paramat paramat deleted the fixcavebiome branch July 16, 2018 02:55
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
Some cave segments are outside the mapchunk.

Previously, biome was being calculated by a function that uses the noise
maps. Points outside the mapchunk resulted in incorrect noise map indexes
that were sometimes outside the noise map size, causing a crash.

Use either noise maps or point noise calculations depending on point
location.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker The issue needs to be addressed before the next release. Bugfix 🐛 PRs that fix a bug @ Mapgen One approval ✅ ◻️ Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants