Skip to content

Add input_scalar and allow setting float input precision #544

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 6, 2021
Merged

Add input_scalar and allow setting float input precision #544

merged 5 commits into from
Oct 6, 2021

Conversation

EmbersArc
Copy link
Contributor

@EmbersArc EmbersArc commented Oct 2, 2021 •
edited
Loading

Continuation of #490, closes #489

I haven't been able to test it yet since the examples fail to build on the main branch.

@EmbersArc
* add input_scalar
e8d614c 
* allow setting display format for input_float
@EmbersArc
formatting
709ab52
@EmbersArc
Copy link
Contributor Author

Should we add InputDouble as well? Might be redundant when InputScalar is available. Maybe InputFloat* and InputInt* are not required anymore either.

sanbox-irl
sanbox-irl reviewed Oct 3, 2021
View reviewed changes
Copy link
Member

@sanbox-irl sanbox-irl left a comment

Choose a reason for hiding this comment

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

Looks great! I think if you rebase and then run cargo fmt --all we should be basically good to go!

imgui/src/input_widget.rs
@@ -530,19 +533,36 @@ impl<'ui, 'p, L: AsRef<str>> InputFloat<'ui, 'p, L> {
value,
step: 0.0,
step_fast: 0.0,
display_format: None,
Copy link
Member

Choose a reason for hiding this comment

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

we should default this display format to "%.3f", as it originally was. That also means that display_format can actually become F instead of Option<F>, and start life with the type &'static str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for the float case I presume? For InputScalar I'd definitely stick with letting imgui pick the format.

imgui/src/input_widget.rs
pub fn build(self) -> bool {
let value: $MINT_TARGET = (*self.value).into();
let mut value: [f32; $N] = value.into();

let (one, two) = self
Copy link
Member

Choose a reason for hiding this comment

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

same thing with the display format as above

imgui/src/lib.rs
@@ -551,6 +551,30 @@ impl<'ui> Ui {
{
InputInt4::new(self, label, value)
}
#[doc(alias = "InputScalar")]
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some doc comments here explaining what InputScalar is, and maybe in particular how it differs from the other Input types (something about basically "let's you do this with u64s and other data types than float or int").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in 41c93de.

@sanbox-irl
Copy link
Member

Should we add InputDouble as well? Might be redundant when InputScalar is available. Maybe InputFloat* and InputInt* are not required anymore either.

InputDouble, I feel is fine if we don't cover for now. f64s aren't super commonly used in DearImGui, but if you feel like putting in the effort...you're welcome to! I'd accept it, but until someone asks, I'm not too worried about it. InputFloat COULD be removed actually, but InputInt's widget has the +/- buttons that this widget doesn't have, and so we'll want to keep that around.

Personally, as long as Dear ImGui has both, confusingly, I'm fine with us having both too. It's a bit odd, but since we can do this all with generics, our InputScalar is pretty good looking and nice to use, compared to Dear ImGui's input scalar. I think we could likely move towards InputFloat being a facade for InputScalar. InputFloatX though lets you use MintTypes, which we can't get InputScalarN to do, at least not in the meantime I think.

@EmbersArc
Copy link
Contributor Author

InputInt's widget has the +/- buttons that this widget doesn't have, and so we'll want to keep that around.

Are you sure about that? Maybe it still shows up when step is set explicitly. I'd try it but still can't compile it, it fails with the message

the trait `imgui_glium_renderer::glium::backend::Facade` is not implemented for `glium::Display`

@sanbox-irl
Copy link
Member

Ahhh this is a cargo problem that we run into! Try running cargo clean and then going again. That's really not great and I'm sorry about that. We have too many gliums

@EmbersArc
Copy link
Contributor Author

No problem! cargo clean didn't work so I edited the cargo files so that they had the same glium version.

Anyway, InputScalar also has the +/- buttons for integers when you set a step:
image
So I think this makes InputInt and InputFloat redundant. But I'll leave this up to you, I'm happy as long as InputScalar is in there.

Thanks for working on imgui-rs by the way :) I like the direction this is going.

@sanbox-irl
Copy link
Member

No problem! I also just pushed simplifying our support of glium to just 0.30, which SHOULD resolve this issue, I believe, but the other maintainer, thomcc, is better at wrangling our many deps than I am.

I didn't realize the step issue! Glad to know about that. That lead to me to check through the implementation, and....
https://github.com/ocornut/imgui/blob/master/imgui_widgets.cpp#L3525

So I agree, but I'd like to accept this PR now and then run InputFloat and InputInt through a deprecation cycle. We'll keep ui.input_float and ui.input_int (though they'll just call input_scalar) since ImGui does the same, and it's important that C++ users should be comfortable in imgui-rs if it's cheap for us to support, but I don't like having the huge duplication of struct definitions there.

What do you think about that plan?

@EmbersArc
Copy link
Contributor Author

Sounds good to me. Thanks again!

@sanbox-irl sanbox-irl merged commit dccbdc4 into imgui-rs:main Oct 6, 2021
@sanbox-irl
Copy link
Member

ach, forgot to name ya in the changelog. will push that and the input float deprecation stuff now. thanks!

@EmbersArc EmbersArc deleted the input-scalar branch October 6, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanbox-irl sanbox-irl sanbox-irl left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add InputDouble/InputScalar and display format settings for InputFloat
2 participants
@EmbersArc @sanbox-irl