Fix: destroy and removeChild performance #10505
Conversation
- change the way render group is stored - added parentRenderGroup - renderGroup now the owned render group
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2f10ef4:
|
update tests tio be more readable
|
/korbit-review |
@GoodBoyDigital I have started a new review of this pull request. You will see my feedback shortly. |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 4 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
| @@ -79,7 +79,7 @@ describe('Container Tests', () => | |||
| // wrong! | |||
| expect(child.toGlobal({ x: 0, y: 0 }, null, true)).toEqual({ x: 0, y: 0 }); | |||
|
|
|||
| updateRenderGroupTransforms(container.renderGroup, true); | |||
| updateRenderGroupTransforms(container.parentRenderGroup, true); | |||
There was a problem hiding this comment.
The changes in the test cases are in line with the refactoring of the 'renderGroup' property in the 'Container' class. However, it would be beneficial to add new test cases to specifically test the new 'parentRenderGroup' property and the new 'getChildren' method in the 'RenderGroup' class. This will ensure that the new changes are working as expected and do not introduce any regressions.
| else if (this.renderGroup) | ||
| { | ||
| this.renderGroup.onChildUpdate(this); | ||
| this.parentRenderGroup.onChildUpdate(this); | ||
| } | ||
| } | ||
|
|
||
| set isRenderGroup(value: boolean) |
There was a problem hiding this comment.
The 'isRenderGroup' property has been refactored, and the setter now throws an error if trying to set it to false. This could be a breaking change for any code that relies on toggling this property. Consider providing a migration path or additional documentation to handle this change.
Zyie
added
✅ Ready To Merge
Description of change
The way rendergroup was being stored kind of bugged me as it was not as intuative as it could be. Consequentially, this meant its a bit harder to understand some of the checks that happen in the code around render groups.
Essentially the
renderGroupproperty onContainercould either be the Containers owned render group or its parent render group. Which is kind of weird!I have modified this so we now have
renderGroupwhich is the the containers renderGroup created whenenableRenderGroupis calledparentRenderGroupa new property which is the render group that is responsible for rendering the container. this is set when a container is added to a rendergroup.Modifications to RenderGroup
_childrenproperty on renderGroup - we don't actually use it anywhere, so not point pushing and splicing it!getChildrenwhich will return them all should someone want to get the list of all renderables in a render group.Benchmark:
removeChildren / destroy is now 1000x faster in the demo from this issue: #10345
Will update a demo once a branch is built.
fixes #10345 and #10377
Pre-Merge Checklist
npm run lint)npm run test)