[bevy_ui/layout] Update internal mappings #16362
Conversation
alice-i-cecile
added
A-UI
alice-i-cecile
added
the
C-Usability
| .get(&child) | ||
| .cloned() | ||
| .unwrap_or_else(|| { | ||
| panic!("failed to resolve taffy id for child entity {child} in {entity}") |
There was a problem hiding this comment.
Previously, we used a field as a scratch pad to push these IDs into, but we silently ignored the ones we failed to resolve. I was unsure if we wanted to continue silently failing or if the panic is more appropriate.
| .expect("root_node_data missing"); | ||
|
|
||
| if root_node_data.camera_entity.is_none() { | ||
| panic!("internal map out of sync"); |
There was a problem hiding this comment.
If I recall correctly, some of these panics are only possible because we expose several methods to pub(super) that could be used arbitrarily in the future and break the state. I'll need to update this one, too, but the idea with this effort is an attempt to make the state more atomic
| root_node_entity: Entity, | ||
| camera_entity: Entity, | ||
| ) -> &mut RootNodeData { | ||
| let user_root_node = *self.entity_to_taffy.get(&root_node_entity).expect("create_or_update_root_node_data called before root_node_entity was added to taffy tree or was previously removed"); |
There was a problem hiding this comment.
This feels weird coming back to, but if memory is serving today, it's related to the non-atomic nature of how the layout module (super) could call these methods.. Theoretically, the node should be upserted when added well before we call set_camera_children.
(also note how set_camera_children has been pub, not pub(super)...)
BenjaminBrienen
added
the
D-Modest
| .unwrap_or_else(|| { | ||
| panic!("failed to resolve taffy id for child entity {child} in {entity}") | ||
| }) |
There was a problem hiding this comment.
if I remember correctly those are not similar in how they handle formatting when not panicking
crates/bevy_ui/src/layout/debug.rs
Outdated
| &mut out, | ||
| ); | ||
|
|
||
| bevy_utils::tracing::info!("{out}"); |
There was a problem hiding this comment.
tracing is already a dependency of bevy_ui
BenjaminBrienen
added
S-Waiting-on-Author
StrikeForceZero
force-pushed
the
dev/bevy_ui/ui_surface_update_mappings_updated
branch
from
4d4faaa to
2856625
Compare
|
I'm not sure if GitHub failed to notify me of the label change or if I accidentally dismissed it. I went ahead and rebased this on top of main. It looks like it overlaps with most, if not all, of the changes from #17596. I was planning to circle back to review whether any of the tests are redundant or could be cleaned up, and to update the other PRs that depend on this one. Before going further, though, I wanted to pause and ask: is this PR still something we want to move forward with? |
BenjaminBrienen
added
S-Needs-Review
6 participants
supersedes #12804
Objective
UiSurfacemappings to be more conciseUiSurfaceSolution
EntityHashSetto track relationships between camera -> root nodesOption<Entity>inRootNodeData(previously namedRootNodePair)Testing
Only via the existing unit/e2e tests, since rebasing the original changes from [bevy_ui/layout] UiSurface update internal mappings #12804.
Since these changes are somewhat stale, it would be worth manually confirming during runtime with various scenarios and attempts to create any undesired behavior or overlooked race conditions.