Refactor draw colored text #73538
Refactor draw colored text #73538
Conversation
github-actions
bot
added
Info / User Interface
github-actions
bot
added
the
BasicBuildPassed
db48x
force-pushed
the
refactor-draw_colored_text
branch
from
a5e4ef0 to
ab4a3d5
Compare
|
There is a conflict to resolve. |
db48x
force-pushed
the
refactor-draw_colored_text
branch
from
ab4a3d5 to
24616b2
Compare
|
still conflicts, but i did pull this branch down and test it and did not notice anything wrong with it at a glance. |
|
0001-Added-code-to-guarantee-that-any-colors-pushed-to-th.patch Added a patch with the change I wanted to see with my comment. If @db48x is okay with this patch then we can merge this. |
|
i actually figured out how to open a PR db48x#1 |
|
I don’t hate it. It would work for the case where a closing color tag has been left out, but not the case where too many closing tags exist. For that I have a partial patch of my own which I had only just started to test; I hit some snags and then got distracted. However, instead of hiding the error wouldn’t it be better to generate an error report? I am not sure that the error report could be ignored, since it would happen every frame, but that’s the only way to keep this type of mistake from accumulating. Finally, have you taken a look at the code I linked to? It has a really nice solution. Instead of embedding markup into a string, then parsing the string, it uses a vector of text segments. Each segment has a color and the code simply renders all of the segments, one after another, wrapping them as necessary. I have not yet adopted this code because the game has so many strings with markup, but I think that is really where we should be headed. I would probably be much more efficient than what we do now. And there would be no way to cause any of these problems. It is past time for me to hit the sack, so I can’t possibly work on this for another 12 hours at least. If you want to strike while the iron is hot then go for it. If you get something you are satisfied with, I have given you access to my fork so that you can push to this branch. Or you can just make a new pull request, if you prefer. |
If you don't specify a color, it will use the current Text theme color as the base. If you do specify it, it will push the color you specify first and pop it after. Also, remove the internal color stack, since ImGui already has a stack of colors.
The conversion is not expensive enough to worry about, and we don't have any functions that can take both an nc_color and an ImVec4 so it isn’t ambiguous either.
…le stack are popped back off
db48x
force-pushed
the
refactor-draw_colored_text
branch
from
4767a3d to
dec5cf3
Compare
|
Should be good to go now, unless I’ve missed something. In the end I skipped trying to report errors; it’s more annoying that it is worth at the moment. |
Summary
Infrastructure "refactor cataimgui::draw_colored_text"
Purpose of change
Simplifies the function so that it doesn't keep track of a redundant color stack, and so that the base color argument is optional. This simplifies 99% of the callers.
Describe the solution
I also added a conversion function to the nc_color class so that instances can be used anywhere ImGui expects an ImVec4.
Testing
The easiest way to test is to look at the new item info window written by mqrause in #73456; it has lots of wrapped text with embedded colors.
Additional context
See also ocornut/imgui#2313 (comment), which has a nicer implementation of the same idea. Amongst several other improvements, it works with proportionally–spaced fonts, while our current solution only works with monospaced fonts.