Update the ui_text code to have less duplications and simplier renderer #8854
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Hi @Districh-ru , would you be okay to merge this change after 1.1.1 release? I am worried that since we are getting close to the release we might face some issues with such a big change. |
There was a problem hiding this comment.
Hi @Districh-ru , I would like to understand the scope of these changes and the intention. Is it correct to say that this pull request does 2 things:
- speed up things by caching some data
- simplifies rendering
I have a question regarding the first part. Could you please explain little bit more for a better understanding of these changes?
| int32_t rows( const int32_t maxWidth ) const | ||
| { | ||
| const_cast<TextBase *>( this )->setMaxWidth( maxWidth ); | ||
|
|
||
| return static_cast<int32_t>( _textLineInfos.size() ); | ||
| } |
There was a problem hiding this comment.
Could you please explain the benefit of doing this except have little extra performance boost? I see 2 issues with this approach:
- using
const_castwhich is very dangerous and could lead to a lot of problem with code execution and also debugging. - we are doing something which shouldn't be done inside this function.
There was a problem hiding this comment.
Ho, @ihhub.
Thanks for highlighting these issues!
Yes we have the performance boost: we do not call getMultiRowInfo() many times for each width(), height(), row() and draw() methods.
And we don not have to maintain the three copies of the almost the same code, but not the same in all places. - This was the main issue I was trying to solve because it was not always easy to understand how to act in the each "almost the same" code part and to remember to do all the changes in all three duplications. :)
Wу already had some barely noticeable issues: width calculation for the width() function and for rendering was different and the results could slightly differ.
Now all text "info" is calculated once and all functions will now use the same data.
We also could not use the results of the TextLineInfo outside this code for the cursor placing and probably some other functions.
I used const_cast for the compatibility with the other code and was planning to update this code after we have have checked this drastic changes. I'll do this changes now to get rid of the const_cast. Also I'll add the text alignment setting: it would simplify the rendering parts.
|
Additionally, I noticed that the text is rendered differently with these changes. Is it intentional? |
If you are talking about the MultiFontText: previously the next line offset was considered as the biggest line height of all texts. IMHO it was done to easily calculate text height ( lines_count * line_height ). If this differences are in the |
|
Closing this PR in favor of #9116 |
This PR changes the fheroes2::Text behavior to calculate text parameters only once and then use them for width, height, rows count and rendering.
This PR changes all texts rendering including buttons text.