fix issue with phantom ui node children #14490
Merged
Conversation
This file contains 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
alice-i-cecile
added
C-Bug
mockersf
approved these changes
Jul 26, 2024
|
Please don't merge yet, I noticed that there was an optimization update for the children update that I should be using. I also realized that it might be better to use |
alice-i-cecile
added
S-Waiting-on-Author
eidloi
force-pushed
the
fix_phantom_children
branch
from
d76025a
to
f1d8190
Compare
|
I have replaced the loop with the iter as in the first update. Discord tells me changing the query wouldn't help 🤷 |
alice-i-cecile
added
S-Needs-Review
alice-i-cecile
approved these changes
Jul 29, 2024
alice-i-cecile
added
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Aug 2, 2024
# Objective The `ui_layout_system` relies on change detection to sync parent-child relation to taffy. The children need to by synced before node removal to avoid trying to set deleted nodes as children (due to how the different queries collect entities). This however may leave nodes that were removed set as children to other nodes in special cases. Fixes #11385 ## Solution The solution is simply to re-sync the changed children after the nodes are removed. ## Testing Tested with `sickle_ui` where docking zone highlights would end up glitched when docking was done in a certain manner: - run the `docking_zone_splits` example - pop out a tab from the top - dock the floating panel in the center right - grab another tab and try to hover the original static docking zone: the highlight is semi-stuck - (NOTE: sometimes it worked even without the fix due to scheduling order not producing the bugged query results) After the fix, the issue is no longer present. NOTE: The performance impact should be minimal, as the child sync relies on change detection. The change detection was also the reason the parent nodes remained "stuck" with the phantom children if no other update were done to them.
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.
Objective
The
ui_layout_systemrelies on change detection to sync parent-child relation to taffy. The children need to by synced before node removal to avoid trying to set deleted nodes as children (due to how the different queries collect entities). This however may leave nodes that were removed set as children to other nodes in special cases.Fixes #11385
Solution
The solution is simply to re-sync the changed children after the nodes are removed.
Testing
Tested with
sickle_uiwhere docking zone highlights would end up glitched when docking was done in a certain manner:docking_zone_splitsexampleAfter the fix, the issue is no longer present.
NOTE: The performance impact should be minimal, as the child sync relies on change detection. The change detection was also the reason the parent nodes remained "stuck" with the phantom children if no other update were done to them.