Letter spacing bugfix #1366

oomek · 2018-02-06T12:38:26Z

Ties letter spacing factors to the font size.

LaurentGomila

The documentation should be updated as well. We don't need to give too much technical detail, but users should at least know what kind of result to expect.

LaurentGomila · 2018-02-06T13:01:47Z

src/SFML/Graphics/Text.cpp

@@ -317,7 +317,9 @@ 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_letterSpacingFactor;
+    float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance;
+    float letterSpacing   = ( whitespaceWidth / 3.0 ) * ( m_letterSpacingFactor - 1.0 );


Minor comment: use floats, not doubles (3.f, 1.f, etc.).

Foaly · 2018-02-06T16:03:10Z

src/SFML/Graphics/Text.cpp

-    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;


Minor code style: the = should be aligned with the ones surrounding it

Foaly · 2018-02-06T16:05:55Z

Great! Besides the documentation that Laurent mentioned I think the commit message should be changed to something more meaningful. Maybe: Fix letter spacing factors (now tied to the font size). Or something like that.

oomek · 2018-02-06T16:08:00Z

I’m at the hospital until evening UK time. Sorry for lagging.

Foaly · 2018-02-06T16:40:43Z

No worries! Take your time. Health comes first :)

Fixed letter spacing factors (now tied to the font size)

oomek · 2018-02-06T22:19:40Z

I'm a Github noob, as you have probably noticed. Is it acceptable now?

oomek · 2018-02-06T22:28:02Z

@LaurentGomila I'm not sure what documentation are you referring to. if it's the comment block in the Text.hpp it's descriptive enough I think.

LaurentGomila · 2018-02-07T05:39:45Z

I'd replace this

/// This method enables you to set a factor to the spacing
/// between letters.

(which is really really bad, but so as many other comments in SFML 😁 )

With something like this

/// This factor doesn't directly apply to the existing
/// spacing between each character, it rather adds a fixed
/// space between them which is calculated from the font
/// metrics and the character size.
/// Note that factors below 1 (including negative numbers) bring
/// characters closer to each other
///

LaurentGomila · 2018-02-07T11:02:56Z

I didn't mean to replace the whole comment, only the part that I posted above. Please keep the indication of default value, and we can also keep the first sentence.

And it's just an example that I wrote quickly, feel free to adapt/correct/complete it instead of just copy-pasting it 😃

oomek · 2018-02-07T11:13:55Z

Thank you for being patient. It's my first pull request done in git. I used the website to publish my code before.

oomek · 2018-02-07T11:15:01Z

And besides, I'm not a native English speaker, so I chose the safest way :)

Foaly · 2018-02-07T11:52:08Z

I know this has the potential to blow up the discussion, but reading over this again, wouldn't it be a better design to set the start of the additional factor at 0 intead of 1. So it would be a factor in percent or negative percent instead of somewhat randomly starting for 1 as no additional offset.

Foaly · 2018-02-07T11:59:42Z

include/SFML/Graphics/Text.hpp

+    /// spacing between each character, it rather adds a fixed
+    /// space between them which is calculated from the font
+    /// metrics and the character size.
+    /// Note that factors below 1 (including negative numbers) bring


Well this is not entirely correct I would say. My proposal:

/// This factor doesn't directly apply to the existing /// spacing between each character. It rather uses a fixed /// space which is calculated from the fonts whitespace /// and the character size. This spacing is scaled by the /// `spacingFactor` and added to the default spacing between /// two characters.

this is not entirely correct

Which part is incorrect?

My proposal

Well, no need to describe all implementation details. Users don't need to know that.

Foaly · 2018-02-07T12:00:12Z

include/SFML/Graphics/Text.hpp

+    /// metrics and the character size.
+    /// Note that factors below 1 (including negative numbers) bring
+    /// characters closer to each other.
+    /// By default the letter spacing factor is 1.
    ///
    /// \param spacing New letter spacing factor


Parameter name is: spacingFactor

LaurentGomila · 2018-02-07T12:26:19Z

wouldn't it be a better design to set the start of the additional factor at 0 intead of 1

Since it's a factor, it makes sense to start at 1. And it remains consistent with the line spacing factor. And if one day we find a better algorithm that use actual factors, then I don't want to have to change the behaviour from the user point of view.

If we go too far in the direction of "this is an offset, not a factor" we'll have to revert all that stuff to use actual offsets in public interface ;)

oomek · 2018-02-07T14:34:50Z

Starting at 1.0 makes sense as going below 0.0 results in letters overlaping each other. We don’t need factoring in (-n,n) range but rather (0,n)

Foaly · 2018-02-08T20:50:56Z

... use actual offset ...

You know that is the secret plan all along 😉

The point I wanted to add was that the factor affects that fixed spacing and not the "default" spacing. But yeah I guess it doesn't matter to much.

oomek · 2018-02-08T21:11:36Z

Soooo, could we finally close that chapter please? I’m having an impression that we are overthinking it a little bit.

Foaly · 2018-02-08T21:31:42Z

If @LaurentGomila is fine with it, it's ok from my side.

LaurentGomila · 2018-02-09T08:20:29Z

The point I wanted to add was that the factor affects that fixed spacing and not the "default" spacing.

I think it is important, that's why I say it in my version of the comment. My point was: users need to know how it behaves, not how it is precisely calculated.

Soooo, could we finally close that chapter please? I’m having an impression that we are overthinking it a little bit.

Proper documentation is not "overthinking". It's probably the most important thing, now that the technical issues have been sorted out. As long as the PR is not blocked in endless arguments, there's no hurry, let's not be afraid to discuss even the most minor details 😉

If @LaurentGomila is fine with it, it's ok from my side.

If the current code has been tested and still works as expected, I'm totally fine with it.

eXpl0it3r · 2018-02-15T20:29:43Z

Squashed and merged in deeb3a9

Modification of Foaly's Commit

1e24d11

Ties letter spacing factors to the font size.

eXpl0it3r added bug m:sfml-graphics s:accepted labels Feb 6, 2018

eXpl0it3r added this to Discussion in SFML 2.5.0 via automation Feb 6, 2018

eXpl0it3r added this to the 2.5 milestone Feb 6, 2018

eXpl0it3r requested a review from LaurentGomila February 6, 2018 13:04

eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.5.0 Feb 6, 2018

LaurentGomila requested changes Feb 6, 2018

View reviewed changes

Foaly reviewed Feb 6, 2018

View reviewed changes

Code style fixes

cbcd93d

Fixed letter spacing factors (now tied to the font size)

Updated documentation for setLetterSpacing function

0cb5f6e

Updated documentation for setLetterSpacing function

0a6739a

Foaly reviewed Feb 7, 2018

View reviewed changes

Updated documentation

6a66905

LaurentGomila approved these changes Feb 9, 2018

View reviewed changes

eXpl0it3r changed the title ~~Modification of Foaly's Commit~~ Letter spacing bugfix Feb 10, 2018

eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Feb 11, 2018

eXpl0it3r closed this Feb 15, 2018

SFML 2.5.0 automation moved this from Ready to Merged / Superseded Feb 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Letter spacing bugfix #1366

Letter spacing bugfix #1366

oomek commented Feb 6, 2018

LaurentGomila left a comment

LaurentGomila Feb 6, 2018

Foaly Feb 6, 2018

Foaly commented Feb 6, 2018

oomek commented Feb 6, 2018

Foaly commented Feb 6, 2018

oomek commented Feb 6, 2018

oomek commented Feb 6, 2018

LaurentGomila commented Feb 7, 2018

LaurentGomila commented Feb 7, 2018 •

edited

oomek commented Feb 7, 2018

oomek commented Feb 7, 2018

Foaly commented Feb 7, 2018

Foaly Feb 7, 2018

LaurentGomila Feb 7, 2018

Foaly Feb 7, 2018

LaurentGomila commented Feb 7, 2018 •

edited

oomek commented Feb 7, 2018

Foaly commented Feb 8, 2018 •

edited

oomek commented Feb 8, 2018

Foaly commented Feb 8, 2018

LaurentGomila commented Feb 9, 2018

eXpl0it3r commented Feb 15, 2018

Letter spacing bugfix #1366

Letter spacing bugfix #1366

Conversation

oomek commented Feb 6, 2018

LaurentGomila left a comment

Choose a reason for hiding this comment

LaurentGomila Feb 6, 2018

Choose a reason for hiding this comment

Foaly Feb 6, 2018

Choose a reason for hiding this comment

Foaly commented Feb 6, 2018

oomek commented Feb 6, 2018

Foaly commented Feb 6, 2018

oomek commented Feb 6, 2018

oomek commented Feb 6, 2018

LaurentGomila commented Feb 7, 2018

LaurentGomila commented Feb 7, 2018 • edited

oomek commented Feb 7, 2018

oomek commented Feb 7, 2018

Foaly commented Feb 7, 2018

Foaly Feb 7, 2018

Choose a reason for hiding this comment

LaurentGomila Feb 7, 2018

Choose a reason for hiding this comment

Foaly Feb 7, 2018

Choose a reason for hiding this comment

LaurentGomila commented Feb 7, 2018 • edited

oomek commented Feb 7, 2018

Foaly commented Feb 8, 2018 • edited

oomek commented Feb 8, 2018

Foaly commented Feb 8, 2018

LaurentGomila commented Feb 9, 2018

eXpl0it3r commented Feb 15, 2018

LaurentGomila commented Feb 7, 2018 •

edited

LaurentGomila commented Feb 7, 2018 •

edited

Foaly commented Feb 8, 2018 •

edited