Great news, Coolthulthu!
I salute you! Praise the sun, and your name in Cataclysmic History!
Wow this is fantastic news mate.
Again thanks so much for every thing you are doing 
Its amazing that you are finishing it and good job finding that nasty one.
The assorted PR will be a landmark in cata history.
Holy fishsticks, go Coolthulu!
Go Coolthulhu, do the thing! Your the coolest eldritch abomination!
Once, I tried a z-level branch (GitHub - BevapDin/Cataclysm-DDA at official-z-level), and stumbled across some of your problems. It’s not fully functional and a bit old. Several rebases might have caused some damage and it might not even compile anymore, this is how I tried to solve some of the problems (maybe it helps):
Most functions form "families" where changing one requires changing the rest. For example, if you add a 'z' parameter to 'move_cost', you need to update all functions using it (so that they can pass some 'z' argument) and all functions used by it (so that it doesn't just drop the z and do something stupid).
Then don’t add a z parameter. Add an overloaded function that accepts a tripoint instead of x,y:
// 2D original (unchanged):
int map::move_cost(int x, int y) {
return ter_at(x, y).move_cost + furn_at(x, y).move_cost;
}
// overload for tripoint which simply forwards to the 2D version:
int map::move_cost(tripoint p) {
return move_cost(p.x, p.y);
}Do this will all (or nearly all) of the map functions and you have a 3D interface in the map class. Of course, it ignores the z-component, but that can be fixed later.
For functions that do not actually access submaps (they only all other map functions, forwarding the coordinates), I moved the content of the 2D-function into the 3D-function (actually only changed the function declaration) and replaced all references to x,y with a single reference to p:
// ter_at and furn_at also have overloaded functions that accept a tripoint, see step above
terrain &map::ter_at(tripoint p) { ... }
// this once was the 2D original:
int map::move_cost(tripoint p) {
return ter_at(p).move_cost + furn_at(p).move_cost;
}
// This was the 3D version:
int map::move_cost(int x, int y) {
return move_cost(tripoint(x, y, 0)); // or use map::abs_sub.z as default z value
}The 2D-function now forwards to the 3D-function, nearly the same as before just the other direction. Of course, ter_at (and the other function that access submaps) will forward the 3D point back to the 2D-ter_at function (and ignore the z-value), but at this point you can use either version (3D or 2D) of a function and it will do the right thing (call the 3D version and it will forward the 3D-point).
The only functions that really need to be ported are those that access the submaps. All the other functions are straight forward replacing code ([tt]x,y[/tt] to [tt]p[/tt] and [tt]int x, int y[/tt] to [tt]tripoint p[/tt]) and adding an overloaded for 2D. At this stage the map is essentially still 2D, but the 3D interface it exposes compiles just fine and works (as long as you only request things on the default z-level).
so that it doesn't just drop the z and do something stupidFor precisely that reason, I used tripoints instead of an additional z parameter. You can't accidentally loose the z-value if you simply forward the tripoint: [code] int map::move_cost(tripoint p) { // the z-value is automatically forwarded with x and y: return ter_at(p).move_cost + furn_at(p).move_cost; // calling a 2D function is now actually more work: return ter_at(p.x, p.y).move_cost + furn_at(p.x, p.y).move_cost; } int map::move_cost(int x, int y, int z) { // Oops, I forgot to forward z to furn_at and the compiler won't warn me return ter_at(x, y, z).move_cost + furn_at(x, y).move_cost; } [/code] The 2D-functions have 1 parameter more (x [b]and[/b] y) than the 3D-functions (only p). The compiler will chose the right one based on the parameter count, you can not accidentally call the wrong one. If you call ter_at with only one parameter, it must be a tripoint otherwise the compiler will complain. If you call it with two parameters those must be ints.
Nice side effect of overloaded functions is this: once you think a specific 2D version (say map::flammable_items_at) is not used anymore and only the 3D version is used, you can remove the implementation of the 2D-function and compile. The compiler will tell you exactly where (if at all) it’s still used.
[quote=“BevapDin, post:47, topic:8935”]-snip-
Overloaded functions[/quote]
That’s brilliant! It shows how long I’ve been away from coding not to concider the possibility.
Yeah, I’m using the abs_sub.z in overloads whenever in map code, g->levz otherwise.
Also using (references to const) tripoints whenever possible.
Though I did it the opposite way: first I created a backend that loads all the submaps, now I’m in the process of adding interfaces.
The reason I’m doing it this way is mostly so that each change can be tested quickly, while also being easy to isolate.
Now that I think about it, doing it your way may be less painful. Since pretty much all functions get their submap in the same way, I could just make a dummy 3D get_submap_at that would drop the third component and later change just this one function and everything would magically start working.
“Less painful” for the players - while I’m working quite fast, there would still have to be a bunch of releases that would be slower (noticeable at map shift) without any user gain.
We should be able to carry some amount of the changes disabled at either compile time or with an option until it is optimized enough and/or there is some user-facing benefit to the changes.
I’m not clear why you need “a backend that loads all the submaps”, the current reality bubble system should be easily tweaked to do what you want.
[quote=“Kevin Granade, post:50, topic:8935”]We should be able to carry some amount of the changes disabled at either compile time or with an option until it is optimized enough and/or there is some user-facing benefit to the changes.
I’m not clear why you need “a backend that loads all the submaps”, the current reality bubble system should be easily tweaked to do what you want.[/quote]
Compile-time disable should be doable.
Clarifying: I changed the current reality bubble system to load an entire “stack” of submaps (from lowest to highest)
By “backend” I meant the technical part of the issue that won’t be accessed directly by most developers.
From what I understood from the discussion in https://github.com/CleverRaven/Cataclysm-DDA/issues/6818 it’s OK to load all the submaps and z-levels shouldn’t be shifted.
Sorry for the noise, finally had a chance to dig into your PR and I see what you mean, everything looks great so far. I thought you meant replacing mapbuffer, but looks like you didn’t change it other than implementing the uniform submap feature.
BTW, if load times for all those submaps become a problem, what I was planning on doing was making the number of submaps loaded (not the reality bubble, just the submaps cached in mapbuffer) larger, but making the actual load operation asynchronous by pushing load/save operations onto a workqueue and processing quads one at a time while waiting for player input. That way you’d have at least 12 turns of player input to catch up after a shift (or more if you put two “rings” of quads onto your preload queue instead of one, but at some point you’re just loading/saving constantly and hardly using any of the loaded data). Additionally you could prioritize loads over saves, and just develop a “tail” of still-loaded submaps that only get written out if the player stops hitting keys for a bit. Plus if the player is moving back and forth quickly you’d still have them loaded.
I didn’t implement this when I moved to loading/saving submaps in quads instead of all at once back in 0.B because it would have been premature optimization. It’s certainly more complicated, and I couldn’t see any load/save pauses after making load/save incremental. I had the same approach in mind in case submap loading ever became super expensive due to running catchup operations on them when they come in range. (field dispersion, rot, monster evolution, etc)
Got to admit I am getting extremely excited over this :).
Thanks again
I’ll bump because I didn’t get any info for a longer while.
I have 2 PRs waiting. First is https://github.com/CleverRaven/Cataclysm-DDA/pull/11445
Second (currently unmergeable, but was mergeable yesterday and for a week before) https://github.com/CleverRaven/Cataclysm-DDA/pull/11472
Is there any particular reason they didn’t get merged? I’d fix them, but I don’t know what’s wrong.
Also, is the part 1 supposed to include removing all 2D overloads and replacing them with 3D variants? That would be quite a bit of code and would make the part 1 bigger (by number of lines of code changed) than all the other parts put together. I personally would prefer to change them when it would be actually useful rather.
The only holdup is me having time to review and test the code, it’s looking great when I have time to look.
I’ll bump once again.
Did another batch of overloads here: https://github.com/CleverRaven/Cataclysm-DDA/pull/11705
I can’t really go further than that at the moment.
Please merge at least some the PRs, I can’t really work here when any non-trivial change to map code requires me reviewing and changing quite a bit of mine. This is heavily slowing down the developments of the z-levels and if not for it, I’d be testing something like spitters spiting at birds over their heads right now.
I’m merging the trap PR right now, unfortunately it accumulated a number of conflicts so it’s taking a bit of work 
(not blaming anyone, It’s pretty much my fault for not getting it done sooner)
Thanks
Should I update (ie. make mergeable to the new master) the other PRs as soon as this one is merged or do you want them to stay in their current state?
Merged, and it will probably be easier for you to bring them up to date than it will be for me.
EDIT: BTW, the main conflicts I saw were g->levz => g->get_levz() and the removal of map::load(bla, bla, overmap);
Updated https://github.com/CleverRaven/Cataclysm-DDA/pull/11472
EDIT: Also updated (and tested if works) https://github.com/CleverRaven/Cataclysm-DDA/pull/11705