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

Add clamp_to_range option to DragValue, rename clamp_range to range (deprecating the former) #4728

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

Wumpf
Copy link
Collaborator

@Wumpf Wumpf commented Jun 28, 2024

Adds the ability to have DragValue not clamp values its presented with and instead apply clamping only once there's any user input.

In action:

Screen.Recording.2024-06-28.at.11.52.22.mov

Alternative name could be only_clamp_on_change, not entirely certain which one is better 🤔

Copy link
Owner

@emilk emilk 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 the feature, but am not sold on the API. Here are three alternatives:

A: follow Slider

egui::Slider always takes a range, but doesn't clamp the input unless the user also calls .clamp_to_range(true). We could do the same with DragValue (changing its behavior) and have something like DragValue::new(…).range(…).clamp_to_range(true), thus more closely mirroring the Slider API. It would mean renaming clamp_range to range (with a deprecation notice) and changing the default behavior (current default: clamping all input, new default

B: have two range functions

DragValue doesn't have a range to clamp to by default. This means DragValue::new(…).only_clamp_on_input(…) is non-sensical.

If we split DragValue::clamp_range into two different functions, e.g. DragValue::clamp_io_range and clamp_output_range, we force the user to make a choice of what behavior they want.

C: clamp_range with enum

Another alternative is .clamp_range(a..=b, egui::ClampBehavior::Always) vs .clamp_range(a..=b, egui::ClampBehavior::OnEdit) or similar. We could use the same enum in Slider


Out of the three alternatives, I'm leaning weakly towards A. WDYT?

@Wumpf
Copy link
Collaborator Author

Wumpf commented Jun 28, 2024

👍 agreed! A sounds best by far to me

@Wumpf
Copy link
Collaborator Author

Wumpf commented Jun 28, 2024

uh the slider docs are actually inconsistent

/// By default, the slider can still show values outside this range,
/// and still allows users to enter values outside the range by clicking the slider value and editing it.
/// If you want to clamp incoming and outgoing values, use [`Slider::clamp_to_range`].

but then

    /// If set to `true`, all incoming and outgoing values will be clamped to the slider range.
    /// Default: `true`.
    #[inline]
    pub fn clamp_to_range(mut self, clamp_to_range: bool) -> Self {

clamp_to_range does indeed default to true. So the slider's docs aren't accurate.
Fixing the docs and following suite on the DragValue, making it clamp by default

@Wumpf Wumpf changed the title Add only_clamp_on_input option to DragValue Add clamp_to_range option to DragValue, rename clamp_range to range (deprecating the former) Jun 28, 2024
@Wumpf Wumpf force-pushed the drag-value-only-apply-change-on-input branch from d615bf7 to 4a5b3b4 Compare June 28, 2024 12:39
@Wumpf Wumpf requested a review from emilk June 28, 2024 12:44
@Wumpf
Copy link
Collaborator Author

Wumpf commented Jun 28, 2024

tested against egui demo and updated rerun version

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Awesome! (assuming you have tested this thoroughly)

crates/egui/src/widgets/drag_value.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/drag_value.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/slider.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf merged commit 10f092d into master Jun 28, 2024
35 checks passed
@Wumpf Wumpf deleted the drag-value-only-apply-change-on-input branch June 28, 2024 14:07
emilk added a commit to rerun-io/rerun that referenced this pull request Jun 28, 2024
…ot changed by looking at them (#6677)

### What

* Fixes #6633
* Depends on emilk/egui#4728


https://github.com/rerun-io/rerun/assets/1220815/7b1d06aa-9235-4545-b3d8-ae4ead26d4f5


Undraft condition:
* [x] land egui pr and point to master branch here instead

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6677?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6677?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6677)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jul 6, 2024
jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 2024
emilk added a commit that referenced this pull request Sep 17, 2024
When using a `DragValue`, there are three common modes of range clamping
that the user may want:

A) no clamping
B) clamping only user input (dragging or editing text), but leave
existing value intact
C) always clamp

The difference between mode B and C is:

```rs
let mut x = 42.0;
ui.add(DragValue::new(&mut x).range(0.0..=1.0));
// What will `x` be here?
```

With this PR, we now can get the three behaviors with:

* A): don't call `.range()` (or use `-Inf..=Inf`)
* B) call `.range()` and `.clamp_existing_to_range(false)`
* C) call `.range()`

## Slider clamping
Slider clamping is slightly different, since a slider always has a
range.

For a slider, there are these three cases to consider:

A) no clamping
B) clamp any value that the user enters, but leave existing values
intact
C) always clamp all values

Out of this, C should probably be the default.

I'm not sure what the best API is for this yet. Maybe an `enum` 🤔 


I'll take a pass on that in a future PR.

## Related
* #4728
* #4881
* #4882
emilk added a commit that referenced this pull request Sep 17, 2024
This deprecates `.clamp_to_range` in favor of more control using
`.clamping`.

## Related
* #4728
* Closes #4881
* #4882
* #5118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants