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 lake shore mapgen corner case #70234

Merged
merged 2 commits into from
Dec 16, 2023

Conversation

ralreegorganon
Copy link
Contributor

Summary

None

Purpose of change

Fixes #38036

Describe the solution

In my original implementation, I overlooked the scenario where a lake shore was bounded on the cardinal directions by river banks and had a lake on the diagonal that bounded the river banks. As a result, the mapgen would not draw any shoreline and just fill the whole location with water.

I also ported this change into the ocean shore code that is derived from this.

Describe alternatives you've considered

None. Ultimately we'll want to revisit the overmap level generation and its handling of the confluence of rivers, lakes, and oceans to not just play whack-a-mole (as much) with weird generation, but this issue is just a legit oversight in my original implementation.

Testing

Set up a test scenario that went through the cases and checked it out:

image

Also teleported around and looked for this particular case occurring naturally. There are still lots of weird interactions at the confluence of lakes and rivers but this fixes the most egregious one.

Additional context

Before
image

After

image

In my original implementation, I overlooked the scenario where a
lake shore was bounded on the cardinal directions by river banks
and had a lake on the diagonal that bounded the river banks. As a
result, the mapgen would not draw any shoreline and just fill the
whole location with water.

There are still lots of weird interactions at the confluence of
lakes and rivers but this fixes the most egregious one.
@ralreegorganon ralreegorganon changed the title Minor lake issues Fix lake shore mapgen corner case Dec 15, 2023
@github-actions github-actions bot added Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Dec 15, 2023
@Vollch
Copy link
Contributor

Vollch commented Dec 15, 2023

This corner case it's just a very tip of the iceberg, there's at least few more patterns which doesn't align well.
It case if you're interested, but i actually fixed most of issues with shore generation.
Most - because river banks can end at weird unsolvable positions after lake gen, and shore mapgen just can't fix it by itself. I think that's the last bad case there.
I had to change overall logic to do so, as attempts to fix all patterns one by one didn't ended very well, but overall it's still heavily based on your original(pretty neat) implementation.

@ralreegorganon
Copy link
Contributor Author

This corner case it's just a very tip of the iceberg, there's at least few more patterns which doesn't align well.
It case if you're interested, but i actually fixed most of issues with shore generation.
Most, because, river banks can end at weird unsolvable positions after lake gen, and shore mapgen just can't fix it by itself. I think that's the last bad case there.
I had to change overall logic to do so, as attempts to fix all patterns one by one didn't ended very well, but overall it's still heavily based on your original(pretty neat) implementation.

Interesting. Like you (and I) had mentioned, ultimately we'll have to revisit rivers (and now oceans here) to make them all cleanly integrate. I don't think the iceberg is quite that big, but I agree there are more. I'll give your changes a deeper read here at some point. When I did this years ago, I was pretty happy with the final result but you can seen in my comments that I got frustrated coming up with something that looked good and wasn't a bunch of conditionals and ultimately punted, so if this meets the criteria that'd be nice...

// I'm pretty unhappy with this block of if statements that follows, but got frustrated/sidetracked
// in finding a more elegant solution. This is functional, but improvements that maintain the result
// are welcome. The basic gist is as follows:

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 16, 2023
@Maleclypse Maleclypse merged commit e93b4ac into CleverRaven:master Dec 16, 2023
24 of 28 checks passed
@I-am-Erk
Copy link
Member

I'd be interested in chatting on discord at some point about how we might do this better. I think I've got some really fuzzy ideas now, but I don't know because it was a hard weekend and I'm very tired.

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.

River meets lake bad mapgen
4 participants