The game code assumes the map goes up to Z=10, but you can't actually go higher than Z=9

[PatrikLundell]

Describe the bug

You can't actually access what should be the top level of the map.

Attach save file

8.test-trimmed.tar.gz

Note that this is actually a test case provided by @IdleSol.

Steps to reproduce

1. Building scaffolding on the top of a wind turbine (Z=9) works, because the check that the level above is free is successful (current level < OVERMAP_HEIGHT).
2. However, once constructed you can't go up, you can't look up, and the debug tool to modify the map won't allow you to access Z=10 either (I didn't try to do these things before construction, but assume they don't work then either).

Expected behavior

If the game is supposed to only reach OVERMAP_HEIGHT - 1 the code should consistently check for that. So far all checks I've encountered have been for OVERMAP_HEIGHT.

What's probably needed is a review of all usages of OVERMAP_HEIGHT (and probably OVERMAP_DEPTH as well) to ensure they all check against the same limit.

Given that the code I've seen consistently have checked against OVERMAP_HEIGHT, I suspect it might be a case of the UI having a faulty restriction. That's supported by the fact that the code didn't blow up or produce a debug report when scaffolding was constructed at Z=10.

Screenshots

No response

Versions and configuration

• OS: Windows
  • OS Version: 10.0.19045.4291 (22H2)
• Game Version: 0.G-9116-gf4014754b9 [64-bit]
• Graphics Version: Tiles
• Game Language: System language []
• Mods loaded: [
  Dark Days Ahead [dda],
  Disable NPC Needs [no_npc_food],
  Portal Storms Ignore NPCs [personal_portal_storms],
  Slowdown Fungal Growth [no_fungal_growth]
  ]

Additional context

No response

[Qrox]

The game used to crash when the character or the view moves to z=10. I don't remember why but I think it had something to do with the game not creating submaps at z=10. #71306 changed a few checks to avoid the aforementioned crash, and I think any check against OVERMAP_HEIGHT should be updated to treat OVERMAP_HEIGHT - 1 as the maximum possible z level.

[PatrikLundell]

Is there a similar issue with OVERMAP_DEPTH, or should it remain?

Edit: Changing this risks recreating the issue the reduction of the available Z level hack worked around.
It's probably better to actually figure out what that problem is and solve it, rather than adjusting everything else until it blows up again.
Now, solving the problem might involve adjust everything else except for the problem causing code, but it's probably easier and safer to address the problem itself.

Edit2:
map.cpp map::inbounds line
static constexpr tripoint map_boundary_max( MAPSIZE_Y, MAPSIZE_X, OVERMAP_HEIGHT + 1 );
looks incorrect. This is a bound, not a vector index. The +1 should go, as far as I can tell.
The same flawed logic appears in the two tinymap::inbounds operations.

Also found in overmap.cpp overmap::inbounds.

Edit 3:
Reverting the block to move to the top level and trying to move up after building scaffolding causes the game to blow up, and hooking up the debugger shows why:
lightmap.cpp map::build_sunlight_cache is called with the Z level of the PC, which is 10, and then the code tries to start at the level above with this construct:
const int zlev_max = clamp( calc_max_populated_zlev() + 1, pzlev + 1, OVERMAP_HEIGHT );
unfortunately, clamp turns out to be unsafe:

{
    return std::max( min, std::min( max, val ) );
}

as it doesn't verify that min < max, so
min(max, val) = 10, but
max(min, 10) = 11...
That's mentioned in a comment, though.

Changing the faulty line to
const int zlev_max = clamp( calc_max_populated_zlev() + 1, std::min (pzlev + 1, OVERMAP_HEIGHT), OVERMAP_HEIGHT );
allowed me to walk up to Z=10 (looking around still won't return when looking down, as I didn't revert those blocks).
If that was the only reason Z=10 was deemed unsafe it should be easily fixed, but if there are more the additional reasons would have to be identified and hunted down.