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: Make per-world #5440

Closed
wants to merge 1 commit into from
Closed

Map generation limit: Make per-world #5440

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Mar 23, 2017

The setting limits map generation but affects nothing else.
Add 'mapgen_limit' to global mapgen parameters.
Move 'blockpos_over_mapgen_limit()' to the only place it is called
from: map.cpp.
Allow teleportation to any part of the world even if over the set
mapgen limit.
Simplify the reading of this limit in mgvalleys.
Remove the 'map_generation_limit' setting.
//////////////////////////////////////////////

Addresses #1608
This is a new version of #4338 i have copied some of the code by SmallJoker.

@rubenwardy
Copy link
Member

rubenwardy commented Mar 23, 2017

I think that this would be better implemented by making per-world settings, personally - as mapgen limit seems to be a setting currently, rather than a mapgen config

@paramat
Copy link
Contributor Author

paramat commented Mar 23, 2017

Mapgen limit is a setting but should not have been, it is a fundamental mapgen parameter like water_level that causes problems when not per-world and when people forget to change the setting for each world they enter.
For many settings i would agree with you but this one belongs grouped with the other basic mapgen paramters.

@paramat
Copy link
Contributor Author

paramat commented Mar 23, 2017

In testing this works.

I am not happy with how the mapgen limit setting is grabbed by passing settings_mgr.mapgen_params from the place of the call, how can we grab the value directly in mapblock.h?
Maybe the blockpos_over_mapgen_limit() function should be moved to map.cpp to avoid having to pass stuff? I'll try it.

@paramat
Copy link
Contributor Author

paramat commented Mar 23, 2017

Updated, need to test.

@paramat paramat changed the title Map generation limit: Make per-world Map generation limit: Make per-world Mar 23, 2017
@paramat paramat added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements and removed WIP labels Mar 23, 2017
@paramat
Copy link
Contributor Author

paramat commented Mar 24, 2017

Tested, adds the new parameter to existing worlds on update, whether the parameter is set non-default or not.
I'll add docs once this gets some approval.

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

Copy link
Member

@nerzhul nerzhul left a comment

Choose a reason for hiding this comment

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

okay for me

@paramat
Copy link
Contributor Author

paramat commented Mar 26, 2017

Added docs and re-added new setting to defaultsettings.cpp (oops).

The setting limits map generation but affects nothing else.
Add 'mapgen_limit' to global mapgen parameters.
Move 'blockpos_over_mapgen_limit()' to the only place it is called
from: map.cpp.
Allow teleportation to any part of the world even if over the set
mapgen limit.
Simplify the reading of this limit in mgvalleys.
Remove the 'map_generation_limit' setting.
@paramat
Copy link
Contributor Author

paramat commented Mar 26, 2017

Retested and fine.
Will make a forum post about updating the setting.

@paramat
Copy link
Contributor Author

paramat commented Mar 27, 2017

ec0c4d3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Mapgen >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants