Extending Font and Text to support multiple textures #2282
Conversation
JonnyPtn
force-pushed
the
test
branch
2 times, most recently
from
4edbbf9
to
27374c5
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2282 +/- ##
==========================================
- Coverage 14.98% 14.87% -0.12%
==========================================
Files 213 208 -5
Lines 15255 15285 +30
Branches 4072 4100 +28
==========================================
- Hits 2286 2273 -13
- Misses 12821 12879 +58
+ Partials 148 133 -15
Continue to review full report at Codecov.
|
|
CI failure looks unrelated, think it just needs retrying |
|
Great feedback @kimci86, thanks. Think I've covered all the comments now |
src/SFML/Graphics/Font.cpp
Outdated
| if (!page) | ||
| { | ||
| page = &*pages.insert(pages.end(), Page{m_isSmooth}); |
There was a problem hiding this comment.
Maybe if (page == nullptr) for explicitness.
I have the feeling
pages.push_back(Page(m_isSmooth));
page = &pages.back();might be clearer than this address-deref-insert-at-end expression.
Last, use constructor syntax Page(m_isSmooth).
There was a problem hiding this comment.
Why deref then take the address? Can't we just use the return value of std::list::emplace_back?
page = pages.emplace_back(m_isSmooth);There was a problem hiding this comment.
Why deref then take the address? Can't we just use the return value of
std::list::emplace_back?page = pages.emplace_back(m_isSmooth);
I can't say why as I didn't originally write this, but using emplace_back seems like a good shout, will do
Maybe
if (page == nullptr)for explicitness.
I don't think comparing to nullptr makes it more explicit, if anything it makes it less intuitive (we're checking if there's not a page, which is exactly how this code reads)
Last, use constructor syntax
Page(m_isSmooth).
why?
src/SFML/Graphics/Font.cpp
Outdated
| glyph.textureRect = findGlyphRect(page, {width, height}); | ||
| // Find a page that can fit well the glyph | ||
| Page* page = NULL; | ||
| for (PageList::iterator it = pages.begin(); it != pages.end() && !page; ++it) |
There was a problem hiding this comment.
As above, I don't consider this an improvement
src/SFML/Graphics/Font.cpp
Outdated
| // Try to find a good position for the new glyph into the texture | ||
| glyph.textureRect = findGlyphRect(*it, {width, height}); | ||
|
|
||
| if (glyph.textureRect.width > 0) | ||
| page = &*it; |
There was a problem hiding this comment.
This assigns to glyph.textureRect even if no good rect is found, and leaves this assigned beyond the for loop. I see that you have another glyph.textureRect = findGlyphRect(*page, {width, height}); further down, so it's correct.
However, this flow is very prone to errors on refactorings. The function is also getting rather big, and variable mutations are mixed with "found" state. A separate function could make things easier:
struct MatchingPage {
Page* page = nullptr;
IntRect textureRect;
};
std::optional<MatchingPage> findMatchingPage(...);// usage:
if (auto matching = findMatchingPage(...)) {
page = matching->page;
glyph.textureRect = matching->textureRect;
...
} else {
...
}There was a problem hiding this comment.
Personally I think this would over-engineer it for little gain - the behaviour is only required in one place so I would prefer to use a lambda limited to this scope, but then I don't think that would really address any of your concerns.
Would like to hear other opinions on this though
JonnyPtn
requested review from
Bromeon and
ChrisThrasher
and removed request for
kimci86 and
Bromeon
JonnyPtn
requested review from
Bromeon and
ChrisThrasher
and removed request for
ChrisThrasher and
Bromeon
|
AFAIK there's no convention in SFML regarding pointer nullability checks. with 🎉 for this style: if (ptr)
if (!ptr)and 👀 for this style: if (ptr != nullptr)
if (ptr == nullptr)I'm fine with either, but would be good to settle. I'll add 1 reaction each. Similar topic: |
Co-authored-by: Chris Thrasher <[email protected]>
Co-authored-by: Chris Thrasher <[email protected]>
|
What else are we waiting for on this? |
|
@SFML/sfml Any more comments on this one? We should decide if we want to move ahead with this PR or not, as merge conflicts start accumulating. |
I defer to the judgment of @binary1248 or anyone else who understands OpenGL since this is outside of my expertise. It seems like a good idea from what I can tell though. |
|
My only concern in #1645 was that performance might take a hit, but that can be optimized after this gets merged. It would still be interesting to hear from text heavy users if/how this commit affects their performance. |
Rebase of #1645 as it seems GitHub has got it's knickers in a twist