OpenRCT2
Split gfx_wrap_string that calculates the dimensions
It is primarily used to to calculate the height of the text input and currently modifies the buffer which the caller must provide. This can be done without modifying the buffer and avoiding a lot of string copying in most of the UI code which should also provide a minor performance improvement.
In e.g. window_park_objective_paint, the function gfx_wrap_string is used to actually insert new-line characters in the string. So, making it const won't be possible here.
Should we provide a new function, just for calculating height?
... could return a std::vector<size_t> from gfx_wrap_string with the calculated 'line break positions'.
I think its often used to also just get the height, so splitting it might be better.
@kennycastro007 any update on this? Was gonna try my hand at this as well
@devlontecron I've been swamped with work and school, if you wanna give it a shot, go for it!
Sorry the title might be also a bit misleading, there should be two functions, one that only calculates the height/width and one that actually modifies the string.
Any preferences for the name of the function calculating the height/width?
Naming is always pretty hard, maybe something like gfx_calculate_string_dimensions? I wouldn't worry about the name too much at first, we can discuss this in the PR once functional.
I'll let @devlontecron take a crack at it and then I'll pick it up afterwards if he can't or help him if he needs it.
@kennycastro007 did you make any progress that I could build off on / continue? no reason to start from scratch
Not yet, like I said, I got swamped with work and school. I never got the chance to start
I mean the issue is still open and no PR or commit references this issue so I guess it's still for the taking.
Great, then can I have a go at it? I have experience coding in C++, but not with open source. I will go through the contributing guidelines, but if you could point me toward the specific files to be refactored, that would be really helpful...
@ZehMatt
@ZehMatt has the name of the function gfx_wrap_string been changed? I can't seem to find it. Same goes for window_park_objective_paint. I am searching for it in this repo. Am I doing something wrong?
The name has changed to GfxWrapString, we changed the coding style at some point, it's located in Drawing.String.cpp
@ZehMatt I would like to discuss a possible approach and clear a few doubts, could we please take this up on discord?
We should keep discussions here at GitHub, Discord is not a good place to keep record of such things.
Right, so looking at the above chat and from a high-level overview of the codebase, I could infer that in src/, there seem to be 12 instances of use of GfxWrapString. Some were used just for height calculation, while others were used to format the input string. But in either case, the input string is consistently formatted, which is undesirable. So, depending upon the use case, we can split the current function to either find the height of the input string or to actually format the input string. The former would do something similar to what tomlankhurst pointed out above(of course in this case the vector is irrelevant to us as we need only outNumLines)
... could return a
std::vector<size_t>fromgfx_wrap_stringwith the calculated 'line break positions'.
While the latter's implementation won't be much different from the current function. Is this correct?
Sounds about right, the return value is also never really used so the function that just determines the amount of lines could directly return that rather than passing it over reference argument.
Right, there is no need to pass it over a reference argument. Since we would be returning a vector containing line break positions, the total number of lines would be the size of this vector + 1.
If this seems fine, I can get started with a rough implementation, or is there something else that I should be considering?
I don't think it needs to return a vector, at the the time where it passes nullptr as the output buffer its only interested in the amount of lines to compute the maximum height.
So only in cases where the input to the field u8string* outWrappedText is a nullptr, we require the function for height calculation? And in all other cases, we would be required to wrap the input string?
If that is the case, you are right and had misinterpreted the problem. Then the second function would actually be a sort of replica of the existing function except that it would be free of a lot of string operations, and would just return an int. Please correct me if I am wrong.
That's what I was thinking Edit: It's possible that this function changed a bit over the time so my initial comment is probably not that accurate anymore, it was required to provide a buffer at the time I created this issue, it's no longer as bad provided it allows nullptr. Still would be good to separate the two things.
