feat: EntityCollection + batch component operations #1
Conversation
✅ Deploy Preview for objecs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@jakeklassen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces Set-based entity storage with a new array-backed EntityCollection (swap-and-pop removal), updates World and Archetype to use ReadonlyEntityCollection/EntityCollection, expands addEntityComponents overloads, adds docs/benchmarks, CI workflow OIDC auth, .gitignore tweaks, several JSDoc updates, and related test adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus on:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.gitignore (1)
5-5: Consider using directory-specific pattern.The rule
tmpwill match both files and directories namedtmp. If only temporary directories should be ignored, usetmp/for clarity and to prevent accidentally ignoring files namedtmp.-tmp +tmp/.github/workflows/release.yml (1)
31-31: Consider adding--provenanceflag for OIDC publishing.When using OIDC authentication with npm, the
--provenanceflag is typically required to generate signed provenance attestations. This flag links the published package to its source and build, improving supply chain security.🔎 Suggested change
- - run: pnpm publish --no-git-checks --access public --filter objecs + - run: pnpm publish --no-git-checks --access public --provenance --filter objecsperformance-plan.md (2)
5-22: Add language specifiers to code blocks for better rendering.The code blocks displaying benchmark results should include a language identifier (e.g.,
textorplaintext) for proper syntax highlighting and rendering.🔎 Proposed fix
-``` +```text > ecs-benchmark@ start > node src/bench.js objecs miniplexApply the same change to the code blocks at lines 111-118 and 121-128.
Also applies to: 111-118, 121-128
131-138: Add blank lines around the comparison table.The table at line 131 should be surrounded by blank lines for proper markdown rendering and better readability.
🔎 Proposed fix
**Comparison with miniplex after Phase 1:** + | Benchmark | objecs | miniplex | Diff | |-----------|--------|----------|------|packages/objecs/src/world.ts (1)
222-274: Well-designed overloads for batch operations.The overloads for
addEntityComponentsprovide excellent DX:
- Single-component calls:
addEntityComponents(entity, "position", { x: 0, y: 0 })- Batch updates:
addEntityComponents(entity, { position: { x: 0, y: 0 }, velocity: { x: 1, y: 1 } })The batch operation is more efficient as it updates archetype membership only once. The implementation correctly handles both cases and maintains type safety through overloads.
Note: The
@ts-ignorecomments at lines 254 and 261 bypass type checking. While this is pragmatic given the dynamic nature of the operation, consider if there's a type-safe alternative using mapped types or conditional types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)benchmark.md(1 hunks)hierarchy-ideas.md(1 hunks)packages/ecs-benchmark/src/cases/miniplex/add_remove.js(1 hunks)packages/ecs-benchmark/src/cases/miniplex/entity_cycle.js(1 hunks)packages/ecs-benchmark/src/cases/miniplex/frag_iter.js(1 hunks)packages/ecs-benchmark/src/cases/miniplex/packed_5.js(1 hunks)packages/ecs-benchmark/src/cases/miniplex/simple_iter.js(1 hunks)packages/examples/src/demos/shmup/enemy/determine-pickable-enemies.ts(1 hunks)packages/objecs/src/archetype.test.ts(6 hunks)packages/objecs/src/archetype.ts(6 hunks)packages/objecs/src/world.test.ts(4 hunks)packages/objecs/src/world.ts(6 hunks)performance-plan.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/objecs/src/world.test.ts (2)
packages/objecs/src/world.ts (2)
ReadonlyEntityCollection(13-27)entities(149-151)packages/objecs/src/archetype.ts (1)
entities(45-49)
packages/objecs/src/archetype.ts (1)
packages/objecs/src/world.ts (4)
EntityCollection(33-136)SafeEntity(4-7)entities(149-151)ReadonlyEntityCollection(13-27)
packages/objecs/src/archetype.test.ts (1)
packages/objecs/src/world.ts (3)
EntityCollection(33-136)ReadonlyEntityCollection(13-27)SafeEntity(4-7)
🪛 LanguageTool
benchmark.md
[grammar] ~4-~4: Ensure spelling is correct
Context: ...rk > node src/bench.js objecs miniplex objecs packed_5 139,836 op/s simple_it...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~21-~21: Ensure spelling is correct
Context: ... | | ---- | --: |--: |--: |--: |--: | | objecs | 139,836 | 80,937 | 44,291 | 7,556 | 2...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
hierarchy-ideas.md
[grammar] ~232-~232: Ensure spelling is correct
Context: ...rchy management - Could be published as objecs-hierarchy package Cons: - Additio...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~275-~275: Ensure spelling is correct
Context: ...d potentially be: - A separate package (objecs-hierarchy) - Built into objECS core wi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
performance-plan.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # objecs Performance Improvement Plan ## Benchm...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~34-~34: Ensure spelling is correct
Context: ...y Architectural Differences | Aspect | objecs | miniplex | |--------|--------|-------...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
performance-plan.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
131-131: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (17)
hierarchy-ideas.md (1)
1-287: Excellent design document—clear, well-structured, and future-proof.This comprehensive walkthrough of hierarchy implementation approaches aligns nicely with the EntityCollection-based improvements in this PR. The progression from simple (Approach 1) to sophisticated (Approach 3) gives users a pragmatic adoption path, and the code examples correctly use the new
addEntityComponents()overload andReadonlySetpatterns that align with the library's direction.The open questions section shows good architectural thinking about synchronization and lifecycle management. The references to battle-tested implementations (Wicked Engine, Bevy, Unity) lend credibility.
packages/examples/src/demos/shmup/enemy/determine-pickable-enemies.ts (1)
7-9: LGTM! Clean refactoring to support the new EntityCollection API.The changes align well with the PR objectives:
- Widening the parameter type from
ReadonlySet<T>toIterable<T>maintains backwards compatibility while supporting the new EntityCollection- Using spread syntax
[...entities]is idiomatic and functionally equivalent toArray.from(entities)packages/ecs-benchmark/src/cases/miniplex/entity_cycle.js (1)
4-6: LGTM! Documentation improvements.The JSDoc additions across all miniplex benchmark files improve type documentation without affecting runtime behavior.
benchmark.md (1)
1-22: LGTM! Benchmark documentation added.The benchmark results clearly document performance improvements across most test cases, aligning with the PR objectives. The static analysis spelling warnings for "objecs" and "miniplex" are false positives (these are package names).
packages/objecs/src/archetype.test.ts (3)
2-7: LGTM! Updated imports for EntityCollection API.The imports correctly include EntityCollection and ReadonlyEntityCollection to support the migration from Set-based storage.
35-35: LGTM! Archetype construction updated for EntityCollection.The test correctly instantiates EntityCollection() for the Archetype constructor, aligning with the new storage implementation.
Also applies to: 54-54, 67-67
44-44: LGTM! Type expectations updated to ReadonlyEntityCollection.The type expectations correctly reflect the new public API returning ReadonlyEntityCollection instead of ReadonlySet.
Also applies to: 109-111
packages/objecs/src/world.test.ts (5)
2-2: LGTM! Import updated for EntityCollection API.The import correctly includes ReadonlyEntityCollection for the migration from Set-based storage.
32-34: LGTM! Type expectation updated for world.entities.The type correctly expects ReadonlyEntityCollection instead of ReadonlySet, aligning with the new collection API.
160-202: LGTM! Comprehensive test coverage for batch component addition.The new test suite thoroughly validates the batch addEntityComponents() overload:
- Verifies multiple components are added correctly
- Confirms archetype membership updates
- Tests error handling for non-existent entities
211-215: LGTM! Archetype test assertions updated for EntityCollection.The test assertions correctly use .size and .has() methods compatible with the ReadonlyEntityCollection interface.
Also applies to: 223-223, 230-231, 235-235
289-299: LGTM! Entity iteration updated for array-backed collection.The spread operator correctly converts the iterable EntityCollection to an array for indexed access, enabling deterministic assertions on specific entities.
Also applies to: 305-309
packages/objecs/src/archetype.ts (1)
113-130: LGTM - Logic correctly filters entities.The
withoutmethod correctly creates a new archetype by filtering entities that don't have the excluded components. The iteration and filtering logic is sound.packages/objecs/src/world.ts (4)
9-27: LGTM - Well-designed readonly interface.The
ReadonlyEntityCollectioninterface provides a clean readonly API surface. Thehas(value: unknown)signature is intentionally flexible (as documented) to allow membership checks without type narrowing, which is appropriate for ECS use cases.
33-43: LGTM - Efficient dual-structure storage.The combination of an array (fast iteration) and a Map (O(1) lookups) achieves the performance goals outlined in the PR. The implementation is clean and properly encapsulated.
63-81: LGTM - Correct swap-and-pop implementation.The swap-and-pop removal logic is correctly implemented:
- O(1) deletion by swapping the target with the last element
- Properly updates the indices map for the swapped element
- Handles the edge case where the target is already the last element
This achieves the O(1) removal performance goal while maintaining array compactness.
97-114: LGTM - Correctly mimics Set.entries() behavior.The
entries()method returns[entity, entity]tuples to maintain API compatibility withSet.entries(), which duplicates values to match the Map interface. The custom iterator implementation avoids creating intermediate arrays for better performance.
jakeklassen
changed the title
Performance: Array + Map entity storage
Replace Set with EntityCollection backed by:
Benchmarks
*entity_cycle regressed due to Map overhead but remains 77% faster than miniplex.
Also includes
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Other
✏️ Tip: You can customize this high-level summary in your review settings.