Added a method to set and get additional spacing between characters. #928

Foaly · 2015-07-21T17:15:20Z

SFML Tasks

Check for forgotten discussion points
Review code

This is my code, that I promised in this forum thread.
It is pretty straight forward. Here is some example code to test with:

#include <SFML/Graphics.hpp>
#include <iostream>

int main()
{
    sf::RenderWindow window(sf::VideoMode(800, 600), "Character Spacing Example");
    window.setFramerateLimit(60);

    // load font
    sf::Font font;
    if(!font.loadFromFile("BilboSwashCaps-Regular.otf"))
    {
        std::cerr << "Failed to load font!" << std::endl;
        return 1;
    }

    // setup the texts
    sf::Text normalText("SFML rocks! 123", font);
    normalText.setCharacterSize(100);
    normalText.setPosition(30, 10);
    normalText.setStyle(sf::Text::Style::Bold);

    sf::Text smallText("SFML rocks! 123", font);
    smallText.setCharacterSize(30);
    smallText.setPosition(30, 150);
    smallText.setStyle(sf::Text::Style::Bold);

    sf::Text stretchedText("SFML rocks! 123", font);
    stretchedText.setCharacterSize(100);
    stretchedText.setPosition(30, 200);
    stretchedText.setStyle(sf::Text::Style::Bold);
    stretchedText.setLetterSpacing(6.f);

    sf::Text smallStretchedText("SFML rocks! 123", font);
    smallStretchedText.setCharacterSize(30);
    smallStretchedText.setPosition(30, 350);
    smallStretchedText.setStyle(sf::Text::Style::Bold);
    smallStretchedText.setLetterSpacing(1.f);


    // run the program as long as the window is open
    while (window.isOpen())
    {
        // check all the windows events
        sf::Event event;
        while (window.pollEvent(event))
        {
            // "close requested" event: we close the window
            if (event.type == sf::Event::Closed)
                window.close();

            else if (event.type == sf::Event::KeyReleased) {
                // close if escape key was pressed
                if (event.key.code == sf::Keyboard::Escape) {
                    window.close();
                }
            }
        }

        // clear the window to black
        window.clear();

        // draw text
        window.draw(normalText);
        window.draw(smallText);
        window.draw(stretchedText);
        window.draw(smallStretchedText);

        // display the windows content
        window.display();
    }
    return 0;
}

The font Bilbo Swash Caps Regular is taken from here.

TankOs · 2015-07-22T07:05:24Z

include/SFML/Graphics/Text.hpp

@@ -145,6 +145,21 @@ class SFML_GRAPHICS_API Text : public Drawable, public Transformable
    void setCharacterSize(unsigned int size);

    ////////////////////////////////////////////////////////////
+    /// \brief Set the addition character spacing


"additional"?

TankOs · 2015-07-22T07:09:57Z

Except one doc string, this looks good to me. 👍 🐈

MarioLiebisch · 2015-07-22T08:10:34Z

I'm not 100% sure. How about introducing scaling of the horizontal (and vertical) offsets rather than an absolute offset? Or in other words, make character spacing and line spacing configurable.

As far as I know, stuff like s p a c e d o u t t e x t would usually scale with the actual font/glyph's default offsets.

Foaly · 2015-07-22T12:07:55Z

Thanks, I updated the doc string!

I also already thought about line spacing. Although I personally didn't need it yet, it has been requested a couple times on the forum, for example here. Laurent more or less agreed to it here. I also think it is a basic typography tool and it would be a useful addition to sf::Text. What does everybody else think? If nobody has anything against it, I would send another commit implementing line spacing.

About the scaling factor vs. absolut offset: I took a quick look around and here is what others do. GIMP uses only absolut offsets for both letter and line spacing. In LibreOffice letter spacing can only be set using absolut offsets. Line spacing can be defined by both offsets and factors (double line, 1.5 line etc.). In CSS the letter-spacing property only takes absolut pixel values. The line-height property takes takes both offset and scaling factors.
So I think we should stick with absolut offsets for letter spacing, since it seems to be the standart way. Since it's a pixel offset, it also gives the user the most control, I'd say. For line-spacing I'd also suggest absolut offsets to be consistend and least confusing.

Another small thing: In this thread the name setLetterSpacing() was suggested. Letter Spacing seems to be the correct term for it. I used setCharacterSpacing() in analogy to setCharacterSize(). What are your opinions on the name? I personally think setLetterSpacing() is actually more self-explanatory.

MarioLiebisch · 2015-07-22T12:32:56Z

Letter spacing might be the traditional term, but modern typefaces have far more characters than just letters (like dingbats). So I'd stick to SFML's character.

Scaling vs. offsets: If offsets are more common, stick with these. :)

mantognini · 2015-07-22T16:17:10Z

Doesn't this implementation add one-too-many m_characterSpacing to the width of non-empty strings?

Bromeon · 2015-07-22T18:07:57Z

Letter spacing might be the traditional term, but modern typefaces have far more characters than just letters (like dingbats).

I don't think this is relevant. If there is a common term (which is the case), we should use it, not work our way around conventions.

Interesting anecdote: in German, a letter has a typesetting origin, it's not simply an alphabetic character.

MarioLiebisch · 2015-07-22T18:09:55Z

Okay, if it's "official". :)

TankOs · 2015-07-23T07:24:35Z

👍 for both absolute offsets and line spacing. What do others say?

Foaly · 2015-07-23T07:35:24Z

@mantognini: I don't think so. If I understand the implementation correct, the bounds are computed before the advance is added. So for the last letter in a non-empty string the width is computed, then the advance is added, but then the loop is left and the bounds stay correct.

OK, so I guess we'll go with setLetterSpacing(). I'll change that. I'll see if I have time to implement line spacing later today, so we have something to discuss.

mantognini · 2015-07-23T08:03:35Z

@Foaly could you make sure that's the case? E.g. By checking that a one char text has always the same size regardless of the spacing. Just to be sure :)

Foaly · 2015-07-23T08:16:04Z

@mantognini: I did a quick check. A text with one character returns exactly the same bound compiled with this branch and with current master.

Also I updated the commit. Changed the names and the documentation accordingly.

Foaly · 2015-07-23T10:31:48Z

Alright, so I implemented line spacing. The implementation follows the letter spacing one closely. It's simply an offset, that is added onto the line spacing from the font.
Right now I broke some of the indentation, but it's about the implemenation for now. Also the more I look at ensureGeometryUpdate(), the more little things I'd like to change I see (const is not used consequently, some code dublication...). So if nobody minds I would like to refactor that method a bit. I won't be able to do it today, but probably on the weekend.

LaurentGomila · 2015-07-23T11:30:49Z

if nobody minds I would like to refactor that method a bit

Then do it in a separate PR please, don't mix unrelated changes.

zsbzsb · 2015-07-23T17:34:08Z

Also the more I look at ensureGeometryUpdate(), the more little things I'd like to change I see (const is not used consequently, some code dublication...).

Look at my text outline PR, I already refactored some stuff quite a bit.

Foaly · 2015-07-25T12:50:17Z

Thanks! Yeah I had something similar in mind. A while back I submitted #442 which already incorporated some of those changes, but the refactoring was never added, because vtab got removed and the PR was closed.

I'll probably have time to submit the changes tonight.

@LaurentGomila I know making a separate pull request is good practice and I can do it, if you want. But are you sure in this case? I mean the changes are quiet small and related in my opinion (added new functionality -> refactored existing method). Also a separate pull request would mean merge conflicts. Your call though.

Bromeon · 2015-07-26T14:13:44Z

@Foaly As a compromise, and to avoid unnecessary work with merge conflicts, you could add to this PR a separate commit that contains only the refactoring.

Foaly · 2015-07-26T15:36:36Z

Alright here are my changes! I'd love to hear some feedback.

zsbzsb · 2015-07-26T20:04:32Z

As I already refactored the code to add a line to an external method in #840 I'm not sure why you did it again (and the method will be different from text outline version since two sets of vertices are required for text outlining). It is just going to cause merge conflicts and doing duplicate work is kinda a waste of time.

Foaly · 2015-07-26T22:37:05Z

If you take a closer look, you'll see that I the method has a different sigature and a slightly different implementation. I found it more expressiv this way.

Bromeon · 2015-07-27T18:04:47Z

But @zsbzsb has got a point: now we have definitive merge conflicts and the merge order determines who is "lucky" and needn't fix his code. Since zsbzsb has opened his pull request already in March, I suggest we use his code... and if needed, you @Foaly adapt it correspondingly for this PR (or already give feedback now).

I don't think it makes a lot of sense to rewrite everything now. We should wait until his PR is merged, so that you can rebase on master.

Foaly · 2015-07-30T15:22:12Z

I opened a pull request with that had this exact refactoring almost two years ago... But anyway, if you want I can take that change out and give @zsbzsb some feedback on how to improve his code.
Maybe I'll find some time later today.

oomek · 2018-02-04T01:31:51Z

src/SFML/Graphics/Text.cpp

@@ -317,8 +317,8 @@ Vector2f Text::findCharacterPos(std::size_t index) const

    // Precompute the variables needed by the algorithm
    bool  isBold          = m_style & Bold;
-    float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance + m_letterSpacing;
-    float lineSpacing     = m_font->getLineSpacing(m_characterSize)+ m_lineSpacing;
+    float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance * m_letterSpacingFactor;


~~float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance * m_letterSpacingFactor;~~
float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance;
float letterSpacing = ( whitespaceWidth / 3.0 ) * ( m_letterSpacingFactor - 1.0 );
whiteSpaceWidth += letterSpacing;

Clearance between characters is usualy 1/3 of the space width, so:
for m_letterSpacingFactor = 0.0 the spacing will be -1/3 of a whitespaceWidth (equiv. to -100 in Photoshop )
for m_letterSpacingFactor = 1.0 the spacing will be 1/3 of a whitespaceWidth (equiv. to 0 in Photoshop )
for m_letterSpacingFactor = 2.0 the spacing will be 1/3 of a whitespaceWidth (equiv. to +100 in Photoshop )
for m_letterSpacingFactor = 3.0 the spacing will be 2/3 of a whitespaceWidth (equiv. to +200 in Photoshop )

@Foaly Please let me know if you could compile those changes to test my theory.

LaurentGomila · 2018-02-04T10:01:04Z

This is a big problem.

Indeed, we should apply the spacing factor to the empty space between characters and not to the global advance. I'm not sure if those arbitrary calculations (whitespace_width / 3) are "correct"; I even wonder if for spacingFactor == 1.0 it produces the exact same result as before this PR.

Can't we use some extra Glyph information? For example, subtracting the glyph's width from its advance should in theory give the empty space. And we shouldn't forget about kerning, too.

oomek · 2018-02-04T11:26:07Z

I seriously doubt we can extract the type width from the font as I looked at some fonts in the editor and the width already contains the clearance between the characters. My math is the closest approximation you can get. For the large font like 60pt the whitespace = 25 pixels, the clearance between the chracters = 9 pixels:
letterSpacing = ( whitespaceWidth / 3.0 ) * ( m_letterSpacingFactor - 1.0 )
so for m_letterSpacingFactor = 1.0
letterSpacing = ( 25.0 / 3.0 ) * ( 1.0 - 1.0 ) = 0.0
for m_letterSpacingFactor = 2.0
letterSpacing = ( 25.0 / 3.0 ) * ( 2.0 - 1.0 ) = 8.333
for m_letterSpacingFactor = 3.0
letterSpacing = ( 25.0 / 3.0 ) * ( 3.0 - 1.0 ) = 16.666
for m_letterSpacingFactor = 0.0
letterSpacing = ( 25.0 / 3.0 ) * ( 0.0 - 1.0 ) = -8.333

then you add it instead of multiplying later:
x += glyph.advance + letterSpacing;

If the getLineSpacing function returns float we should be fine, but if it's an int we are screwed for small characters. That's why I would need this compiled to make sure I'm right.

Foaly · 2018-02-04T15:53:36Z

Thank you for testing and reporting! Indeed there seems to be an error in the implementation.
Right now the letterSpacingFactor is applied only to the advance of a glyph. The actual distance between two glyphs is put together from the advance of the first glyph and the kerning offset between the second and first. They are applied at different points, so I must have missed it.
I would like to try taking the kerning into account first, before applying some scaling. I would be interested in how you came up with / 3.0?

oomek · 2018-02-04T15:59:09Z

I've rendered some fonts in Photoshp in large scale and counted pixels and checked how the clearance is behaving for different letter spacings. it was almost always 1/3 of a space. I did not count the offset between the characters as this changes from character to character, but rather the delta between different letter spacing. Delta was the same for each glyph pairs. For Arial additional clearance is whiteSpace / 2.77(7) * ( factor -1.0 )

oomek · 2018-02-04T16:04:32Z

If .advance returns the distance with kerning applied that's even better as according to my tests, the delta in pixels is the same for each letter pair when you set +100 or +200 tracking in Photoshop

LaurentGomila · 2018-02-04T18:45:20Z

I seriously doubt we can extract the type width from the font as I looked at some fonts in the editor and the width already contains the clearance between the characters.

I don't know what editor you're talking about, but in SFML, Glyph::advance is the total distance to the next glyph, and Glyph::bounds::width is the exact width of the glyph's pixels. So the difference between those should be exactly what we are looking for. I'd definitely give it a try as a fix for this PR.

If .advance returns the distance with kerning applied

It doesn't, and it can't since kerning depends not only on the character but also on the next one (in other words, it is defined for each possible pair of characters). Kerning is retrieved with a dedicated function from sf::Font.

oomek · 2018-02-04T18:58:01Z

It's FontForge.
Glyph::bounds::width will return from what I understand the distance between the vertical metric lines, which is definitely not something we are looking for as it contains the clearance between characters.
I'm attaching a screenshot

LaurentGomila · 2018-02-04T20:01:20Z

Glyph::bounds::width will return from what I understand the distance between the vertical metric lines, which is definitely not something we are looking for as it contains the clearance between characters.

No it does not. Again, it's defined as the width of the glyph's bitmap, so there's precisely no extra space in it. It's not taken from the font's metrics, just deduced from the bitmap.

However I just tested my idea, and it doesn't work. The "extra space" is not what we want to use, as some glyphs will have a big one and some others none at all, leading to inconsistent results within the same text. Moreover, it doesn't correctly applies to the space character, which has only extra space.

And regardless of the chosen solution, I'm not sure what to do with the kerning: it's a negative offset, which means that it brings glyphs closer to each other. Applying the factor to it would make the glyphs even more close, not sure this is what we want.

oomek · 2018-02-04T20:04:29Z

Could you try my method just to be sure. If it's close enough I'm fine with it as long as it does not produce uneven spacings between characters.

LaurentGomila · 2018-02-04T20:13:39Z

I'll try to find how other libraries implement it, and I'll give your solution a try if I get some time to test it.

oomek · 2018-02-04T20:14:27Z

Thanks

LaurentGomila · 2018-02-04T21:19:11Z

Qt have both options, and the relative spacing is implemented exactly as in this PR: they multiply the whole advance by the factor; which leads to the same weird results. This is really disappointing.

Others seem to use absolute offsets rather than relative factors -- maybe this is the reason 😞

oomek · 2018-02-05T05:02:57Z

I've built SFML with my changes and the result is better than I anticipated.
Vide below:
https://www.youtube.com/watch?v=K5ISsAalO6k

LaurentGomila · 2018-02-05T05:34:49Z

This is indeed really nice :)

You should also test interaction with kerning (add "AV" to your text, for example), and tiny character sizes (below 10).

LaurentGomila · 2018-02-05T08:07:59Z

After thinking more about it, I think it is a good idea to base the spacing on something that is specific to the font and character size, but remains the same for each glyph. It's not accurate, because strictly speaking, applying a factor should modify the existing spacing and not some artificial one, but as we've seen, this is what produces the best looking results. In the end, it's like a fixed offset, but which depends on the character size (I guess this is what we'd get with letter-spacing: 2em in CSS, for example).

As an additional test, you can try with a monospace font, just to make sure it still looks good with those fonts.

If your modification successfully passes all these tests, then I'm ok for applying it. But I'd like to get feedback from other members (including @Foaly and other people involved in this PR), if possible.

oomek · 2018-02-05T11:24:41Z

The spacing of small fonts and AV pairs looks also fine, except one thing. Smal fonts are a bit blurry when I adjust the scaling factor, but that’s ro be expected as the position is no longer snapped to pixels and small fonts position is optimizd to look crisp. I’m experimenting now to see if it can be overcome by quantizing the offsets.

Foaly · 2018-02-05T14:17:13Z

Oh the irony! Excuse me but I think this is hilarious!
I haven't gotten around to do some test myself, but from what I read from your descriptions, I see two ways of fixing this.
I guess the proper solution would be to somehow™ get the actual "whitespace" between two characters (meaning the distance between rightmost black pixel of the left character and the leftmost black pixel of the right character) containing kerning and everything and then apply the factor to that.
As @laurent explained this is not easily achievable, but maybe there is a way noone here has thought about yet. This would have the nice feature that the spacing is font and character size dependent. Since this seems hard to do a somewhat "dirty" fix would be to use magic numbers and ratios to achieve similar looking results (as @oomek suggested). I'm not a fan of such a design and would prefer a mathematically sane calculation, but I guess if there is no cleaner way I would be fine with it.

The second, simpler solution is to simply add an offset, that is added between two characters. For historic reasons I am gonna favour that one.

oomek · 2018-02-05T14:25:40Z

I do not see any irony here. Why would you prefer a fixed offset that is not dependant on the font size?

LaurentGomila · 2018-02-05T14:30:28Z

As @laurent explained this is not easily achievable

It is. I tested it, and it looks terrible. Extra spacing between characters must not depend on the actual one, but rather on some fixed value which only depends on character size.

The second, simpler solution is to simply add an offset, that is added between two characters.

I personally like @oomek's solution. It depends on the character size so it is indeed a factor, however it remains consistent within the whole text (doesn't depend on individual characters).

I would like to know your specific feedback/comments about this solution 😃

Foaly · 2018-02-05T14:41:21Z

Well it is ironic, because we spend about two years discussing whether to use factors or offsets. And now it turns out factors have this major flaw. But it was more of a design discussion and appearently this was overlooked. So I guess I also have to appologize for not testing thoroughly enough.
That is why I said I favour offset. Also it is the simplest solution to the problme (as in similar result, but no magic numbers).
But like @LaurentGomila explained the proper solution would be to have something character size dependent, which would also be fine by me. So thinking about it again maybe using the whitespaceWidth isn't such a "magic numbers" solution. Could you post a link to branch with your changes?

oomek · 2018-02-05T14:54:03Z

SegoeUI
https://www.youtube.com/watch?v=SYVYpDhi_AE
Consolas Monospaced
https://www.youtube.com/watch?v=N6kshXQ9rUU

Forgive me for my lack of professionalism, but I did not fork SFML, I didn't want to add a duplicate of your commit with just some minor changes. would it be ok to just upload the text.cpp alone and post here the link?

Foaly · 2018-02-05T14:57:52Z

Sure! Or maybe just the diff.

oomek · 2018-02-05T15:15:19Z

oomek@1e24d11

oomek · 2018-02-05T17:37:07Z

Not sure still If I have to ammend the lineSpacing as well. Will look into it when my Fibromyalgia flare goes away.

LaurentGomila · 2018-02-05T17:48:03Z

Line spacing should be ok.

Foaly · 2018-02-05T20:55:42Z

Looks fine by me. I'd say you could send a pull request for it.
The line spacing works much simpler and should be fine, but of course feel free to take a look :)

TankOs added feature m:sfml-graphics s:undecided not an issue and removed not an issue labels Jul 22, 2015

TankOs reviewed Jul 22, 2015
View reviewed changes

Foaly force-pushed the characterSpacing branch from 2b9de2b to 6b4a214 Compare July 22, 2015 11:27

Foaly force-pushed the characterSpacing branch from 6b4a214 to ca2672e Compare July 23, 2015 08:07

Foaly force-pushed the characterSpacing branch from a0a44a0 to 52d6829 Compare July 26, 2015 15:25

oomek reviewed Feb 4, 2018

View reviewed changes

Foaly deleted the characterSpacing branch February 10, 2018 17:48

SFML deleted a comment from eXpl0it3r Dec 31, 2022

Added a method to set and get additional spacing between characters. #928

Added a method to set and get additional spacing between characters. #928

Conversation

Foaly commented Jul 21, 2015 • edited by eXpl0it3r

TankOs Jul 22, 2015

Choose a reason for hiding this comment

TankOs commented Jul 22, 2015

MarioLiebisch commented Jul 22, 2015

Foaly commented Jul 22, 2015

MarioLiebisch commented Jul 22, 2015

mantognini commented Jul 22, 2015

Bromeon commented Jul 22, 2015

MarioLiebisch commented Jul 22, 2015

TankOs commented Jul 23, 2015

Foaly commented Jul 23, 2015

mantognini commented Jul 23, 2015

Foaly commented Jul 23, 2015

Foaly commented Jul 23, 2015

LaurentGomila commented Jul 23, 2015

zsbzsb commented Jul 23, 2015

Foaly commented Jul 25, 2015

Bromeon commented Jul 26, 2015

Foaly commented Jul 26, 2015

zsbzsb commented Jul 26, 2015

Foaly commented Jul 26, 2015

Bromeon commented Jul 27, 2015

Foaly commented Jul 30, 2015

oomek Feb 4, 2018

Choose a reason for hiding this comment

oomek Feb 4, 2018 • edited

Choose a reason for hiding this comment

oomek Feb 4, 2018

Choose a reason for hiding this comment

LaurentGomila commented Feb 4, 2018 • edited

oomek commented Feb 4, 2018 • edited

Foaly commented Feb 4, 2018

oomek commented Feb 4, 2018 • edited

oomek commented Feb 4, 2018 • edited

LaurentGomila commented Feb 4, 2018

oomek commented Feb 4, 2018

LaurentGomila commented Feb 4, 2018

oomek commented Feb 4, 2018

LaurentGomila commented Feb 4, 2018

oomek commented Feb 4, 2018

LaurentGomila commented Feb 4, 2018 • edited

oomek commented Feb 5, 2018

LaurentGomila commented Feb 5, 2018

LaurentGomila commented Feb 5, 2018 • edited

oomek commented Feb 5, 2018

Foaly commented Feb 5, 2018

oomek commented Feb 5, 2018

LaurentGomila commented Feb 5, 2018 • edited

Foaly commented Feb 5, 2018 • edited

oomek commented Feb 5, 2018

Foaly commented Feb 5, 2018

oomek commented Feb 5, 2018

oomek commented Feb 5, 2018

LaurentGomila commented Feb 5, 2018

Foaly commented Feb 5, 2018

Foaly commented Jul 21, 2015 •

edited by eXpl0it3r

oomek Feb 4, 2018 •

edited

LaurentGomila commented Feb 4, 2018 •

edited

oomek commented Feb 4, 2018 •

edited

oomek commented Feb 4, 2018 •

edited

oomek commented Feb 4, 2018 •

edited

LaurentGomila commented Feb 4, 2018 •

edited

LaurentGomila commented Feb 5, 2018 •

edited

LaurentGomila commented Feb 5, 2018 •

edited

Foaly commented Feb 5, 2018 •

edited