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

Map generation limit: Fix checks for block/sector over-limit #4965

Closed
wants to merge 2 commits into from
Closed

Map generation limit: Fix checks for block/sector over-limit #4965

wants to merge 2 commits into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Dec 27, 2016

Fix the maths that check if any part of a mapblock or sector is over the
set map_generation_limit.
Therefore avoid the loading of any over-limit blocks that were previously
generated when map_generation_limit was larger. The set limit can vary
for a world because it is not yet a per-world mapgen parameter, even when
it is sometimes it will be changed deliberately.
Therefore avoid a player being returned to world centre if they re-enter
a world while being over-limit.

Fix the createSector() crash caused by a mob spawning over-limit in an
over-limit mapblock
/////////////////////////////////////////////////////////
Map generation limit: Cache as 'const' not 'const static'
//////////////////////////////////////

Addresses #4923
See #4923 (comment) and onwards for an explanation of the bugs. 2 of the bugs have now been fixed in Lua.

This commit fixes the loading of over-limit mapblocks that were generated before a reduction in map_generation_limit. The maths of blockpos_over_limit() were wrong in the positive direction.

This commit also fixes the *ServerMap::createSector() crash caused by mobs spawning over-limit, this has the same maths error and is fixed in the same way.

//////////////////////////////

Mapblock minp is at n * 16, maxp is at n * 16 + 15 since mapblock zero is from 0 to 15.

In the positive direction we are checking for
mapblockmaxp > limit
n * 16 + 15 > limit
which is
p.X * blocksize + blocksize - 1 > limit
(p.X + 1) * blocksize - 1 > limit

In the negative direction we are checking for
mapblockminp < -limit
n * 16 < -limit
which is
p.X * blocksize < -limit

All variables are integers so If we work in units of nodes we can avoid division float rounding errors.

Sectors are vertical columns of mapblocks, so we can use the same maths for sector over limit.

//////////////////////////////

Tested.
The mapgen limit should probably not be 'static' but i will fix these in following PR.

@paramat
Copy link
Contributor Author

paramat commented Dec 27, 2016

With map_gen_limit is set to 300 before starting a new world this code seems to make a world too small by a mapchunk in the positive directions, with mapgen limit 300 world stops at 207 when you would expect 287.
In negative directions it stops at -272 which is expected.
I think i know why, mapgen works not only with the 5^3 block mapchunk but with a block shell around it. generating caves and dungeons up to 16 nodes beyond the mapchunk border, 287 + 16 goes over-limit while -272 - 16 does not.
So this seems correct behaviour.

When world is generated beyond 300 first then the limit set later, the world loads out to 287 as expected, blocks within the limit are loaded.

@paramat
Copy link
Contributor Author

paramat commented Dec 29, 2016

I tested the crash in *ServerMap::createSector() caused by mobs spawning at the world edge, now fixed by this commit.
So currently i'm happy with this, ready for review.

@paramat paramat changed the title Mapgen limit: Fix crash and don't load over-limit Map generation limit: Fix checks for block/sector over-limit Dec 29, 2016
|| p.Z < -map_gen_limit / MAP_BLOCKSIZE
|| p.Z > map_gen_limit / MAP_BLOCKSIZE);
return (p.X * MAP_BLOCKSIZE < -map_gen_limit
|| (p.X + 1) * MAP_BLOCKSIZE - 1 > map_gen_limit
Copy link
Member

Choose a reason for hiding this comment

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

why remove 1 from (p.X + 1) * MAP_BLOCKSIZE ? are you sure it's not (p.X + 1) * (MAP_BLOCKSIZE -1) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure, see my working in first post, it does look weird though i know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i add my maths working as a comment?

Copy link
Member

Choose a reason for hiding this comment

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

I think due to the complexity of the calcul and explanation in the code is a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

@paramat paramat Jan 6, 2017

Choose a reason for hiding this comment

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

The min nodes of blocks are at (n * mapblocksize)
So the max nodes are at (n * mapblocksize) - 1
This is where the -1 comes from.

Fix the maths that check if any part of a mapblock or sector is over the
set map_generation_limit.
Therefore avoid the loading of any over-limit blocks that were previously
generated when map_generation_limit was larger. The set limit can vary
for a world because it is not yet a per-world mapgen parameter, even when
it is sometimes it will be changed deliberately.
Therefore avoid a player being returned to world centre if they re-enter
a world while being over-limit.

Fix the createSector() crash caused by a mob spawning over-limit in an
over-limit mapblock
@paramat
Copy link
Contributor Author

paramat commented Jan 6, 2017

@nerzhul added comments, will fix 'static' in a following PR.

@nerzhul
Copy link
Member

nerzhul commented Jan 7, 2017

thanks for the comment. Please just fix the static in this bugfix too as you modify the related code part, no interest to have 2 PR here

@paramat
Copy link
Contributor Author

paramat commented Jan 7, 2017

Ok.

@paramat
Copy link
Contributor Author

paramat commented Jan 7, 2017

@nerzhul done.

@nerzhul
Copy link
Member

nerzhul commented Jan 8, 2017

Seems okay to me, i like the static const settings fix also

@paramat
Copy link
Contributor Author

paramat commented Jan 8, 2017

ddcf842
8c1b4f2

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.

None yet

3 participants