-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
UI Slider Widget #7116
UI Slider Widget #7116
Conversation
Co-authored-by: Mike <mike.hsu@gmail.com>
This is only controversial because it's the first real widget, and will set the tone for others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After taking a closer look at Angular Material Slider Example I understand better your initial approach. Apologies for not seeing what you were seeing at the time.
This is the slider widget as you specified it earlier.
struct SliderWidget {
value: f32
min: f32
max: f32
step: f32
}
Let's consider an example of a slider of Unicorns:
In the Angular Material example, the contents of the slider widget is concrete. Meaning that min
, max
and step
and value
refers to unicorns.
- What is the minimum and maximum amount of unicorns the user can specify?
step
is how many unicorns should we increase or decrease with anytime we move the slider.- When "submitting the form" the contents of
value
is the final amount of unicorns you wanted.
This means that you encode "what the values of the slider-widget means" on a widget level (cars, unicorns, particles, ++) and in order to figure out where to position the slider-handle you calculate the abstract progress: value / max - min
.
Over in the previous pr #6236 I gave the feedback that perhaps all this was unnecessary and in order to know the position of the slider-handle we kinda only need the value
as long as value is kept in a range of 0.0..=1.0
struct SliderWidget {
value: f32
}
And while that lets us know where to place the slider-handle it really does not address "where do we translate from the abstract progress value 0.0..=1.0
into Unicorns, which is what we really are interested in when "submitting the form" or otherwise polling the data.
And at this point I really don't have a good answer to that.
So I propose we go with your initial design as that is something I can see answering both these issues:
struct SliderWidget {
value: f32
min: f32
max: f32
step: f32
}
On a detail level, would it make sense to have the slider handle store its own progress value and split them up into multiple systems?
Idea:
struct SliderWidget {
value: f32
min: f32
max: f32
step: f32,
slider_handle: Entity
}
struct SliderHandle(f32);
// One system that checks user input and updates the SliderWidget values
fn update_slider_widget( .. ) { .. }
// One system that updates the slider-handles based on the SliderWidgets
fn update_slider_handles(widgets: Query<&SliderWidget>, handles: Query<&mut SliderHandle>) {
for widget in &mut widgets {
if let Ok(mut handle) = handles.get(widget.slider_handle) {
handle.0 = widget.value / widget.max - widget.min;
}
}
}
// One system that updates the styles based on the slider-handle values
fn update_slider_handle_positions(handles: Query<&SliderHandle, &Node, &mut Style>, ) {
for (handle, node, mut style) in &handles {
// do the style.position update here
}
}
I don't think we want to couple the |
In general, I like where this is going, but I think this could be split in a couple of simpler PRs. For example, the RelativeCursorPosition could be it's own PR and I think introducing a WidgetPlugin + ImagePlugin could also be it's own PR. With this, this PR would be strictly limited to only adding support for a SliderWidget and nothing else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely on the right path!
The slider example should showcase a continuous slider as well as a stepped slider. Especially we need to showcase how min
and max
of the slider can be used to represent different things.
@@ -197,6 +198,43 @@ pub struct ButtonBundle { | |||
pub z_index: ZIndex, | |||
} | |||
|
|||
/// A UI node that is a slider | |||
#[derive(Bundle, Clone, Debug, Default)] | |||
pub struct SliderBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this over to the slider.rs
and have everything be contained there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Honestly think we should do the same with ButtonBundle
, TextBundle
and ImageBundle
and just keep the core building blocks for UI in node_bundles.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like having every bundles in a single file and not need to search around to find each bundle definition, but I understand that this won't scale super well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I want to do this in this PR. It was originally supposed to be a simple slider widget, but it's slowly turning into a widget rework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a case of it being the first "somewhat" advanced widget and will define the structure for all widgets. But I do agree with @IceSentry that we should split it apart into 3 PRs:
- Widget restructuring with WidgetPlugin
- RelativeCursorPosition functionality that will be useful for multiple systems
- SliderWidget that depends on both of the previous PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's continue the WidgetPlugin
discussion over here, if necessary #7190
|
||
/// A UI node that is a slider | ||
#[derive(Bundle, Clone, Debug)] | ||
pub struct SliderHandleBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely on the right path!
The slider example should showcase a continuous slider as well as a stepped slider. Especially we need to showcase how min
and max
of the slider can be used to represent different things.
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
I wonder whether the slider value will be updated if the UI is invisible. |
Backlog cleanup: closing due to inactivity, and in favour of the ongoing Bevy editor UI work. |
Yeah, fundamentally we need a blessed widget abstraction before widgets with more than one entity can realistically be merged :( |
Objective
Added a premade UI slider widget. Fixes #6196.
Solution
RelativeCursorPosition
component that updates alongsideInteraction
, which stores the cursor position relative to the node.Slider
component that stores values specific to slidersSliderBundle
and aSliderHandleBundle
update_slider_value
andupdate_slider_handle
systemsui
directoryChangelog
RelativeCursorPosition
componentSlider
componentSliderBundle
SliderHandleBundle
update_slider_value
andupdate_slider_handle
systemsYou might've seen the same pull request before. Unfortunately, I had been inactive for some time and when I returned, the original pull request (#6236) had merge conflicts. I've tried fixing them, but failed miserably, since I've never done it before and decided to just write the slider again and open another PR.