-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Imgui input popups #77863
base: master
Are you sure you want to change the base?
Imgui input popups #77863
Conversation
e209c7e
to
94f13a1
Compare
ecba856
to
ca626ca
Compare
ca626ca
to
355fd1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far, but I do have a couple of suggestions.
popup.set_label( _( "Pickup Rule:" ) ); | ||
std::string description = _( | ||
"* is used as a Wildcard. A few Examples:\n" | ||
"\n" | ||
"wooden arrow matches the itemname exactly\n" | ||
"wooden ar* matches items beginning with wood ar\n" | ||
"*rrow matches items ending with rrow\n" | ||
"*avy fle*fi*arrow multiple * are allowed\n" | ||
"heAVY*woOD*arrOW case insensitive search\n" | ||
"\n" | ||
"Pickup based on item materials:\n" | ||
"m:kevlar matches items made of Kevlar\n" | ||
"M:copper matches items made purely of copper\n" | ||
"M:steel,iron multiple materials allowed (OR search)" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anything with a long description I would much rather see them use a custom ImGui window that can use both mono and variable‐width text, columns, colors, and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love a flexible solution too, but I don't want to do that in this PR (and honestly don't plan to do it any time soon after it either).
pos = description.find( "\n", offset ); | ||
std::string str = description.substr( offset, pos - offset ); | ||
cataimgui::TextColoredParagraph( description_default_color, str, std::nullopt, | ||
str_width_to_pixels( width ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leave off the width to wrap to the available width of the window.
ImGui::SetNextItemWidth( input_width ); | ||
} | ||
|
||
ImGui::InputText( "##string_input", text.data(), max_input_length, flags, callback, data ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImGui comes with a helper function that lets you directly use a std::string
as an argument here. You’ll have to import the code from the ImGui repository, but it does string resizing properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've seen that and also tried it out and it seemed to work properly, but @katemonster33 pointed out that casting std::string::c_str
to non-const to be written to is undefined behavior, so I refrained from adding it.
|
||
void string_input_popup_imgui::set_text( const std::string &txt ) | ||
{ | ||
snprintf( text.data(), text.size(), "%s", txt.c_str() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you call set_text
with a string that is longer than text
can hold, this chops off the end of the input instead of resizing text
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is intentional. If we actually want to support inputs >255 chars, we should make a multiline version of the popup.
Summary
Infrastructure "Imgui input popups"
Purpose of change
General imgui migration and I need this for #55503.
Describe the solution
Make new classes for string and number input popups. The number input is templated to work on any number type (in theory, need to learn more template magic to really enforce that without having to implement
number_input_popup::draw_input_control
for every type we want). Some places don't usestring_input_popup
as a popup but as an integrated element, so I can't simply replace it completely.When using non-uilist input history, I had to use imgui callback input handling instead of our usual input_context, because imgui's input fields reset the buffer when redrawing while they're the focused element. This is kinda ugly because it mixed two methods of input handling, but I don't see a better solution right now.
notable string input features:
notable number input features:
Issues:
Describe alternatives you've considered
Mirroring the code style of the old popup for easier transitioning. But it's the only place in the code base where that style is used and nobody insisted I keep it.
Testing
Int and float input is implemented in the settings menu and tested there.
String input is implemented in a few places, mostly tested in world creation world name and V menu item filtering.
Additional context
Screenshots slightly outdated since color updates.
float input:
int input:
item filter input: