[3.x] Spatial and CanvasItem children change to LocalVector
#107480
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AThousandShips
changed the title
Spatial children from linked list to linearSpatial children from linked list to vector
lawnjelly
force-pushed
the
spat_children_linear_list
branch
from
87b1be2 to
b459fd5
Compare
lawnjelly
changed the title
Spatial children from linked list to vectorSpatial and CanvasItem children change to LocalVector
lawnjelly
commented
Jun 13, 2025
| struct Data { | ||
| LocalVector<CanvasItem *> children; | ||
| uint32_t parent_child_id = UINT32_MAX; | ||
| } data; |
There was a problem hiding this comment.
We should likely move all CanvasItem data into a Data struct as we do with Spatial and Node, to prevent name conflicts and subtle bugs. But this is for a follow up.
This was referenced Jun 13, 2025
Ivorforce
reviewed
Jun 17, 2025
lawnjelly
force-pushed
the
spat_children_linear_list
branch
2 times, most recently
from
edb7252 to
97ed666
Compare
Ivorforce
approved these changes
Jun 18, 2025
Ivorforce
reviewed
Jun 18, 2025
scene/3d/spatial.cpp
Outdated
Comment on lines
224
to
225
| uint32_t c = data.index_in_parent; | ||
| LocalVector<Spatial *> &parent_children = data.parent->data.children; |
There was a problem hiding this comment.
Personally I prefer not creating aliases if they aren't performance critical or change semantical meaning, because then you'll have to remember less information reading the code top to bottom.
But that's not critical, feel free to leave as is if you prefer the other way.
Ivorforce
reviewed
Jun 18, 2025
lawnjelly
force-pushed
the
spat_children_linear_list
branch
from
97ed666 to
8e48d57
Compare
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spatialmaintains its own list ofSpatial-only children (separate from theNode), in order to (in theory) make child iteration faster for only spatial children. The same happens withCanvasItemandCanvasItemchildren.The problem is that these lists are stored as a linked list, and there is no reason to require linked list. Linked lists are bad for cache.
Benchmark
benchmark_spatial_children.zip
before: 157 fps
after: 280 fps
The benchmark creates a large number of
Spatialchildren to the root node, then rotates the root. This stresses the_propagate_transform_changed()which utilizes the children list.Notes
Spatialchildren list entirely and utilizing theNodechildren list instead with fast casting, but this PR is still faster (because of no need for cast checks).NULLchecks in the original were redundant, I don't think these pointers can beNULLgiven the code here.SceneTreeFTInow it is faster, instead ofNodechildren. Not 100% on this yet, but worth looking into.CanvasItem, the same applies.