A-star refactor and fix of the recurring "Can't reach tile" bug

[SomeTroglodyte]

Definitely read commits individually - I had to do some major refactors, foremost separating the whole thing into its own sub-package and into single-class files first, to get a handle on foreign-authored code here.

Fixes #15179
Fixes #15165
Probably #14711 too

Read comments in the first linked issue for reasoning. All actual code changes verified with Claude using careful anti-confirmation-bias wording.

Left a few TODO's for lines that look highly suspicious but do not impact the case found in these issues. Hopefully @Ambeco can take a look later.

Contains an entire save including a json-only snapshot of a required mod for the unit test - I hope that's OK @IVENEND

[Fanfblrik]

Thanks for the careful write-up on #15165 / #15179 — the early-exit explanation matches the crash path well.

The getMovementToTilesAtPosition change looks correct to me: any true end-search predicate aborts BFS, so there was no completeness guarantee for same-turn tiles, and a later getShortestPath could then return a destination missing from the cached tilesSameTurn. Passing { _, _ -> false } (while still limiting to turn 1) restores what the comment already promised. Nice that getMovementToTilesAtPosition_returnsRightTiles was updated to expect the full outer ring.

Also appreciated:

• regression test focused on getDistanceToTiles reachability rather than full AI automate (less flaky)
• FixedPointMovement operator fixes (div was inverted; unused but dangerous)

A few non-blocking notes:

1. Perf — Completing the turn-1 BFS instead of early-exiting is clearly necessary for correctness. Any rough sense of cost vs the old early exit on large maps / many AI units? If it's negligible, great.

2. TODOs in calculateNeighborNode — Especially addedNeighborNodes.set(...) immediately followed by clear(...) on the same index: that looks like a no-op / latent bug. Agree leaving for @Ambeco is fine if out of scope here; just flagging that this one seems more than stylistic.

3. Test mod snapshot — Understandable for a real repro (and thanks for asking @IVENEND). Long-term it is a large blob (~Regnum Germaniae JSON). If maintainers are fine shipping it, no objection; otherwise a trimmed ruleset that still reproduces the road/MP case would be nicer.

LGTM on the actual crash fix from my reading; deferring final A* internals sign-off to people closer to the original implementation.

[Fanfblrik]

Agree with the comment — early exit here can't guarantee a complete same-turn set.

[SomeTroglodyte]

Especially since there's a kind of precedent in the original code: bfsAllMatchingTiles

[Fanfblrik]

Is this intentional? As written it never leaves the bit set. Worth confirming with @Ambeco whether one of these lines is leftover.

[SomeTroglodyte]

Sounds more like a copy/paste repurposing mistake to me and one of the indexes was meant to be a different tile. Didn't want to wrack my smoking brain any further or guess.

[Fanfblrik]

Good catch. Worth a tiny unit test for div/times so this doesn't regress silently?

[SomeTroglodyte]

Maybe. Plus maybe a test hammering FixedPointMovement with prime bits and making sure all convert to Float and back unchanged... Incidentally, mods could go and produce movement costs not a multiple of 1/30th, then that whole card house would collapse.

But I'm done with this, need some relaxation, so if I forget a follow-up feel free to add on top.

[IVENEND]

I hope that's OK @IVENEND

I have no problem at all.
Thanks for investigating the issue.

[SomeTroglodyte]

Thanks for investigating the issue

Thanks for making success possible!

[SomeTroglodyte]

If maintainers are fine shipping it

Not shipped as part of any release but source tarballs.

trimmed ruleset that still reproduces

That's significant effort I tried to avoid. Or - I felt IVENEND isolating that case was such a stroke of luck I didn't want to waste.

Any rough sense of cost vs the old early exit

Can't be large given the rarity of the crash (in the test case 10% more tiles are scanned), and besides - buying being a bit faster with incorrect behaviour and crashes isn't a good deal - my personal opinion. Large car companies seem to disagree.

[Fanfblrik]

Follow-up on the FixedPointMovement review thread: opened SomeTroglodyte#12 targeting AStarRefactor with a small unit test for div / times and float round-trips. Happy to fold it in here if that is easier than merging the side PR.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved.

[yairm210]

Another case of "my head really isn't there atm"
But should the test resources be populated with an entire ruleset? Is that intentional?

[SomeTroglodyte]

But should the test resources be populated with an entire ruleset? Is that intentional?

Definitely intentional as the comments should show. Entire ruleset - the save plus the ruleset are a known repro, and because of the mechanics of the bug it would be a high effort to construct a simpler repro - a bit like cryptography if you will, like reconstructing a plain text input for a given hash, except here the limiting measure of effort is human. And my human processing unit is unable to imagine how to construct a repro using only G&K, or only TestGame-created objects, despite mostly understanding the mechanics of the bug.