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

Add ability to set mapgen vertical and horizontal limits separately. #6975

Open
Megaf opened this issue Jan 27, 2018 · 20 comments
Open

Add ability to set mapgen vertical and horizontal limits separately. #6975

Megaf opened this issue Jan 27, 2018 · 20 comments
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature request Issues that request the addition or enhancement of a feature @ Mapgen

Comments

@Megaf
Copy link
Contributor

Megaf commented Jan 27, 2018

Issue type
  • Feature request
Minetest version
All
Summary

We have a fantastic option that allow us to limit the maximum map size from the centre.
However, I do believe it would be rather interesting having the ability to set y axis independently. Moreover, it would be very important to be able to adjust specific upper and lower y limits independently.

@Megaf Megaf changed the title -Feature request - Add ability to set mapgen vertical and horizontal limits sparely. [Feature request] Add ability to set mapgen vertical and horizontal limits sparely. Jan 27, 2018
@Megaf Megaf changed the title [Feature request] Add ability to set mapgen vertical and horizontal limits sparely. [Feature request] Add ability to set mapgen vertical and horizontal limits separately. Jan 27, 2018
@Ezhh
Copy link
Contributor

Ezhh commented Jan 27, 2018

Not merely interesting, but given recent problems on my server, in some cases potentially vital. I cannot effectively utilise this setting because I need an upper y limit that is more than the horizontal limit I wish enforced, but my lower y limit can be far less than this. Ideal would be:

  • horizontal_limit
  • upper_y_limit
  • lower_y_limit
    (obviously alternative names including the word "mapgen" are fine)

@SmallJoker
Copy link
Member

SmallJoker commented Jan 27, 2018

Of course we can overcomplicate everything with a bunch of new settings. But two are already enough for most cases:
minp_limit = (-3000,-2000,-3000)
maxp_limit = ( 3000, 2000, 3000)
EDIT: For compatibility, this would be as easy as: (pseudocode)
minp_limit = (-mapgen_limit,-mapgen_limit,-mapgen_limit)
maxp_limit = ( mapgen_limit, mapgen_limit, mapgen_limit)

@Ezhh
Copy link
Contributor

Ezhh commented Jan 27, 2018

I honestly don't mind how it's done if it means that flexibility is there.

@paramat paramat added @ Mapgen Feature request Issues that request the addition or enhancement of a feature labels Jan 28, 2018
@paramat
Copy link
Contributor

paramat commented Jan 28, 2018

Ahhh i hadn't thought of just specifying minp and maxp, good idea, supported. That allows any cuboid to be specified.
This should be easy to do (i'm not volunteering, i'm super busy).

@needsometech
Copy link

This would be a nice feature to have.

@Ferk
Copy link
Contributor

Ferk commented Feb 1, 2018

I'd call it mapgen_limit_minp, rather than simply minp_limit to clarify what limit it refers to (there's client_mapblock_limit , csm_flavour_noderange_limit, and possibly more)

@paramat paramat self-assigned this Feb 8, 2018
@paramat paramat removed their assignment May 15, 2018
@Megaf
Copy link
Contributor Author

Megaf commented Sep 3, 2018

Up!

@paramat
Copy link
Contributor

paramat commented Sep 4, 2018

This should be fairly simple if anyone wants to code it. I'll review it.

@paramat
Copy link
Contributor

paramat commented Sep 6, 2018

@Ezhh maybe?

@Ezhh Ezhh modified the milestone: 0.4.18 Sep 6, 2018
@Ezhh
Copy link
Contributor

Ezhh commented Sep 6, 2018

Will look at it.

@Ezhh
Copy link
Contributor

Ezhh commented Sep 6, 2018

Based on what I figured out so far, you currently enter mapgen_limit = some number in minetest.conf, and that gets copied to map_meta.txt (this is, in my opinion, a really bad approach, but that's possibly another issue). From checking with paramat, the world then ignores minetest.conf for mapgen_limit and always reads it from map_meta.txt. (Please correct me if I have misunderstood.)

If we assume we add two new settings, mapgen_limit_max and mapgen_limit_min, we need to make sure we keep compatibility for worlds using mapgen_limit. How do we want to do this?

  1. If mapgen_limit is defined, ignore the two new values (but the default value gets added for mapgen_limit in map_meta even if mapgen_limit was not defined??? (please tell me if I am wrong here, but if I'm not - bad bad bad). This makes this approach... and maybe other things... difficult.
  2. Decide that anything defined in the two new values overwrites anything defined in mapgen_limit, so we could then just choose to set the max or min values, and the rest would default to mapgen_limit, which may or may not be the default/max size.

This is... messy. I'd like some thoughts before going any further.

@paramat
Copy link
Contributor

paramat commented Sep 7, 2018

Oops this really is not 'simple' now we've looked into it.
Your first paragraph is correct, although i disagree the settings system is bad (as we have discussed on IRC).

I'm actually not sure how the settings system would cope with the backwards compatibility problem you've described, i've probably misled you.
However it doesn't much matter because the actual problem here was our initial approach which would deprecate 'mapgen_limit', very messy and complex and i disapprove of that.

@paramat
Copy link
Contributor

paramat commented Sep 7, 2018

I'm only happy to add this feature if it can be done cleanly and simply, i'm still not sure if that is possible, but here's a better approach i might be able to support:

Keep 'mapgen limit' but for horizontal only, no deprecation needed.
Add 1 or 2 new values for y, either 1 value symmetric around y, or one for above and one for below as suggested in a comment above.

Some things i realised:

Engine spawning code, and probably more, assumes that (X = 0, Z = 0) exists in a world, i don't want to make this feature unnecessarily complex by allowing a world that doesn't include those XZ co-ordinates. So keeping 'mapgen limit' and having it act unchanged horizontally seems best.

Vertically, biomes of games are very often defined around a sea level at y = 1.
At least one of our mapgens (Carpathian) only works having terrain level centred around y = 0.
Not having y = 0 in a world is unnecessary and likely to cause other unforseen complications.

So i disapprove of the minp, maxp approach. The world must continue to contain (0, 0, 0).

@Ezhh
Copy link
Contributor

Ezhh commented Sep 7, 2018

Let's say we add mapgen_y_limits = (min, max). Are we going to assume these are two non-negative numbers then add the negative via the code to enforce the inclusion of 0?

Secondly, assuming we've made sure we keep 0 y present somehow, we've now just shifted the earlier problem to y only, because both mapgen_limit and mapgen_y_limit will be written to map_meta.txt, even if using the defaults. How do we decide which of the two values to use? Just going for a "whichever is lower" would be possible, but then we're probably back to messy again. Alternatively we could check mapgen_limit against MAX_MAP_GENERATION_LIMIT and switch to mapgen_y_limit for y if those match?

@paramat
Copy link
Contributor

paramat commented Sep 8, 2018

First question yes, since the horizontal value is a positive value measured outwards from (0, 0).

Second, yes this is what i've been thinking about too. Erm, maybe set the y-limits to an unusable value like NULL, 0 or -1 by default as a signal to not use those values and use mapgen_limit in the old way (since zero is useless, it causes the central mapchunk to not generate). However i doubt map_meta.txt will store NULL.

So the 3 values checked against are first set to mapgen_limit, then if both y-limits are non-NULL / non-0 / < 0 use those for y.

I'm also wondering about whether to have 1 or 2 y-limits. It somewhat makes sense to have 2 as in a world up is often so different to down, whereas sideways is usually the same. It often only makes sense to go 1000-2000 up for mountains, while down is often to -31000.

@Ezhh
Copy link
Contributor

Ezhh commented Sep 8, 2018

Similar direction to what I've been thinking.

Independent y limits, rather than z/x, is my main interest, so I don't mind only having options for that, but I feel that unless +/- y can be distinct the worth of this drops a long way. Someone can easily want a game that doesn't involve much digging but has space exploration (I've somewhat gone this way on RC), while another might be all about the digging and therefore want depth, but very little height.

@SmallJoker
Copy link
Member

SmallJoker commented Sep 8, 2018

If we assume we add two new settings, mapgen_limit_max and mapgen_limit_min, we need to make sure we keep compatibility for worlds using mapgen_limit. How do we want to do this?

@Ezhh: KISS.

  1. Default mapgen_limit_max to (31000, 31000, 31000), then try to read from map_meta
  2. Default mapgen_limit_min to (-31000, -31000, -31000), then try to read from map_meta
  3. Try to read legacy setting mapgen_limit
    • Exists: Take the smaller area of mapgen_limit_max, mapgen_limit_min and mapgen_limit and update the min/max settings to the new value
    • Does not exist: Do not do anything
  4. Use the mapgen_limit_max/-min settings on C++ side to replace the mapgen_limit variable

Splitting the X, Y, and/or Z axis to limit the map is nonsense; it fits so easily into two position settings.

@Ezhh
Copy link
Contributor

Ezhh commented Sep 8, 2018

There is no "does not exist" case, unless my understanding is wrong. After world creation, mapgen_limit is always present in map_meta, even if you don't have it in minetest.conf (it will just take the default).

I had rejected just taking the lowest value because it didn't feel like that would be supported, but if you are fine with that, I might look at that option again.

@paramat
Copy link
Contributor

paramat commented Sep 9, 2018

Exists: Take the smaller area of mapgen_limit_max, mapgen_limit_min and mapgen_limit and update the min/max settings to the new value

Taking the smaller does not work, as sometimes you will want the larger of new and old settings.

I feel more consideration of need is necessary before going ahead. One good thing about looking at this is that we are now properly assessing it, which i didn't do before.

@paramat
Copy link
Contributor

paramat commented Sep 9, 2018

Thought some more, i think this is a useful feature and if we're going to do it it's worth doing it completely using 'minp maxp' for complete control, as long as we have the limitation of (0, 0, 0,) is always present. This has the advantage that we can use 2 vectors instead of a larger number of scalars.
I also agree to deprecating 'mapgen limit'. I think i was over-concerned about settings issues, i suspect there will be a way to solve those.

@rubenwardy rubenwardy changed the title [Feature request] Add ability to set mapgen vertical and horizontal limits separately. Add ability to set mapgen vertical and horizontal limits separately. Sep 9, 2018
@paramat paramat added the Concept approved Approved by a core dev: PRs welcomed! label Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature request Issues that request the addition or enhancement of a feature @ Mapgen
Projects
None yet
Development

No branches or pull requests

6 participants