Add a closure API over the unit spatial index - #478
Conversation
|
@codex review |
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: a2098355c2
ℹ️ 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 scratch lists were handed out by nesting depth, which assumes they come back in the order they went out. Callbacks may yield, and two that yield can resume in either order - the first to resume would then lower the depth while the second still held its list, and the next query would reset a list still being read. A slot is now owned until its own holder returns it. The documentation told callers to keep a callback in a field and pass it repeatedly, next to the sentence saying the call destroys it. Both cannot be true. The call destroys it, because the usual call site is a lambda written in place and nothing else would free it; a caller who wants no per-call allocation wants the list overloads, which is why they remain. The new tests counted through locals captured by a closure. Wurst captures those by value, so the counter read after the call would have stayed at zero and the checks would have failed whether or not the API worked.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a655bf027c
ℹ️ 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".
error() reports and returns, so the guard printed its message and then carried on with the slot index one past the pool - handing every overflowing call the same list to reset and read, which is the sharing the slots were introduced to prevent. Borrowing now answers -1 when the pool is exhausted and each entry point returns, destroying the callback it was handed so ownership stays consistent with the ordinary path.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de977d8cae
ℹ️ 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".
A pool has to be handed back, and a callback which fails never hands anything back. The slot was then gone for the rest of the map, so sixteen recoverable failures would leave every later closure query rejected - and the API documents that a callback can fail, which is what makes that reachable rather than theoretical. Owning the list per call costs one allocation on a path which already allocates a closure, and a failed callback now costs that list rather than the ability to query at all. Destroying it also clears the unit references it collected, which a released pool slot did not - a wide query kept everything it returned reachable, in every slot it had used. The list overloads still allocate nothing, which is the reason they remain.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
1 participant
A closure API over the unit spatial index, for the shape almost every caller actually wants.
Why
Running a query is three steps: ask, decide which matches count, act on those. The list overloads hand back an
ArrayListand leave the last two to the caller, which makes the raw shape the default and the common case something you assemble yourself.Deciding collapses into an
ifinside the callback, so no filter object and no result collection are needed for it. The list overloads stay for the cases that genuinely want the raw result.Not a replacement for ClosureForGroups
ClosureForGroups.forUnitsInRangestays exactly as it is. The index beats a Warcraft group on Lua and loses to one on Jass, so which is faster depends on the backend — that belongs to the caller, not to a silent substitution inside the function everyone already calls.The names differ for the same reason.
forEachUnitInRangerather thanforUnitsInRangemeans a map importing both packages does not get an ambiguous call on a name it already uses.Behaviour otherwise matches, including ownership: the call destroys the callback, as
forEachIndoes, because the usual call site is a lambda written in place and nothing else would free it. Passing the same callback twice therefore uses a destroyed object. A caller who does not want a closure per call wants the list overloads — they allocate nothing, which is the reason they remain.Correctness
The snapshot is closed before any callback runs. Matches are copied out first. A callback is caller code: it can fail, which would leave the global query snapshot open for good along with the unit handles it holds, and it can yield, which would let two queries read and close each other's snapshot. This is the same rule the list overloads follow, for the same reason.
Nesting works. One scratch list per nesting level, so a callback may start another query without disturbing the iteration it is inside.
forUnitsOfType,forUnitsAll,forUnitsSelectedandforNearestUnithave no index equivalent and are not mirrored here.Tests
Added to
StdlibIngameTests, beside the existing spatial parity checks — which is whereUnitSpatialIndexTestsalready says this kind of verification belongs, since it needs a live game:forEachUnitInRangeUntilstops when the callback says so, and stops after the callback that asked.StdLibOwnTestsgreen, which compiles the whole library including every test package.