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. |
Weibye
left a comment
There was a problem hiding this comment.
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?
stepis how many unicorns should we increase or decrease with anytime we move the slider.- When "submitting the form" the contents of
valueis 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. |
Weibye
left a comment
There was a problem hiding this comment.
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.
|
|
||
| /// A UI node that is a slider | ||
| #[derive(Bundle, Clone, Debug, Default)] | ||
| pub struct SliderBundle { |
There was a problem hiding this comment.
Could we move this over to the slider.rs and have everything be contained there?
There was a problem hiding this comment.
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.
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.
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.
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.
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 { |
Weibye
left a comment
There was a problem hiding this comment.
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
RelativeCursorPositioncomponent that updates alongsideInteraction, which stores the cursor position relative to the node.Slidercomponent that stores values specific to slidersSliderBundleand aSliderHandleBundleupdate_slider_valueandupdate_slider_handlesystemsuidirectoryChangelog
RelativeCursorPositioncomponentSlidercomponentSliderBundleSliderHandleBundleupdate_slider_valueandupdate_slider_handlesystemsYou 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.