Put the unit index on the shared spatial partition - #475
Merged
Conversation
SpatialPartition was added to be the one grid both indexes share, and then nothing consumed it. UnitSpatialIndex kept its own cell chains, cached positions, grid math and rebuild, so the generic package was carrying a second copy of all of it. The unit layer now owns only what the partition has no business knowing: the unit-to-id mapping, the sweep that decides how stale a cached position may be, and the Warcraft rules about Locust and hidden units. Gone from it are cellHead, nextInCell, prevInCell, cellOfUnit, lastX, lastY, the grid origin and dimensions, the cell math and both grid walks. Three things the partition needed to be able to serve it. A box query, which it had no equivalent of. Accessors for a cached position, so a consumer can re-decide an uncertain result under its own staleness model rather than being forced to read a live one. And a bound on the relink loop in setPos: it iterated all eight groups on every position update, which the sweep calls once per unit per tick, where a single-group consumer needs one iteration. The unit layer keeps its own dense registry rather than reusing the partition's. They are different sets: the partition's holds every entry any consumer has put in the grid, and both numbers derived from the unit list - the sweep budget and the staleness bound - must count only the units this sweep is responsible for. The suite caught that, by way of the partition's own tests leaving entries behind. Resolution of an uncertain hit now happens after the walk rather than inside it, because the partition hands back a snapshot of ids. That removes the reason the position write-back had to be guarded: a relink can no longer make an entry be visited twice by the query reading it, so it goes through spatialPartitionSetPos and re-buckets properly instead of only updating the cache when the cell happened not to change. SPATIAL_INDEX_CELL_SIZE is gone; cell geometry belongs to the partition, so SPATIAL_PARTITION_CELL_SIZE is the knob. A config override of the old name no longer compiles rather than silently doing nothing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1806dcb6b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Making the grid shared without making the id namespace shared only looked safe because nothing else consumes it yet. Every row the partition owns is keyed by the id alone - the position, the group links, the registry slot - so a second consumer counting up from 1, as one naturally would, would share rows with the unit index: each would overwrite the other's cached position, relink its groups on every move, and delete its entry on removal. Groups separate what a query sees, not what an id addresses. Allocation moves into the partition, with a free list so ids are still recycled, and the unit layer asks for one instead of keeping its own counter. Releasing an id whose entry is still linked is refused rather than handing a live row to the next caller, which is the mistake this arrangement makes easy to write. Exhaustion is now answerable: the allocator reports it and returns 0, and a unit that cannot be given an id stays unindexed rather than aliasing entry 0, which is the chain sentinel. The refusal path is not covered by a test. `error()` is fatal in the compiletime harness, so a test that provokes it fails rather than asserting it; the test covers what can be asserted, that independently allocated ids are distinct, that their rows stay independent, and that a released id is handed out again.
Both operate on the partition's whole population, which was unremarkable while nothing shared it and is a trap now that something does. A rebuild re-derives the extent for every consumer. That is not incidental: there is one grid, and an entry still linked under the old cell indices would be invisible to the new one, so re-linking everything is the only correct thing it can do. Clear drops every consumer's entries. There is no way to ask it for only one layer's, because groups say what a query sees, not who owns a row, and it does not return ids to the allocator either.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a14406abf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The interpreter models units properly - position, owner, visibility - so the belief that only grid arithmetic was testable here was wrong. What stopped the tests was the index gating itself off with isLua, which the interpreter reports as false, so its init never wired the events and nothing registered. The guard stays: isLua is folded, and that is what makes the package vanish on Jass. The tests move into the implementation package instead, where they can drive registration and the sweep directly. Sixteen tests. Membership and radius, including that the boundary is measured from the unit origin. A unit moved without notifying is still returned, which is what the padding is for, and notifying or letting the sweep run relocates it. Removal takes a unit out. Locust and hidden units are skipped by range queries and deliberately kept by the player query. Box membership, owner scoping, and nested queries keeping their own snapshot segments. Three of them are budget gates, asserting work done rather than wall time so they mean the same thing on every machine. A large radius over empty space skips coarse blocks and visits one entry. A small query among 120 spread-out units visits 2, not the population. A query covering everything visits each entry exactly once, which is how a relink during a walk would show up. Checked that they bite rather than assuming it: dropping the live distance test in the annulus turns radiusIsMeasuredFromTheUnitOrigin red with Expected <false>, Actual <true>. One gate was wrong on the first run and is now correct: the small-radius test asserted that coarse blocks were skipped, but its window fits inside a single block, so there is nothing to skip. That belongs to the large-radius gate, which asserts it.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d15d92219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three rounds of review have now found the same thing in three places, so this fixes the shape rather than the third instance. A shared structure is keyed by two namespaces, entry ids and groups, and neither had an owner: whoever got there first won, silently. Entry ids. The allocator added last round was still not the only way in - spatialPartitionSetPos accepts any valid id and registers it, which is how a consumer that addresses rows by ids it chose itself works, and that is a legitimate way to use the package. So insertion now claims the row, and the allocator steps over anything already claimed. Two doors, one record of what is taken, and neither door has to know about the other. Groups. The same defect one level up, and worse, because a group is what a query filters by: a second consumer that had picked group 0 would get units back from its own queries with no way to tell them apart. Groups are allocated now, claimed by direct use exactly as ids are, and the unit index asks for one instead of hardcoding zero. SpatialPartitionTests allocates its two rather than naming them, so the tests demonstrate the contract instead of bypassing it. Both are pinned by tests that cannot pass by accident of ordering: the id one takes a row well ahead of the allocator and then drains past it, and fails without the skip.
Member
Author
|
@codex review |
Neither is drift. UnitSpatialIndex carries its tests because the index gates itself off with isLua, so nothing registers in the interpreter unless registration and the sweep are driven directly, and exporting those would put four internals into the public contract of a package maps depend on. SpatialPartition is all implementation, and the split its size suggests would mean exporting the flat arrays its queries read, which is the access pattern the package exists for. Both trade a line count against something the repository guards harder, so the reasons are in the headers rather than in a review thread.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f9580b096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Last round put the check on one of the two paths. The free list was still popped unconditionally, so an id released and then taken directly through spatialPartitionSetPos was queued and claimed at the same time, and the next allocation handed that live row to a second consumer. The fix was half applied, which is worse than not applied: it reads as done. Both paths now consult the one record. A queued id is a candidate, not a guarantee, so the pop loop discards claimed entries rather than trusting the queue. Those are the only two sources of an id, so this is the whole surface. Also here, two ways the layer could claim to work while doing nothing: SPATIAL_INDEX_CELL_SIZE comes back, deprecated. Removing it broke any map that had set it, and a shared grid cannot honour a units-only cell size anyway, so it defaults to agreeing with the partition and a stale override is reported at init instead of quietly running on a different grid. And a failed group allocation no longer activates the index. A group is what a query filters by, so without one every query matches nothing; leaving indexActive false says that, rather than reporting a healthy index that answers every question with an empty set.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60959f6876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Six rounds of review kept arriving at the same place from new angles, and it is a boundary rather than a defect. Allocation guarantees that two consumers are never handed the same row, including rows taken directly. It does not guarantee that a consumer only writes to rows it was given, because a row is reachable by its number and nothing about a number says who owns it. Closing that gap means an owner token on every mutator, paid on every position update, to catch a bug a caller can simply not write. The structure exists to be fast and assumes correct use, so the boundary is written down instead.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce927da3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two things that let the layer look like it worked. Activation was gated on the group but not on the grid. A rebuild that refuses the extent leaves the grid unready and returns, and init carried on seeding and setting indexActive, so spatialIndexHealthy reported a usable index whose queries match nothing. Same shape as the group gate added last round, which is the sibling I should have looked for then: activation now requires everything init depends on, not the one thing that was reported. And the allocated-slot figure read the fresh-allocation cursor, which misses a high row taken directly and later recycled - handed out again from the free list without the cursor ever reaching it. Diagnostic only, but it is the number that says how far the id-keyed arrays actually reach, so it now tracks the highest id in use wherever one becomes live.
Measured against the merged compiler on a real map. An inlined early return becomes a boolean the caller tests before every statement that follows it, and four of these sat on the query path, so the innermost entry walk ran six of those tests per visited entry. None of the four returns was load-bearing. Both partition queries guarded a whole body and returned 0, but nothing has been pushed when that guard fails, so the count at the end is 0 regardless: a positive guard around the body says the same thing. The two result accessors and the enumeration filter are single expressions written as a branch and two returns. On zombie-defense, addRangeMatches goes from 791 lines with 23 guard tests to 636 with none, and the per-entry walk now runs its statements unconditionally. No behaviour change: `and` short-circuits, so a Locust unit still costs the one native it did before.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
What
SpatialPartition(#472) was added to be the one grid the spatial indexes share, and then nothing consumed it —UnitSpatialIndexkept its own cell chains, cached positions, grid math and rebuild. This makes the unit index an actual consumer.The unit layer now owns only what the partition has no business knowing: the unit-to-id mapping, the sweep that decides how stale a cached position may be, and the Warcraft rules about Locust and hidden units. Removed from it:
cellHead,nextInCell,prevInCell,cellOfUnit,lastX,lastY, the grid origin and dimensions,cellCoordX/Y,cellAt,linkIntoCell,unlinkFromCell, and both grid walks.The public API of
UnitSpatialIndexis unchanged apart from one config constant, below, soSpatialIndexForUnitsneeds no change.What the partition needed to be able to serve it
spatialPartitionEntryX/Y. A consumer that knows more about staleness than the partition does has to be able to re-decide an uncertain result from the cached position, instead of being forced into a live read.setPos. It iterated allMAX_GROUPSon every position update, and the sweep calls that once per unit per tick. It now iterates the groups actually declared, so a single-group consumer does one iteration instead of eight.Two things worth reviewing
The unit layer keeps its own dense registry. Not duplication for its own sake: the partition's registry holds every entry any consumer has put in the grid, and the two numbers derived from the unit list — the sweep budget and the staleness bound — must count only the units this sweep is responsible for, or another layer's destructables stretch the cycle covering the units. The suite caught this, via the partition's own tests leaving entries behind.
Uncertain hits are now resolved after the walk. The partition returns a snapshot of ids, so resolution happens outside the traversal. That removes the reason the write-back in #473 had to be guarded — a relink can no longer cause an entry to be visited twice by the query reading it — so it now goes through
spatialPartitionSetPosand re-buckets properly, rather than updating the cache only when the cell happened not to change.Performance: expected direction, not measured
Honest accounting, because this touches the hot path and I have not measured emitted Lua.
If measurement shows that, the obvious lever is unchecked
@inlineaccessors for the result loop, which knows its index is in range — six comparisons per result. I have not added them speculatively.Checks
grill typecheck: 0 errorsgrill test: 484/484UnitSpatialIndexTestscovers grid arithmetic only and every query path is asserted inStdlibIngameTests, which needs a running map. This is the change most worth running the map benchmark against, on both the sparse-large-radius and dense-cluster cases.Breaking
SPATIAL_INDEX_CELL_SIZEis removed — cell geometry belongs to the partition, soSPATIAL_PARTITION_CELL_SIZEis the knob. AUnitSpatialIndex_configoverride of the old name now fails to compile rather than silently doing nothing.