Feature: Region-based pathfinder for ships (buoys no longer required!)

[Kuhnovic]

Remarks for reviewers:

• The code contains lots of debug drawing code which I need in order to fix things. Please ignore this code for now.
• If the destination is unreachable an random path is generated. This makes the ship move around aimlessly. This makes it visually clear that the ship is lost, but perhaps it looks a little too silly.
• The node limiting of the tile-level ship pathfinder is based on the assumption that Remove: Settings for NPF & YAPF #10623 will be merged soon, which will only use the ExitDir-based variant of the ship node.
• Although there is no need for buoys anymore, users can still use them as waypoints if they want to. I don't think we should remove them.

Motivation / Problem

Anybody who has tried using ships has undoubtedly noticed that ships get lost and need buoys to guide them to the destination. On water the YAPF ship pathfinder quickly reaches its 10000 (default) node limit and throws in the towel. This means we either have to increase the node limit which can lead to large performance hits, or use buoys to create an intermediate destination that is still within this 10000 node limit. It is also very unclear how far buoys have to be spaced apart since you can't see how far the pathfinder is able to search, which depends on the complexity of the map.

Description

The map is divided up into so called WaterRegions of 16x16 tiles (screenshot is from when it was still 8x8). Within such a region connected-component-labeling (CCL) is used to identify different areas of water that are connected. This is done once when a map is created / loaded, and regions are invalidated when changes are made to the map. Invalidated regions get updated automatically when they are needed.

The YAPF ship pathfinder first does a call to a "WaterRegion pathfinder" which only looks at the high level interconnectivity between regions (either via region edges or via aqueducts). This creates a high level path to the destination. The regular YAPF ship pathfinder then looks for a path several regions ahead of its current position. Effectively this gives the tile-level pathfinder an "intermediate destination". When close to the final destination the lookahead is ignored and the pathfinding is simply done using the tile-level YAPF pathfinder.

The high level search is MUCH faster than the tile-level search. I had 5000 ships (the game's total vehicle limit) traveling across a 1024x1024 map with very complicated water geometry, this ran great on my Intel i7 3610QM laptop from 2012. It still runs at about 1,5x the normal game speed when I hold down TAB.

Another big benefit is quick rejection of cases where no path is available. Such cases are horrible for any A* pathfinder because it requires searching the entire map until there are no new nodes to be added (or until a predetermined node limit is reached, like YAPF does). Since the high level pathfinder effectively searches 256 tiles per region it is able to very quickly determine if there is any path at all, and if there is none then the tile-level pathfinder won't even run.

Limitations

The main downside is that it no longer guarantees optimal paths. Through extensive tweaking I was able to get very good paths that are often optimal or only slightly suboptimal. IMO a slightly suboptimal path is still a lot better than having to fiddle with buoys to even reach the destination at all.

Credit where credit is due

Shoutout to JazzyJaffa from the tt forums. I stumbled across his thread and that inspired me to go this route
https://www.tt-forums.net/viewtopic.php?t=58905 . I ended up writing my own implementation, simply because I'm stubborn, and because I wanted to make some different design decisions, e.g. go for square regions instead of arbitrary shapes.

"Why didn't you use Jump Point Search?"

I actually had a sort-of working JPS implementation, but I did not continue with it because:

• If there is no path, JPS will still search the whole map (assuming a worst case). It will do it more efficiently than regular A*, but it still doesn't scale well to large maps.
• JPS does not take turning penalties into account, which can cause ships to do unnecessary 90 degree turns relatively often. This can be worked around but is pretty tedious.
• JPS assumes we can travel to any direction from the start of search, but this is not the case since ships might have to turn or reverse. This makes the "expansion pattern" around the start very complicated.
• I ran into the patch from JazzyJaffa and got the idea for regions :)

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, gs_changelog.hpp need updating.
  • The compatibility wrappers (compat_*.nut) need updating.
• This PR affects the NewGRF API? (label 'needs review: NewGRF')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker

[github-advanced-security]

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[rubidium42]

There is one thing to consider with the connectivity of a region; a region can be connected to multiple other regions, but you might not be able to go through the region. See the attached example where a ship gets lost, while it works just fine in master.
poc.zip

[Kuhnovic]

There is one thing to consider with the connectivity of a region; a region can be connected to multiple other regions, but you might not be able to go through the region. See the attached example where a ship gets lost, while it works just fine in master. poc.zip

@rubidium42 The algorithm takes this into account. I opened your savegame and it runs fine right out of the gate, so I'm afraid it's a bug in the region invalidation / updating, which has been very difficult to get right. All regions are updated once when a game is loaded, so that would explain why it worked for me. Can you load your savegame again and (hopefully) confirm that it runs just fine for you as well?

[rubidium42]

There is one thing to consider with the connectivity of a region; a region can be connected to multiple other regions, but you might not be able to go through the region. See the attached example where a ship gets lost, while it works just fine in master. poc.zip

@rubidium42 The algorithm takes this into account. I opened your savegame and it runs fine right out of the gate, so I'm afraid it's a bug in the region invalidation / updating, which has been very difficult to get right. All regions are updated once when a game is loaded, so that would explain why it worked for me. Can you load your savegame again and (hopefully) confirm that it runs just fine for you as well?

Yes, I can confirm that after reloading it works. That, sadly, makes the problem actually even worse, as we now know the current implementation causes desyncs. See docs/desync.md for more information about them, but essentially this is a cache mismatch.

[Kuhnovic]

Yes, I can confirm that after reloading it works. That, sadly, makes the problem actually even worse, as we now know the current implementation causes desyncs. See docs/desync.md for more information about them, but essentially this is a cache mismatch.

I was able to reproduce the issue, should be fixed now. After reading more about desyncs I realize that the way I store my region data needs to be made more robust. I think I should try to store everything in the m1,m2 etc tiledata an not in a separate data structure. I'll see if I can come up with a different solution for this.

[Kuhnovic]

All debug drawing code should now be removed. I fully agree with LordAro's comment that some form of debug drawing would be convenient, but I won't make that part of the scope of this PR. It's big enough already :p

I've also been thinking about simplifying the water region invalidation. There are InvalidateWaterRegions calls in many places in the code, and even though I spend quite some time on it there could be edge cases that I've missed. This could lead to water regions not getting updated on an important event, say for example when a dock is built. This in turn could leads to ships not finding a path or taking huge detours.

As an alternative I can simply invalidate a water region when DoClearSquare (in landscape.cpp) is called. This would essentially invalidate the water region on any change. That includes changes to non-water tiles. It would be quite robust, but also a bit of a primitive approach. If you have an opinion on this please let me know.

[rubidium42]

While you're fixing these, also please do a rebase as there's a (merge) conflict.

[rubidium42]

Did you notice the trailing white space error already?

[rubidium42]

I referred to the x_or_y as I thought that was what the TODO is about. If both are pretty clear, then I'd say: remove the TODO.

[TrueBrain]

Tnx again for all your work on this. It looks awesome :D