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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - fix upsert_leaf not setting a MeasureFunc for new leaf nodes #7351

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - fix upsert_leaf not setting a MeasureFunc for new leaf nodes #7351

wants to merge 4 commits into from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jan 24, 2023 •
edited
Loading

Problem

The upsert_leaf method creates a new MeasureFunc and, if required, a new leaf node, but then it only adds the new MeasureFunc to existing leaf nodes.

Solution

Add the MeasureFunc to new leaf nodes as well.

@ickshonpe
changes:
a83c9ef 
    The upsert_leaf method creates a new MeasureFunc and if required a new leaf node,
    but the new MeasureFunc was only added to existing leaf nodes, not newly created nodes.
    This adds the new MeasureFunc to new leaf nodes as well.
    I'm not quite sure how much impact this had or whether this fixes any other bugs.
    It might have caused slight glitches or some UI updates to be delayed for a frame.
@ickshonpe
cargo fmt --all
fb814ad
@rparrett
Copy link
Contributor

Oops, this seems like a regression from #6743.

I think this could be a one line diff with new_leaf_with_measure?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Jan 24, 2023
alice-i-cecile
alice-i-cecile requested changes Jan 24, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This LGTM, but I agree that we should be able to do this more concisely.

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 24, 2023
@ickshonpe
Copy link
Contributor Author

Oops, this seems like a regression from #6743.

ah, I should have checked the history.

I think this could be a one line diff with new_leaf_with_measure?

I think this version is probably better, even though it's more verbose.
In the original, the two code paths confound the unused_variable lint.

@rparrett
Copy link
Contributor

Hm. At a glance, new_leaf_with_measure seems to do less work than new_leaf + set_measure.

@alice-i-cecile alice-i-cecile requested a review from Weibye January 24, 2023 15:04
@rparrett
Copy link
Contributor

rparrett commented Jan 24, 2023 •
edited
Loading

In the original, the two code paths confound the unused_variable lint.

I'm not seeing this on my end. CI runs without error locally with the minimal fix.

@alice-i-cecile alice-i-cecile 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 Jan 24, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 24, 2023
@ickshonpe
fix upsert_leaf not setting a MeasureFunc for new leaf nodes (#7351)
cf612c8 
# Problem

The `upsert_leaf` method creates a new `MeasureFunc` and, if required, a new leaf node, but then it only adds the new `MeasureFunc` to existing leaf nodes.

## Solution

Add the `MeasureFunc` to new leaf nodes as well.
@bors
Copy link
Contributor

bors bot commented Jan 24, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title fix upsert_leaf not setting a MeasureFunc for new leaf nodes [Merged by Bors] - fix upsert_leaf not setting a MeasureFunc for new leaf nodes Jan 24, 2023
@bors bors bot closed this Jan 24, 2023
@ickshonpe
Copy link
Contributor Author

ickshonpe commented Jan 24, 2023 •
edited
Loading

In the original, the two code paths confound the unused_variable lint.

I'm not seeing this on my end. CI runs without error locally with the minimal fix.

I meant that in the concise version, the linter didn't detect that the MeasureFunc variable was unused because it was still consumed on the other code path. The longer version is more robust in that we'd get an unused variable warning in that case. I agree that it isn't clear whether there is some hidden performance cost though. Anyway, it doesn't seem worth obsessing about so I put things back as they were.

@rparrett
Copy link
Contributor

rparrett commented Jan 24, 2023 •
edited
Loading

Makes sense, thanks for clarifying. Agreed.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@ickshonpe @ItsDoot
fix upsert_leaf not setting a MeasureFunc for new leaf nodes (bevyeng…
7753e60 
…ine#7351)

# Problem

The `upsert_leaf` method creates a new `MeasureFunc` and, if required, a new leaf node, but then it only adds the new `MeasureFunc` to existing leaf nodes.

## Solution

Add the `MeasureFunc` to new leaf nodes as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@rparrett rparrett rparrett approved these changes

@Weibye Weibye Awaiting requested review from Weibye

Assignees
No one assigned
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
Milestone
0.10
Development

Successfully merging this pull request may close these issues.
3 participants
@ickshonpe @rparrett @alice-i-cecile