Fix #12255 Updating TargetCamera on multi camera scenes not allowing layout to be calculated #12268
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
Check to make sure the layout is getting calculated when TargetCamera is being updated on a node.
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
TrialDragon
added
A-UI
Make sure the children are being cleaned up and maintain taffy references to camera nodes
StrikeForceZero
force-pushed
the
fix/12255
branch
from
7f42846
to
71c028e
Compare
StrikeForceZero
force-pushed
the
fix/12255
branch
from
ce49fbb
to
538dd38
Compare
aevyrie
approved these changes
Mar 3, 2024
There was a problem hiding this comment.
I don't have enough experience in this area of code to do much other than a rubber stamp. The only things I noticed were some very tiny nits, non blocking. This adds a regression test, which is great. The only thing I'm uncertain of is if this breaks some untested invariant about taffy nodes somewhere else.
StrikeForceZero
commented
Mar 3, 2024
alice-i-cecile
approved these changes
Mar 4, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Mar 5, 2024
…layout to be calculated (#12268) # Objective - Fixes #12255 Still needs confirming what the consequences are from having camera viewport nodes live on the root of the taffy tree. ## Solution To fix calculating the layouts for UI nodes we need to cleanup the children previously set whenever `TargetCamera` is updated. This also maintains a list of taffy camera nodes and cleans them up when removed. --- ## Changelog Fixed #12255 ## Migration Guide changes affect private structs/members so shouldn't need actions by engine users.
spectria-limina
pushed a commit
to spectria-limina/bevy
that referenced
this pull request
Mar 9, 2024
… allowing layout to be calculated (bevyengine#12268) # Objective - Fixes bevyengine#12255 Still needs confirming what the consequences are from having camera viewport nodes live on the root of the taffy tree. ## Solution To fix calculating the layouts for UI nodes we need to cleanup the children previously set whenever `TargetCamera` is updated. This also maintains a list of taffy camera nodes and cleans them up when removed. --- ## Changelog Fixed bevyengine#12255 ## Migration Guide changes affect private structs/members so shouldn't need actions by engine users.
mtsr
pushed a commit
to mtsr/bevy
that referenced
this pull request
Mar 15, 2024
… allowing layout to be calculated (bevyengine#12268) # Objective - Fixes bevyengine#12255 Still needs confirming what the consequences are from having camera viewport nodes live on the root of the taffy tree. ## Solution To fix calculating the layouts for UI nodes we need to cleanup the children previously set whenever `TargetCamera` is updated. This also maintains a list of taffy camera nodes and cleans them up when removed. --- ## Changelog Fixed bevyengine#12255 ## Migration Guide changes affect private structs/members so shouldn't need actions by engine users.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2024
# Objective Fix the regression for Root Node's Layout behavior introduced in #12268 - Add regression test for Root Node Layout's behaving as they did before 0.13.1 - Restore pre 0.13.1 Root Node Layout behavior (fixes #12624) ## Solution This implements [@nicoburns suggestion ](https://discord.com/channels/691052431525675048/743663673393938453/1221593626476548146), where instead of adding the camera to the taffy node tree, we revert back to adding a new "parent" node for each root node while maintaining their relationship with the camera. > If you can do the ecs change detection to move the node to the correct Taffy instance for the camera then you should also be able to move it to a `Vec` of root nodes for that camera. --- ## Changelog Fixed #12624 - Restores pre 0.13.1 Root Node Layout behavior ## Migration Guide If you were affected by the 0.13.1 regression and added `position_type: Absolute` to all your root nodes you might be able to reclaim some LOC by removing them now that the 0.13 behavior is restored.
StrikeForceZero
added a commit
to StrikeForceZero/bevy
that referenced
this pull request
Mar 30, 2024
StrikeForceZero
added a commit
to StrikeForceZero/bevy
that referenced
this pull request
Mar 30, 2024
mockersf
pushed a commit
that referenced
this pull request
Apr 1, 2024
# Objective Fix the regression for Root Node's Layout behavior introduced in #12268 - Add regression test for Root Node Layout's behaving as they did before 0.13.1 - Restore pre 0.13.1 Root Node Layout behavior (fixes #12624) ## Solution This implements [@nicoburns suggestion ](https://discord.com/channels/691052431525675048/743663673393938453/1221593626476548146), where instead of adding the camera to the taffy node tree, we revert back to adding a new "parent" node for each root node while maintaining their relationship with the camera. > If you can do the ecs change detection to move the node to the correct Taffy instance for the camera then you should also be able to move it to a `Vec` of root nodes for that camera. --- ## Changelog Fixed #12624 - Restores pre 0.13.1 Root Node Layout behavior ## Migration Guide If you were affected by the 0.13.1 regression and added `position_type: Absolute` to all your root nodes you might be able to reclaim some LOC by removing them now that the 0.13 behavior is restored.
StrikeForceZero
added a commit
to StrikeForceZero/bevy
that referenced
this pull request
Apr 3, 2024
StrikeForceZero
added a commit
to StrikeForceZero/bevy
that referenced
this pull request
Apr 30, 2024
StrikeForceZero
added a commit
to StrikeForceZero/bevy
that referenced
this pull request
May 19, 2024
Apply rustfmt Move test_initialization next to other tests Add doc comment to is_root_node_pair_valid Move helper methods into the single test case where they are used Add tests for helper methods Remove trait helpers to reduce complexity of tests Mark specific test functions as unreachable Move ui_surface test only methods into mod tests as trait Fix tests after rebase Add tests for bevy_ui/layout/ui_surface Widen type for parameter children in UiSurface::update_children Add missing asserts and Debug fields in UiSurface from bevyengine#12268 and bevyengine#12698
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
Still needs confirming what the consequences are from having camera viewport nodes live on the root of the taffy tree.
Solution
To fix calculating the layouts for UI nodes we need to cleanup the children previously set whenever
TargetCamerais updated. This also maintains a list of taffy camera nodes and cleans them up when removed.Changelog
Fixed #12255
Migration Guide
changes affect private structs/members so shouldn't need actions by engine users.