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

sf::Text setString optimization thoughts #413

Closed
SuperV1234 opened this Issue Jun 23, 2013 · 20 comments

Comments

Projects
None yet
6 participants
@SuperV1234

SuperV1234 commented Jun 23, 2013

Referring to file: https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Graphics/Text.cpp

Line 67: why doesn't setting the string check for string equality like setting the color/style/character size/font? This means that if I update a sf::Text with a string that rarely changes every frame in my game or application the geometry is recalculated every frame.
Consider checking for equality, or telling the user that the geometry is recalculated even if the string is equal to the one already present in the docs (so that he can check for equality in his code).

@Oberon00

This comment has been minimized.

Show comment
Hide comment
@Oberon00

Oberon00 Jun 23, 2013

Contributor

why doesn't setting the string check for string equality like setting the color/style/character size/font?

Probably because comparing strings is (depending on string length) much slower than comparing integers or pointers.

if I update a sf::Text with a string that rarely changes

This, on the other hand, seems to be far more likely than updating with a color/style/character size/font that rarely changes, so i guess the checks currently done hardly ever prevent a call to updateGemoetry() whereas a check in setString() would probably do -- at the cost of an expensive check.

Maybe the checks should be removed altogether, simplifying the code and even saving some instructions for the (very probably) most common use cases.

Contributor

Oberon00 commented Jun 23, 2013

why doesn't setting the string check for string equality like setting the color/style/character size/font?

Probably because comparing strings is (depending on string length) much slower than comparing integers or pointers.

if I update a sf::Text with a string that rarely changes

This, on the other hand, seems to be far more likely than updating with a color/style/character size/font that rarely changes, so i guess the checks currently done hardly ever prevent a call to updateGemoetry() whereas a check in setString() would probably do -- at the cost of an expensive check.

Maybe the checks should be removed altogether, simplifying the code and even saving some instructions for the (very probably) most common use cases.

@ghost ghost assigned LaurentGomila Jun 23, 2013

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 23, 2013

Member

Actually, I'd start by moving the geometry updating into the drawing code and just flag the text object "dirty" whenever something changes (plus create some way to trigger this without drawing).

The current code will update the geometry whenever something is changed, even if all these changes happen right after each other without ever drawing anything.

Member

MarioLiebisch commented Jun 23, 2013

Actually, I'd start by moving the geometry updating into the drawing code and just flag the text object "dirty" whenever something changes (plus create some way to trigger this without drawing).

The current code will update the geometry whenever something is changed, even if all these changes happen right after each other without ever drawing anything.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 23, 2013

@MarioLiebisch I thought about this too, but what if someone changes the character size, then gets the glyph width, then changes the style size? He would get the wrong glyph width because the changes weren't "registered" yet.

SuperV1234 commented Jun 23, 2013

@MarioLiebisch I thought about this too, but what if someone changes the character size, then gets the glyph width, then changes the style size? He would get the wrong glyph width because the changes weren't "registered" yet.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 23, 2013

Member

Those could simply trigger the update as well (if required). There's just one if() additional overhead, but you save a lot more when updating more than one detail at once.

Member

MarioLiebisch commented Jun 23, 2013

Those could simply trigger the update as well (if required). There's just one if() additional overhead, but you save a lot more when updating more than one detail at once.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 23, 2013

@MarioLiebisch Didn't think about that - I agree with you now.

SuperV1234 commented Jun 23, 2013

@MarioLiebisch Didn't think about that - I agree with you now.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 24, 2013

@MarioLiebisch I'm trying to create a pull request - however, the "const" modifier in the draw function and the getters prevents me to achieve what we discussed earlier: take a look.

https://github.com/SuperV1234/SFML/commit/ba21c251c4f4ff796df05e6c1e2ee206d3186882

Any idea?

SuperV1234 commented Jun 24, 2013

@MarioLiebisch I'm trying to create a pull request - however, the "const" modifier in the draw function and the getters prevents me to achieve what we discussed earlier: take a look.

https://github.com/SuperV1234/SFML/commit/ba21c251c4f4ff796df05e6c1e2ee206d3186882

Any idea?

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Jun 24, 2013

Contributor

Make vertex array and the bool mutable and the update method const. But that's quite fishy, Laurent might not like.

Contributor

FRex commented Jun 24, 2013

Make vertex array and the bool mutable and the update method const. But that's quite fishy, Laurent might not like.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 24, 2013

@FRex How about this?

https://github.com/SuperV1234/SFML/commit/792b01d7a9d4a064cbf9e27078869da178637fcf

It's a little fishy, but understandable, in my opinion. I tested it and I get much better performance in my game.

SuperV1234 commented Jun 24, 2013

@FRex How about this?

https://github.com/SuperV1234/SFML/commit/792b01d7a9d4a064cbf9e27078869da178637fcf

It's a little fishy, but understandable, in my opinion. I tested it and I get much better performance in my game.

@Oberon00

This comment has been minimized.

Show comment
Hide comment
@Oberon00

Oberon00 Jun 24, 2013

Contributor

Please be consistent with SFML style and use 4 spaces instead of just two for indentation, otherwise the diffs are hardly readable (without appending ?w=1 to the URL).

Contributor

Oberon00 commented Jun 24, 2013

Please be consistent with SFML style and use 4 spaces instead of just two for indentation, otherwise the diffs are hardly readable (without appending ?w=1 to the URL).

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 24, 2013

@Oberon00: comparison between my master and Laurent's master shows no difference in the tab/spacing

SuperV1234 commented Jun 24, 2013

@Oberon00: comparison between my master and Laurent's master shows no difference in the tab/spacing

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Jun 24, 2013

Contributor

@SuperV1234 https://github.com/SuperV1234/SFML/commit/ba21c251c4f4ff796df05e6c1e2ee206d3186882 did seem to have lots of things just nudged two places to the left.
I think const cast is undefined behaviour if object was really const before. Not sure about mutables. And it needs Laurent input.

Contributor

FRex commented Jun 24, 2013

@SuperV1234 https://github.com/SuperV1234/SFML/commit/ba21c251c4f4ff796df05e6c1e2ee206d3186882 did seem to have lots of things just nudged two places to the left.
I think const cast is undefined behaviour if object was really const before. Not sure about mutables. And it needs Laurent input.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 24, 2013

Member

Don't bother doing a pull request. The modification is really easy to implement, but it first requires some thinking.

Member

LaurentGomila commented Jun 24, 2013

Don't bother doing a pull request. The modification is really easy to implement, but it first requires some thinking.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Jun 24, 2013

@LaurentGomila Alright, I'm deleting my fork but leaving this issue open for the future.

SuperV1234 commented Jun 24, 2013

@LaurentGomila Alright, I'm deleting my fork but leaving this issue open for the future.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 23, 2014

Member

Laurent, do you want a typical cache: m_vertices and m_bounds become mutable, updateGeometry() is declared const and called only before draw(), and a bool m_geometryNeedUpdate variable is added?

And should I keep the current check for equality with the previous value (and add a new one for strings)?

Member

Bromeon commented Mar 23, 2014

Laurent, do you want a typical cache: m_vertices and m_bounds become mutable, updateGeometry() is declared const and called only before draw(), and a bool m_geometryNeedUpdate variable is added?

And should I keep the current check for equality with the previous value (and add a new one for strings)?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 23, 2014

Member

I've never been able to decide which implementation was the "cleanest": mutable attributes + const update method, or a const_cast (or something else?). So... do whatever you prefer ;)

Member

LaurentGomila commented Mar 23, 2014

I've never been able to decide which implementation was the "cleanest": mutable attributes + const update method, or a const_cast (or something else?). So... do whatever you prefer ;)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 23, 2014

Member

Ok. But should I keep the equality check? It catches the cases where the same value is set repeatedly, but it incurs an overhead for strings that may be notable in scenarios with a lot of long texts (unlikely, but who knows how sf::Text is used). Maybe leave it as is (i.e. no strings, but the rest) and add the cache?

By the way, const_cast is undefined behavior if the object itself is declared const (not only references/pointers to it), so this is not an option.

Member

Bromeon commented Mar 23, 2014

Ok. But should I keep the equality check? It catches the cases where the same value is set repeatedly, but it incurs an overhead for strings that may be notable in scenarios with a lot of long texts (unlikely, but who knows how sf::Text is used). Maybe leave it as is (i.e. no strings, but the rest) and add the cache?

By the way, const_cast is undefined behavior if the object itself is declared const (not only references/pointers to it), so this is not an option.

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 Mar 23, 2014

mutable attributes + const update sounds good, const_cast just feels dirty.

SuperV1234 commented Mar 23, 2014

mutable attributes + const update sounds good, const_cast just feels dirty.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 23, 2014

Member

The string comparison will be useful only if setString is called very often, like every frame. In this case, it's very likely that the same string is used (I can't see any use case where the string contents would really change every frame). If setString is only called when the string changes, the extra overhead of the comparison will most likely be negligible.

Member

LaurentGomila commented Mar 23, 2014

The string comparison will be useful only if setString is called very often, like every frame. In this case, it's very likely that the same string is used (I can't see any use case where the string contents would really change every frame). If setString is only called when the string changes, the extra overhead of the comparison will most likely be negligible.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 23, 2014

Member

const_cast just feels dirty.

It is, see above ;)

If setString is only called when the string changes, the extra overhead of the comparison will most likely be negligible.

True, especially since operator== detects faster whether two strings are different than whether they are equal. So I'll implement a check for equality everywhere.

Member

Bromeon commented Mar 23, 2014

const_cast just feels dirty.

It is, see above ;)

If setString is only called when the string changes, the extra overhead of the comparison will most likely be negligible.

True, especially since operator== detects faster whether two strings are different than whether they are equal. So I'll implement a check for equality everywhere.

@Bromeon Bromeon added the resolved label Mar 29, 2014

@Bromeon Bromeon assigned Bromeon and unassigned LaurentGomila Mar 29, 2014

@Bromeon Bromeon closed this in 666da80 Mar 29, 2014

@SuperV1234

This comment has been minimized.

Show comment
Hide comment
@SuperV1234

SuperV1234 commented Mar 29, 2014

👍

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this issue Apr 15, 2014

Cached sf::Text attributes
Two optimizations:
- If a value remains the same, nothing happens
- Recompute geometry only before drawing and bound access, not after each set

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