Skip to content
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

wrap the text in the martial arts selection window #75737

Merged

Conversation

db48x
Copy link
Contributor

@db48x db48x commented Aug 16, 2024

Summary

Interface "wrap the text in the martial arts selection window"

Purpose of change

Some of those descriptions are long enough to make the window huge.

Testing

Create a character whose profession gives them a choice of martial art (the Martial Artist, for example). After starting the game, change your martial arts style. Note that these steps will show you two very similar but separate windows.

Additional context

Fixes #75674

Some of those descriptions are long enough to make the window huge.

Fixes CleverRaven#75674
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Character / World Generation Issues and enhancements concerning stages of creating a character or a world <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 16, 2024
@sonphantrung
Copy link
Contributor

I think you should put the wrap60 function into output.{cpp,h} for ease of use in the future, instead of repeating the same declaration over and over again

@db48x
Copy link
Contributor Author

db48x commented Aug 16, 2024

DRY is a nice rule of thumb, but I have good reasons not to apply it here.

First, the decision about how wide or narrow to make the text is really a local one. wrap60 is just a shorthand for that local decision, because I otherwise would have had to repeat myself for each entry in the butchery menu, and twice in the new character version of the martial arts menu. But it isn’t some dictum that I want to apply globally. It isn’t even something that I recommend globally.

But it’s really worse than that. It’s a huge hack! Think about what we’re doing. We are scanning through a string character by character, counting up to 60. When we go past 60, we look for the previous space and call that the end of a line. We then copy that line into a new string, and append it to a vector. Repeat until the end of the string. Now we have a vector of strings, each one representing a line of text to be shown on the screen.

Then we copy them all into a fresh new string, but with newline characters in between them. Already this is madness. If we knew that this is what we wanted, we should have been making a copy in the previous step, but simply replacing that space with a newline character, avoiding the whole vector stage. It’s wasted effort; I’m just glad it’s not very expensive.

Then we pass that string to the uilist. What does it do? It calls that same damned foldstring function to break it into a vector of lines again! Yea, that’s exactly what it does.

But it’s gets worse still, because then we look for color tags. Oh yea, those stupid color tags! They’re really going to mess anything up if they are present, because we count them as visible characters back in the first step. Good thing neither the butchery window nor the martial arts window uses them in their entry descriptions. Anyway, handling the color tags requires making yet another copy of the string, before and after the tag, so that’s more wasted effort.

Finally we pass some text to ImGui. It loops over it again, character by character, checking for newlines (which we’ve already handled ourselves), checking to see if any glyphs are clipped by the boundaries of the window (again, not going to happen), and finally drawing each glyph. At least that last step is not wasted; it’s the only thing here that we really want to do.

But it’s even worse than all the wasted effort. You see, all the way back in the first step we decided where to wrap by counting characters. This is completely wrong! ImGui works by pixels, so we should wrap by how wide the text is, not how many characters it has. Actually, foldstring counts by bytes, not by characters, which is even more broken. So not only is the effort wasted, it’s stupid too. We are only saved by the mere coincidence that we happen, for the moment, to be using a monospaced font. Really we should be telling the uilist how many pixels wide it should be, and then letting it decide where to wrap the text based on that width.

Except that it isn't capable of both wrapping long text into a paragraph and also handle color tags at the same time. That is, there is no ImGui primitive that directly allows us to draw the text correctly wrapped, in the presence of color or style changes. Obviously it is up to us to handle the color tags, since that’s our own code’s way of doing things, but even once we work out what text to put on what line there is no public ImGui function we can call that will draw the text in the right place. ImGui has been punting on designing the right interface for this problem for a while, so currently it only has some internal functions that we can call. They’ll do the job even if they aren’t perfect. But I have to go rewrite cataimgui::draw_colored_text to use them.

There is also one other thing that I want to do when I rewrite cataimgui::draw_colored_text, which is to split it into a parsing step that handles color and style followed by a drawing step that handles color and style. The parsing step can do all of the allocating of new strings, and then the result can be saved. Then the drawing step can step through the saved state to correctly draw all the text, taking into account the available space in the window (remember that ImGui windows can be resized by the user, and thus can be a different size on every frame). This will allow us to do the parsing once. One of the stupid things I failed to mention above is that we do all of that work every frame. We’ve hacked ImGui slightly so that it’s not running flat out at 60fps, but that still means we do all of that work every time the user presses a key to change their selection.

So, in short, the wrap60 function can be repeated as many times as is convenient because it’s just a quick fix. I’m going to rip it all out sooner or later anyway.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 16, 2024
@Maleclypse Maleclypse merged commit 3e04a29 into CleverRaven:master Aug 18, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Character / World Generation Issues and enhancements concerning stages of creating a character or a world Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Martial arts selection menu needs a size override
3 participants