-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Feature/text fits within bounds #2274
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2274 +/- ##
==========================================
+ Coverage 14.93% 16.01% +1.07%
==========================================
Files 208 212 +4
Lines 15244 17946 +2702
Branches 4074 4417 +343
==========================================
+ Hits 2277 2874 +597
- Misses 12829 14875 +2046
- Partials 138 197 +59
Continue to review full report at Codecov.
|
src/SFML/Graphics/Text.cpp
Outdated
} | ||
|
||
// Precompute the variables needed by the algorithm | ||
bool isBold = m_style & Bold; |
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.
Could this be const bool
instead, doesn't appear to be changing after this line so I'd guess so.
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.
Done :)
include/SFML/Graphics/Text.hpp
Outdated
/// \return true if every drawn character will appear within \a bounds | ||
/// | ||
//////////////////////////////////////////////////////////// | ||
bool fitsWithinBounds(const sf::FloatRect& bounds, std::size_t& oob) const; |
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.
Is there any way to change this interface such that it doesn't require using an out parameter?
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.
What is the preferred SFML way to deal with optional returns? A trivial struct, std::optional<>, etc? Or perhaps just returning oob always would be a better option?
I'm not super familiar with SFML's preference for this, so any advice to meet code style is appreciated.
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.
std::optional<std::size_t>
return type would probably be the best option.
Name should then be changed accordingly: firstOutOfBoundsChar
or firstOobCharIndex
or so.
src/SFML/Graphics/Text.cpp
Outdated
const bool isBold = m_style & Bold; | ||
float whitespaceWidth = m_font->getGlyph(U' ', m_characterSize, isBold).advance; | ||
const float letterSpacing = (whitespaceWidth / 3.f) * (m_letterSpacingFactor - 1.f); | ||
whitespaceWidth += letterSpacing; | ||
const float lineSpacing = m_font->getLineSpacing(m_characterSize) * m_lineSpacingFactor; | ||
|
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.
Can this code be extracted into a function such this and the code you changed in findCharacterPos
share a common implementation?
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.
Good point, done in 1430fbb.
include/SFML/Graphics/Text.hpp
Outdated
/// \return true if every drawn character will appear within \a bounds | ||
/// | ||
//////////////////////////////////////////////////////////// | ||
bool fitsWithinBounds(const sf::FloatRect& bounds, std::size_t& oob) const; |
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.
std::optional<std::size_t>
return type would probably be the best option.
Name should then be changed accordingly: firstOutOfBoundsChar
or firstOobCharIndex
or so.
src/SFML/Graphics/Text.cpp
Outdated
// Compute a series of variables that are commonly used when building a text layout | ||
TextLayoutStats getTextLayoutStats(uint32_t style, const sf::Font* font, unsigned int characterSize, float letterSpacingFactor, float lineSpacingFactor) |
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.
Nitpick: empty line between struct definition and comment
Can you please link them? I wonder whether it wouldn't make more sense to have a |
This is only partially helpful to the user and doesn't seem terribly clear or ergonomic. I'd say it's worth doing this feature properly and making text objects aware of line length, which would also support the best implementation where we can wrap the lines at the point of updating the geometry |
These threads all pose the question or have a community member suggest the iterative Given that the common answer to this question is the poorly performant one, I think something should be made available as an alternative. But this is just a suggestion; if it's outside the design philosophy of SFML then feel free to close.
Agreed, have amended the function name and signature.
Fully agree. However, this would mean each Text object now contains fields for getting/setting text wrapping (enable/disable), bounding box AABB, justification (left, right, center, other?) as well as other possible considerations like RTL charsets and so on. I did not feel comfortable blowing the complexity of the class up to do so, especially without any prior discussion. I'm perfectly happy if this review is closed and that conversation is had, it's just outside the scope of what I was trying to achieve here (simple native multiline rendering without crippling performance). P.S. Apologies for the lengthy delay in coming back to this. COVID got me pretty bad. |
Has this change been discussed on the forum or in an issue before?
Yes, this has come up a number of times on the forums over the years. Most of the links to any implementations are dead at this point, so I went off my best guess at the suggested implementation (see my code example).
Does the code follow the SFML Code Style Guide?
To my understanding, yes.
Have you provided some example/test code for your changes?
Yes, attached at the bottom.
Description
Implemented a new method in sf::Text in order to aid with optimal text wrapping.
The general problem:
Drawing wrapped text using sf::Text.
My attempts
There's no built-in support for this, so for a user to implement this behaviour themselves they'll need to call sf::Text::findCharacterPos() iteratively to determine where to wrap the text. This presents a performance problem because determining the position of a single character requires determining the position of the previous character (and so on, recursively) in order to calculate character width, kerning, etc. If I'm remembering my CS classes correctly, complexity of this approach is O=n^3 (n=length of string).
To improve the performance of this operation, I also attempted to perform a binary search through the text to find the first character which exceeds the bounds. This reduces the complexity to O=n^2.
In this PR, I've implemented a new function sf::Text::fitsWithinBounds() which reduces the complexity of the operation down to O=n (a single linear search). I feel like this function is reasonable counterpart to sf::Text::findCharacterPos(), as one gets a position from an index and the other gets an index from a position (approximately).
This could be optimised further by performing text wrapping at render/geometry build time, but seeing as text wrapping is not a desired native SFML feature I didn't think too much more about this path.
Tasks
I have only tested this on windows under msvc 2019. Unfortunately, I don't have any other dev environment to test this on.
Sample code:
Here's a complete program implementing the aforementioned three approaches to text wrapping on a dynamically resizing text box. A string of several hundred words is enough to demonstrate the performance differences between the three implementations.
Change the macro on line 5 to: