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

Improve UiSurface #11423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve UiSurface #11423

wants to merge 3 commits into from

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Jan 19, 2024 •
edited
Loading

Objective

Follow up to #11405.

Solution

This PR contains several improvements to UiSurface that will hopefully prevent crashes in the future.

I also noticed a lot of Taffy methods that, despite returning Result, never actually have an error condition. So I made those .unwrap_unchecked().

Feel free to nitpick this, I appreciate it.

Changelog

  • UiSurface Improvements
    • Added SurfaceError & SurfaceResult that will be returned by most methods of UiSurface. (Instead of panicking)
    • Changed most UiSurface methods to return SurfaceResult.
    • Minor performance improvements.

Migration Guide

  • Changed most UiSurface methods to return SurfaceResult.

@alice-i-cecile
Copy link
Member

I also noticed a lot of Taffy methods that, despite returning Result, never actually have an error condition. So I made those .unwrap_unchecked().

Can you make a PR to taffy to remove those? I'd be happy to review and release that sort of fix :)

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change labels Jan 19, 2024
@doonv
Copy link
Contributor Author

doonv commented Jan 19, 2024

Can you make a PR to taffy to remove those? I'd be happy to review and release that sort of fix :)

Looks like someone beat me to it: DioxusLabs/taffy#520

eidloi
eidloi reviewed Mar 13, 2024
View reviewed changes
crates/bevy_ui/src/layout/mod.rs
}

/// Update the children of the taffy node corresponding to the given [`Entity`].
pub fn update_children(&mut self, entity: Entity, children: &Children) {
pub fn update_children(&mut self, entity: Entity, children: &Children) -> SurfaceResult<()> {
Copy link

Choose a reason for hiding this comment

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

I ran into this so many times, thanks for the fix. However, I would like to point out that the panics we get from this is a symptom and not the cause of the issue. Nodes aren't synced well, which leads to the panics and fixing all this might hide the problem altogether, leading to inconsistencies in the visible UI. (I had this when the nodes went out of sync, the layout was all whacky, and when a sub-tree was deleted it crashed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eidloi eidloi eidloi left review comments

Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Milestone
No milestone
Development

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