Skip to content

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

Closed
ickshonpe wants to merge 4 commits intobevyengine:mainfrom
ickshonpe:fix_upsert_leaf_not_setting_a_measure_func_for_new_nodes
Closed

[Merged by Bors] - fix upsert_leaf not setting a MeasureFunc for new leaf nodes#7351
ickshonpe wants to merge 4 commits intobevyengine:mainfrom
ickshonpe:fix_upsert_leaf_not_setting_a_measure_func_for_new_nodes

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

rparrett commented Jan 24, 2023

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

ickshonpe commented Jan 24, 2023

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

rparrett commented Jan 24, 2023

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

alice-i-cecile commented Jan 24, 2023

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

@Weibye Weibye Awaiting requested review from Weibye

+1 more reviewer

@rparrett rparrett rparrett approved these changes

Reviewers whose approvals may not affect merge requirements

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