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

Added support for outlined text #840

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@zsbzsb
Member

zsbzsb commented Mar 24, 2015

Considering all the requests over the years for outlined text (and the totally hackish workarounds) and also that my current project will definitely need this at some point, I went ahead and implemented it. I made the PR for code review.

Some key points are the following.

  • Outlined glyphs are stored in the same page as normal glyphs (can be changed if this is deemed that it will fill up the page texture too fast).
  • Text::[set|get]Color(...) is now marked as deprecated, instead everyone should use Text::[set|get]FillColor(...)
  • Font::getGlyph(...) has a new parameter, outline thickness which is defaulted to 0 (to avoid API breakage). The parameter determines how thick the outline will be in pixels.
  • Since outlined glyphs are stored in the same page/texture the key type has been changed to Int64 to continue being unique now that the outline thickness is part of the key.
  • I refactored Text::ensureGeometryUpdate() and split the duplicated code to append vertices to separate functions ::addLine(...) and ::addGlyphQuad(...). These can be renamed if someone could think of more appropriate names.
  • Outline thickness is unsigned, I don't see any easy ways with freetype to make the outline protrude inwards with a negative thickness. It is a float, but a negative value causes distorted rendering.
  • Nothing should change with the current rendering of text if the user decides not to use outlines.

As stated before, please review this and give some feedback.

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window({ 700, 400 }, "Outline test");
    window.setVerticalSyncEnabled(true);

    sf::Font font;
    font.loadFromFile("C:/Windows/Fonts/arial.ttf");

    sf::Text text(L"\u0476ZyG\noLPfkdf", font, 140);
    text.setPosition({ 20, 20 });

    text.setFillColor(sf::Color::Red);

    text.setOutlineThickness(5);
    text.setOutlineColor(sf::Color::White);

    text.setStyle(sf::Text::Underlined | sf::Text::Italic | sf::Text::StrikeThrough);

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

        window.clear();

        window.draw(text);

        window.display();
    }
}

Example Image

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 24, 2015

Member

Sounds interesting. :) Just skipped through the code. You're essentially creating two glyphs, one for the inside, one for the outline?

Out of interest (haven't had time to try it): Is the outline really just the outline? Or does it include parts covered by the inner part of the font. In essence, could you render the outline only while keeping the inside transparent?

Member

MarioLiebisch commented Mar 24, 2015

Sounds interesting. :) Just skipped through the code. You're essentially creating two glyphs, one for the inside, one for the outline?

Out of interest (haven't had time to try it): Is the outline really just the outline? Or does it include parts covered by the inner part of the font. In essence, could you render the outline only while keeping the inside transparent?

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 24, 2015

Member

You're essentially creating two glyphs, one for the inside, one for the outline?

Yes, its the only way to colorize the outline differently.

Is the outline really just the outline? Or does it include parts covered by the inner part of the font. In essence, could you render the outline only while keeping the inside transparent?

I was wondering the same thing so I tested that yesterday when I was working on it. Actually, freetype when stroking the outline draws on both sides of the character outline. So if you don't render the fill (or set to transparent) the rendered outline will extend to both sides (up to outline thickness) of the character's outline. I don't see any way to make it so that it doesn't do that.

transparent fill

I also just added a note in the documentation about this.

Member

zsbzsb commented Mar 24, 2015

You're essentially creating two glyphs, one for the inside, one for the outline?

Yes, its the only way to colorize the outline differently.

Is the outline really just the outline? Or does it include parts covered by the inner part of the font. In essence, could you render the outline only while keeping the inside transparent?

I was wondering the same thing so I tested that yesterday when I was working on it. Actually, freetype when stroking the outline draws on both sides of the character outline. So if you don't render the fill (or set to transparent) the rendered outline will extend to both sides (up to outline thickness) of the character's outline. I don't see any way to make it so that it doesn't do that.

transparent fill

I also just added a note in the documentation about this.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 24, 2015

Member

Ah, well, that's partially what I wanted. :)

But yeah, no issue, just been curious about this.

Member

MarioLiebisch commented Mar 24, 2015

Ah, well, that's partially what I wanted. :)

But yeah, no issue, just been curious about this.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 28, 2015

Member

@LaurentGomila @eXpl0it3r @binary1248 @MarioLiebisch @mantognini @TankOs

If no one has anything against this change, can we try for a 2.3 inclusion?

Member

zsbzsb commented Mar 28, 2015

@LaurentGomila @eXpl0it3r @binary1248 @MarioLiebisch @mantognini @TankOs

If no one has anything against this change, can we try for a 2.3 inclusion?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 28, 2015

Member

Haven't had a chance to try it yet, but I definitely like it (and planned to do something similar in the past).

Member

MarioLiebisch commented Mar 28, 2015

Haven't had a chance to try it yet, but I definitely like it (and planned to do something similar in the past).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 28, 2015

Member

I have nothing to say against this PR.

Member

LaurentGomila commented Mar 28, 2015

I have nothing to say against this PR.

Show outdated Hide outdated include/SFML/Graphics/Text.hpp Outdated
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 4, 2015

Member

Tested quickly on Ubuntu, seems to run smoothly. 👍

Beside my to comments, the code in sf::Font could use some refactoring but it's not really related to this PR so... Commits need to be squashed too. Then we could include it in 2.4 I guess (can someone @SFML/sfml validate this and set the milestone?).

Member

mantognini commented Jul 4, 2015

Tested quickly on Ubuntu, seems to run smoothly. 👍

Beside my to comments, the code in sf::Font could use some refactoring but it's not really related to this PR so... Commits need to be squashed too. Then we could include it in 2.4 I guess (can someone @SFML/sfml validate this and set the milestone?).

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Jul 6, 2015

Member

Commits need to be squashed too

Can be done once the changes are verified.

Then we could include it in 2.4 I guess (can someone @SFML/sfml validate this and set the milestone?).

Sounds great 👍

Member

zsbzsb commented Jul 6, 2015

Commits need to be squashed too

Can be done once the changes are verified.

Then we could include it in 2.4 I guess (can someone @SFML/sfml validate this and set the milestone?).

Sounds great 👍

Show outdated Hide outdated include/SFML/Graphics/Text.hpp Outdated
@@ -143,6 +144,15 @@ bool Font::loadFromFile(const std::string& filename)
return false;
}
// Load the stroker that will be used to outline the font
FT_Stroker stroker;
if (FT_Stroker_New(static_cast<FT_Library>(m_library), &stroker) != 0)

This comment has been minimized.

@Bromeon

Bromeon Jul 27, 2015

Member

Is the stroker resource released if one of the subsequent allocations (charmap) fails?
It's assigned to a member variable, but is a cleanup() call guaranteed in all cases?

@Bromeon

Bromeon Jul 27, 2015

Member

Is the stroker resource released if one of the subsequent allocations (charmap) fails?
It's assigned to a member variable, but is a cleanup() call guaranteed in all cases?

This comment has been minimized.

@zsbzsb

zsbzsb Jul 27, 2015

Member

cleanup() is always called in the destructor and loadFromFile(...). I didn't change any of the program flow regarding the cleanup function so if there is a problem with it then it has been in the code base for quite some time. ;)

@zsbzsb

zsbzsb Jul 27, 2015

Member

cleanup() is always called in the destructor and loadFromFile(...). I didn't change any of the program flow regarding the cleanup function so if there is a problem with it then it has been in the code base for quite some time. ;)

Show outdated Hide outdated include/SFML/Graphics/Text.hpp Outdated
@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Aug 12, 2015

Member

Anything else? Is the lineLength and lineTop naming fine now for the addLine(...) function?

(I also now rebased onto master)

PS: +360 is a nice number (full circle) so let's not change anything. :)

Member

zsbzsb commented Aug 12, 2015

Anything else? Is the lineLength and lineTop naming fine now for the addLine(...) function?

(I also now rebased onto master)

PS: +360 is a nice number (full circle) so let's not change anything. :)

@TankOs TankOs added this to the 2.4 milestone Aug 12, 2015

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 12, 2015

Member

👍 🐈
Seems good to me, well done.

Member

TankOs commented Aug 12, 2015

👍 🐈
Seems good to me, well done.

@TankOs TankOs added s:accepted and removed s:undecided labels Aug 12, 2015

Show outdated Hide outdated src/SFML/Graphics/Text.cpp Outdated
@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 14, 2015

Member

Any chance we could get this merged so @Foaly could get #928 updated?

Member

zsbzsb commented Sep 14, 2015

Any chance we could get this merged so @Foaly could get #928 updated?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 14, 2015

Member

Looks good.

It feels weird to have an integer outline thickness, but there's no other way since it's used in the lookup key.

Member

LaurentGomila commented Sep 14, 2015

Looks good.

It feels weird to have an integer outline thickness, but there's no other way since it's used in the lookup key.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 14, 2015

Member

It feels weird to have an integer outline thickness

Blame freetype's stroker API for that.

there's no other way since it's used in the lookup key

That wasn't really an issue, if we had to use a float as part of the key we could have fun with casting (same way sf::Packet handles things).

Member

zsbzsb commented Sep 14, 2015

It feels weird to have an integer outline thickness

Blame freetype's stroker API for that.

there's no other way since it's used in the lookup key

That wasn't really an issue, if we had to use a float as part of the key we could have fun with casting (same way sf::Packet handles things).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 14, 2015

Member

Why reinterpret-casting? Wouldn't rounding and static-casting suffice?

It's of course an annoying limitation, but should we expose the implementation details? What if we switch to another backend in the future (if there is one) that supports floats? 😃

Member

Bromeon commented Sep 14, 2015

Why reinterpret-casting? Wouldn't rounding and static-casting suffice?

It's of course an annoying limitation, but should we expose the implementation details? What if we switch to another backend in the future (if there is one) that supports floats? 😃

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 15, 2015

Member

Why reinterpret-casting? Wouldn't rounding and static-casting suffice?

We would lose the benefit of floats then, because the range [x - 0.5 ... x + 0.5] of thicnkess will all map to the same set of glyphs (x). It's used as a lookup key, so we really need one entry per different outline thickness value.

It's of course an annoying limitation, but should we expose the implementation details?

Since implementation doesn't allow us to implement something else... yes :p

Member

LaurentGomila commented Sep 15, 2015

Why reinterpret-casting? Wouldn't rounding and static-casting suffice?

We would lose the benefit of floats then, because the range [x - 0.5 ... x + 0.5] of thicnkess will all map to the same set of glyphs (x). It's used as a lookup key, so we really need one entry per different outline thickness value.

It's of course an annoying limitation, but should we expose the implementation details?

Since implementation doesn't allow us to implement something else... yes :p

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 15, 2015

Member

How about accepting a float parameter, but documenting that it only supports like 0.1 steps (i.e. multiply by 10)? This limitation could later on be lifted.

Or we could just accept float and say it actually only supports integer values? This sounds like the worse idea to me though.

Member

MarioLiebisch commented Sep 15, 2015

How about accepting a float parameter, but documenting that it only supports like 0.1 steps (i.e. multiply by 10)? This limitation could later on be lifted.

Or we could just accept float and say it actually only supports integer values? This sounds like the worse idea to me though.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 18, 2015

Contributor

I think we should prohibit negative outline thickness, since it doesn't make sense and returns ugly results. So if negative values are passed set it to 0.
Other than that everything looks good!

Contributor

Foaly commented Sep 18, 2015

I think we should prohibit negative outline thickness, since it doesn't make sense and returns ugly results. So if negative values are passed set it to 0.
Other than that everything looks good!

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 18, 2015

Member

I think we should prohibit negative outline thickness, since it doesn't make sense and returns ugly results.

That's an option -- although simply documenting it is also one, and if the user insists, then that's his problem. It's not SFML's task to enforce correct usage of the library. All we should do is prevent common mistakes with reasonable effort. I'm not sure if passing negative thicknesses can be called common, it makes no sense in the first place...

So if negative values are passed set it to 0.

No. Never do that, it just hides logic errors. Assertions are a more meaningful approach here.

However, I would personally prefer if we first decide on a defined way to handle user errors (in the forum), and apply that consistently throughout the library. Otherwise we end up with a patchwork of assertions, program terminations, sf::err() outputs, ignored errors, silently corrected errors and undefined behavior.

Thus, I would not introduce any error checks now. Otherwise you'd have to do it in hundreds of other places, and by then we'll better have agreed on one...

Member

Bromeon commented Sep 18, 2015

I think we should prohibit negative outline thickness, since it doesn't make sense and returns ugly results.

That's an option -- although simply documenting it is also one, and if the user insists, then that's his problem. It's not SFML's task to enforce correct usage of the library. All we should do is prevent common mistakes with reasonable effort. I'm not sure if passing negative thicknesses can be called common, it makes no sense in the first place...

So if negative values are passed set it to 0.

No. Never do that, it just hides logic errors. Assertions are a more meaningful approach here.

However, I would personally prefer if we first decide on a defined way to handle user errors (in the forum), and apply that consistently throughout the library. Otherwise we end up with a patchwork of assertions, program terminations, sf::err() outputs, ignored errors, silently corrected errors and undefined behavior.

Thus, I would not introduce any error checks now. Otherwise you'd have to do it in hundreds of other places, and by then we'll better have agreed on one...

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 19, 2015

Member

I have now added a note to the docs about using a negative outline thickness (as float outlines are supported). I then wrapped the casting with a second static cast just to be on the safe side. I have also avoided introducing any error checking as @Bromeon suggested.

@eXpl0it3r this should be ready as all issues have been addressed. Unless someone wants to find something else to change :) I have also rebased onto the latest 2.3.2 master.

Member

zsbzsb commented Sep 19, 2015

I have now added a note to the docs about using a negative outline thickness (as float outlines are supported). I then wrapped the casting with a second static cast just to be on the safe side. I have also avoided introducing any error checking as @Bromeon suggested.

@eXpl0it3r this should be ready as all issues have been addressed. Unless someone wants to find something else to change :) I have also rebased onto the latest 2.3.2 master.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 20, 2015

Member

I created a little editor to experiment with outlined texts here: https://gist.github.com/Bromeon/d9510ecfd0b6884ce88f

The functionality works, apart from the following issues:

  • Bold text is not outlined correctly. This would need a special case, because of the way in which bold text is implemented.
  • Wenn you change the outline thickness with F5/F6, you'll see that it doesn't appear continuous. The outline seems to shrink sometimes when it should grow. I'm not sure if this is a problem of OpenGL rasterization, or if there is an error with rounding or so.
  • When a newline is appended at the end, and strikethrough or underline style is used, then the outline of that \n whitespace is drawn on the next line (even though there's no text to outline).
Member

Bromeon commented Sep 20, 2015

I created a little editor to experiment with outlined texts here: https://gist.github.com/Bromeon/d9510ecfd0b6884ce88f

The functionality works, apart from the following issues:

  • Bold text is not outlined correctly. This would need a special case, because of the way in which bold text is implemented.
  • Wenn you change the outline thickness with F5/F6, you'll see that it doesn't appear continuous. The outline seems to shrink sometimes when it should grow. I'm not sure if this is a problem of OpenGL rasterization, or if there is an error with rounding or so.
  • When a newline is appended at the end, and strikethrough or underline style is used, then the outline of that \n whitespace is drawn on the next line (even though there's no text to outline).
@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 23, 2015

Member

@Bromeon Thanks for testing and writing a small test to find these issues 😄

Bold text is not outlined correctly. This would need a special case, because of the way in which bold text is implemented.

This was actually another issue with the glyph key creation. For some reason the bitshifting wasn't working right when shifting 32bit integers and then combining into a 64 bit key. So I wrapped each part in a static_cast<Uint64>(...) and now it works correctly.

When** you change the outline thickness with F5/F6, you'll see that it doesn't appear continuous. The outline seems to shrink sometimes when it should grow. I'm not sure if this is a problem of OpenGL rasterization, or if there is an error with rounding or so.

Looking at this it seems to be a mix of FreeType stroking/rasterization and floating point inaccuracy. Rounding the outline coordinates (when rendering) only seemed to make the problem worse so I don't think it is OpenGL rasterization. If you look closely you will notice that the outline does enlarge/shrink as requested, the issue is that it moves just slightly around the stationary text fill which gives the illusion that it is shrinking when it should be growing (and vice versa).

I also tried rendering at 2x the size and then shrinking the rendered text and it is hardly an issue. This alone makes me think it comes down to floating point inaccuracy and is something we can't exactly fix. Let's also be realistic, most users are not going to be using such small values changes for the outline thickness. If you take your test code and increment the outline thickness by .4f the issue almost entirely disappears.

When a newline is appended at the end, and strikethrough or underline style is used, then the outline of that \n whitespace is drawn on the next line (even though there's no text to outline).

Fixed, this was a fault of the original code flow that it would actually append an invisible line on the ending blank line. I corrected this by adding a check to make sure the line has a length before adding any lines. It may even make text rendering slightly faster.

Member

zsbzsb commented Sep 23, 2015

@Bromeon Thanks for testing and writing a small test to find these issues 😄

Bold text is not outlined correctly. This would need a special case, because of the way in which bold text is implemented.

This was actually another issue with the glyph key creation. For some reason the bitshifting wasn't working right when shifting 32bit integers and then combining into a 64 bit key. So I wrapped each part in a static_cast<Uint64>(...) and now it works correctly.

When** you change the outline thickness with F5/F6, you'll see that it doesn't appear continuous. The outline seems to shrink sometimes when it should grow. I'm not sure if this is a problem of OpenGL rasterization, or if there is an error with rounding or so.

Looking at this it seems to be a mix of FreeType stroking/rasterization and floating point inaccuracy. Rounding the outline coordinates (when rendering) only seemed to make the problem worse so I don't think it is OpenGL rasterization. If you look closely you will notice that the outline does enlarge/shrink as requested, the issue is that it moves just slightly around the stationary text fill which gives the illusion that it is shrinking when it should be growing (and vice versa).

I also tried rendering at 2x the size and then shrinking the rendered text and it is hardly an issue. This alone makes me think it comes down to floating point inaccuracy and is something we can't exactly fix. Let's also be realistic, most users are not going to be using such small values changes for the outline thickness. If you take your test code and increment the outline thickness by .4f the issue almost entirely disappears.

When a newline is appended at the end, and strikethrough or underline style is used, then the outline of that \n whitespace is drawn on the next line (even though there's no text to outline).

Fixed, this was a fault of the original code flow that it would actually append an invisible line on the ending blank line. I corrected this by adding a check to make sure the line has a length before adding any lines. It may even make text rendering slightly faster.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 29, 2015

Member

And last reviewing/testing round? 😉

Member

eXpl0it3r commented Sep 29, 2015

And last reviewing/testing round? 😉

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 29, 2015

Contributor

I did a test with the latest version of this branch and the test code that Bromeon provided.
Everything looks good and seems to work fine.

The only thing I noticed was that starting at a outline thickness of 16 you start to see weird artefacts around dots (like the dot on the i or with a colon). But I guess that is the stroker from FT and we can do much about that. Example image:
unbenannt

Contributor

Foaly commented Sep 29, 2015

I did a test with the latest version of this branch and the test code that Bromeon provided.
Everything looks good and seems to work fine.

The only thing I noticed was that starting at a outline thickness of 16 you start to see weird artefacts around dots (like the dot on the i or with a colon). But I guess that is the stroker from FT and we can do much about that. Example image:
unbenannt

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2015

Member

I also experience the artifact mentioned by Foaly at several punctuation characters. It only appears at extreme outlines though. It might be a limitation of the stroker. With C:/Windows/Fonts/MinionPro-Medium.otf, the artifacts are round instead of edgy 😀

Other than that, it seems good -- bold text and newlines at the end have been fixed!

Member

Bromeon commented Sep 30, 2015

I also experience the artifact mentioned by Foaly at several punctuation characters. It only appears at extreme outlines though. It might be a limitation of the stroker. With C:/Windows/Fonts/MinionPro-Medium.otf, the artifacts are round instead of edgy 😀

Other than that, it seems good -- bold text and newlines at the end have been fixed!

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 30, 2015

Member

Do we wait for the deprecation macro PR to be merged, or do we merge this one now and add the deprecated stuff in a new PR?

Member

LaurentGomila commented Sep 30, 2015

Do we wait for the deprecation macro PR to be merged, or do we merge this one now and add the deprecated stuff in a new PR?

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 30, 2015

Member

I can also reproduce the artifacts and looking and the glyph texture can confirm it is how FreeType is stroking the glyph (they don't seem to handle intersecting vertices very well which seems to also be part of the reason negative outlines look ugly).

Member

zsbzsb commented Sep 30, 2015

I can also reproduce the artifacts and looking and the glyph texture can confirm it is how FreeType is stroking the glyph (they don't seem to handle intersecting vertices very well which seems to also be part of the reason negative outlines look ugly).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2015

Member

Do we wait for the deprecation macro PR to be merged, or do we merge this one now and add the deprecated stuff in a new PR?

I wouldn't mind if #969 were merged soon, it's something that's required by various upcoming features. Also, the deprecation is a really small and not API-concerning modification, so it requires less review and discussion than actual features like this PR.

I can also reproduce the artifacts

Ok. I think this pull request shouldn't be held back because of minor imperfections like that. If it really matters, we can still fix/workaround it at a later stage. But it's well possible that no one even notices in real-world scenarios.

If somebody else could review the code, that would be nice!

Member

Bromeon commented Sep 30, 2015

Do we wait for the deprecation macro PR to be merged, or do we merge this one now and add the deprecated stuff in a new PR?

I wouldn't mind if #969 were merged soon, it's something that's required by various upcoming features. Also, the deprecation is a really small and not API-concerning modification, so it requires less review and discussion than actual features like this PR.

I can also reproduce the artifacts

Ok. I think this pull request shouldn't be held back because of minor imperfections like that. If it really matters, we can still fix/workaround it at a later stage. But it's well possible that no one even notices in real-world scenarios.

If somebody else could review the code, that would be nice!

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2015

Member

I don't know about you guys, but I prefer having Text::addLine() and Text::addGlyphQuad() defined as free functions local to the translation unit. They don't have to access any members and are just adding to the external interface (even though they are private).

Member

binary1248 commented Oct 2, 2015

I don't know about you guys, but I prefer having Text::addLine() and Text::addGlyphQuad() defined as free functions local to the translation unit. They don't have to access any members and are just adding to the external interface (even though they are private).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 2, 2015

Member

I agree with @binary1248, an anonymous namespace would be ideal. Furthermore, those functions should take the sf::VertexArray& container as the first parameter.

Member

Bromeon commented Oct 2, 2015

I agree with @binary1248, an anonymous namespace would be ideal. Furthermore, those functions should take the sf::VertexArray& container as the first parameter.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Oct 4, 2015

Member

So if I understand correctly, you want to see Text::addLine(...) and Text::addGlyphQuad(...) moved to the anonymous namespace and also the vertex array as the first argument?

Member

zsbzsb commented Oct 4, 2015

So if I understand correctly, you want to see Text::addLine(...) and Text::addGlyphQuad(...) moved to the anonymous namespace and also the vertex array as the first argument?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 4, 2015

Member

Exactly 😃

Member

Bromeon commented Oct 4, 2015

Exactly 😃

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 25, 2015

Member

Any progress, @zsbzsb?

Member

eXpl0it3r commented Oct 25, 2015

Any progress, @zsbzsb?

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Oct 27, 2015

Member

I was waiting for the deprecation macro to be merged, so I will be updating this in the next few days.

Member

zsbzsb commented Oct 27, 2015

I was waiting for the deprecation macro to be merged, so I will be updating this in the next few days.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 10, 2015

Member

What's the upper limit of days for "few"? 😜

Member

eXpl0it3r commented Nov 10, 2015

What's the upper limit of days for "few"? 😜

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Nov 10, 2015

Member

@eXpl0it3r All changes have now been made. Text::[get|set]Color(...) has been marked as deprecated with the new macro and the examples were also updated. Also the addLine(...) and addGlyphQuad(...) functions have now been moved into an anonymous namespace.

Everything should now be done so I think it is final review time 😄

Member

zsbzsb commented Nov 10, 2015

@eXpl0it3r All changes have now been made. Text::[get|set]Color(...) has been marked as deprecated with the new macro and the examples were also updated. Also the addLine(...) and addGlyphQuad(...) functions have now been moved into an anonymous namespace.

Everything should now be done so I think it is final review time 😄

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Nov 29, 2015

Member

@SFML Anything else?

Member

zsbzsb commented Nov 29, 2015

@SFML Anything else?

Show outdated Hide outdated include/SFML/Graphics/Text.hpp Outdated
@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

It looks mostly good. I haven't tested the code again now -- there have not been any functional changes since the last iteration, right?

Member

Bromeon commented Nov 29, 2015

It looks mostly good. I haven't tested the code again now -- there have not been any functional changes since the last iteration, right?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 29, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Nov 29, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Dec 29, 2015

Member

All is done..... again 😀

Member

zsbzsb commented Dec 29, 2015

All is done..... again 😀

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 31, 2015

Member

Merge in 957cabb

Member

eXpl0it3r commented Dec 31, 2015

Merge in 957cabb

@eXpl0it3r eXpl0it3r closed this Dec 31, 2015

@zsbzsb zsbzsb deleted the zsbzsb:feature/outlined_text branch Feb 12, 2016

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