-
Edit: Moved the thread to SFML development and give a more descriptive title in light of the sub-forum change
#include <SFML/Graphics.hpp>
#include <cstdlib>
void breakFont(sf::Font& f)
{
for (int i = 0; i < 30; ++i)
{
char a = 'a' + std::rand() % ('z' - 'a' + 1); //random char from 'a'..'z'
f.getGlyph(a, 30u, false);
}
}
int main(int argc, char * argv[])
{
std::srand(std::time(0x0));
sf::RenderWindow app(sf::VideoMode(640u, 480u), "Font");
app.setFramerateLimit(30u);
bool broken = false;
const std::string filename("OpenDyslexic-Regular.ttf");
sf::Font font;
font.loadFromFile(filename);
sf::Text text("font is broken", font, 30u);
while (app.isOpen())
{
sf::Event eve;
while (app.pollEvent(eve))
{
if (eve.type == sf::Event::Closed) app.close();
if (!broken && eve.type == sf::Event::KeyPressed)
{
broken = true;
font.loadFromFile(filename);
breakFont(font);
}
}
app.clear();
app.draw(text);
app.display();
}
}
Change filename to any font you want to use, it shouldn't matter, I used OpenDyslexic from http://opendyslexic.org/
Basically sf::Text displays garbage when sf::Font is reloaded and glyphs of same size get loaded in other order, if no glyphs get loaded it'll just display nothing, but is well defined, operator[] on map will insert new default constructed GlyphTable and return it's default constructed sf::Texture resulting in nothing.
It's a non issue for almost all sane use cases but I'm putting it out here with example so that it's known, I couldn't find it when I searched, if you want to ensure sf::Text is ok you can call text.setString(text.getString()) clear the string and set it again to force updateGeometry to be called (you can't call it directly, it's private).
-
Hmmm, what's actually also kind of scary is that simply reloading the font, will make the text vanish:
#include <SFML/Graphics.hpp>
int main()
{
sf::RenderWindow app(sf::VideoMode(640u, 480u), "Font");
app.setFramerateLimit(30u);
bool broken = false;
const std::string filename("DroidSans.ttf");
sf::Font font;
font.loadFromFile(filename);
sf::Text text("font is broken", font, 30u);
while (app.isOpen())
{
sf::Event eve;
while (app.pollEvent(eve))
{
if (eve.type == sf::Event::Closed) app.close();
if (!broken && eve.type == sf::Event::KeyPressed)
{
broken = true;
font.loadFromFile(filename);
text.setFont(font);
text.setCharacterSize(30u);
text.setString("font is broken");
}
}
app.clear();
app.draw(text);
app.display();
}
}
While you usually don't reload fonts, I think this is still an issue that should get fixed. ;)
-
I like how you respond a bit over a year later. ;D
It's not too real of a problem... but if you really want to fix it, putting an unsinged in sf::Font, bumping it on reload and on setPixelated(because you probably come from that thread) and then saving and checking it in ensureGeometryUpdate will work.
-
Oh right we're in 2015. ;D
I don't really understand your suggested fix, but I hope others will take a look.
-
He suggests that sf::Font maintains a "change id", which it increments every time the whole set of glyphs are invalidated. So if a sf::Text sees that the font id has changed (compared to the one the font had when it last built its vertex array), then the font has changed and it must discard and rebuild its own vertex array.
It is similar to what sf::Texture does so that sf::RenderTarget knows when it has changed, and can invalidate its internal texture cache.
This is probably the only way to handle this, if we don't want the font to be able to directly notify objects that depend on it, but I can't think of a clean way to add such a function. That would be a private member of sf::Font, and sf::Text would be declared as friend to access it?
-
In my own code where I have similar situation I call that variable dirtyness and it has a public getter.
There is no reason to make it 100% private because that'd harm any new text layout class that isn't sf::Text because they couldn't check for that.
Hmmm, what's actually also kind of scary is that simply reloading the font, will make the text vanish:
Rendering garbage sounded more dramatic and I originally wanted to make the garbage spell out something meaningful by manipulating the order of loading glyphs but it didn't work. :P
-
In my own code where I have similar situation I call that variable dirtyness and it has a public getter.
Yes, I guess we can hardly find a better name for this... :(
-
Yes, I guess we can hardly find a better name for this... :(
"tainted" would be an option.
-
Or "invalidated".
-
Anything in -ed is wrong, because this is an ID, not a boolean.
-
How could "refreshCount" then?
-
Here it is, I named the variable Laurent:
https://gist.github.com/FRex/ced31eca2872ca610c96
;D ;D ;D
Ok, jokes aside, the name can be changed later but I'm asking about all the other code:
https://github.com/FRex/SFML
The if in Text.cpp and the surrounding code is a bit :( but otherwise I think it looks fine:
https://github.com/FRex/SFML/blob/master/src/SFML/Graphics/Text.cpp#L245
-
// Bump Laurent
++m_Laurent;
Ouch!
-
Do you want these unsigned ints to be sf::Uint64 instead? (For the use case where someone somehow makes exactly 2^32 loads between two draws.)
Also, special thanks to exploiter for merging xcb last night. I had to install xcb-devel, xcb-util-devel, xcb-util-image-devel and xcb-util-wm-devel to get SFML to compile. :P
Well.. at least I confirmed it still works on Fedora 21.
-
Do you want these unsigned ints to be sf::Uint64 instead? (For the use case where someone somehow makes exactly 2^32 loads between two draws.)
Yes, this can't hurt, and will be more consistent with the one in sf::Texture (*).
(*) I'm surprised you still haven't suggested to apply the same API (public getLaurent() ;D) to sf::Texture.
-
Done...
As for sf::Texture: In theory there is no reason why it's private but I don't care about it, since it's possible use (caching in own GL code while using sf::Texture) is waaay more niche than this (own text layout class).
-
I hate to nitpick, but it should be 'dirtiness', not 'dirtyness' ;)
-
It should but that is a common misspelling, including in "buffer dirtyness" context. :P
So.. we will see what Laurent says. ;D
-
It should but that is a common misspelling
Hmm... I expected more rigor from you :P
If the correct spelling is "dirtiness" then "dirtiness" must be used.
-
Done too. ;)
Have you looked at the if I mentioned?
https://github.com/FRex/SFML/blob/master/src/SFML/Graphics/Text.cpp#L245
Because of the way it is now, every time text gets drawn without a font set, it will reset it's vertices and bounds but I'd say it's OK enough.
-
Yes of course, it's more than ok.
-
Do you want a pull request for this?
-
I'd like to have other opinions, but since there's none, let's do this PR, yes.
-
If there's no other way, I'm okay with it.
What's the opinion of the others?
-
So... After re-reading this whole thread again, I still can't see why nobody considered just making sf::Text (yet another) friend of sf::Texture so that it can check m_cacheId itself. Any time font texture data is updated, the sf::Text would check the cache ID and make sure that the geometry is also updated. Yes... it is a bit more conservative (i.e. might update more) than what FRex has proposed, but it is much simpler if you ask me, and more importantly, it doesn't require changes to the public API.
-
Because:
There is no reason to make it 100% private because that'd harm any new text layout class that isn't sf::Text because they couldn't check for that.
-
But the question now would be: If exposing it through the API is only important for custom text classes, wouldn't the user be able to take care of this themselves? I figured that the original issue was that the SFML classes did not communicate among themselves enough, thus leading to the garbage text. If the user knows when they load a new font, surely they can also notify their own text class to update its geometry as well no?
-
Hmm... this is a valid point. Although it might be really complicated for the user to do it externally, instead of relying on a direct feature in sf::Font. But not impossible.
-
In theory, you can require users to notify sf::Text too... but it's super tedious. :P
IMO: There is no reason to make sf::Font a friend of sf::Text for this, SFML is supposed to not be an all inclusive toolset so making use of class harder for users (who can't add a friend declaration) seems really counterproductive.
This is not a change BTW, it's a 100% backwards compatible addition, nothing is gone or altered so any 2.x user code will still work.
-
Well... I still don't know. What sf::Font does now is basically "caching". The problem is that sf::Text assumes that the cache stays valid forever hence it never checked for it's validity (however that might have been implemented). Caching, be it in sf::Texture via the cache ID or now in sf::Font is really an internal detail if you ask me. Having to expose it through the public API just so non-SFML classes can be built on top of this specific implementation doesn't feel right.
Strictly speaking, one could even argue that when you set an sf::Text's font, you promise not to reload it without setting it again. Reloading the font without re-setting it would break that promise since you are in effect "changing the font" without informing (re-setting the font) the sf::Text. This implies that the (m_font != &font) check would have to be removed though, but I honestly can't think of a reasonable use for that check anyway :P.
-
I don't know either, I more often than not take "this cements the stupid stuff away so no one has to deal with it ever again" and "don't be annoying" over "feels right (to make users do tedious stuff)". But then again, I write mostly for myself.
You see that I let this thread sit for a year unnoticed, it's because for me it's just trivia about sf::Text and sf::Font - I don't care because I never reuse sf::Texture and sf::Font instances myself.
Exploiter noticed it because I pointed to it in pixelated text thread, saying that dirtyness counter would fix this issue as well. Your fix would handle both too but it'd be a fair bit more... obnoxious to users who reuse sf::Font instances (fortunately not me :P).
-
Strictly speaking, one could even argue that when you set an sf::Text's font, you promise not to reload it without setting it again. Reloading the font without re-setting it would break that promise since you are in effect "changing the font" without informing (re-setting the font) the sf::Text. This implies that the (m_font != &font) check would have to be removed though, but I honestly can't think of a reasonable use for that check anyway :P.
Well in my example (http://en.sfml-dev.org/forums/index.php?topic=14019.msg123543#msg123543) I did reload the font and did reset the font on the text object and yet it still didn't render, which in my opinion really shouldn't happen...
Anyways what the general verdict on the open PR?
-
Well in my example I did reload the font and did reset the font on the text object and yet it still didn't render
-->
This implies that the (m_font != &font) check would have to be removed
-
Ah didn't realize why it should get removed. ;)
-
Strictly speaking, one could even argue that when you set an sf::Text's font, you promise not to reload it without setting it again. Reloading the font without re-setting it would break that promise since you are in effect "changing the font" without informing (re-setting the font) the sf::Text.
I kind of agree with you, but shouldn't we make things consistent? Like if you set a texture and you "reload" as much as you want without having to resetting it.
Should we add a vote to the thread?
-
I agree that something should probably be done about it. I just don't have a good feeling about adding something to the public interface just because of SFML's internal optimization. The interface guarantees behaviour between the library and user code, independent of implementation. If the implementation doesn't fulfil those guarantees then it should be fixed without adding even more to the public interface which means even more guarantees. In this case the new guarantee would mean that we won't be able to abstract from the optimization because it has been made public.
If a user wants to create their own text class then they are free to implement their own optimizations as well. But they will have to bear in mind that they are not able to reuse something that is internal to SFML. They will have to make it work independent of SFML, which in the end is a better choice anyway if you want to build something that should be reusable.
-
Just a quick note on the status of this PR (https://github.com/SFML/SFML/pull/769) since no consensus was found since January on whether we should:
* "help" users who reuse `sf::Font` instances by adding a public versioning index (aka `dirtiness`) to "notify" users that the font has changed [this PR],
* do the same but privately (by making `sf::Font` and `sf::Text` friends) and recommend users who implement some utility like `sf::Text` to not rely on this feature [highly debatable IMO],
* do nothing and let the users manually notify `sf::Text` when they reload a font. [opposite direction of this PR]
Applying this PR would make things more consistent with `sf::Texture`. I think we should do it because if the user has to keep track of which `sf::Font` a `sf::Text` instance uses then it will become quite a mess very quickly (you'll need some very clever resource manager). Also, `sf::Text` takes a reference to a font and the user has to guarantee the font will live long enough, so why wouldn't it make sure it's displaying the right thing?
-
This topic is still open for discussion. The problem summarized: Reloading sf::Font while it is used by sf::Text instances invalidates the glyph resources, and sf::Text does currently not recognize this, leading to bad rendering. Hiura summarized possible solutions just above.
The question is also whether a sf::Font instance can be considered "the same" after it has been reloaded, or after it has been re-assigned using operator= (which is another case to consider). If so, we would use the object's address to identify it, i.e. any font constructed in the same place would be considered identical. In an extreme case, even manual destruction + placement new should not break this.
The other option is that any reload should be considered as a fresh instance, and texts referring to it become invalid. This is probably less intuitive for the user. But we should also consider that in SFML 3, the bool Font::loadFromXy() methods will probably vanish in favor of static Font Font::fromXy() factories that can throw exceptions. Reloading then effectively equals creating a new instance (even if the same address is being reused).
I'm not sure whether we should enlarge the public interface with a "dirtyness counter" (or "generation", which is a better name in my opinion). Consequently, it would have to be done for other classes relying on some cache as well. And as binary1248 states, it exposes an implementation detail (namely the fact that SFML uses a cache), and in case that changes one day, the API has to change as well.
So I'm personally rather in favor of implementing this privately. It can still be made public if we see that there are many people using another front-end to sf::Font so that this becomes an actual issue; but let's not do it prematurely and limit ourselves in the future.
-
So I'm personally rather in favor of implementing this privately. It can still be made public if we see that there are many people using another front-end to sf::Font so that this becomes an actual issue; but let's not do it prematurely and limit ourselves in the future.
That could indeed be a good starting point and make things evolve a bit. After all, few are the people who reload a font, aren't they?
-
So I'm personally rather in favor of implementing this privately. It can still be made public if we see that there are many people using another front-end to sf::Font so that this becomes an actual issue; but let's not do it prematurely and limit ourselves in the future.
I thought the whole problem was that we couldn't implement it privately? Or what exactly did you mean with that?
-
It is possible, the question was whether we should do it.
The argument against making the validity/generation/dirty flag private was that sf::Font clients other than sf::Text cannot query the font whether its values are up-to-date, instead they need to be notified manually when a font is reloaded.
The argument for making it private was that we wouldn't expose implementation details of an internal optimization (that might possibly change in the future), and that other sf::Font clients combined with font reloading is an extremely rarely occurring situation, not justifying the price.
-
Bump!
Do we need a poll?
-
Maybe.. Personally I'd say we could start with what Nexus said:
So I'm personally rather in favor of implementing this privately. It can still be made public if we see that there are many people using another front-end to sf::Font so that this becomes an actual issue; but let's not do it prematurely and limit ourselves in the future.
-
A few years later... :D
Digging this older thread out, as we've once again did indeed run into someone wanting build their on text implementation.
Quick re-cap, FRex's PR (http://) was eventually superseded by Foaly's PR (http://), which is what we're having these days, where sf::Font friends sf::Text and then we can access the texture cache ID owned by the font from the text class.
This works great for our own text class, but anyone wanting to implement their own text class, won't be able to figure out if the font has changed.
We decided against resolving this limitation for the time being and see if there are people who actually want to take advantage of.
Today, someone Discord (JSpirit) asked about having the cache ID public, but they are not the first one to ask. Over the past few years, we've seen this request not very regularly, but still more than enough, that we shouldn't ignore it.
But now, standing in the middle of the SFML 3 development, we also have a new option, that is to not only add to the API, but to change it, if it helps with this problem.
Anyone got an idea that's better than having a public dirtiness flag?
-
Two ideas that we briefly discussed on Discord where
- Callbacks
- Observer Pattern
kimci86 wrote a quick code mock up to understand what the observer pattern could look like:
class FontObserver
{
public:
virtual void onFontUpdate() = 0;
};
class Font
{
public:
void addObserver(FontObserver&);
void removeObserver(FontObserver&);
private:
void notifyObservers()
{
for(auto* obs : m_observers)
obs->onFontUpdate();
}
std::vector<FontObserver*> m_observers;
};
-
void sf::Font::onUpdate(const std::function<void(your-args-here)>&) is a compelling interface option to me. std::function provides all the utilities we need to easily implement this. I'm not convinced we need to support an arbitrary number of callables, at least not initially.