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

Fix duplicate OMTs, take two #68398

Merged

Conversation

akrieger
Copy link
Member

Summary

None

Purpose of change

Followup to #68324. I wasn't happy with the solution and felt it deserved more documentation. I also realize I missed an else clause during world generation which could have affected what omts spawn over others (if you have sequential linear OMTs of different types). I'm not sure this is actually a case that happens but in theory there could be a linear omt that requires a predecessor and that predecessor itself could be linear (I dunno) so we should allow stacking those.

Describe the solution

Redo the logic with extra wextra comments. Make the mapgen logic and savegame deserialization logic use almost literally exactly the same code. I could've factored it into a helper function maybe but the way it would possibly mutate local or non-local state made me a bit wary to do so. I could still try though. I want to get this in front of eyes sooner though.

Describe alternatives you've considered

Testing

Loaded the old save from #67837 and validated the deserialize logic works. Created a new save in the same build, crossroads still have manholes sometimes.

You can still get 4 way streets over manholes over 4 way streets. Not sure if this means those intersections are susceptible to extra spawning, but the one I found did not seem over full.
road_over_manhole_over_road

Roads still overwrite trails correctly (presuming trails are linear and roads are linear, this means my safety net for linear-over-linear is working).
road_over_trail

And extra special verification that savegame recover works. This save had 7 duplicate omts serialized. It only pushes the one halfway through the list.
verified_recovery

Additional context

@github-actions github-actions bot added Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` labels Sep 29, 2023
@akrieger
Copy link
Member Author

This PR looks scary but that's because git diff sucks.

First commit extracts the logic for deserializing layers into the space above the main loop with no logical changes, just an ordering change. The second commit actually implements the described functionality.

@Procyonae here's that redo I mentioned in discord. Mind taking a look?

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Sep 29, 2023
@akrieger akrieger force-pushed the seven_red_lines_all_perpendicular branch 2 times, most recently from f304d91 to 2f4de78 Compare September 29, 2023 23:54
@akrieger akrieger marked this pull request as ready for review September 30, 2023 00:26
@akrieger akrieger force-pushed the seven_red_lines_all_perpendicular branch from 2f4de78 to bfe53b2 Compare September 30, 2023 00:26
@akrieger akrieger changed the title [WIP] Fix duplicate OMTs, take two Fix duplicate OMTs, take two Sep 30, 2023
@Procyonae
Copy link
Contributor

I think road over trails is the only instance of sequential linear OMTs of different types for now, manholes aren't linear, with railways potentially getting (re)added and unhardcoding trail maps #67470 it's very much a desired feature though so thanks for catching that ^^ (I'm also interested in seeing if I can make the stream mutable in innawoods #67065 work as a connection once #68389 is in, in which case that would intersect roads and trails too)
My understanding of what you've done is fairly minimal but it looks good, sorry again for causing this mess in the first place '>-<

@Maleclypse Maleclypse merged commit 45badc4 into CleverRaven:master Oct 2, 2023
21 of 25 checks passed
@akrieger akrieger deleted the seven_red_lines_all_perpendicular branch October 2, 2023 16:19
detahramet pushed a commit to detahramet/Cataclysm-DDA that referenced this pull request Nov 6, 2023
* Extract layers deserialize so information can be used for omt deduplication

* Redo linear omt deduplication with extra comments and more consistency.
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 [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants