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

Remove agricultural location from craters to prevent intermittent failures in oermap generation #73409

Closed
wants to merge 1 commit into from

Conversation

kevingranade
Copy link
Member

Summary

None

Purpose of change

I've been seeing intermittent failures in the overmap_terrain_coverage test due to placement of these Crater specials.

Describe the solution

I noticed a correlation with rural roads being reported and tried just removing the agricultural location from the special definition.

Describe alternatives you've considered

Removing Craters is an option, but I'd rather not throw them away over a flaw in the system.
There's definitely some underlying issue here that I haven't spotted, but I don't know what it is.

Testing

I ran the test repeatedly (a thousand times) overnight with the following invocation:
printf 'overmap_terrain_coverage\n%.0s' {0..1000} | parallel --bar -j 5 "tests/cata_test "{}" | tee big_test_results/test_{#}"
This invokes up to 5 instances of the test in parallel and writes their results out to files in the given subdirectory.
In previous testing I saw this failure more like one time out of a hundred, so it's very unlikely I saw this many passes without addressing the issue.

@Procyonae
Copy link
Contributor

Wouldn't adding a new overmap_location that's just agricultural but without rural_road and adding a comment referencing this be better?

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Map / Mapgen Overmap, Mapgen, Map extras, Map display json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Apr 30, 2024
@kevingranade
Copy link
Member Author

Do you fully understand what's happening here? I'm just treating symptoms.

#73429 adjusts the crater definition to fall back to placing small craters if it can't place anything next to the root, which seems better than my change in principle, though I'm a bit concerned with how much effort these take to troubleshoot.

@kevingranade kevingranade marked this pull request as draft May 1, 2024 16:20
@Procyonae
Copy link
Contributor

Nah I were just suggesting a less aggressive fix if you thought the issue was definitely connected to rural roads, #73429 makes sense as a fix though, it's unfortunate making an errorless mutable is as much an art as a science

@kevingranade kevingranade deleted the non-agricultural-craters branch May 9, 2024 23:50
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 [JSON] Changes (can be) made in JSON 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.

2 participants