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

Don't crash when forest mapgen requests groundcover from a biome with no groundcover #72918

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

akrieger
Copy link
Member

@akrieger akrieger commented Apr 8, 2024

Summary

None

Purpose of change

Fixes #72877, #72872. #72812 exposed that some forest mapgen code assumes adjacent biomes have groundcover. This is not true.

Describe the solution

Patch the code to fallback to regional groundcover like the other fallback cases use, in case the chosen adjacent biome in the feathering algorithm does not have groundcover of its own.

Also, noticed the adjacent biomes array wasn't being fully populated, with the last entry being skipped due to an off by one error in the initialization loop. Fix that too.

Describe alternatives you've considered

Testing

Use one of the repro saves from the issue, it doesn't crash anymore when mapgenning an innawoods cave tile.

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 Apr 8, 2024
src/mapgen_functions.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Apr 8, 2024
@akrieger
Copy link
Member Author

akrieger commented Apr 8, 2024

So to test the crash, what I did is I moved cave_innawood out of the first slot for forest terrains, and the crash stopped happening even on the save that already had a cave generated. There's something funky that arrangement is exposing, but I can't take the time to look harder so I'm just monkeypatching the crash away.

@akrieger akrieger force-pushed the full_frontal_groundcover branch from 2616d77 to a5f321c Compare April 8, 2024 23:45
@github-actions github-actions bot added the <Bugfix> This is a fix for a bug (or closes open issue) label Apr 8, 2024
@akrieger akrieger changed the title Don't when forest mapgen requests groundcover from a biome with no groundcover Don't crash when forest mapgen requests groundcover from a biome with no groundcover Apr 8, 2024
@akrieger akrieger force-pushed the full_frontal_groundcover branch from a5f321c to 843d076 Compare April 8, 2024 23:46
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Apr 8, 2024
@Procyonae
Copy link
Contributor

There's something funky that arrangement is exposing, but I can't take the time to look harder so I'm just monkeypatching the crash away.

It probably expects the first one in the list to be the oter the others are mimicking somewhere along the line, I didn't really think twice when alphabetising them.
I'm going to bed now but I'll have a look in the morning.

@mqrause
Copy link
Contributor

mqrause commented Apr 9, 2024

I got a theory: cave_innawood is a terrain that does not exist in vanilla dda. So because that is loaded first, it can't use the actual region settings from vanilla and creates a new empty biome, which causes mapgen to trip over it.

std::string first_ter = forest_biome_ter_keys.get_string( 0 );
load_forest_biome( forest_biome_jo, forest_mapgen_settings.unfinalized_biomes[first_ter],
overlay );

@akrieger
Copy link
Member Author

akrieger commented Apr 9, 2024

squint

I'll change this to a debugmsg then which explains what needs to happen in the json, and fix the json for innawood.

@akrieger
Copy link
Member Author

akrieger commented Apr 9, 2024

The other option is changing it so the forest biome regional setting loading code always prefers to inherit from the forest biome of the name of the region, if its available and in the terrains list (which seems to be always?), otherwise to use the first biome.

@akrieger akrieger marked this pull request as draft April 9, 2024 18:12
@mqrause
Copy link
Contributor

mqrause commented Apr 9, 2024

The other option is changing it so the forest biome regional setting loading code always prefers to inherit from the forest biome of the name of the region, if its available and in the terrains list (which seems to be always?), otherwise to use the first biome.

From what I can see, the loading code is kinda sketchy because it treats regional settings and overlays the same. But overlays can skip all the required checks, because they expect a regional setting to already exists to inherit all the required data from. Except they don't in cases like this. So it should be forbidden to just create a new forest_biome object when loading an overlay.

@akrieger
Copy link
Member Author

akrieger commented Apr 9, 2024

oic, because forest_mapgen_settings.unfinalized_biomes[first_ter] will create an empty biome if the string is novel. And this function is called from both loadpaths. I can throw an error instead if overlay = true and the first terrain is not seen yet.

@Procyonae
Copy link
Contributor

I had a look yesterday but couldn't determine what the best way to actually make it work sensibly is, sorry for radio silence tho I were having an off day

@akrieger akrieger force-pushed the full_frontal_groundcover branch from 843d076 to 9069c27 Compare April 12, 2024 15:12
@akrieger akrieger marked this pull request as ready for review April 12, 2024 15:13
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 12, 2024
src/regional_settings.cpp Outdated Show resolved Hide resolved
@akrieger akrieger force-pushed the full_frontal_groundcover branch from 9069c27 to 02ac488 Compare April 13, 2024 22:21
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 14, 2024
@I-am-Erk I-am-Erk merged commit 38e2c35 into CleverRaven:master Apr 14, 2024
25 of 26 checks passed
@akrieger akrieger deleted the full_frontal_groundcover branch April 14, 2024 18:21
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` 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.

Crash when getting close to any caves in recent Innawood experimental build
4 participants