-
Notifications
You must be signed in to change notification settings - Fork 0
chore: streamline activity derivation #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 823110dcc5
ℹ️ 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".
| function findPlacement( | ||
| group: GroupState, | ||
| spatial: SpatialIndex, | ||
| maxAttempts = 256 | ||
| ): Coordinate | null { | ||
| while (group.freeStack.length) { | ||
| const key = group.freeStack.pop(); | ||
| if (key === undefined) break; | ||
| const { cx, cy } = unpackCell(key); | ||
| const coord = cellToWorld(cx, cy); | ||
| const bounds = boundsForCoord(coord); | ||
| if (!hasCollision(bounds, spatial)) return coord; | ||
| } | ||
|
|
||
| for (let i = 0; i < maxAttempts; i += 1) { | ||
| const { cx, cy } = spiralNext(group.spiral); | ||
| const coord = cellToWorld(cx, cy); | ||
| const bounds = boundsForCoord(coord); | ||
| if (!hasCollision(bounds, spatial)) return coord; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid hard cap that drops new agents in large groups
The new spiral placement stops after maxAttempts = 256, returning null when all 256 candidate cells collide. When a group grows beyond ~256 cells (or when collisions are dense near the anchor), addAgent will skip the new agent entirely, leaving it without a layout entry and therefore never rendered or selectable. This is a regression from the previous radius-based search that expanded with layout size. Consider scaling attempts with group size (or continuing until a free cell is found) to avoid silently dropping agents in larger sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
Report
Code reduction
Effect.succeed, trimming boilerplate insrc/activity/machine.ts.src/activity.tsand removed a redundant call intoderiveState.public/src/lib/format.ts.Complexity impact
deriveStateWithHold; fewer allocations on hot paths.Tradeoffs
deriveStateSyncnow bypassesEffect.runSync; ifderiveStatebecomes effectful in the future, restore the runSync path.deriveStateWithHoldcomputesbaseStatedirectly to avoid redundant parsing; logic remains equivalent, but any future changes toderiveStateneed to be mirrored here.Testing
npm run build(fails: missing@types/node/effecttypes in this environment).