Feature(v3) refactored zIndexing #669
Conversation
wouterlucas
left a comment
wouterlucas
left a comment
There was a problem hiding this comment.
How are we mixing children that have a zIndex value and ones that do not (thus are null)?
Are we doing sorted first, unsorted after? Assuming all unsorted are below the sorted ones?
| import type { CoreNode } from '../CoreNode.js'; | ||
|
|
||
| //Bucket sort implementation for sorting CoreNode arrays by zIndex | ||
| export const bucketSortByZIndex = (nodes: CoreNode[], min: number): void => { |
There was a problem hiding this comment.
Is a bucket Sort really faster then relying on the native .sort() function?
The native .sort() function has been there forever and I don't imagine there is a device that doesn't support it anymore.
Sure it might be slower if its really badly implemented, but net gain to the previous implementation would remain equal as the old implementation also relied on that.
| // fetch render bounds from parent | ||
| this.setUpdateType(UpdateType.RenderBounds | UpdateType.Children); | ||
| // Since this node has a new parent, to be safe, have it do a full update. | ||
| this.setUpdateType(UpdateType.All); |
There was a problem hiding this comment.
wouldn't this slow it down by quite a lot if we're just re-assigning a parent?
do we really need to update all?
| const useIncremental = changedCount <= 2 && changedCount < n * 0.05; | ||
|
|
||
| // when changed count is less than 5% of total children, use incremental sort | ||
| if (useIncremental === true) { |
There was a problem hiding this comment.
do you know what gain's this provides? I wonder if its worth the extra check + calc versus just sorting it the old fashioned .sort way if we have to.
| //distribute nodes into buckets | ||
| for (let i = 0; i < nodes.length; i++) { | ||
| const node = nodes[i]!; | ||
| const index = node.props.zIndex - min; |
There was a problem hiding this comment.
what happens to a child that doesn't have a zIndex?
|
|
||
| let left = 0; | ||
| let right = nodes.length - 1; | ||
| const targetZIndex = node.props.zIndex; |
There was a problem hiding this comment.
same here? what happens if there is no zIndex for this child?
This PR should reduce the zIndexing strain we have upon node creation. Especially when using either or the same zIndex values in one parent node.
To do: