PatrikLundell
commented
2 weeks ago No. Something is wrong somewhere.
The land in the middle of the lake should be deep water (and that's the case when compiled with a master version with just a missing constant restored).
We'll see if I can figure out what's gone wrong here. I'm not too optimistic.
Encountered this while trying to figure out what this error report is supposed to mean
Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/map.cpp:8061:48: error: unnecessary temporary object created while calling emplace_back [modernize-use-emplace,-warnings-as-errors]
8061 | loaded_grids.emplace_back( tripoint( gridx, gridy, gridz ) );
Sure, it compiles with
loaded_grids.emplace_back( gridx, gridy, gridz ); and seems to behave the same, but the compiler doesn't complain if I add another gridz, so I don't trust that.
Edit: Caused by the Empty Space PR. Deep water is empty space, and the ground beneath wanted a roof... We'll see what the testing does with the weirdo change demanded.
Summary
None
Purpose of change
Fix #73474, i.e. failure to generate treetops "randomly". Fix recent bug introduced into add_roofs: when looking for terrain to add roofs to, the ground beneath the deep water got it placed over the deep water. Reverted the logic back to what it was before Empty Space was introduced. Put it into this PR because this is what makes it apparent (of add_roofs aren't called, bugs in it won't be noticed).
Describe the solution
Change map::shift to call the loadn operation that calls add_roofs and processes all Z levels in a single call. This required a change to how the Z level loop is organized. Also changed nested 'if' logic with essentially repeated code in each to figure out which direction in which to iterate up front and get rid of the duplicated code.
Rejected calls from maps that don't support Z levels, as such calls would make little sense (and there aren't any currently), as well as offsets out of bounds (rather than just issue a debug message) to ensure nothing blows up further down due to an unsupported offset.
Describe alternatives you've considered
Also typifying the code.
Testing
Loading a save and walk in all 8 directions and look at tree tops when trees were encountered. No treetops found to be absent. Also stepped a few levels down stairs and a level up to a roof.
Additional context
I wouldn't mind if a reviewer double checked the logic for iterations and determination that submaps have to be loaded.
It can also be noted that the changed code processes submaps in a different order from before when it comes to Z levels and stores newly loaded submap coordinates in a different order. However, I don't think the Z level processing order matters, and I don't think the order in which loaded grid coordinates are processed by 'actualize' matters.
Also, I failed to see any reason the Z level cache handling would have to be done in conjunction with the actual shifting (the cache variable isn't used further), and so concluded it can all be done before the shifting, rather than integrated in the same loop.
This ought to fix all game play cases of magic treetop/roof placement associated with the loading of generated maps. Dynamic changes to maps aren't addressed until affected maps are loaded (or associated code explicitly places what should be placed, rather than relying on loading being triggered). The game play condition is made because I don't really understand how the editmap stuff works. It may or may not require something to trigger a loading of the third dimension.