Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SalamiArmi
Copy link

@SalamiArmi SalamiArmi commented Nov 18, 2022

  • 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.

  • Tested on Windows
  • Tested on Linux
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

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:

  • WRAP_METHOD_NAIVE for the naive implementation directly inspired by the prior forum discussions.
  • WRAP_METHOD_BINARY_SEARCH for the most optimal implementation I could think of without changing the API.
  • WRAP_METHOD_FITSWITHINBOUNDS to use the method implemented in this PR.
#define WRAP_METHOD_NAIVE 0
#define WRAP_METHOD_BINARY_SEARCH 1
#define WRAP_METHOD_FITSWITHINBOUNDS 2

#define WRAP_METHOD WRAP_METHOD_FITSWITHINBOUNDS

#include <SFML/Graphics/Font.hpp>
#include <SFML/Graphics/Text.hpp>
#include <SFML/Graphics/RenderWindow.hpp>
#include <SFML/Window/VideoMode.hpp>
#include <SFML/Window/Event.hpp>

#include <algorithm>

sf::Text g_text;
sf::Font g_font;

std::vector<std::string> tokenise(const std::string& in, char delimiter)
{
	std::vector<std::string> ret;

	size_t seekPtr = 0;
	size_t nextLineBreak;
	while ((nextLineBreak = in.find_first_of(delimiter, seekPtr)) != std::string::npos)
	{
		ret.emplace_back(in.substr(seekPtr, nextLineBreak - seekPtr));
		seekPtr = nextLineBreak + 1;
	}
	// remaining
	ret.emplace_back(in.substr(seekPtr, std::string::npos));

	return ret;
}

template <typename _urItStart, typename _urItEnd, typename _pred>
constexpr decltype(auto) binary_lower_bound(_urItStart&& start, _urItEnd&& end, _pred&& pred)
{
	static_assert(std::is_same_v< std::decay_t<decltype(start)>, std::decay_t<decltype(end)> >, "Iterators incompatible!");
	using _iterator = std::decay_t<decltype(start)>;

	_iterator
		low = start,
		high = end;

	while (low < high)
	{
		const typename std::iterator_traits<_iterator>::difference_type midpointIdx = std::distance(low, high) / 2;
		_iterator midpointIt = low;
		std::advance(midpointIt, (std::max<typename std::iterator_traits<_iterator>::difference_type>)(midpointIdx, 0u));

		if (pred(midpointIt))
		{
			low = midpointIt + 1;
		}
		else
		{
			high = midpointIt;
		}
	}
	return low;
}

void drawWrappedText(const std::string& str, float width, uint8_t fontSize, sf::RenderWindow& window)
{
	g_text.setFont(g_font);
	g_text.setCharacterSize(fontSize);

	auto lines = tokenise(str, '\n');

	// reasonable cutoff for this demo
	constexpr size_t maxLineCount = 100;

	for (size_t lineIdx = 0; lineIdx < std::min(lines.size(), maxLineCount); ++lineIdx)
	{
		g_text.setString(lines[lineIdx]);

		const auto wrapBegin = lines[lineIdx].begin();
		const auto wrapEndMax = lines[lineIdx].end();
#if WRAP_METHOD==WRAP_METHOD_NAIVE
		auto wrapEnd = wrapEndMax;
		for (auto it = wrapBegin; it != wrapEndMax; ++it)
		{
			size_t idx = std::distance(wrapBegin, it);
			if (g_text.findCharacterPos(idx).x >= width)
			{
				wrapEnd = it;
				break;
			}
		}
#elif WRAP_METHOD==WRAP_METHOD_BINARY_SEARCH
		auto wrapEnd = binary_lower_bound(wrapBegin, wrapEndMax,
			[&width, &wrapBegin](auto&& it)
			{
				size_t i = std::distance(wrapBegin, it);
				return g_text.findCharacterPos(i).x < width;
			});
		// Check lower bound validity
		if (wrapEnd != wrapBegin)
		{
			size_t i = std::distance(wrapBegin, wrapEnd);
			sf::Vector2f pos = sf::Vector2f(g_text.findCharacterPos(i));
			if (pos.x >= width)
			{
				wrapEnd--;
			}
		}
		else
		{
			continue;
		}
#elif WRAP_METHOD==WRAP_METHOD_FITSWITHINBOUNDS
		size_t oobIndex;
		if (g_text.fitsWithinBounds({ { 0, 0 }, { width, 9999999.0f } }, oobIndex))
		{
			continue;
		}
		auto wrapEnd = wrapBegin + oobIndex;
#endif

		// no wrap
		if (wrapEnd == wrapEndMax)
		{
			continue;
		}

		// seek backwards through any amount of whitespace
		while (wrapEnd != wrapBegin && *wrapEnd == ' ')
		{
			wrapEnd--;
		}

		if (wrapEnd != wrapBegin)
		{
			std::string&& newStr = { wrapEnd, wrapEndMax };
			lines[lineIdx].erase(wrapEnd, wrapEndMax);
			lines.insert(lines.begin() + lineIdx + 1, newStr);
		}
		else
		{
			lines[lineIdx].erase(wrapEnd, wrapEndMax);
		}
	}
	if (lines.size() >= maxLineCount)
	{
		lines.erase(lines.begin() + maxLineCount, lines.end());
	}

	if (!lines.empty())
	{
		sf::String finalStr = lines[0];
		for (size_t sfmlStrLineIdx = 1; sfmlStrLineIdx < lines.size(); ++sfmlStrLineIdx)
		{
			finalStr.insert(finalStr.getSize(), '\n');
			finalStr.insert(finalStr.getSize(), lines[sfmlStrLineIdx]);
		}

		g_text.setString(finalStr);
		window.draw(g_text);
	}
}

int main()
{
	sf::RenderWindow window(sf::VideoMode({ 1280, 720 }), "example");
	sf::Vector2u halfWindowDims(window.getSize().x / 2, window.getSize().y / 2);

	bool res = g_font.loadFromFile("assets/verdana.ttf");
	assert(res);

	sf::Clock clockTextWrap;

	const std::string lorem =
R"(Lorem ipsum dolor sit amet, consectetur adipiscing elit.Pellentesque ut odio tristique purus congue finibus.Quisque sed nulla ac justo vehicula malesuada.Integer aliquet lacus ac enim pellentesque sagittis.Aliquam elementum nisi elit, sit amet egestas sem tincidunt ac.Suspendisse sapien massa, consectetur vel tristique at, facilisis id justo.In maximus mauris et dui finibus, non laoreet sem vehicula.Integer lobortis suscipit felis vitae ultrices.Aliquam laoreet gravida erat, sed elementum est.Fusce in massa nunc.Etiam vestibulum urna nec lorem pellentesque mattis.Vestibulum ut condimentum libero, id aliquam nibh.Nam commodo mauris at ipsum euismod ornare.
Nullam tristique massa nec lectus condimentum mattis.Pellentesque orci ante, cursus vel massa nec, dictum maximus tellus.Donec arcu felis, elementum quis massa eget, porttitor elementum dolor.Suspendisse imperdiet augue vel dolor vulputate, ut eleifend ligula egestas.Mauris in arcu facilisis, semper nunc et, lobortis massa.Phasellus gravida erat risus, sit amet dapibus sapien tristique at.Quisque congue luctus leo quis euismod.Aliquam rutrum imperdiet risus, mattis molestie eros.Duis auctor dolor eu risus interdum efficitur.
Nam gravida sem nec nisl sodales laoreet.Ut non mauris eu orci finibus ultrices vitae sed quam.Etiam et aliquam lacus.Donec pulvinar orci eget felis convallis blandit.Vivamus porttitor aliquet ipsum, vel ultrices ex pulvinar a.Pellentesque orci orci, mollis id elementum ut, feugiat ut ex.Curabitur egestas vitae nisl non vestibulum.Nulla nisl metus, suscipit quis pulvinar vitae, porttitor faucibus purus.Nulla consectetur blandit diam.Morbi porta risus massa, et ullamcorper lacus volutpat quis.Phasellus mollis turpis ac leo faucibus, at viverra ligula maximus.
Suspendisse potenti.Nulla urna diam, suscipit in ultricies nec, gravida vitae ipsum.Sed varius arcu sed scelerisque blandit.Ut fermentum tristique sapien ut scelerisque.Proin in sem non massa dapibus laoreet aliquam eu nisl.Nam blandit malesuada nibh, efficitur tincidunt felis.Maecenas purus risus, aliquet nec odio ut, tristique eleifend purus.Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia curae; Proin vel ligula et metus convallis fringilla et non urna.Morbi viverra, nulla hendrerit vehicula semper, erat neque pulvinar odio, eget convallis lacus sem sit amet turpis.Proin sodales non mi id porta.Pellentesque pellentesque eros justo, pulvinar aliquet tellus vulputate sit amet.Maecenas sed massa vel lacus luctus fringilla.
Donec imperdiet sollicitudin purus, at molestie turpis ultricies non.Donec rhoncus eleifend placerat.Curabitur sapien ex, efficitur at dignissim sit amet, luctus vel dolor.Nullam ac lorem at dui efficitur suscipit non id augue.Maecenas aliquam posuere quam, sed pellentesque felis.Nam eu mi eu leo auctor iaculis sit amet ac sem.Suspendisse potenti.)";

	while (window.isOpen())
	{
		sf::Event event;
		while (window.pollEvent(event))
		{
			if (event.type == sf::Event::Closed)
				window.close();
		}

		window.clear();

		float width  = halfWindowDims.x + (std::sin(clockTextWrap.getElapsedTime().asSeconds()) * halfWindowDims.x);
		drawWrappedText(lorem, width, 12, window);

		window.display();
	}

	return 0;
}

@SalamiArmi SalamiArmi marked this pull request as ready for review November 18, 2022 05:52
@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #2274 (a628c74) into master (ca9531b) will increase coverage by 1.07%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
include/SFML/Graphics/Text.hpp 0.00% <ø> (ø)
src/SFML/Graphics/Text.cpp 0.00% <0.00%> (ø)
include/SFML/System/String.hpp 0.00% <0.00%> (-100.00%) ⬇️
include/SFML/System/FileInputStream.hpp 0.00% <0.00%> (-50.00%) ⬇️
src/SFML/Window/Unix/VulkanImplX11.cpp 2.81% <0.00%> (-9.85%) ⬇️
src/SFML/Audio/AlResource.cpp 9.09% <0.00%> (-7.58%) ⬇️
src/SFML/Window/Win32/VulkanImplWin32.cpp 5.06% <0.00%> (-6.71%) ⬇️
src/SFML/Network/TcpSocket.cpp 43.39% <0.00%> (-1.05%) ⬇️
src/SFML/Graphics/Shape.cpp 85.51% <0.00%> (-1.03%) ⬇️
src/SFML/Graphics/BlendMode.cpp 94.59% <0.00%> (-0.97%) ⬇️
... and 180 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca9531b...a628c74. Read the comment docs.

}

// Precompute the variables needed by the algorithm
bool isBold = m_style & Bold;
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

/// \return true if every drawn character will appear within \a bounds
///
////////////////////////////////////////////////////////////
bool fitsWithinBounds(const sf::FloatRect& bounds, std::size_t& oob) const;
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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.

Comment on lines 395 to 400
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;

Copy link
Member

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?

Copy link
Author

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.

/// \return true if every drawn character will appear within \a bounds
///
////////////////////////////////////////////////////////////
bool fitsWithinBounds(const sf::FloatRect& bounds, std::size_t& oob) const;
Copy link
Member

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.

Comment on lines 100 to 101
// 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)
Copy link
Member

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

@eXpl0it3r
Copy link
Member

Yes, this has come up a number of times on the forums over the years.

Can you please link them?
The idea behind the question isn't whether it's come up on the forum as a request once, but more whether it's been discussed that this is a feature we want to add to SFML.

I wonder whether it wouldn't make more sense to have a findOutOfBoundsCharacter() instead, given that the information that it doesn't fit on its own is relatively useless, meaning in which scenarios will you just need a false/true value and in which scenarios are you actually interested in something else?

@JonnyPtn
Copy link
Contributor

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

@SalamiArmi
Copy link
Author

SalamiArmi commented Feb 7, 2023

@eXpl0it3r

Yes, this has come up a number of times on the forums over the years.

Can you please link them? The idea behind the question isn't whether it's come up on the forum as a request once, but more whether it's been discussed that this is a feature we want to add to SFML.

These threads all pose the question or have a community member suggest the iterative findCharacterPos method.
https://en.sfml-dev.org/forums/index.php?topic=18605.0
https://en.sfml-dev.org/forums/index.php?topic=20346.0
https://en.sfml-dev.org/forums/index.php?topic=14976.15
https://en.sfml-dev.org/forums/index.php?topic=15014.msg105951
https://en.sfml-dev.org/forums/index.php?topic=13817.msg96897

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.

I wonder whether it wouldn't make more sense to have a findOutOfBoundsCharacter() instead, given that the information that it doesn't fit on its own is relatively useless, meaning in which scenarios will you just need a false/true value and in which scenarios are you actually interested in something else?

Agreed, have amended the function name and signature.

@JonnyPtn

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants