Optimize the logic of remove and add entity. #1930
Conversation
packages/core/src/Entity.ts
Outdated
| children.splice(index, 0, child); | ||
| for (let i = index + 1, n = childCount + 1; i < n; i++) { | ||
| children[i]._siblingIndex++; | ||
| children.length = childCount + 1; |
There was a problem hiding this comment.
Maybe can mergechildren.length = childCount + 1; ?
packages/core/src/Entity.ts
Outdated
| for (let i = childCount; i > index; i--) { | ||
| const swapChild = children[i - 1]; | ||
| swapChild._siblingIndex = i; | ||
| children[i] = swapChild; |
packages/core/src/Scene.ts
Outdated
| for (let i = entity._siblingIndex; i < count; i++) { | ||
| const child = rootEntities[i + 1]; | ||
| rootEntities[i] = child; | ||
| child._siblingIndex = i; |
There was a problem hiding this comment.
Same with before warp suggestion
WalkthroughThis update introduces two new static methods in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Scene
participant Entity
Caller->>Scene: addRootEntity(index, entity)
Scene->>Entity: _addToChildren(children, entity, index)
Entity-->>Scene: Children array updated
Scene-->>Caller: Confirmation of addition
sequenceDiagram
participant Caller
participant Scene
participant Entity
Caller->>Scene: removeRootEntity(entity)
Scene->>Entity: _removeFormChildren(children, entity)
Entity-->>Scene: Children array updated
Scene-->>Caller: Confirmation of removal
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1930 +/- ##
=======================================
Coverage 68.93% 68.94%
=======================================
Files 961 961
Lines 100286 100271 -15
Branches 8655 8657 +2
=======================================
- Hits 69131 69128 -3
+ Misses 30895 30883 -12
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (2)
packages/core/src/Entity.ts (1)
98-116: Add test coverage for index bounds checking.The index bounds checking logic in
_addToChildrenis not covered by tests. Please add test cases to verify the behavior when:
- Index is negative
- Index is greater than the children count
- Index is undefined (append mode)
Would you like me to generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-115: packages/core/src/Entity.ts#L105-L115
Added lines #L105 - L115 were not covered by testspackages/core/src/Scene.ts (1)
342-342: Add test coverage for root entity management.The new index-based root entity management functionality is not covered by tests. Please add test cases to verify:
- Adding root entities at specific indices
- Removing root entities
Would you like me to generate test cases for these scenarios?
Also applies to: 344-344
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Entity.ts(4 hunks)packages/core/src/Scene.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Entity.ts
[warning] 105-115: packages/core/src/Entity.ts#L105-L115
Added lines #L105 - L115 were not covered by tests
[warning] 622-622: packages/core/src/Entity.ts#L622
Added line #L622 was not covered by tests
packages/core/src/Scene.ts
[warning] 340-340: packages/core/src/Scene.ts#L340
Added line #L340 was not covered by tests
[warning] 344-344: packages/core/src/Scene.ts#L344
Added line #L344 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
| static _removeFormChildren(children: Entity[], entity: Entity): void { | ||
| const count = children.length - 1; | ||
| for (let i = entity._siblingIndex; i < count; i++) { | ||
| const child = children[i + 1]; | ||
| children[i] = child; | ||
| child._siblingIndex = i; | ||
| } | ||
| children.length = count; | ||
| entity._siblingIndex = -1; | ||
| } |
There was a problem hiding this comment.
Fix typo in method name.
The method name has a typo: _removeFormChildren should be _removeFromChildren.
Apply this diff to fix the typo:
- static _removeFormChildren(children: Entity[], entity: Entity): void {
+ static _removeFromChildren(children: Entity[], entity: Entity): void {Note: This change will require updating all references to this method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static _removeFormChildren(children: Entity[], entity: Entity): void { | |
| const count = children.length - 1; | |
| for (let i = entity._siblingIndex; i < count; i++) { | |
| const child = children[i + 1]; | |
| children[i] = child; | |
| child._siblingIndex = i; | |
| } | |
| children.length = count; | |
| entity._siblingIndex = -1; | |
| } | |
| static _removeFromChildren(children: Entity[], entity: Entity): void { | |
| const count = children.length - 1; | |
| for (let i = entity._siblingIndex; i < count; i++) { | |
| const child = children[i + 1]; | |
| children[i] = child; | |
| child._siblingIndex = i; | |
| } | |
| children.length = count; | |
| entity._siblingIndex = -1; | |
| } |
| @@ -337,11 +337,11 @@ | |||
| const oldScene = entity._scene; | |||
| if (oldScene !== this) { | |||
| if (oldScene && isRoot) { | |||
| oldScene._removeFromEntityList(entity); | |||
| Entity._removeFormChildren(oldScene._rootEntities, entity); | |||
There was a problem hiding this comment.
Update method name to match the fix in Entity class.
Once the typo in Entity._removeFormChildren is fixed, update the references here to use the corrected name.
Apply this diff to fix the references:
- Entity._removeFormChildren(oldScene._rootEntities, child);
+ Entity._removeFromChildren(oldScene._rootEntities, child);Also applies to: 378-378
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 340-340: packages/core/src/Scene.ts#L340
Added line #L340 was not covered by tests
Summary by CodeRabbit
New Features
Refactor