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

SliderFloat(), DragFloat(): Clamp the value returned from the input box #413

Closed
wants to merge 1 commit into from

Conversation

nmlgc
Copy link

@nmlgc nmlgc commented Nov 25, 2015

v is updated with the value of the input box on every edit, but it should never be set to a value outside the [v_min, v_max] range.

@ocornut
Copy link
Owner

ocornut commented Nov 25, 2015

This is intended by design to allow going outside the bounds when explicitly typing a value.
Did you feel it was a bug, or did you have a real issue with that?
I would consider adding it as an option but we have a problem fitting options in those API.

@nmlgc
Copy link
Author

nmlgc commented Nov 25, 2015

Even with the current behavior, the value from the input box actually is clamped to the bounds after hitting Return, but not while the user is still typing. This commit would simply clamp the value after every keystroke, while still allowing the input box to contain and display any value outside the bounds. (I was actually surprised it was that simple in the end.)

I see that it might make sense to temporarily return values outside that range for previewing purposes etc., but that's indeed rather unintuitive in my opinion. Callers that actually depend on the returned value being inside the range would still have to clamp manually, even though they already pass the bounds to ImGui.

@ocornut
Copy link
Owner

ocornut commented Nov 26, 2015

Even with the current behavior, the value from the input box actually is clamped to the bounds after hitting Return,

Where can I seeing this behavior? AFAIK it is not clamped when entering a value via the input text. If I ctrl+click a slider or a drag in the demo and input a bigger value it stays unclamped after hitting return. If input value is clamped somewhere it would a bug.

Again not clamping text input it is intentional, in many applications and especially with sliders, ranges are more often than not a limitation rather than a necessity. In term of flexibility being unable to go past the limits of a slider can be really limiting (pun intended).

Now, I agree that ideally we should allow to select both behaviors, but in the absence of an option I believe not clamping text inputs is more beneficial and makes tools more flexible.

@ocornut
Copy link
Owner

ocornut commented Dec 25, 2015

@nmlgc Any news on it? I'm not sure how repro or see a case where input values are ever clamped.

@nmlgc
Copy link
Author

nmlgc commented Dec 25, 2015

Yeah, turns out I was testing this wrongly in my code and the values actually aren't clamped after hitting Return.

@ocornut
Copy link
Owner

ocornut commented Dec 26, 2015

OK. So while I can't take the PR as is I would like eventually to add the option to force clamp but I don't know how to right now (in term of passing the options to the function or storing it into ImGui state). The user can always test for the return value of Slider/Drag and clamp if it is really needed so I don't think there is a major urgency here, but it'd be good to document that behavior with more clarity.

Knowing this do you feel you still need the option? And we can start to find a solution, else I would be tempted to take note of it in the todo list and drop it for now.

@nmlgc
Copy link
Author

nmlgc commented Dec 31, 2015

Yes, clearly documenting that the value is not meant to be clamped should be enough for now.

@ocornut
Copy link
Owner

ocornut commented Jan 2, 2016

Cool. Adding an extra comment and marking issue for later reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants