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

Changed map::shift to call add_roofs and compressed the code #73478

Merged
merged 3 commits into from
May 4, 2024

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented May 3, 2024

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.

@github-actions github-actions bot added Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions labels May 3, 2024
@PatrikLundell PatrikLundell marked this pull request as draft May 3, 2024 15:22
@PatrikLundell
Copy link
Contributor Author

PatrikLundell commented May 3, 2024

No. Something is wrong somewhere.
Screenshot (339)
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.

@PatrikLundell PatrikLundell marked this pull request as ready for review May 3, 2024 16:17
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label May 3, 2024
@Maleclypse Maleclypse merged commit 3a6c4bb into CleverRaven:master May 4, 2024
23 of 34 checks passed
@PatrikLundell PatrikLundell deleted the fix_shift branch May 4, 2024 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Treetops fail to generate "randomly"
2 participants