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

Fix scrolling in UI example #8069

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Conversation

rparrett
Copy link
Contributor

Objective

Fixes #8056
Alternative to #8061

Solution

Prior to this PR, the inner container was being constrained to the height of the outer container. The scroll range was being determined by the inner container's height and a sum obtained by iterating over every list item in the container.

With this PR, I let the inner container stretch to the height of its contents. The scroll range is simple to get from the outer and inner container heights.

I'm not sure why it was implemented this way, but it's possible that this was working around a very old stretch/taffy bug or something.

There's some additional explanation of what is going on and why the old implementation worked in Bevy 0.9 but not Bevy 0.10 in #8061.

 Before                After
┌─────Container─────┐ ┌─────Container─────┐
│ ┌─ScrollingList─┐ │ │ ┌─ScrollingList─┐ │
│ │    Item 1     │ │ │ │    Item 1     │ │
│ │    Item 2     │ │ │ │    Item 2     │ │
│ │    Item 3     │ │ │ │    Item 3     │ │
│ │    Item 4     │ │ │ │    Item 4     │ │
└─┴───────────────┴─┘ └─┼───────────────┼─┘
       Item 6           │    Item 6     │
       Item 7           │    Item 7     │
       Item 8           │    Item 8     │
                        └───────────────┘

This also cleans up a few unnecessary styles.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples A-UI Graphical user interfaces, styles, layouts, and widgets labels Mar 13, 2023
Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

This looks a lot more like the kind of fix I would expect for this issue. This implementation of a ScrollView looks non-ideal as it seems that it will cause a relayout on every scroll position change. Ideally I'd want a scrollview that only affect drawing and not layout. However, that issue was already present and is not introduced by this fix.

@rparrett
Copy link
Contributor Author

rparrett commented Mar 13, 2023

This implementation of a ScrollView looks non-ideal as it seems that it will cause a relayout on every scroll position change.

I think this is probably as good as it gets in "user approachable example code," right now, right? Or do we have some way of offsetting a UI node without causing a relayout (other than modifying the GlobalTransform in PostUpdate)?

@nicoburns
Copy link
Contributor

nicoburns commented Mar 13, 2023

I think this is probably as good as it gets in "user approachable example code," right now, right?

Probably. I'm not really familiar with Bevy's rendering code. But this really ought to be built-in. In which case we could probably do something better.

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, makes much more sense and is much less complicated than the old implementation.

@mockersf mockersf 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 13, 2023
@mockersf mockersf added this to the 0.10.1 milestone Mar 13, 2023
@cart cart enabled auto-merge March 15, 2023 06:40
@cart cart added this pull request to the merge queue Mar 15, 2023
Merged via the queue into bevyengine:main with commit 1d4910a Mar 15, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
mockersf pushed a commit that referenced this pull request Mar 27, 2023
mockersf pushed a commit that referenced this pull request Mar 27, 2023
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-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples 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.

Scrolling in ui example doesn't work
6 participants