General `Progress` representation for UI Widgets by Weibye · Pull Request #6620 · bevyengine/bevy

Weibye · 2022-11-14T18:52:30Z

Objective

Both the SliderWidget UI Slider Widget #6236 and the ProgressBarWidget Adding a progress-bar widget #6517 are going to need some representation of progress.
As we've discussed in those PRs, it would make sense to have both of them use the same underlying representation.

Solution

This attempts setting up that underlying and general representation of progress. It is indented to solve the needs of the widgets but ideally would be suitable for additional purposes such as gameplay or loading.
Additional identified use-case: Getting Progress from a Task in a AsyncComputeTaskPool

~~Please review and critique both the design but also where to place it. Maybe bevy_ui is not the best place for this in the long term?~~
~~My intention is that this will be moved over to the bevy_widgets-crate once that gets set up (see Adding a progress-bar widget #6517 (review)) but there might be other, better places for it.~~
Consensus is that it should live in bevy_utils.

Weibye · 2022-11-14T18:53:52Z

@AxiomaticSemantics, @Pietrek14, review please :)

ManevilleF

Interesting tooling, I wonder if having Num as a trait bound would be useful, as it probably will always be numeric

ManevilleF · 2022-11-14T19:25:18Z

+where
+    T: PartialOrd<T>,
+{
+    pub fn new(value: T, min: T, max: T) -> Self {


Should this function check if value is actually in range ?

It does, by calling the from_range(), which has the check.

Weibye · 2022-11-14T19:34:27Z

Interesting tooling, I wonder if having Num as a trait bound would be useful, as it probably will always be numeric

That is probably a very reasonable assumption. I can't think of any use-cases where this would not be a number of some kind.

@ManevilleF Is there any Num traits in the std lib? I can only find mentions of num-traits and I'm a bit hesitant to pull additional dependencies in for this use case.

Pietrek14 · 2022-11-14T20:53:32Z

+    pub fn set_progress(&mut self, new_value: T) -> Result<T, &str> {
+        if self.bounds().contains(&new_value) {
+            self.value = new_value;
+            Ok(self.value)


I don't think you need to return any value. An Ok(()) should be enough here.

alice-i-cecile · 2022-11-14T21:02:52Z

It is indented to solve the needs of the widgets but ideally would be suitable for additional purposes such as gameplay or loading.

With this in mind, I would actually consider sticking it in bevy_utils? Folks who are using a different UI solution should feel okay importing this.

alice-i-cecile

Some doc nits. Additionally:

This should live in bevy_utils
Add, AddAssign, Sub and SubAssign traits.
ProgressError type.
progress_normalized -> normalized method name.
percent() constructor method.
Derive Clone and PartialEq on the struct.

Weibye · 2022-11-14T22:07:05Z

Moved everything to bevy_utils and I think I have addressed all comments. feel free to add more :)

alice-i-cecile

There's progress! Some feedback on tests, but once that's done this LGTM.

Weibye · 2022-11-15T12:46:54Z

Additional use-case: Getting Progress from a Task in a AsyncComputeTaskPool

Weibye · 2022-11-15T17:37:10Z

@mockersf Could you please have a look at this from a "general utility across the engine"-perspective? I'm sure you have some valuable insight.

alice-i-cecile · 2022-11-26T20:38:42Z

Controversial label added because while this is well-made, it's not entirely clear it's worth the complexity.

sixfold-origami · 2022-11-26T20:43:08Z

I took a quick look at this, and honestly it doesn't really seem necessary. I really don't think "what percentage full is this thing" is not a complicated piece of functionality to abstract over. Over in traditional UI, we've been doing just fine without a special construct for progress, since percentFill: f32 works just fine in nearly all cases.

Additionally, I think this abstraction will quickly start to get applied to too many different use cases to still be meaningful. In my experience, this sort of functionality quickly has its meaning diluted as it's improperly applied to different contexts. Already, I would argue that input sliders and loading bars are two separate areas that don't need to be coupled.

mockersf · 2022-11-26T22:17:32Z

Already, I would argue that input sliders and loading bars are two separate areas that don't need to be coupled.

Speaking of "progress" for a slider doesn't make much sense...

Pietrek14 · 2022-11-26T22:42:32Z

I think this would be useful, else we would have to repeat this logic in Slider (#6236) and ProgressBar (#6517), which is not the end of the world, but I would much rather have it as a seperate struct. This is a common enough use-case to probably appear in future UI widgets, so rewriting the same thing in all of them could be pretty cumbersome.

As to the name, I feel like calling the slider value "progress" isn't that uncommon, though I don't mind if the name is ultimately changed.

Weibye · 2022-11-27T10:36:12Z

Thanks for all the feedback! Conclusion is to close this and use a normalized float value of "progress" 0.0..=1.0 for the UI elements.

This can always be reopened if there's a different need where this is the right tool for the job.

Weibye requested a review from alice-i-cecile November 14, 2022 18:53

ManevilleF reviewed Nov 14, 2022

View reviewed changes

Weibye changed the title ~~General Progress representation~~ General Progress representation for UI Widgets Nov 14, 2022

Weibye added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Utils Utility functions and types labels Nov 14, 2022

Pietrek14 reviewed Nov 14, 2022

View reviewed changes

Comment thread crates/bevy_ui/src/lib.rs Outdated

Comment thread crates/bevy_ui/src/lib.rs Outdated

Weibye marked this pull request as ready for review November 14, 2022 20:46

Pietrek14 reviewed Nov 14, 2022

View reviewed changes

alice-i-cecile reviewed Nov 14, 2022

View reviewed changes

Comment thread crates/bevy_math/src/lib.rs Outdated

alice-i-cecile reviewed Nov 14, 2022

View reviewed changes

Comment thread crates/bevy_ui/src/lib.rs Outdated

alice-i-cecile reviewed Nov 14, 2022

View reviewed changes

Comment thread crates/bevy_ui/src/lib.rs Outdated

alice-i-cecile requested changes Nov 14, 2022

View reviewed changes

alice-i-cecile reviewed Nov 15, 2022

View reviewed changes

Comment thread crates/bevy_utils/src/lib.rs Outdated

alice-i-cecile reviewed Nov 15, 2022

View reviewed changes

Comment thread crates/bevy_utils/src/lib.rs Outdated

alice-i-cecile requested changes Nov 15, 2022

View reviewed changes

Pietrek14 reviewed Nov 15, 2022

View reviewed changes

Comment thread crates/bevy_utils/src/lib.rs Outdated

Weibye commented Nov 15, 2022

View reviewed changes

Comment thread crates/bevy_utils/src/progress.rs Outdated

Weibye requested a review from mockersf November 15, 2022 17:35

Weibye requested review from ManevilleF, Pietrek14 and alice-i-cecile and removed request for ManevilleF November 15, 2022 21:02

Weibye added 20 commits November 24, 2022 22:12

Use un-prefixed getters

a6ae752

set_progress now returns Result

5815bec

Inline function

0676a07

Update comment

8359502

map_range -> remap_range

33daf2a

Move everything over to bevy_utils

7f81a05

Formatting

1305464

Revert any changes to bevy_ui

7804a09

Derive Clone and PartialEq

1dee2a9

progress_normalized -> normalized

942a477

Doc-fixes, cleanup and improvements

8a82b97

Formatting

c35573b

Derive Eq as well

1addc74

Adding tests

31b06f9

use assert_eq and assert_ne instead

c28d651

Add / Sub assign and more tests

b2787df

Moving Progress to own module

af087de

Adding thiserror

bdd7b51

Use same thiserror as all other crates in bevy

cf2e379

Clamp instead of panic

e98edb5

Weibye force-pushed the progress-struct branch from fd7c7e2 to e98edb5 Compare November 24, 2022 21:13

Downscope to no longer be generic

2a320e3

alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label Nov 26, 2022

Weibye closed this Nov 27, 2022

Weibye mentioned this pull request Jan 7, 2023

UI Slider Widget #6236

Closed

Uh oh!

Uh oh!

Conversation

Weibye commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

Weibye commented Nov 14, 2022

Uh oh!

ManevilleF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ManevilleF Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Weibye Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Weibye commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pietrek14 Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Weibye commented Nov 14, 2022

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Weibye commented Nov 15, 2022

Uh oh!

Uh oh!

Uh oh!

Weibye commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Nov 26, 2022

Uh oh!

sixfold-origami commented Nov 26, 2022

Uh oh!

mockersf commented Nov 26, 2022

Uh oh!

Pietrek14 commented Nov 26, 2022

Uh oh!

Weibye commented Nov 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Weibye commented Nov 14, 2022 •

edited

Loading

Weibye commented Nov 14, 2022 •

edited

Loading

alice-i-cecile commented Nov 14, 2022 •

edited

Loading

Weibye commented Nov 15, 2022 •

edited

Loading