Skip to content

Improve UiSurface #11423

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
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

alice-i-cecile commented Jan 19, 2024

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

@eidloi eidloi Mar 13, 2024

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.)

@janhohenheim
Copy link
Member

janhohenheim commented May 17, 2025

Triage: has merge conflicts
@doonv do you want to update this PR or should I tag it S-Needs-Adoption? :)

@janhohenheim janhohenheim added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@eidloi eidloi eidloi left review comments

Reviewers whose approvals may not affect merge requirements

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 S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@doonv @alice-i-cecile @janhohenheim @eidloi