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

Improve UI update_clipping_system comments #8147

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

doup
Copy link
Contributor

@doup doup commented Mar 21, 2023

Objective

  • Improve update_clipping_system comments

Solution

  • Add extra comments
  • Reorder CalculatedClip updating match so it's reads (old, new) vs previous (new, old)
  • Remove clone on children iteration

Copy link
Contributor

@ickshonpe ickshonpe 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. No problems with the changes but just got a few (a lot now 😄) suggestions for extra improvements.

crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
Comment on lines 51 to 56
(Some(mut old_clip), Some(clip)) => {
if old_clip.clip != clip {
*old_clip = CalculatedClip { clip };
}
}
}
Copy link
Contributor

@ickshonpe ickshonpe Mar 21, 2023

Choose a reason for hiding this comment

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

I was going to suggest to use set_if_neq but then I checked and we never actually query for changes in CalculatedClip so the neq gate is unnecessary:

Suggested change
(Some(mut old_clip), Some(clip)) => {
if old_clip.clip != clip {
*old_clip = CalculatedClip { clip };
}
}
}
(Some(mut calculated_clip), Some(inherited_clip)) => *calculated_clip = CalculatedClip { inherited_clip },
}

crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
@ickshonpe
Copy link
Contributor

#8095 that is very related needs some reviews, be grateful if you have a few minutes spare to look at it.

@doup
Copy link
Contributor Author

doup commented Mar 22, 2023

Thanks for the comments! I was going to review them, but suddenly I'm getting a segfault out of nowhere. So frustrating. The good thing is that I can learn how this kind of stuff is debugged.

Anyway, sure I can check #8095. Btw, I want to review more UI PRs (mostly yours and nico's), but first I want to finish reading the UI related code. I did few reviews last couple of weeks but I realized I was lacking understanding. ^_^

@doup doup mentioned this pull request Mar 22, 2023
@james7132 james7132 added C-Docs An addition or correction to our documentation A-UI Graphical user interfaces, styles, layouts, and widgets labels Mar 22, 2023
Co-Authored-By: ickshonpe <27962798+ickshonpe@users.noreply.github.com>
@doup
Copy link
Contributor Author

doup commented Mar 22, 2023

@ickshonpe I've applied most of the suggestions. I think I'm going to skip #8147 (comment) I prefer not to touch that part. In any case, I've asked about it in https://github.com/bevyengine/bevy/pull/8095/files#r1144342397, we can follow there.

@hymm hymm 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 Mar 24, 2023
@james7132 james7132 added this pull request to the merge queue Mar 28, 2023
Merged via the queue into bevyengine:main with commit e2b8cc8 Mar 28, 2023
@doup doup deleted the improve-ui-clip-docs branch March 30, 2023 10:38
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-Docs An addition or correction to our documentation 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.

5 participants