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 neighbour tests on rotated mapgen #51913

Merged
merged 14 commits into from
Sep 29, 2021

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Sep 27, 2021

Summary

Bugfixes "Fix neighbour tests on rotated mapgen"

Purpose of change

We have infrastructure to allow nested chunks to be placed dependent on which terrains are adjacent to the one being generated, and which joins were linking it to those terrains in a mutable special.

However, neither of these took into account map rotation, so a connection that the map authour thought should connect to the north might connect at a different edge when that map is rotated, but still depend on the neighbour terrain to the (unrotated) north.

This made these features very difficult to use in practice. You had to use them only with terrain that did not rotate.

For example, @John-Candlebury had trouble with rotations when attempting to use this feature for the Modular Habitat System in #49849.

Describe the solution

This PR includes the changes from #51848 and fixes the known bug I mentioned therein.

Rotate all the data in mapgendata so that the directions are interpreted relative to the mapgen's _north orientation, rather than relative to 'true' north.

To help with debugging this I also added the OMT rotation value to the debugging overmap UI.

I discovered that the ants_end mapgen was backwards while testing this, and that is also fixed in this PR.

I renamed direction_XY to displace_XY and added a displace overload for direction, for consistency with similar functions for om_direction and cube_direction.

Lastly, I added operator+(cube_direction, int). This isn't actually used in the final version (it was in an earlier revision) but it makes sense for it to exist so I have left it in.

Describe alternatives you've considered

None.

Testing

I used ant tunnels for all my testing, since I've been working with them a lot lately. I added some up slope chunks as markers at each of the edges of the ants_food map based on their respective neighbours / joins.

For neighbours, here is an example of the behaviour before with the wrong edges marked:
neighbour-check-before
here are some examples of the behaviour afterwards, working correctly:
neighbour-check-after
neighbour-check-after-2

(you can also see the incorrect ants_end mapgen in the above images, which was later fixed)

For joins, I used the same technique. We should see only one marker in each case, by the entrance. Here is an example before:
join-checks-before
and an example after:
join-checks-after

Additional context

To be able to use these types in regular mapgen they need to be promoted
to headers.
When a mutable special is placed, keep a record of every join used and
store that in the overmap.  Display this information in the overmap
editor to aid with debugging.
We want you to be able to specify the chosen chunk as a mapgen value.
Similarly to neighbourhood checks in chunk placement, add join checks to
chunk placement.
This map was oriented the opposite way round from what it should have
been.
There used to be a function called direction_XY that converted a
direction to a point.  This didn't match the naming scheme used for
analogous functions for om_direction and cube_direction.

Rename it to displace_XY and also create a tripoint version displace.
@BrettDong BrettDong added [C++] Changes (can be) made in C++. Previously named `Code` Map / Mapgen Overmap, Mapgen, Map extras, Map display labels Sep 27, 2021
@anothersimulacrum anothersimulacrum added the <Bugfix> This is a fix for a bug (or closes open issue) label Sep 27, 2021
@@ -229,6 +229,10 @@ struct tripoint {
return point( x, y );
}

tripoint rotate( int turns, const point &dim = { 1, 1 } ) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tripoint rotate( int turns, const point &dim = { 1, 1 } ) const {
/* Rotates just the x,y component of the tripoint. See point::rotate()
* NOLINTNEXTLINE(cata-use-named-point-constants) */
tripoint rotate( int turns, const point &dim = { 1, 1 } ) const {

Previously the mapgen feature of placing chunks based on neighbours used
the absolute cardinal directions on the map.  This is unhelpful when the
map is rotated.

Rotate the neighbours to match the rotation of the map so that these
checks make sense for the mapgen.
For symmetry with the - overload.
Previously these checks were based on absolute map directions.  After
this change they are instead rotated with the map, so that they are
easier to use logically within the map.
It can be useful to know the rotation of a particular tile for debugging
purposes.  This may not be obvious for linear terrain.  Add it to the
debugging output in the overmap editor.
@jbytheway jbytheway force-pushed the neighbour_test_rotations branch from e211d00 to b1b5758 Compare September 27, 2021 20:40
Turns out the new microlab mapgen violates this check, intentionally, so
it's too strict for now.  We might be able to re-add it later.
@jbytheway jbytheway force-pushed the neighbour_test_rotations branch from f59dd46 to a615851 Compare September 28, 2021 01:51
@kevingranade kevingranade merged commit 3f3a6af into CleverRaven:master Sep 29, 2021
@jbytheway jbytheway deleted the neighbour_test_rotations branch September 29, 2021 02:03
@kevingranade
Copy link
Member

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/has-anyone-noticed-some-odd-terrain-roads-lately/27140/4

@jbytheway
Copy link
Contributor Author

The discourse conversation linked above points out that this probably broke road mapgen. Will look into it.

jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this pull request Oct 8, 2021
The changes in CleverRaven#51913 altered how neighbours were entered into the
mapgendata context.  For JSON mapgen this fixed issues, but for
hardcoded mapgen it could break things.

In particular, for road mapgen it caused problems.

Add a new flag to allow specific oter_ts to opt out of this new
behaviour, and apply it to roads.
ralreegorganon added a commit to ralreegorganon/Cataclysm-DDA that referenced this pull request Dec 16, 2023
The changes in CleverRaven#51913 broke the rotation of lake shores when used as
predecessors by essentially pre-rotating the entries in the mapgendata
neighbors even though the shore itself is not a rotatable terrain.

As a result, the predecessor omt generation would be rotated in the
direction of the final mapgen, negating the predecessor application logic
that would pre-rotate it in the opposite direction so that the final
mapgen rotation would put it in the correct spot.

The end result would be that the lake shores used as predecessor terrain
would be incorrect in all orientations except north.

To fix this, I applied the IGNORE_ROTATION_FOR_ADJACENCY flag that was
added in CleverRaven#52168 to lake and ocean shores so that their mapgendata is
provided as expected.

...however, that implementation only worked for the final overmap terrain
and not predecessors because the mapgendata constructor that swaped
overmap terrain types didn't reevaluate the flag to see if the neighbors
should be rotated. Consequently, if the final one is rotated, all the
predecessors got rotated data even if they set this flag.

I updated it so now it checks for a difference in rotation between
the original and new overmap terrain type, and adjusts the neighbors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Map / Mapgen Overmap, Mapgen, Map extras, Map display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants