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

Slider unable to produce range of odd values #3483

Closed
ExpertOfNil opened this issue Oct 17, 2023 · 3 comments · Fixed by #3488
Closed

Slider unable to produce range of odd values #3483

ExpertOfNil opened this issue Oct 17, 2023 · 3 comments · Fixed by #3488
Labels
bug Something is broken

Comments

@ExpertOfNil
Copy link

Description

When defining the egui::Slider bounds with a range having odd bounds (e.g. 3..=31) and step size of 2_f64, instead of having selectable values {3, 5, 7, ..., 29, 31}, values {4, 6, 8, ..., 30, 31} are provided. This is not remedied with .clamp_to_range(true). Similarly, providing a range 3..=501 and a step size of 2_f64 produces values {4, 10, 20, 30, ..., 490, 500, 501}. NOTE this scenario is also not remedied by using ranges starting at 1.0, either.

To Reproduce

egui::Grid::new("CameraConfigGroup")
    .num_columns(2)
    .spacing([40.0, 4.0])
    .striped(true)
    .show(ui, |ui| {
        ui.add(egui::Label::new("Value 1:"));
        ui.add(
            egui::Slider::new(&mut self.slider_value1, 3.0..=31.0)
                .step_by(2_f64)
                .fixed_decimals(1),
        );
        ui.end_row();
        ui.add(egui::Label::new("Value 2:"));
        ui.add(
            egui::Slider::new(&mut self.canny_edges.threshold2, 3.0..=501.0)
                .step_by(2_f64)
                .fixed_decimals(1),
        );
    });

Expected Behavior

  • Value 1: Selectable values {3.0, 5.0, 7.0, ..., 29.0, 31.0}
  • Value 2: Selectable values {3.0, 5.0, 7.0, ..., 499.0, 501.0}

Actual Behavior

  • Value 1: Slider may start at 3.0, but once moved, it will only provide values {4.0, 6.0, 8.0, ..., 30.0, 31.0}, even when manually editing the text value
  • Value 2: Slider may start at 3.0, but once moved, it will only provide values {4.0, 10.0, 20.0, ..., 490.0, 500.0, 501.0}. Manually editing the text will only allow even numbers, with the exception of 501.0.

Screenshots

image
image
image
image

Hypothesis

I believe this may be the cause of the issue (slider.rs, lines 501-514):

fn set_value(&mut self, mut value: f64) {
    if self.clamp_to_range {
        let start = *self.range.start();
        let end = *self.range.end();
        value = value.clamp(start.min(end), start.max(end));
    }
    if let Some(max_decimals) = self.max_decimals {
        value = emath::round_to_decimals(value, max_decimals);
    }
    if let Some(step) = self.step {
        value = (value / step).round() * step;
        // should be: value = (value + step).round()?
    }
    set(&mut self.get_set_value, value);
}

Desktop

  • OS: Ubuntu 20.04
  • Eframe: v0.23.0
    • Also tried using eframe = { git = "https://github.com/emilk/egui", branch = "master", features = ["persistence"] }
@ExpertOfNil ExpertOfNil added the bug Something is broken label Oct 17, 2023
@YgorSouza
Copy link
Contributor

The solution is probably to take the start of the range into account when rounding.

if let Some(step) = self.step {
    let start = self.range.start();
    value = start + ((value - start) / step).round() * step;
}

@ExpertOfNil
Copy link
Author

If I'm understanding what's going on in Slider::set_value correctly, it can be mocked up using Slider::from_get_set:

let range = 3.0..=31.0;
let start = *range.start();
ui.add(egui::Slider::from_get_set(
    range,
    |v: Option<f64>| -> f64 {
        if let Some(v) = v {
            self.slider_value = start + ((v - start) / 2_f64).round() * 2_f64;
        }
        return self.slider_value;
    },
));

Using @YgorSouza suggestion, I get the desired behavior.

@emilk
Copy link
Owner

emilk commented Oct 19, 2023

Yeah, I agree that the step should start at the start value, and @YgorSouza's solution seems sounds - please make a PR 🙏

YgorSouza added a commit to YgorSouza/egui that referenced this issue Oct 19, 2023
emilk pushed a commit that referenced this issue Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants