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

Add text alignment to sf::Text #2713

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

Conversation

Hapaxia
Copy link
Contributor

@Hapaxia Hapaxia commented Sep 28, 2023

Adds ability to align all lines of sf::Text to the left, centre or the right.

It provides a single enum choice for the alignment and simply (horizontally) offsets the lines so they align according to the choice.

I has been tested with empty lines and also with underlines and strikethroughs as well to confirm those lines behave correctly.

Here is an image of it in action:
An image showing the different types of line alignment with different types of decorations

I spoke to a member of the team about if this would be wanted and they said it would be welcome.

I hope it's exactly the correct code style.


Thanks a lot for making a contribution to SFML! 🙂

Before you create the pull request, we ask you to check the follow boxes. (For small changes not everything needs to ticked, but the more the better!)

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?

Description

Please describe your pull request.

Tasks

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

How to test this PR?

Describe how to best test these changes. Please provide a minimal, complete and verifiable example if possible, you can use the follow template as a start:

#include <SFML/Graphics.hpp>

int main()
{
    sf::Font font;
    if (!font.loadFromFile("arial.ttf"))
        return EXIT_FAILURE;

    sf::Text text(font);
    text.setString("THIS\nIS\nA\nMULTI-LINE\nTEXT");
    text.setLineAlignment(sf::Text::Center);

    sf::RenderWindow window(sf::VideoMode({1280, 720}), "Minimal, complete and verifiable example");
    window.setFramerateLimit(60);

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

        window.clear();
        window.draw(text);
        window.display();
    }
}

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 92 lines in your changes are missing coverage. Please review.

Comparison is base (9d1d9cd) 39.43% compared to head (9bcdbc4) 0.00%.
Report is 175 commits behind head on master.

❗ Current head 9bcdbc4 differs from pull request most recent head 91bf37e. Consider uploading reports for the commit 91bf37e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #2713       +/-   ##
==========================================
- Coverage   39.43%   0.00%   -39.44%     
==========================================
  Files         229     128      -101     
  Lines       19824    7910    -11914     
  Branches     4739    2024     -2715     
==========================================
- Hits         7817       0     -7817     
+ Misses      11192    7890     -3302     
+ Partials      815      20      -795     
Files Coverage Δ
src/SFML/Graphics/Text.cpp 0.00% <0.00%> (-83.94%) ⬇️

... and 208 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@ChrisThrasher
Copy link
Member

I fixed up some minor style issues to get those out of the way early. Thanks for submitting this! I'll take a closer look when I have some more time later to dig into this. Unfortunately that may be a few days from now.

@ChrisThrasher ChrisThrasher changed the title add text alignment to sf::Text Add text alignment to sf::Text Sep 28, 2023
@ChrisThrasher ChrisThrasher added this to the 3.0 milestone Sep 28, 2023
@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

Thanks!

I thought it was about time I got around to doing this. 😃

Let me know if you would like me to adjust anything.

@lapinozz
Copy link

This is a cool feature and it seems to fit nicely in sf::Text without too many changes, sweet!
I think we could change it so that instead of having a m_lineOffsets member the line offset update function could return the vector.
Since it doesn't seem to be used/useful outside the geometry update and it's recreated each time anyways

@ChrisThrasher
Copy link
Member

Looks like clang-tidy caught something suspicious. Check out those CI logs to see if there's anything we need to fix.

https://github.com/SFML/SFML/actions/runs/6343566229/job/17231737846?pr=2713#step:8:76

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

Looks like clang-tidy caught something suspicious.

I don't think it likes the fact that I've added something called lineOffset and passed it to the same function that an underlineOffset is passed.
If required, I can change it to something more verbose: horizontalOffset perhaps?

This is a cool feature and it seems to fit nicely in sf::Text without too many changes, sweet! I think we could change it so that instead of having a m_lineOffsets member the line offset update function could return the vector. Since it doesn't seem to be used/useful outside the geometry update and it's recreated each time anyways

Thanks!
It's also used by findCharacterPos but it does re-generate it there too. I did think that maybe the m_lineOffsets vector could be used elsewhere (even for getting the number of lines) but I didn't want to change too much!
Part of the reason of storing it was to avoid it being constantly created and destroyed when nothing is changing. Rather, it gets resized to the same size (so stays the same) and just updates the values.

Of course, if it's preferred, I can switch it to create the vector on the fly every update; it is pretty small, after all.

@lapinozz
Copy link

Thanks! It's also used by findCharacterPos but it does re-generate it there too. I did think that maybe the m_lineOffsets vector could be used elsewhere (even for getting the number of lines) but I didn't want to change too much! Part of the reason of storing it was to avoid it being constantly created and destroyed when nothing is changing. Rather, it gets resized to the same size (so stays the same) and just updates the values.

Oh I didn't notice it was used in that other function. If we want to keep the vector as a member then we could get rid of the local vector in the function and use the member directly. Because right now it always allocates a new vector.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

Indeed. That temporary vector is used for a different semantic purpose but I believe the member vector could be re-used for that purpose until it's needed without any issues.

@ChrisThrasher
Copy link
Member

If required, I can change it to something more verbose: horizontalOffset perhaps?

Addressing this clang-tidy error in some way will be required. Either fix whatever it's complaining about or make a compelling case for why we should disable this error.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

If required, I can change it to something more verbose: horizontalOffset perhaps?

Addressing this clang-tidy error in some way will be required. Either fix whatever it's complaining about or make a compelling case for why we should disable this error.

Okay, no problem. I'll change it to what I suggested and hope that it's happy with that thing instead!

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

Updated to change that variable name. Hopefully, the clang-tidy checks won't have an issue with that now.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Sep 28, 2023

If you have clang-tidy installed you can build the tidy target to run clang-tidy. I'd recommend doing that since it's much faster than waiting for CI to run.

By the way you can run git pull --rebase to pull a branch that has diverged from your local branch to avoid creating a merge commit. That will get your local copy in sync with the remote copy.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

I don't, unfortunately, sorry.

I didn't know that. I rarely use command line; I tend to use the Desktop app. Thank you!

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

It has failed now on formatted. It seems to be suggesting to spread a line over multiple lines. Is that correct? I gather there's maybe a hard maximum limit on line length?

@ChrisThrasher
Copy link
Member

It has failed now on formatted. It seems to be suggesting to spread a line over multiple lines. Is that correct? I gather there's maybe a hard maximum limit on line length?

That's because when you made that merge commit you reverted the formatting fixes I made.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

That's not exactly true. All of your corrections are still present. I made sure to take all of your versions when merging.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Sep 28, 2023

My bad it wasn't related to that merge commit. It was that some new code hit the line limit and started wrapping. If you have clang-format-14 installed you can build the format target to fix formatting. I went ahead and fixed it myself and pushed a new commit.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 28, 2023

Thanks. I've also pulled and rebased your change this time as you suggested previously 😃
Should I edit and re-format the other addLine calls so they are formatted similarly?

@kimci86
Copy link
Contributor

kimci86 commented Sep 30, 2023

So this has no impact for a single-line text. What about positioning text relative to the local origin so that it has impact on a single-line text too? It would also make is simple to center or right-align text on a given vertical line by setting the text position on said vertical line.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Sep 30, 2023

It's designed to align with the other lines within the text block only so a single line should not be affected indeed.

To centre around - or align to the right using - the origin, we can simply offset its position by -width/2 or -width respectively.

Whether this is up to the user to do or SFML is a design decision that the team would have to make but it should be possible without too much 'upset' of the code. However, I didn't want to change too much at once just for a single small feature.

Copy link
Member

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

I rebased on master because we recently merged some new sf::Text tests. I also added unit tests for this new API and removed some duplicate code. Run git pull -r to update your local branch since I had to change the commit history.

@@ -463,7 +498,9 @@ void Text::ensureGeometryUpdate() const
break;
case U'\n':
y += lineSpacing;
x = 0;
++line;
horizontalOffset = (line < m_lineOffsets.size()) ? m_lineOffsets[line] : 0.f;
Copy link
Member

Choose a reason for hiding this comment

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

Under what circumstances would the current line exceed the size of m_lineOffsets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't. I was being super-extra-cautious.

I had the test elsewhere but removed it when I was certain it was never going to exceed it.

It increases the line parameter before using it as an index and I haven't done a test to check if this still works for text where the last line end with \n but I believe the added method should force it to consider that an extra line.

lineWidths.push_back(position.x);

// update line offsets from widths depending on line alignment
m_lineOffsets.resize(lineWidths.size());
Copy link
Member

Choose a reason for hiding this comment

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

Could we use 1 vector here instead of 2? As in, could we remove lineWidths and replace that with a more clever use of m_lineWidths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was briefly mentioned early here but wasn't sure if it would be clear to use the same vector for two semantically different things: widths and offsets, even if it is temporary, private and "internal".

@kimci86
Copy link
Contributor

kimci86 commented Oct 2, 2023

It's designed to align with the other lines within the text block only so a single line should not be affected indeed.

In my opinion this is a sign that the design is not good (i.e. a function that does nothing in many cases). I suggest we make a conscious choice about the wanted behavior and not realize after the fact that it is a weird behavior. What do you all think?

@kimci86
Copy link
Contributor

kimci86 commented Oct 2, 2023

On another topic: What about non-integer positions? Should we round them to get a sharp look? We can see some lines are blurry on the centered blocks in this PR description's screenshot.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

It's designed to align with the other lines within the text block only so a single line should not be affected indeed.

In my opinion this is a sign that the design is not good (i.e. a function that does nothing in many cases). I suggest we make a conscious choice about the wanted behavior and not realize after the fact that it is a weird behavior. What do you all think?

I, personally, don't think it's weird to expect that telling a text object to align its (multiple) lines to not affect single lines. However, I can also see how this could benefit from automatically moving it around the origin.

Please discuss and I'll get right on it if wanted.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

On another topic: What about non-integer positions? Should we round them to get a sharp look? We can see some lines are blurry on the centered blocks in this PR description's screenshot.

This is clearly a simple enough fix as well. I think this might be a good idea indeed.

Would rounding to integer be affected by views? I don't think so (because this is the local co-ordinate system) but I'm not 100% on this.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

I rebased on master because we recently merged some new sf::Text tests. I also added unit tests for this new API and removed some duplicate code. Run git pull -r to update your local branch since I had to change the commit history.

I've done exactly that so we should be synchronized. I like the update; it removes duplication of similar code.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

In addition, does sf::Text support vertical text? It doesn't look like it does (looking at its code) but if it does, this alignment inclusion might need some modification.

Re-uses line offsets vector for temporary line widths to avoid using a secondary vector.

Removes check to test if index is out of range as it would never be so.

Rounds offset to nearest integer to improve base clarity (this can still be affected - as usual - if transformed).
@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

I've done some slight modifications to address so of the minor concerns raised.

That is, the pointless test for being out of range, the vector being re-used instead of creating a temporary one and internal rounding of the offsets.

However, the decision as whether the alignment affects the position of the visible text object (and its bounds) still needs to be made before I just go ahead and add that.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

Updated screenshot (identical test code) to show how the rounding affects the text:
An updated image showing the different types of line alignment with different types of decorations including rounding

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Oct 5, 2023

NOTE: the tests now need to check that local bounds match their integer positions, not the fractional positions. Global bounds in the check will also be affected.

@Hapaxia
Copy link
Contributor Author

Hapaxia commented Jan 22, 2024

Can someone please let me know what I need to do to get this ready to be included?

In addition, I'd like to add the suggested idea of aligning to its origin (e.g. aligning to the right would mean that text would be left of its position) and would like confirmation that this is the wanted behaviour. I'm also not sure what the origin should be used for if the text is being moved. For example, if the text is right-aligned and the text is now left of the position, an origin of (x = 0) would be at the right-hand side of the text.

@ChrisThrasher ChrisThrasher modified the milestones: 3.0, 3.1 Jan 25, 2024
Copy link
Member

@vittorioromeo vittorioromeo left a comment

Choose a reason for hiding this comment

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

Minor suggestions

/// \brief Enumeration of the text alignment options
///
////////////////////////////////////////////////////////////
enum LineAlignment
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum LineAlignment
enum class LineAlignment

@@ -415,11 +475,13 @@ class SFML_GRAPHICS_API Text : public Drawable, public Transformable
Color m_fillColor{Color::White}; //!< Text fill color
Color m_outlineColor{Color::Black}; //!< Text outline color
float m_outlineThickness{0.f}; //!< Thickness of the text's outline
LineAlignment m_lineAlignment{Left}; //!< Line alignment for a multi-line text
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LineAlignment m_lineAlignment{Left}; //!< Line alignment for a multi-line text
LineAlignment m_lineAlignment{LineAlignment::Left}; //!< Line alignment for a multi-line text

src/SFML/Graphics/Text.cpp Show resolved Hide resolved
src/SFML/Graphics/Text.cpp Show resolved Hide resolved
@@ -54,6 +54,7 @@ TEST_CASE("[Graphics] sf::Text", runDisplayTests())
CHECK(text.getFillColor() == sf::Color::White);
CHECK(text.getOutlineColor() == sf::Color::Black);
CHECK(text.getOutlineThickness() == 0);
CHECK(text.getLineAlignment() == sf::Text::Left);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CHECK(text.getLineAlignment() == sf::Text::Left);
CHECK(text.getLineAlignment() == sf::Text::LineAlignment::Left);

@@ -88,6 +90,7 @@ TEST_CASE("[Graphics] sf::Text", runDisplayTests())
CHECK(text.getFillColor() == sf::Color::White);
CHECK(text.getOutlineColor() == sf::Color::Black);
CHECK(text.getOutlineThickness() == 0);
CHECK(text.getLineAlignment() == sf::Text::Left);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CHECK(text.getLineAlignment() == sf::Text::Left);
CHECK(text.getLineAlignment() == sf::Text::LineAlignment::Left);

Comment on lines 169 to 170
text.setLineAlignment(sf::Text::Center);
CHECK(text.getLineAlignment() == sf::Text::Center);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
text.setLineAlignment(sf::Text::Center);
CHECK(text.getLineAlignment() == sf::Text::Center);
text.setLineAlignment(sf::Text::LineAlignment::Center);
CHECK(text.getLineAlignment() == sf::Text::LineAlignment::Center);

Comment on lines 172 to 173
text.setLineAlignment(sf::Text::Right);
CHECK(text.getLineAlignment() == sf::Text::Right);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
text.setLineAlignment(sf::Text::Right);
CHECK(text.getLineAlignment() == sf::Text::Right);
text.setLineAlignment(sf::Text::LineAlignment::Right);
CHECK(text.getLineAlignment() == sf::Text::LineAlignment::Right);


SECTION("Change alignment")
{
text.setLineAlignment(sf::Text::Center);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
text.setLineAlignment(sf::Text::Center);
text.setLineAlignment(sf::Text::LineAlignment::Center);

text.setLineAlignment(sf::Text::Center);
CHECK(text.getLocalBounds() == sf::FloatRect({-16.5, 5}, {33, 13}));
CHECK(text.getGlobalBounds() == sf::FloatRect({83.5, 205}, {33, 13}));
text.setLineAlignment(sf::Text::Right);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
text.setLineAlignment(sf::Text::Right);
text.setLineAlignment(sf::Text::LineAlignment::Right);

@eXpl0it3r
Copy link
Member

Can someone please let me know what I need to do to get this ready to be included?

As it's "just" an addition it has a bit of a lower priority. So it mostly takes time to get around to it.

If you can rebase it and apply @vittorioromeo's suggestions, then we can hopefully get around to it soon.

also includes asserts for lineOffsets vector and updated tests.
@Hapaxia
Copy link
Contributor Author

Hapaxia commented Jan 31, 2024

I just meant so that I could make it ready when it's needed.

Implemented @vittorioromeo suggestions. Thanks!
I only used enum as it seems to be SFML's style; I would've used enum class otherwise.

It's the first time I've seen that test file!

Copy link
Member

@vittorioromeo vittorioromeo left a comment

Choose a reason for hiding this comment

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

Just one more stylistic improvement. LGTM otherwise, I will test it soon

Comment on lines +624 to +640
// convert widths into offsets depending on line alignment
for (std::size_t i = 0; i < m_lineOffsets.size(); ++i)
{
switch (m_lineAlignment)
{
case LineAlignment::Right:
m_lineOffsets[i] = maxWidth - m_lineOffsets[i];
break;
case LineAlignment::Center:
m_lineOffsets[i] = (maxWidth - m_lineOffsets[i]) / 2.f;
break;
case LineAlignment::Left:
m_lineOffsets[i] = 0.f;
break;
}
m_lineOffsets[i] = std::round(m_lineOffsets[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// convert widths into offsets depending on line alignment
for (std::size_t i = 0; i < m_lineOffsets.size(); ++i)
{
switch (m_lineAlignment)
{
case LineAlignment::Right:
m_lineOffsets[i] = maxWidth - m_lineOffsets[i];
break;
case LineAlignment::Center:
m_lineOffsets[i] = (maxWidth - m_lineOffsets[i]) / 2.f;
break;
case LineAlignment::Left:
m_lineOffsets[i] = 0.f;
break;
}
m_lineOffsets[i] = std::round(m_lineOffsets[i]);
}
// convert widths into offsets depending on line alignment
const auto applyLineOffset = [&](float value)
{
if (m_lineAlignment == LineAlignment::Left)
return 0.f;
if (m_lineAlignment == LineAlignment::Center)
return (maxWidth - lineOffset) / 2.f;
assert(m_lineAlignment == LineAlignment::Right);
return maxWidth - lineOffset;
};
for (float& lineOffset : m_lineOffsets)
lineOffset = std::round(applyLineOffset(lineOffset));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite an elegant use of more modern features.
However, I'd question why the multiple ifs are in place of a switch. Also, maybe we could test for center and right and default to left (i.e. 0 offset) if those options are not given. Which brings to the point that even the switch should have a default label.

@vittorioromeo
Copy link
Member

In addition, I'd like to add the suggested idea of aligning to its origin (e.g. aligning to the right would mean that text would be left of its position) and would like confirmation that this is the wanted behaviour.

I'm not too sure about this. Thoughts? @eXpl0it3r @ChrisThrasher @Bromeon

@ChrisThrasher
Copy link
Member

I’m about to go traveling without my computer so I can’t do any serious code reviews until the week of February 19th.

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

Successfully merging this pull request may close these issues.

None yet

6 participants