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

Adding a progress-bar widget #6517

Closed
wants to merge 5 commits into from
Closed

Conversation

Weibye
Copy link
Contributor

@Weibye Weibye commented Nov 7, 2022

Objective

Solution

2022-11-07.22-47-29.mp4
  • Same as UI Slider Widget #6236, the general design-approach here is "Widgets as lose collections of hierarchies of bundles".
  • For the loading-bar widget to work, the user needs to create a NodeBundle with a ProgressBarWidget-component attached, that has a child NodeBundle with a ProgressBarInner-component.
  • It is completely up to the user to make sure these requirements are met.

Changelog

Added

  • Progress bar widget

Notes

  • Please critique this general approach. I know how we can setup a lot of other widgets the same way, but I have not yet any clear answers for the bigger-picture problems such as "widget composition" and "widget styling".
  • My gut feeling says it will be easier to tackle those problems once we have some real uses of various widgets in Bevy.

@Weibye Weibye added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets labels Nov 7, 2022
@alice-i-cecile
Copy link
Member

@bzm3r review plz!

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Specific feedback about this widget:

  • genuinely useful
  • needs to be more robust to misconfiguration and alternate use cases: vertical orientation, LTR, etc
  • needs a name that encourages devs to use it as e.g. a life bar. ProgressBar maybe?
  • needs an example demonstrating how to use and position it in screen space (yeah I know this currently sucks)

General thoughts:

  • the "spawn complex trees of widgets" pattern is miserable. So much boilerplate, so hard to parse
  • updating the value of data within components is great
  • I worry about the poor scaling of O(n) change detection systems every frame for elements that rarely change. A push-based model feels more natural for thing like buttons
  • differentiating widgets of the same type is going to get annoying without general-purpose relations unless you use an event-based architecture (which is annoying in its own ways)
  • widgets really feel like they belong in their own bevy_widgets crate?
  • I really want to be able to disable / not include systems for widgets I'm not using in my game: the random perf loss every single frame bothers me

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 7, 2022
@Weibye Weibye changed the title Adding a loading-bar widget Adding a progress-bar widget Nov 8, 2022
@Weibye
Copy link
Contributor Author

Weibye commented Nov 8, 2022

widgets really feel like they belong in their own bevy_widgets crate?

We should also setup a new category of examples for widgets IMO. Should these two things happen in this PR, or somewhere else?

@Weibye Weibye marked this pull request as draft November 8, 2022 08:11
- Start cleanup
@alice-i-cecile
Copy link
Member

I think in this PR: it's already going to be controversial, so let's do this right from the start.

@@ -67,6 +68,7 @@ bevy_scene = ["bevy_internal/bevy_scene"]
bevy_sprite = ["bevy_internal/bevy_sprite"]
bevy_text = ["bevy_internal/bevy_text"]
bevy_ui = ["bevy_internal/bevy_ui"]
bevy_widget = ["bevy_internal/bevy_widget"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know if this is the correct setup, please let me know how to adjust.

Comment on lines +26 to +29
/// Maps a value from one range of values to a new range of values.
pub fn map_range(value: f32, old_range: (f32, f32), new_range: (f32, f32)) -> f32 {
(value - old_range.0) / (old_range.1 - old_range.0) * (new_range.1 - new_range.0) + new_range.0
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to bevy_math: Couldn't find anything that was already providing the same functionality but I may be mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we make this discoverable for others down the line?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we make this discoverable for others down the line?

Perhaps as an extension to either scalars or Range values, such that we can have either value.map_range(a..b, c..d) or (a..b).map_range(c..d, value)? In the former case the intervals are more intuitive (a..b is clearly the "old" range of value), whereas in the latter the arguments would be reversed, such that a..b becomes the output range.

@Weibye Weibye marked this pull request as ready for review November 8, 2022 19:50
// ui camera
commands.spawn(Camera2dBundle::default());

// background that fills the entire viewport
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Why not just use the default clear color of bevy? I'm not really against this, but just trying to see where the example could be simplified a little.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you need the root node to take the whole screen to center the progress bar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Previously we have been using auto-margin to centre elements on the page. This is not the best practice, see #6535. In order to centre the element you need the parent element to cover the whole area.

The root / background does not strictly need a background colour, I added it for stylistic choices but can remove it if desired.

@ghost
Copy link

ghost commented Nov 10, 2022

I'd like to see support for generics for min, max, progress values if that is reasonable. (Most of my use of progress bars are based on {u,i}N values)

This implementation should likely be coordinating with the proposed UI slider widget WRT internal representation of progress.

@Weibye
Copy link
Contributor Author

Weibye commented Nov 12, 2022

I'd like to see support for generics for min, max, progress values if that is reasonable. (Most of my use of progress bars are based on {u,i}N values)

I'm not too versed in generic rust yet, so I'm not entirely sure how that would look but if you'd provide a code snippet of what that would look like, I'm happy to include it :)

Copy link
Contributor

@bzm3r bzm3r left a comment

Choose a reason for hiding this comment

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

I like this.

Comment on lines +26 to +29
/// Maps a value from one range of values to a new range of values.
pub fn map_range(value: f32, old_range: (f32, f32), new_range: (f32, f32)) -> f32 {
(value - old_range.0) / (old_range.1 - old_range.0) * (new_range.1 - new_range.0) + new_range.0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we make this discoverable for others down the line?


impl Plugin for WidgetPlugin {
fn build(&self, app: &mut App) {
app.register_type::<ProgressBarWidget>()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about ProgressBar as a name. The core widget here seems to be much more general than just a bar for progress.

A rough idea for a more general/useful name: ResourceMeter? Not sold on that idea, but it is more general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general UI terminology, the name of the widget is usually "ProgressBar".

A loading screen, a health bar, mana bar, XP bar, various resource trackers you find in games are just specific implementations of that widget.

/// Creates a new [`ProgressBarWidget`].
pub fn new(progress: f32, min: f32, max: f32) -> Self {
if min > max {
panic!("Min should not be larger than max");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be sufficient to just put an assert here? assert(!(min > max), "Min should not be larger than max")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that might be a better option

@bas-ie
Copy link
Contributor

bas-ie commented Oct 6, 2024

Backlog cleanup: closing due to inactivity, and in light of the significant development effort surrounding Bevy editor which will be surfacing any amount of new UI/widgets.

@bas-ie bas-ie closed this Oct 6, 2024
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-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants