Allow several pipe groups to share a centroid - #87
Merged
Conversation
Group.Location is the rounded centroid of the group's pumpjack centers:
Location = new Location(
(int)Math.Round(_sumX / _entities.Count, 0),
(int)Math.Round(_sumY / _entities.Count, 0));
That is a derived value, and nothing makes it unique. Two groups can round
to the same tile. DelaunayTriangulation used it as a dictionary key through
ToDictionary and Add, both of which throw ArgumentException on a duplicate,
so a collision took the whole plan down with "An item with the same key has
already been added".
Key by centroid to a list of groups instead. Where the triangulation used to
map a line endpoint to one group, it now maps to every group at that point.
Groups sharing the popped group's own centroid need handling separately. The
triangulation sees a single point for all of them, so they can never appear
as the far end of a line, yet they are the closest partners there are. They
are added directly, after the line pass.
This leaves every existing plan byte for byte identical, which is the point
of doing it on its own. When no two groups share a centroid each bucket holds
exactly one group, the line pass adds the same partners in the same order,
and the direct pass adds nothing because the only group at the popped group's
centroid is itself. Measured: the full suite passes 4299 of 4299 with zero
changed Verify snapshots.
No test fails without this change, and that is worth stating plainly rather
than hiding. The collision does not occur on any of the 1147 big-list
blueprints as they are grouped today, and a search over roughly 8000
synthetic layouts did not produce one either. The only known reproduction is
big-list blueprint 904 once the pumpjack terminal offsets are corrected for
Factorio 2.1, which is the next change. Verified against it: with those
offsets applied on top of this commit, blueprint 904 plans instead of
throwing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P9FADuTnjE7SFEQWnpNhfc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Group.Locationis the rounded centroid of the group's pumpjack centers:That is a derived value and nothing makes it unique. Two groups can round to the same tile.
DelaunayTriangulationused it as a dictionary key throughToDictionaryandAdd, both of which throw on a duplicate, so a collision took the whole plan down withSystem.ArgumentException: An item with the same key has already been added.The fix
Key by centroid to a list of groups. Where the triangulation used to map a line endpoint to one group, it now maps to every group at that point.
Groups sharing the popped group's own centroid need handling separately: the triangulation sees a single point for all of them, so they can never appear as the far end of a line, yet they are the closest partners there are. They are added directly, after the line pass, with a duplicate guard because a group can be reached both ways.
It changes no plans
That is the point of doing it on its own. When no two groups share a centroid, each bucket holds exactly one group, the line pass adds the same partners in the same order, and the direct pass adds nothing because the only group at the popped group's centroid is itself.
Measured: 4299 of 4299 tests pass with zero changed Verify snapshots.
No test fails without this, and that is worth stating plainly
The collision does not occur on any of the 1147 big-list blueprints as they are grouped today. A search over roughly 8000 synthetic layouts did not produce one either. So there is no reproduction to write a failing test against right now.
The only known reproduction is big-list blueprint 904 once the pumpjack terminal offsets are corrected for Factorio 2.1 (issue #81), which is the next change. Verified against it: with those offsets applied on top of this commit, blueprint 904 plans instead of throwing.
This is therefore a defensive fix landing ahead of the change that exposes it, deliberately separated so a 2743-file snapshot diff does not bury a real logic fix.
Checks
dotnet test: 4299 passed, 0 failed, 0 changed snapshots.dotnet build /p:UseLuaSettings=true: clean.src/luaregenerated and committed;tools/check-lua.shpasses both the Lua 5.2.4 syntax check and thesample.luarun.🤖 Generated with Claude Code
https://claude.ai/code/session_01P9FADuTnjE7SFEQWnpNhfc