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 corresponding measure from Taffy when a CalculatedSize component is removed. #8294

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

ickshonpe
Copy link
Contributor

Objective

When a CalculatedSize component from a UI Node entity is removed, the corresponding Taffy measure isn't removed which will mess up the layout in confusing, unpredictable ways.

Solution

Iterate through all the entities with removed CalculatedSize components and remove the corresponding Taffy measures.

* Added a `RemovedComponents<CalculatedSize>` parameter to `flex_node_system`
* Iterate through all entities with removed `CalculatedSize` components and remove the corresponding Taffy measure, if it exists.
@@ -320,6 +328,11 @@ pub fn flex_node_system(
// clean up removed nodes
flex_surface.remove_entities(removed_nodes.iter());

// When a `CalculatedSize` component is removed from an entity, we need to remove the measure from the corresponding taffy node.
for entity in removed_calculated_sizes.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about moving this into a separate system?

This one seems to be getting quite big and doing a lot of different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely needs to be done, and I found this bug while working on some other changes that require flex_node_system to be split up. I'm not quite sure how they'll work out yet though, so I just put in this PR that fixes the bug with minimal changes etc.

@TimJentzsch
Copy link
Contributor

How challenging would it be to add a test for this?
I think that would be very nice to have to avoid regressions in the future

@ickshonpe
Copy link
Contributor Author

How challenging would it be to add a test for this? I think that would be very nice to have to avoid regressions in the future

It's not too difficult, but flex_node_system needs to be split up so we can run just the layout algorithms without a window.

@james7132 james7132 added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Apr 4, 2023
@cart cart added this pull request to the merge queue Apr 5, 2023
Merged via the queue into bevyengine:main with commit 6e67d3e Apr 5, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants