-
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
Implement a better way to wrap text containing color tags #76513
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
[JSON & C++ formatters](https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/DEVELOPER_TOOLING.md)
[JSON & C++ formatters] reported by reviewdog 🐶
Line 126 in 3b7e83e
Paragraph *Paragraph::cyan( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 131 in 3b7e83e
Paragraph *Paragraph::magenta( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 136 in 3b7e83e
Paragraph *Paragraph::brown( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 141 in 3b7e83e
Paragraph *Paragraph::light_red( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 146 in 3b7e83e
Paragraph *Paragraph::light_green( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 151 in 3b7e83e
Paragraph *Paragraph::light_blue( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 156 in 3b7e83e
Paragraph *Paragraph::light_cyan( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 161 in 3b7e83e
Paragraph *Paragraph::pink( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 166 in 3b7e83e
Paragraph *Paragraph::yellow( std::string_view str ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 171 in 3b7e83e
Paragraph *Paragraph::spaced() { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 178 in 3b7e83e
Paragraph *Paragraph::separated() { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 183 in 3b7e83e
static void TextEx( const std::string_view str, float wrap_width, uint32_t color ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 192 in 3b7e83e
float widthRemaining = ImGui::CalcWrapWidthForPos(ImGui::GetCursorScreenPos(), wrap_width); |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 228 in 3b7e83e
static void TextSegmentEx( const Segment &seg, float wrap_width, bool styled) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 255 in 3b7e83e
void TextColoredParagraph( nc_color default_color, const std::string_view str, std::optional<Segment> value, float wrap_width ) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 284 in 3b7e83e
} |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 67 in 3b7e83e
void TextColoredParagraph( nc_color color, std::string_view str, std::optional<Segment> value = std::nullopt, float wrap_width = 0.0f ); |
[JSON & C++ formatters] reported by reviewdog 🐶
Line 963 in 3b7e83e
info_size.y -= (3.0*ImGui::GetTextLineHeightWithSpacing()) - ImGui::GetFrameHeightWithSpacing(); |
eacf3d8
to
8c9fef2
Compare
Oops. Dumb mistake; I probably should have just left it until the morning when I could think more clearly. |
8c9fef2
to
6c71c21
Compare
Should this also fix #75901? |
It does; good catch. Weirdly I've never actually seen that bug report. I wonder how many more there are that I haven’t noticed because they don’t use the correct keywords. |
Looks like a spurious build failure to me. |
I would appreciate documenting that You mention that this leaves remaining imperfections - what are they? |
Perhaps the most obvious flaw is that at certain wrap widths the first word of a segment will not be word–wrapped but instead will be split across two lines. This is due to a design decision to allow text rendering to make progress even if ImGui is asked to render it in a space too narrow for a whole word. Fixing this is doable but not a high priority at the moment. The fix involves duplicating the ImGui methods that we currently call and tweaking their implementation. We could of course just modify ImGui directly, but that makes upgrading more difficult. Because we don't currently allow the user to resize any ImGui windows it’ll probably never actually come up. There is also a really funny–looking problem that happens when a paragraph contains embedded newlines. Getting the correct rendering in that case merely requires the caller to split their strings if they contain any newlines. This is no great hardship, but I would prefer the API to handle it more gracefully. Fixing this is again doable, and again it’ll need to be done by duplicating and tweaking some of the ImGui methods that we call. |
@kevingranade, is there anything else that needs to be done before this can be merged? |
attempting to build this PR on Linux with clang 18.1.8 produces
I got this with a non-clean |
That’s weird. You can see that it is running the linker to build the executable, and that |
I cannot reproduce the problem, even with the same parameters. |
Yea, you would think that would be easy. Then use it on the debug item spawn window.
6c84d1a
to
fc861e6
Compare
The failures are all unrelated to this particular patch. |
Nice. They’re supposed to still be there, but beneath the spell info. It worked before; perhaps I broke it when I rebased it last. |
and rename RightAlign to BeginRightAlign to address review comments.
Hmmmmm. Let me have a look. |
This fixed the sizing error for me.
Whoops, I was trying to suggest that change, not commit it outright. Feel free to revert it if I broke stuff. |
Yea, I guess we might as well do that. |
I'm also having this problem on master now this is merged with |
If using |
Summary
Interface "Improved text wrapping in spellbook and spellcasting menus"
Purpose of change
Long strings that need to wrap onto multiple lines and also contain color codes are hard to wrap correctly. This gets us most of the way to a correct solution.
It also lets us draw text with fewer allocations, allowing us to gradually get away from creating strings with color tags only to throw them away.
Fixes #77297
Also Fixes #76298
Also fixes #75901
Testing
Best place to test it is in the spellcasting window, using psi abilities. These frequently include color.
Additional context
This isn’t a perfect solution. It relies on ImGui’s built‐in text wrapping in several significant ways, and this comes at the cost of some remaining imperfections. Luckily these imperfections are less intrusive than the existing problems, and we can fix them as we have time and inclination.