Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove warning for children in UI hierarchies without Style #15736

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

alice-i-cecile
Copy link
Member

Objective

As discussed in #15591, this warning prevents us from storing leaf nodes without a Style component. Because text sections (as distinct entities) should not be laid out using taffy, this warning is incorrect.

Users may also have other uses for doing this, and this should generally increase flexibility without posing particularly serious correctness concerns.

Solution

  • removed warning about non-UI children with UI parents
  • improved the warning about UI parents with non-UI parents
    • this warning should stay, for now, as it results in a genuine failure to perform taffy layout
    • that said, we should be clearer about the cause and potentially harmful results of this!

Testing

I inserted an empty entity into the hierarchy in the button example as a leaf node, and it ran with no warnings.

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 8, 2024
@alice-i-cecile alice-i-cecile requested review from UkoeHB and cart October 8, 2024 16:45
@alice-i-cecile
Copy link
Member Author

This fix was much simpler than the more specific UI -> non-UI -> UI pattern that @cart called out as problematic here. I also think it's somewhat better, as non-UI -> UI still has not-particularly well defined behavior in general.

We could treat that case as fine if and only if there's no UI root above it by treating it as the start of a new tree, but really that feels quite a bit more complex for users to reason about and will involve a more complex fix and ongoing validity checks.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 8, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 8, 2024
@alice-i-cecile alice-i-cecile added S-Needs-SME Decision or review from an SME is required and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 8, 2024
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Oct 8, 2024

Waiting on an opinion from Cart before merging :) I want to make sure that this lines up with what he had in mind.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix was much simpler than the more specific UI -> non-UI -> UI pattern that @cart called out as problematic

Ah yeah this is definitely the right path forward conceptually. It being simpler / just removing code is icing on the cake!

@cart cart added this pull request to the merge queue Oct 8, 2024
Merged via the queue into bevyengine:main with commit 2f63ebc Oct 8, 2024
33 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Oct 13, 2024
# Objective

With the warning removed in
#15736, the rules for the UI tree
changes.
We no longer need to traverse non `Node`/`GhostNode` entities.

## Solution

- Added a filter `Or<(With<Node>, With<GhostNode>)>` to the child
traversal query so we don't unnecessarily traverse nodes that are not
part of the UI tree (like text nodes).
- Also moved the warning for NoUI->UI entities so it is actually
triggered (see comments)

## Testing

- Ran unit tests (still passing)
- Ran the ghost_nodes and ui examples, still works and looks fine 👍 
- Tested the warning by spawning a Node under an empty entity.

---

---------

Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-SME Decision or review from an SME is required X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants