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

split map_generation_limit into x/y/z components #3199

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@EUGD
Copy link

commented Sep 23, 2015

This enables generation of non-regular (cube) shaped maps.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

👍

Haven't tested it, but it looks okay on quick inspection.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2015

Hmm, looks good to me, but I would've personally made the setting into a V3S16 instead of 3 separate settings.

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

I'm with kwoleker on using a v3s16.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 24, 2015

Well it should actually be unsigned, right? The way the pre-existing (and current) code works it expects an absolute value in distance from zero, and I don't see much reason to change that (probably even be desirable, to keep maps centered around zero, a logical uniformity with no significance to players). I don't have any real objection to bundling it up, but it will require the creation of a new getter function getV3U16() in settings.cpp / settings.h

nevermind signed is definitely best, to match with general map coords. The code treats everything as absolute value, anyway, so the actual sign won't matter. I wrote it up immediately but don't have access to my linux machine to test it myself until probably tomorrow.

also, what name would be good, for the bundled setting? i think it would be good to keep the 'map_generation_' prefix so it sits with the existing 'map_generation_limit'. something like 'map_generation_volume(x, y, z)'?

went with 'map_generation_volume (x, y, z)'.

@Zeno-

View changes

src/mapblock.h Outdated
g_settings->getU16("map_generation_limit_y")) * BS;
const static float map_gen_limit_z_bs = MYMIN(max_map_gen_limit,
g_settings->getU16("map_generation_limit_z")) * BS;
return (p.X < -map_gen_limit_x_bs

This comment has been minimized.

Copy link
@Zeno-

Zeno- Sep 24, 2015

Contributor

There is no need to surround this in parenthesis. return is a keyword, not a function.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 25, 2015

so what's up with the check results here? did i just do the commits in the wrong order, and they're really fine?

@ShadowNinja

View changes

src/map.cpp Outdated
const static u16 map_gen_limit_x = MYMIN(max_map_gen_limit,
g_settings->getV3S16("map_generation_volume").X);
const static u16 map_gen_limit_z = MYMIN(max_map_gen_limit,
g_settings->getV3S16("map_generation_volume").Z);

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 26, 2015

Member

You should only call getV3S16 once (this happens in a few other places too).

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

Do we really want to keep two seperate settings for this? If compatability really matters I'd prefer a "limit can be either a number or a v3s16" style system.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 26, 2015

as far as i know no other configuration settings work in that way, and so it would require some pretty significant new code and/or changes. honestly this is all probably really a soon-deprecated sideways implementation of what should rather be (and may sooner rather than later need to be) map metadata, anyway. EDIT: mapgen noise params work this way, so there is precedence for it. still, not sure i actually like this functionality. it's a little less intuitive than just two separate values, and it literally comes down to saving one line in a config file, from user perspective, while adding three more functions (probably never used anywhere else) to 'settings', which serve as a further step to accessing the value.

about repeated calls to the function versus storing, yeah that occurred to me but i wasn't sure how big of a deal it was or if there really was any preference. i'll change it.

@est31

View changes

src/map.cpp Outdated
const static u16 map_gen_limit_x = MYMIN(max_map_gen_limit, map_gen_volume.X);
const static u16 map_gen_limit_z = MYMIN(max_map_gen_limit, map_gen_volume.Z);
if(p2d.X < -map_gen_limit_x / MAP_BLOCKSIZE
|| p2d.X > map_gen_limit_x / MAP_BLOCKSIZE

This comment has been minimized.

Copy link
@est31

est31 Sep 26, 2015

Contributor

We need double indentation here, due to code style: http://dev.minetest.net/Code_style_guidelines

This comment has been minimized.

Copy link
@EUGD

EUGD Sep 27, 2015

Author

😝 was hoping nobody would notice. look at how beautiful the code is without this 'style'! will change back. 😢

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 27, 2015

Member

Also note that the || should be at the end of the lines.

This comment has been minimized.

Copy link
@est31

est31 Sep 27, 2015

Contributor

@ShadowNinja Do we really require that?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 27, 2015

Member

@est31: I looked through the style guide and didn't find a restriction on logic operator positioning either way, but it should still be done that way to stay consistent with the rest of the code.

@est31

View changes

src/map.cpp Outdated

const static u16 map_gen_limit_x = MYMIN(max_map_gen_limit, map_gen_volume.X);
const static u16 map_gen_limit_z = MYMIN(max_map_gen_limit, map_gen_volume.Z);
if(p2d.X < -map_gen_limit_x / MAP_BLOCKSIZE

This comment has been minimized.

Copy link
@est31

est31 Sep 26, 2015

Contributor

There is a missing space here between if and (.

@est31

View changes

minetest.conf.example Outdated
# Alternate setting for the generation of non-cubic maps.
# * (Length,Height,Width), or alternatively the sizes of each axis (X,Y,Z).
# * map_generation_limit takes precedence, when lower.
#map_generation_volume = (31000,31000,31000)

This comment has been minimized.

Copy link
@est31

est31 Sep 26, 2015

Contributor

perhaps rename this to map_generation_extent?

This comment has been minimized.

Copy link
@paramat

paramat Sep 26, 2015

Member

+1 for '.. extent'.
'volume' is a little misleading or imprecise.

(Length,Height,Width), or alternatively the sizes of each axis (X,Y,Z).

Not clear, it is actually (Length / 2, Height / 2, Width / 2) or half the size of each axis. Needs rewording. Also should make clear the vector is inverted for the negative world corner.

This comment has been minimized.

Copy link
@EUGD

EUGD Sep 27, 2015

Author

will change to 'extent' for now, but would prefer something that isn't a direct synonym of 'limit'. 'map_generation_limit' setting is already made super redundant and existing just for compatibility, naming the replacement too close seems bad.

This comment has been minimized.

Copy link
@est31

est31 Sep 27, 2015

Contributor

volume sounds bad for me because it can be misinterpreted for pregeneration.

@est31

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2015

Looks good, except comments.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

Commits need squashing.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 27, 2015

i have been trying to rebase since the second commit, can't figure it out. i think i may have permanently damaged things by deleted and re-cloning the directory? i can't find the branch, now.

well i'd thought i'd done it, but i'm not seeing any changes on this page? used 'git rebase -i 9ce30ab0e83f0caa9e0227ce2e6c5eb3bdf93ab7', changed every line but the first from 'pick' to 'squash', saved and then pushed to master (eugd/minetest). no idea what else to do now.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 27, 2015

Hm, it's best to prepare PRs in 'topic branches', not in the master branch of your fork, there are instructions in the help section pf github https://help.github.com/categories/collaborating/
I use the linux terminal acting on a local copy of my branch:

git rebase -i HEAD~22
(22 being number of commits to combine)
pick, squash, squash, squash ... (from top to bottom)
[ctrl-O] [enter] [ctrl-X] [enter]. (to execute)
(edit message)
[ctrl-O] [enter] [ctrl-X] [enter]. (to execute)
git push -f origin {branchname} ... ('-f' is a force push and is essential for rebasing)

Perhaps you missed out the -f?

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 27, 2015

Hm, it's best to prepare PRs in 'topic branches', not in the master branch of your fork

yes i think that's my problem - but will this actually outright prevent rebasing? i'll try your exact instructions again in a bit.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

james@james-Vostro-1520:~$ cd Desktop
james@james-Vostro-1520:~/Desktop$ cd minetest
james@james-Vostro-1520:~/Desktop/minetest$ git rebase -i HEAD~22
[detached HEAD 26cee8a] Update map.cpp
 Author: EUGD <******************************>
 6 files changed, 79 insertions(+), 24 deletions(-)
Successfully rebased and updated detached HEAD.
james@james-Vostro-1520:~/Desktop/minetest$ git push -f origin
warning: push.default is unset; its implicit value is changing in
Git 2.0 from 'matching' to 'simple'. To squelch this message
and maintain the current behavior after the default changes, use:

  git config --global push.default matching

To squelch this message and adopt the new behavior now, use:

  git config --global push.default simple

When push.default is set to 'matching', git will push local branches
to the remote branches that already exist with the same name.

In Git 2.0, Git will default to the more conservative 'simple'
behavior, which only pushes the current branch to the corresponding
remote branch that 'git pull' uses to update the current branch.

See 'git help config' and search for 'push.default' for further information.
(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode
'current' instead of 'simple' if you sometimes use older versions of Git)

Username for 'https://github.com': eugd
Password for 'https://eugd@github.com': 
Everything up-to-date
james@james-Vostro-1520:~/Desktop/minetest$

detached HEAD

is this my problem? that i'm operating only in a local copy, and then the push is failing?

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

but will this actually outright prevent rebasing?

No. Working in branches is the standard and highly recommended way, working in master will cause all kinds of problems and confusion for you in future.

warning: push.default is unset

I had this message too a while ago. It's just a configuration that is best set to 'simple', but it's not critical because it defaults to simple anyway.

Successfully rebased and updated detached HEAD.
~/Desktop/minetest$ git push -f origin

This should have been 'git push -f origin master' you must specify the branchname to push too, maybe this is why the message about config popped up. 'detached HEAD' isn't a problem, it looks like you rebased but failed the push by leaving out the branchname.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

james@james-Vostro-1520:~$ cd Desktop
james@james-Vostro-1520:~/Desktop$ git clone https://github.com/EUGD/minetest.git
Cloning into 'minetest'...
remote: Counting objects: 35298, done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 35298 (delta 5), reused 0 (delta 0), pack-reused 35283
Receiving objects: 100% (35298/35298), 35.97 MiB | 686.00 KiB/s, done.
Resolving deltas: 100% (24563/24563), done.
Checking connectivity... done.
james@james-Vostro-1520:~/Desktop$ git checkout -b splitlimit
fatal: Not a git repository (or any of the parent directories): .git
james@james-Vostro-1520:~/Desktop$ cd minetest
james@james-Vostro-1520:~/Desktop/minetest$ git checkout -b splitlimit
Switched to a new branch 'splitlimit'
james@james-Vostro-1520:~/Desktop/minetest$ git rebase -i
There is no tracking information for the current branch.
Please specify which branch you want to rebase against.
See git-rebase(1) for details

    git rebase <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=origin/<branch> splitlimit

james@james-Vostro-1520:~/Desktop/minetest$ git rebase splitlimit -i
Successfully rebased and updated refs/heads/splitlimit.
james@james-Vostro-1520:~/Desktop/minetest$ git rebase splitlimit -i HEAD~22
Successfully rebased and updated detached HEAD.
james@james-Vostro-1520:~/Desktop/minetest$ git rebase -i HEAD~22
[detached HEAD 0822bee] Update map.cpp
 Author: EUGD <******************>
 6 files changed, 79 insertions(+), 24 deletions(-)
Successfully rebased and updated detached HEAD.
james@james-Vostro-1520:~/Desktop/minetest$ git push -f origin master
Username for 'https://github.com': EUGD
Password for 'https://EUGD@github.com': 
Everything up-to-date
james@james-Vostro-1520:~/Desktop/minetest$ git branch -v
* (detached from refs/heads/splitlimit) 0822bee Update map.cpp
  master                                6059f1c Update mapblock.h
  splitlimit                            6059f1c Update mapblock.h

so how did i wind up with a detached HEAD, again?

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

i've noticed that this can very strongly affect map generation, especially when changing the vertical component. the generator seems to try packing all the same number of decorations into the unexpectedly limited space. it's not 'broken', except as far as it would possibly be preferrable for seed:map to be as absolute as possible, and for different generation limits to just give accurate cuts out of the same world.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Detached head is normal not an error.

the negative axes always seem to be a bit (MAP_BLOCKSIZE, 16?) shorter than the positive

That's normal because world is generated in mapchunks (80^3 nodes) that are not centered on 0, 0, 0 but on 8, 8, 8. For example the central mapchunk is -32, -32, -32 to 47, 47, 47.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

have i mistaken what the chunksize setting does? i thought that let you define the size of those mapgen chunks, with 5x5x5 merely the default setting? i have currently set it to 1 for testing this (non-cubic world), in order to escape the 80-node minimum it otherwise creates.

the next thing i might add to this, if it's not merged first, would be updating mapgen to generate/store partial chunks, so users can still have chunksize set greater than 1 to get the best effect.

have you seen what i wrote about how this pull somewhat breaks mapgen? any idea of the cause of it, or how it might be fixed? i think this is kind of a problem, although opinion may vary - IMO limiting world size shouldn't change the composition of the map itself, it should rather give a smaller cut out of a reliably uniform (per seed) map.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

i thought that let you define the size of those mapgen chunks, with 5x5x5 merely the default setting

Correct.

the generator seems to try packing all the same number of decorations into the unexpectedly limited space

Can you describe this some more? screenshots? No idea why so far.

@EUGD

This comment has been minimized.

Copy link
Author

commented Sep 29, 2015

lots of above-ground dungeons, like so;
http://i.imgur.com/iXWYfI2.jpg
also cave gen seems to be affected? that's harder for me to say with as much absolute certainty as they dungeons, though. trees are very close together on minimal case maps (single block in any direction), but again harder to judge. the dungeons are definitely EVERYWHERE, best seen if you can find an ice-plane/glacier seed.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

Ah that's expected for dungeons, one dungeon is generated per mapchunk, if you set a tiny chunksize of 1 mapblock, a size very rarely used, each generated dungeon is much more likely to overgenerate and protrude from the mapchunk.
In mgv6 the number of decorations may be fixed per mapchunk, so denser decorations would be expected. Number of caves are also be fixed per mapchunk and are of fixed size, so tiny mapchunks will create overly dense cave systems.
So essentially don't worry chunksize is usually only reduced to 4 minimum.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

If the code this commit affects has been changed by someone else since you started work on this you will end up with merge conflicts which are confusing to solve. It may be easier for you to make a text copy of your commit, restart your fork (this time keeping master branch a clean up to date copy of MT code) and make a new PR.
Wait until we make a decision though, then make the PR and we'll quickly merge it before merge conflicts arise.

For backwards compatibility limit should accept the previous single value or a vector v3s16.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

ShadowNinja commented 10 days ago

Do we really want to keep two seperate settings for this? If compatability really matters I'd prefer a "limit can be either a number or a v3s16" style system.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

Essentially edit the existing 'map_generation_limit' code to also be able to accept a v3s16.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2015

I like the idea ShadowNinja has personally. I think we should gow ith that.

@EUGD

This comment has been minimized.

Copy link
Author

commented Oct 8, 2015

I changed back to V3U16 after some thought about it (signed values would actually break, just leaving it unsigned with the warning not to exceed 31000 handles everything). Not sure if I should remove the now-orphaned V3S16-related functions in 'settings' or not.

I'm still totally unclear on just how this is operating within mapgen code, in terms of offset from zero or 8,8,8 or what (negative axes seem to be reliably one mapblock smaller than positive). So I'm just leaving the commentary vague, for now (how it already was anyway).

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

I think it should be v3s16, you are comparing against a v2s16 p2d and a v3s16 p.
Then you won't need to add getters and setters for v3u16.
v3s16 is the standard type for any world node co-ordinate.
Why does it break with a v3s16 mapgenlimit?
Don't worry about the documentation, vague is fine we can edit that later.

@EUGD

This comment has been minimized.

Copy link
Author

commented Oct 8, 2015

the comparison logic in mapblock.h/map.cpp expects an absolute value, and if you give it a negative it will 'break' by which I mean never generate anything. the original (existing) code used u16 presumably for this reason. The getters and setters for V3S16 are also added by this commit, anyway, just leaving them in orphaned now (should be removed? not sure on style standards concerning orphaned code, especially for very general utility functions like this).

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

Currently your code might cause 'comparison of signed and unsigned' warnings.
If map_gen_limit is entered by the user as negative values then i would expect nothing to generate, the documentation can be clear about using positive values.
The previous code was probably wrong to use u16 values.

@EUGD

This comment has been minimized.

Copy link
Author

commented Oct 8, 2015

OK, changed (side perk of 'orphaned' general purpose functions, I guess - leaving them in under that rationale, lacking contrary guidance).

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 9, 2015

Perhaps best remove the getters and setters for v3u16, i doubt they will be used much since v3s16 is preferred, so are just code bloat.

Enable generation of non-cubic maps
Changes map_generation_limit configuration setting and associated functions to use V3S16 type.

Updates minetest.conf.example:
	changes commentary on "map_generation_limit" to explain new functionality
Updates defaultsettings.cpp:
	changes default setting format
Updates map.cpp:
	changes operation of over-limit check to support new setting format
Updates mapblock.h:
	changes operation of in-line functions "objectpos_over_limit()" and "blockpos_over_limit()" to support new setting format
Updates settings.cpp and settings.h:
	adds new functions for getting/setting V3U16 (and V3S16), to facilitate the new setting. Adds new function "getMapGenerationLimit()" which supports different types.
@EUGD

This comment has been minimized.

Copy link
Author

commented Oct 13, 2015

Removed V3U16 functions. Anything else?

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

+1 but needs checking by someone else too.

v3s16 Settings::getMapGenerationLimit() const
{
v3s16 mgl;
if (get("map_generation_limit").find("(")==std::string::npos){

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 15, 2015

Contributor

Code style nit: operators and operands are smushed together here

}


v3s16 Settings::getMapGenerationLimit() const

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 15, 2015

Contributor

I personally don't feel as if Settings is the appropriate place for this function from an organizational standpoint.

Could it be made a static method of Map instead, perhaps??

This comment has been minimized.

Copy link
@est31

est31 Oct 15, 2015

Contributor

+1 bad place for this function.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 15, 2015

I feel this setting should be added to MapgenParams to make it per-world, but that can be done in a later commit.

mgl.Z = mgl.X;
return mgl;
}
mgl = getV3S16("map_generation_limit");

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 15, 2015

Contributor

Maybe it would be a better idea to get the map_generation_limit string as a raw string and then try parsing it both ways, separate from settings. The way it is right now is unoptimal, and also makes this function vulnerable to a TOCTOU-type problem.

@EUGD

This comment has been minimized.

Copy link
Author

commented Oct 15, 2015

i thought minetest.conf was read once at startup?

@est31

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

It can be changed afterwards. Its ok to cache often used settings, as our current settings are slow, but caching isn't vulnerable to TOCTOU.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 17, 2015

Remember before this can be merged it will need to be 'rebased agianst master' (take into account changes made by others since you started work on this, in other words make your work 'based' on latest MT code) so you will need to reset your local master branch to an exact copy of latest MT code, i'm not sure how this is done but should be easy, ask around or search git documentation. So hopefully no need to completely restart your fork.

@RHRhino

This comment has been minimized.

Copy link

commented Oct 30, 2015

This feature would be super useful! I hope that this will be added soon.
@EUGD Are you still working on it?

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 1, 2015

Needs rebasing first.

@RHRhino

This comment has been minimized.

Copy link

commented Nov 25, 2015

@EUGD seems to be busy or inactive.
@paramat @est31 @kwolekr Has someone some time to finish it?

@paramat

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

Obviously this will need taking up by someone else, if at all, but 👎 on the feature, settable limit is fine but i prefer to keep it simple and keep the world cubic.
@EUGD i'll close this in a month if no response.

@EUGD

This comment has been minimized.

Copy link
Author

commented Mar 9, 2016

I'm ultimately of the opinion that this should not be implemented as it exists, but rather as part of a much more basic reform and improvement to the map settings. However, as that seems unlikely to ever happen, just go and put it in, just to add the functionality? Either way I think it'll have to be someone else with a better understanding of git to add it (after 'rebasing it'), which I have absolutely no problem with.

@paramat

This comment has been minimized.

Copy link
Member

commented Mar 10, 2016

Ok thanks for responding. Will close then.

@paramat paramat closed this Mar 11, 2016

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

I don't have a problem with what this PR accomplishes, but last time I checked the code needed fixing before merge and the original author did not follow up on it.

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.