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 the With<Parent> query filter from bevy_ui::render::extract_uinode_borders #9285

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jul 26, 2023

Objective

Remove the With<Parent> query filter from the parent_node_query parameter of the bevy_ui::render::extract_uinode_borders function. This is a bug, the query is only used to retrieve the size of the current node's parent. We don't care if that parent node has a Parent or not.

Removed the `With<Parent>` query filter from `parent_node_query`. This is a minor bug, it is meant to be a query to retrieve the size of the current node's parent, not a query for a node with a parent.
@ickshonpe ickshonpe changed the title ` Remove the With<Parent> query filter from bevy_ui::render::extract_uinode_borders Jul 26, 2023
Copy link
Member

@Selene-Amanita Selene-Amanita left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I have a naive question: when do we check the coherence of a hierarchy and when do we not do it? propagate_transforms does it for example, but maybe it's only to prevent an infinite recursion?

@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Jul 29, 2023
@Selene-Amanita Selene-Amanita added this to the 0.11.1 milestone Jul 29, 2023
@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 29, 2023
@@ -277,7 +277,7 @@ pub fn extract_uinode_borders(
Without<ContentSize>,
>,
>,
parent_node_query: Extract<Query<&Node, With<Parent>>>,
parent_node_query: Extract<Query<&Node>>,
Copy link
Member

@mockersf mockersf Jul 29, 2023

Choose a reason for hiding this comment

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

Suggested change
parent_node_query: Extract<Query<&Node>>,
node_query: Extract<Query<&Node>>,

and also where it's used

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, since it's the query used to get the parent, it makes sense to be named "parent_node_query"

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to name it based on what is is, not where it's used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to name it based on what is is, not where it's used

agreed I think, but keep the name parent_node for the value retrieved from the query, where it is the actual Node component of the parent.

@james7132
Copy link
Member

propagate_transforms does it for example, but maybe it's only to prevent an infinite recursion?

It does it because it would be unsound use of unsafe if the hierarchy is improperly structured, due to the mutable access on multiple threads.

@ickshonpe
Copy link
Contributor Author

Looks good to me, but I have a naive question: when do we check the coherence of a hierarchy and when do we not do it? propagate_transforms does it for example, but maybe it's only to prevent an infinite recursion?

There are also checks in UiSurface::get_layout and UiSurface::update_children but they probably aren't sufficient for identifying all the possible problems. I think we still just panic if a node in the middle of the UI layout tree is removed without removing its descendants as well.

@alice-i-cecile alice-i-cecile enabled auto-merge July 31, 2023 19:20
@alice-i-cecile alice-i-cecile disabled auto-merge July 31, 2023 19:21
@alice-i-cecile
Copy link
Member

@ickshonpe I think the rename was incomplete: CI failures look real. Once CI is passing this will be merged!

@ickshonpe
Copy link
Contributor Author

Ah yep I see, I'll fix it now.

@mockersf mockersf added this pull request to the merge queue Jul 31, 2023
Merged via the queue into bevyengine:main with commit da59de9 Jul 31, 2023
cart pushed a commit that referenced this pull request Aug 10, 2023
…_uinode_borders` (#9285)

# Objective

Remove the `With<Parent>` query filter from the `parent_node_query`
parameter of the `bevy_ui::render::extract_uinode_borders` function.
This is a bug, the query is only used to retrieve the size of the
current node's parent. We don't care if that parent node has a `Parent`
or not.

---------

Co-authored-by: François <[email protected]>
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants