-
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
Imguify help menu #78195
base: master
Are you sure you want to change the base?
Imguify help menu #78195
Conversation
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 pretty good so far, but I do have a few minor requests.
src/help.cpp
Outdated
std::string ret = "<color_light_blue>"; | ||
ret += std::string( length, c ); | ||
ret += "</color>"; | ||
return ret; |
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.
Instead of allocating a new string with color tags and then immediately throwing it away, why not just do something like this?
ImGui::PushStyleColor( c_light_blue );
while ( c --> 0 ) {
ImGui::TextUnformatted( c );
}
ImGui::PopStyleColor();
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'm not sure it's worth emulating terminal style separators instead of just using the imgui ones, though.
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.
It’s also used for the border around the title, which I’m fine with. For extra points, use the gui width for the title, but make the border use the mono font as it currently does. You would want to measure the width of the title in pixels, then calculate the correct number of "=" will fit around it (since it won’t just be the same number of characters as the title).
You could also go nuts and add a large gui font. Give it +50% size (or maybe +25%, something that gives you a nice value when used with the default of 16px text size) and maybe a higher weight.
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.
TextUnformatted() is adding a space after each call with SameLine (can't figure out what the SameLine args want) and I'm having to pass c as a string implicitly converted to a const char * bc TextUnformatted doesn't let me pass a character or 1 length string for whatever reason but then = isn't rendering while _ is?. (I'm just using regular single width = not the box drawing one) Nvm I were being a dumbass
I'm not sure it's worth emulating terminal style separators instead of just using the imgui ones, though.
That's fair
For extra points, use the gui width for the title, but make the border use the mono font as it currently does.
I saw that we can calc the pixel width of individual characters ye, I'll give it a go ^^
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.
Oh, for the character thing you could use ImGui::Text("%c", c)
if you want. That avoids needing to allocate a two–byte string :) (Internally ImGui will use a fixed–sized buffer reserved for the purpose, iirc).
{ | ||
const help_category &cat = data.help_categories[loaded_option]; | ||
format_title( cat.name.translated() ); | ||
if( ImGui::BeginTable( "HELP_PARAGRAPHS", 1, |
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 out of curiosity, why do you need a table here?
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'm currently just using it to make the text scrollable while the "title" stays still but I'll probably make that only happen if TERMY isn't small if I keep it at all (Need to make it focus on the table to start or else up/down arrows don't scroll until you click).
What's the preferred way to indent the text? I see we do right alignment via tables with empty columns so I were planning on doing indentation padding the same way here.
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.
Ah, for scrolling use a child window rather than a table; that’ll be a bit more obvious. And maybe a comment in the code just to make sure that the reader knows what you’re going for.
Right alignment uses a table because it’s a nice trick, not because it is the right thing to do in general.
For indentation, simply change the X position. You can do ImGui::SameLine(0.0, 0.0)
to get the default spacing between widgets, but this may not be suitable for indenting a paragraph. There is also ImGui::Indent()
which will indent by a themeable amount, but this is usually used for nested tree nodes and other things rather than typographical indentation. Paragraphs are usually indented by 1em, so measure an “M” (ImGui::CalcTextSize("M").x
) then indent by that amount.
src/help.cpp
Outdated
for( const translation ¶graph : cat.paragraphs ) { | ||
std::string translated_paragraph = paragraph.translated(); | ||
parse_tags_help_window( translated_paragraph ); | ||
cataimgui::draw_colored_text( translated_paragraph, wrap_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.
If you use cataimgui::TextColoredParagraph
instead, with no wrap_width
, then you’ll get pixel–correct wrapping even on text containing color tags. cataimgui::draw_colored_text
will be retired eventually.
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's what I were using to begin with it was just failing to interpret this correctly so I switched and then forgot about it, didn't appreciate draw_colored_text was temporary, should I just be using TextColoredParagraph everywhere?
I'll take a look to see if I can fix that easily or I'll open an issue otherwise
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.
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.
It messes up a bit when the “paragraph” that you give it contains embedded newline characters. Currently the best way to avoid the problem is to split paragraphs into lines and then call TextColoredParagraph on each one.
Also, it doesn’t handle incorrect tags very gracefully, so maybe you just need to fix the JSON for those.
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.
Also, it doesn’t handle incorrect tags very gracefully, so maybe you just need to fix the JSON for those.
The JSON is fine that's showing the 4 rotations of buildings, it's failing to recognise the bc of the preceding <
The current window and draw_colored_text handles it correctly. I can probably just stick a space between them in the JSON for this one instance like but it shouldn't be needed.
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.
Oooh, I see now. I’ll see if I can improve the code to handle it better.
Adding spaces between the < and also doesn't fix this
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.
Pretty annoying that it can't just handle \n tho
I know, I really gotta get in there and fix that bug. I’ll make it a goal for tomorrow.
Adding spaces between the < and also doesn't fix this
If you look at text.cpp line 297, you’ll see that it finds the first <
and then scans until it finds a >
. It calls get_color_from_tag
to get a color, but in this case it finds <</whatever>
as the tag which doesn’t match anything. Nobody really thought this through, I think.
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.
BTW, we could consider using images here, if you want. Of course ASCII art is not a bad stylistic choice either.
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 think I'm pretty pro ASCII stuff in general but in this case it just seems more awkward to do anything else, it can't be a single image bc it's showing the current binds which I think is important and trying to piece together images around the binds would probably be just as bad, it looks clear in game as is
It could be done with something without lines like
NW N NE 7 8 9 y k u
W O E 4 5 6 h . l
SW S SE 1 2 3 b j n
(still using the current binds) but that feels less useful
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 is perfectly happy to overlap things; it could be a single image with text overlaid. Imagine a nice little compass rose with the points labeled by keybinds. We can measure the exact bounding box of each keybind, and position them correctly so that they always line up right, don’t overlap, don’t spill out of the window, etc. Instead of using spaces and ImGui::NewLine()
to roughly position things, we can use ImGui::SetCursorPos()
to put things at exact coordinates.
Or there are unicode characters:
↖↑↗
←❂→
↙↓↘
db00e86
to
e89090a
Compare
See #78560 |
Procyonae#3 might interest you as well |
Remove the item spacing in category titles so the ascii art looks right
Thanks ^_^ |
How are you meant to handle game window resizing with TextColoredParagraph? I were going to add it recalcing the wrap width on resize but it doesn't use that and doesn't seem to auto resize at all so you have to fully exit and reenter the menu |
We do this by not specifying the size in pixels but in percent. We want the window to be 100%, or 1.0×, the size of the game window.
By not specifying a fixed window size in pixels: Procyonae#4 |
… tags DRAW_NOTE_COLORS is redundant, it already shows when adding a map note HELP_DRAW_DIRECTIONS can be done in json instead now
Make the help window full screen and resize it when the game resizes
Do you really want to go down that road? Step back and think about it: what you’re doing is creating a general purpose document formatting program. Next you’re going to want tables so that the columns will line up right. I would suggest just you just remove the intermediary format and write the documentation directly in ImGui, but I know you want mods to be able to add to the help information. Maybe we should consider just borrowing something instead? |
I were just going to leave it at that tbh, subtitles and separators let you seperate messages up in obvious ways to make it clear what a player can scroll past if they don't care about a header and then the monospace thing is necessary bc we're supporting non monospace fonts for some reason that I still don't understand
We already hardcode way too much text stuff as is that gets neglected bc of it I would do these as tags but that makes it somewhat awkward with the not parsing the tags every frame thing |
Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details. Click to expand
This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to Hints for adding a new word to the dictionary
|
Yea, right. You’d be the first person in history to never add just one more feature :D And what if someone comes along in six months and wants to “improve” it? Yes, doing it without allocating is not easy. Even the markdown parser I linked to allocates on parsing (it builds stacks for things like nested lists), but I think it can parse the document just once and then rerender it every frame without allocating. That would be good enough. Also, if the help file were a markdown document then we could render it to HTML and ship it with the game, or put it on a webpage, killing N birds with one stone. |
Oh I didn't realise you meant make it read literal markdown, I don't fancy figuring out implementing some 1000 line 3rd party thing tho that would still need altering to handle monofont, our curses colours and the keybind tags if nothing else |
Have you looked at the code? It’s really straight forward. The way it handles fonts is actually to leave it up to us. The example code is just an example, we could write whatever we want. It suggests using a normal font, a bold font, a large font, and one that is both large and bold, but we don't have to bother if we don’t want to. In fact, for headings we could just keep using ascii art! :D |
I mean ignoring that I'd imagine kevin would probably veto adding 8k lines of external code just to support md I can't get it to compile https://github.com/Procyonae/Cataclysm-DDA/compare/ImguifyHelpMenu...Procyonae:Cataclysm-DDA:MarkdownTest?expand=1 (I'm not suggesting any of this would want to be setup like this I were just trying to do it the simplest way I could, I want to at least check it functions, looks decent with ImTui and can be easily modified for monofont tags, curses colour tags and keybind tags before asking kevin) |
Heh, I did something similar yesterday, but I got a little further than you: Obviously I haven’t yet done anything about the tags. Also if you look at the text closely you’ll notice a very subtle bug in how we handle whitespace. In HTML multiple consecutive whitespace characters, including newlines, are rendered as a single space. The md4c library doesn’t really handle this directly, since the primary envisioned use was to export to HTML and let the browser handle it. Therefore any place where I have wrapped a paragraph across multiple lines in the markdown file it fails to render a space character. It skips right over the newline character and immediately starts rendering the next line. Look for the text “…setting in theInterface tab…” in the second paragraph for an example. |
It also would need transifex handling for the markdown files, this all seems like overkill when idk where else we'd even use md |
What do you mean by “transifex handling”? |
The markdown files would need translating somehow, transifex is how we currently do translation |
Oh, unless you mean translations. Those are trivial; we would just let the translators provide a translated version of the whole markdown file. To create the initial versions of those files we could manually extract the text from the gettext files. |
Summary
Interface "Imguify help menu"
Purpose of change
It is inevitable
Describe the solution
Not done yet still looking to:
Improve layout (padding, maybe add a way to shrink the characters per line or just lower it)
Better accessibility (auto focusing on the first option, maybe prepending something to read category titles for screen readers, swapping out the draw directions snippet for screen readers etc)
Better handling for tiny screens eg phones (I'll probably drop the title staying at the top if the y screen dimension is too small and make the options one column wide if the x screen dimension is too small if nothing else)
Add visible buttons to switch between categories
Display the binds to quit out?
Handling for resizing the cataclysm window while open
Window is now fullscreen bc there's no real reason not to and it allows fitting more categories (see #78126)
Adds basic read/unread highlighting to category names on selection screen
Adds keyboard selection of categories (arrow keys and enter) in addition to hotkeys and mouse selection
Allows left/right or tab/backtabbing between categories
All pre-existing functionality is maintained as far as I can tell
Describe alternatives you've considered
Making read/unread status of categories more permanent
Splitting the options into tabs by mod but I think I'll save that for now, have an idea for some custom tabs that want longer in the oven
Adding more JSON functionality like adding separators, subtitles past just caps/colours etc
Splitting the category paragraphs over two columns but that looked awful to me
It could be cool if from the base menu you could search like "bite" and it'd find you the relevant lines in categories that contain "bite" so you could use it as more of a quick lookup type thing (probably would be especially convenient for mods with extra mechanics and classes etc)
Pretending ImGui is the devil and hissing at it rather than helping move towards 0.I
Testing
Menu displays properly both in category selection and when displaying a category
Mouse/keyboard/hotkey selections all work, navigating with left/right/tab/backtab work
Read/unread works inc with navigation
2024-11-27.21-54-34.mp4
Looks not terrible on the minimum size screen the window version will let me use
2024-11-27.21-58-22.mp4
No ImTui curses testing, will try to figure out compiling on WSL
Additional context
I think this is my first front facing UI PR so happily lecture me on how I'm doing everything wrong