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

Change: simplified water region evaluation, removed savegame data #11750

Merged
merged 1 commit into from Jan 21, 2024

Conversation

Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Jan 10, 2024

Motivation / Problem

This is closely related to the recently merged region-based ship pathfinder #10543

There was some discussion with @JGRennison and @SamuXarick on discord, there were worries about desyncs. In 10543 I've tried to prevent desyncs by saving the "is the region initialized "state of all regions in the savegame. The idea was to synchronize the "initialized-ness" in a bug-compatible way, meaning that if it wasn't initialized on the server then it wouldn't be initialized on the joining client either. But this is what can happen:

  • server starts a game
  • some action, say terraforming, doesn't properly invalidate water region X. Region X is now incorrectly marked as up-to-date.
  • a client joins the game. The client needs to scan / label (i.e. update) all water regions that are labeled as up-to-date. This should put the client's water region cache in sync with the one on the server...
  • ... but the cache on the server is corrupted, because the "up-to-dateness" of region X is incorrectly set to true. Region X on the server has a different state than region X on the client. This could leads to desyncs.

The current solution of syncing the up-to-date-state of the regions simply doesn't solve this.

Link to discord: https://discord.com/channels/142724111502802944/1008473233844097104/1194656849237127308

Description

So I'm taking a slightly more simple and brute force approach: invalidate a water region on any change. This includes changes to tiles that might not be entirely relevant to the water regions. This by itself shouldn't really have big impact on performance, region invalidation is just a matter of setting a boolean.

Limitations

  • This approach will invalidate more regions than necessary, which in turn will cause more water regions having to update (re-do their connected component labeling). This will have some impact on performance, but I don't think it will be significant.
  • I lowered the savegame version and removed the region data from the savegame. This will render some savegames incompatible, but only ones that were created with recent builds or the nightlies. I can change this if that's not acceptable.
  • There's some debug code that's nice for testing, but ultimately has to be removed.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@JGRennison
Copy link
Contributor

This does not handle the case of water-based objects built on water.
These should trigger an invalidation because they're non-navigable, but the region isn't invalidated.
See: CmdBuildObject/BuildObject/MakeObject.

(You can use a GRF like AuzObjects Water to test this).

@Kuhnovic Kuhnovic force-pushed the simplify_water_region_invalidation branch from 023c635 to a94fa97 Compare January 10, 2024 19:30
@glx22 glx22 added the preview This PR is receiving preview builds label Jan 10, 2024
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

I have no way of judging this code, but if you say it works, I will approve it. It just does need a bit of cleanup :)

src/landscape.cpp Outdated Show resolved Hide resolved
src/pathfinder/water_regions.cpp Outdated Show resolved Hide resolved
src/pathfinder/water_regions.cpp Outdated Show resolved Hide resolved
src/water_cmd.cpp Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Jan 14, 2024

This will definitely need to handle the saveload chunk being removed (unless that happens automagically.)

@TrueBrain
Copy link
Member

One last thing, sorry about that: I think we should bump the savegame version. Otherwise you can create a savegame which you cannot load, with a very "corrupted savegame" error in the older version. Better to have it say it is not compatible, I think.

src/landscape.cpp Show resolved Hide resolved
src/pathfinder/water_regions.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Up to @PeterN to merge this, if he is also okay with these changes.

Tnx for addressing these issues btw, it is appreciated!

@PeterN PeterN merged commit b38d3c2 into OpenTTD:master Jan 21, 2024
20 checks passed
@Kuhnovic Kuhnovic deleted the simplify_water_region_invalidation branch January 22, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants