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

Fix for broken text when the font is reloaded. #1345

Merged
merged 1 commit into from Jan 24, 2018

Conversation

Foaly
Copy link
Contributor

@Foaly Foaly commented Jan 15, 2018

Hi!
I took the liberty to write an update for this PR #769. There has been no activity in the last 2 years. I updated the implementaion to the decision in the forum discussion. https://en.sfml-dev.org/forums/index.php?topic=14019.30
Hopefully we can fix the bug soon!

@@ -393,6 +393,7 @@ class SFML_GRAPHICS_API Text : public Drawable, public Transformable
mutable VertexArray m_outlineVertices; ///< Vertex array containing the outline geometry
mutable FloatRect m_bounds; ///< Bounding rectangle of the text (in local coordinates)
mutable bool m_geometryNeedUpdate; ///< Does the geometry need to be recomputed?
mutable Uint64 m_fontTextureId; ///< The fonts texture id
Copy link
Member

Choose a reason for hiding this comment

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

Comment looks weird, either add an apostrophe or leave the s away from fonts.

class RenderTarget;
class RenderTexture;
class InputStream;
class Text;
class Window;
Copy link
Member

Choose a reason for hiding this comment

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

No need to reorder the declarations here... Add a single line instead of modifying 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though having them alphabetically made more sense then random.

Copy link
Member

Choose a reason for hiding this comment

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

👍

// Do nothing, if geometry has not changed
if (!m_geometryNeedUpdate)
// Do nothing, if geometry has not changed and the font texture has not changed
if (!m_geometryNeedUpdate && m_font->getTexture(m_characterSize).m_cacheId == m_fontTextureId)
Copy link
Member

Choose a reason for hiding this comment

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

In certain situations, this will dereference a NULL pointer causing undefined behaviour. The check for m_font is already done further down. Either move it up or move this down.

return;

// Save the current fonts texture id
m_fontTextureId = m_font->getTexture(m_characterSize).m_cacheId;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that m_fontTextureId isn't initialized to a value prior to this line, meaning that in the comparison above, it would contain an undefined value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed

@Foaly
Copy link
Contributor Author

Foaly commented Jan 16, 2018

By the way if someone wants to test I've been using exploiters old test code

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window(sf::VideoMode(800, 600), "Example");

    bool broken = false;

    const std::string filename("a font");

    sf::Font font;
    font.loadFromFile(filename);

    sf::Text text("font is broken", font, 30u);

    // 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();
                }

                if (!broken)
                {
                    broken = true;
                    font.loadFromFile(filename);
                    //text.setFont(font);
                    //text.setCharacterSize(30u);
                    //text.setString("font is broken");
                }
            }
        }

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

        window.draw(text);

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

@mantognini
Copy link
Member

mantognini commented Jan 16, 2018

Works like a charm on mac. 👍

return;

// Save the current fonts texture id
m_fontTextureId = m_font->getTexture(m_characterSize).m_cacheId;
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that m_fontTextureId isn't initialized to a value prior to this line, meaning that in the comparison above, it would contain an undefined value.

class RenderTarget;
class RenderTexture;
class InputStream;
class Text;
class Window;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Foaly
Copy link
Contributor Author

Foaly commented Jan 21, 2018

I think this can be merged now.

@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Jan 21, 2018
@eXpl0it3r eXpl0it3r added this to Ready in SFML 2.5.0 Jan 21, 2018
@eXpl0it3r eXpl0it3r merged commit c24de5f into SFML:master Jan 24, 2018
@eXpl0it3r eXpl0it3r moved this from Ready to Merged / Superseded in SFML 2.5.0 Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

4 participants