More mapgen sanity checking and control #54344
Conversation
github-actions
bot
added
astyled
|
This is probably better for "real" map gen than setting the palettes to clear furniture, and it adds the removal of items. You probably don't want to remove all items for camp constructions, though, as that risks screwing the player over when the new construction is built on top of collected loot (and the player doesn't get any indication where the construction will be placed, although the version 2 sites tries to provide a limited indication). Also, you don't want a 100 page spam report of every item in every tile that's happen to be in the construction location. Thus, this seems to be a useful addition that should be used with a little bit of care. |
Maleclypse
added
Code: Tests
Indeed. In some cases I've removed the explicit clearing of furniture where it is replaced by this new feature.
I haven't touched camp constructions. They need a different approach to fix them.
They probably will lead to some spam in the current state of things, but it would only be one report per tile, not one per item.
Thanks :). |
jbytheway
force-pushed
the
clearing_mapgen
branch
2 times, most recently
from
5881bf6
to
1e2f12e
Compare
|
Added docs and fixed a few more issues. Tests fail rarely now (I had about a hundred successes in a row at best). Marking this ready for review now, but I should note that I haven't fixed any issues in mods except for Magiclysm. Other mods (in-repo and otherwise) will probably trigger this error quite often. But it's only a |
Nested mapgen was being forced to include a terrain map in its JSON definition, even when it is perfectly reasonable to lack that. Weaken the error message accordingly.
Add support for flags in the various mapgen objects. Use these flags to control the behaviour of terrain wiping in mapgen. When terrain is changed, should other stuff on the tile be removed or not? Two flags set different default behaviours: always remove stuff or never do. If neither flag is present, then an error will be raised.
We need to remove things before e.g. terrain is changed.
This is a shorthand way to remove everything except the terrain on a particular tile, generally used in nested mapgen to clear space for something else in the nest.
jbytheway
force-pushed
the
clearing_mapgen
branch
from
1e2f12e
to
296dde3
Compare
jbytheway
force-pushed
the
clearing_mapgen
branch
from
296dde3
to
e67c183
Compare
kevingranade
force-pushed
the
clearing_mapgen
branch
from
e67c183
to
5039c34
Compare
|
Just rebased (no conflicts) to poke the build. |
Fix lots of situations that led to out-of-bounds spawns, or colliding spawns. These generally occur with nested mapgen, but can also occur due to predecessor mapgen, hardcoded mapgen, or map extras. Where terrain definitions conflict with other stuff that's already present on the map we use one of a variety of solutions, according to each individual situation: - Add one of the two new flags. - Remove the terrain definition entirely. - Add targeted removal instructions.
jbytheway
force-pushed
the
clearing_mapgen
branch
from
5039c34
to
dad1d15
Compare
github-actions
bot
added
the
BasicBuildPassed
|
Looks like it finally got through most of the tests and the one remaining failure is unrelated. |
Summary
Infrastructure "Better checking of mapgen definitions"
Purpose of change
We have a lot of mapgen that runs on top of existing maps. This can happen due to nested chunks, map extras, predecessor mapgen, update mapgen, or some hardcoded things.
Describe the solution
This PR adds a sanity check for whenever mapgen changes the terrain on a tile where something (furniture, trap, or item) already exists. This is often a sign of a problem. When it happens, it flags a warning message.
It also provides a variety of solutions to this problem that mappers can use:
ALLOW_TERRAIN_UNDER_OTHER_DATAandERASE_ALL_BEFORE_PLACING_TERRAINwhich specify the desired default behaviour if a particular piece of mapgen wishes to use the same behaviour whenever this happens. The first flag suppresses the warning while the second causes everything on the tile to be removed before setting the terrain (which is normally what you want).remove_allmapgen command that erases everything on a tile, for when the mapper needs to be more targeted in removing only in some cases.The recently-added exhaustive mapgen unit test allows these problems to be quickly discovered, so I ran that a lot and fixed all the issues it found (although, as usual, there's probably a long tail of low-probability issues I didn't fix).
I only ran the tests for Magiclysm, not other mods, so there are probably issues there too.
I still need to write documentation, so opening this as a draft.
Describe alternatives you've considered
I considered enhancing the
terraindefinition itself to allow specifying how to handle this situation. That might be better than adding theremove_allcommand for fixing these issues, but I thinkremove_allis probably something we want to have regardless.Testing
Running the unit tests a lot.
Additional context
I started this work back in October, but realised it was infeasible to fix all the issues without having a unit test to help, so I have been diverted by adding that unit test in the time since and only recently returned to this.