Extending Font and Text to support multiple textures #2282

JonnyPtn · 2022-11-22T16:24:26Z

Rebase of #1645 as it seems GitHub has got it's knickers in a twist

codecov · 2022-11-22T16:45:48Z

Codecov Report

Merging #2282 (7da2848) into master (8c6b578) will decrease coverage by 0.11%.
The diff coverage is 0.96%.

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

Impacted Files	Coverage Δ
include/SFML/Graphics/Font.hpp	`0.00% <ø> (ø)`
include/SFML/Graphics/Text.hpp	`0.00% <ø> (ø)`
src/SFML/Graphics/Font.cpp	`0.00% <0.00%> (ø)`
src/SFML/Graphics/Text.cpp	`0.00% <0.00%> (ø)`
include/SFML/Graphics/Glyph.hpp	`75.00% <100.00%> (ø)`
include/SFML/System/String.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
include/SFML/System/Clock.hpp	`50.00% <0.00%> (-50.00%)`	⬇️
include/SFML/Graphics/Shape.hpp	`0.00% <0.00%> (-50.00%)`	⬇️
include/SFML/Network/Packet.hpp	`0.00% <0.00%> (-50.00%)`	⬇️
include/SFML/Graphics/Transformable.hpp	`0.00% <0.00%> (-50.00%)`	⬇️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c6b578...7da2848. Read the comment docs.

JonnyPtn · 2022-11-22T17:54:22Z

CI failure looks unrelated, think it just needs retrying

include/SFML/Graphics/Font.hpp

src/SFML/Graphics/Font.cpp

src/SFML/Graphics/Text.cpp

JonnyPtn · 2022-11-23T13:57:59Z

Great feedback @kimci86, thanks.

Think I've covered all the comments now

Bromeon

Thanks a lot for this! 👍

include/SFML/Graphics/Font.hpp

src/SFML/Graphics/Font.cpp

Bromeon · 2022-11-27T12:02:51Z

src/SFML/Graphics/Font.cpp

+        if (!page)
+        {
+            page = &*pages.insert(pages.end(), Page{m_isSmooth});


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).

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);

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

Bromeon · 2022-11-27T12:07:26Z

src/SFML/Graphics/Font.cpp

-        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)


&& page == nullptr

As above, I don't consider this an improvement

Bromeon · 2022-11-27T12:19:33Z

src/SFML/Graphics/Font.cpp

+            // 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;


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 { ... }

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

src/SFML/Graphics/Text.cpp

Bromeon · 2022-11-28T11:49:08Z

AFAIK there's no convention in SFML regarding pointer nullability checks.
To get a quick impression what people prefer, maybe react to this message...

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: if (integer == 0), here I would advocate always explicit checks.

ChrisThrasher

I don't know enough about textures, fonts, and text to review the core logic in depth but I did try to comment on the parts that I do understand. This looks promising though!

include/SFML/Graphics/Font.hpp

src/SFML/Graphics/Font.cpp

src/SFML/Graphics/Text.cpp

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>

JonnyPtn · 2023-03-10T18:13:32Z

What else are we waiting for on this?

Bromeon · 2023-04-03T06:59:53Z

@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.

ChrisThrasher · 2023-04-03T07:02:08Z

@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.

binary1248 · 2023-04-03T07:47:13Z

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.

eXpl0it3r mentioned this pull request Nov 22, 2022

Font text multiple textures #1645

Closed

eXpl0it3r added bug m:sfml-graphics s:accepted labels Nov 22, 2022

eXpl0it3r added this to Backlog in SFML 3.0.0 via automation Nov 22, 2022

eXpl0it3r added this to the 3.0 milestone Nov 22, 2022

JonnyPtn force-pushed the test branch 2 times, most recently from 4edbbf9 to 27374c5 Compare November 22, 2022 16:41

kimci86 reviewed Nov 22, 2022

View reviewed changes

JonnyPtn requested a review from kimci86 November 23, 2022 13:58

Bromeon reviewed Nov 27, 2022

View reviewed changes

hobby8 and others added 11 commits November 28, 2022 08:51

Extending Font and Text to support multiple textures

6649b74

Fix formatting

5c0733e

Tidy up type declarations in font.hpp

b6091fa

Fix doxygen comment format

01c59c2

Updates for SFML3/c++17

375281f

Add some range based for loops

b5f74ab

Update texture validity check to cover all textures used by a font

599c9a5

Missing include

0cc4933

formatting

75bda4c

Review feedback

2ac5379

Review feedback

9384602

JonnyPtn force-pushed the test branch from 9383930 to 9384602 Compare November 28, 2022 08:53

JonnyPtn requested review from Bromeon and ChrisThrasher and removed request for kimci86 and Bromeon November 28, 2022 08:53

JonnyPtn requested review from Bromeon and ChrisThrasher and removed request for ChrisThrasher and Bromeon November 28, 2022 08:53

Emplace instead of push

ec47b4a

SFML deleted a comment from ChrisThrasher Dec 5, 2022

ChrisThrasher reviewed Dec 7, 2022

View reviewed changes

JonnyPtn and others added 6 commits December 8, 2022 09:05

Update include/SFML/Graphics/Font.hpp

ca00b8b

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>

Update src/SFML/Graphics/Font.cpp

a2302a4

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>

Smally tidy up

f320815

Review feedback

f1f5653

Correct variable name

15208a9

Formatting

7da2848

ChrisThrasher removed the s:accepted label Mar 20, 2023

binary1248 approved these changes Apr 3, 2023

View reviewed changes

eXpl0it3r removed this from the 3.0 milestone Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending Font and Text to support multiple textures #2282

Extending Font and Text to support multiple textures #2282

JonnyPtn commented Nov 22, 2022

codecov bot commented Nov 22, 2022 •

edited

JonnyPtn commented Nov 22, 2022

JonnyPtn commented Nov 23, 2022

Bromeon left a comment

Bromeon Nov 27, 2022 •

edited

ChrisThrasher Nov 28, 2022 •

edited

JonnyPtn Nov 28, 2022

Bromeon Nov 27, 2022 •

edited

JonnyPtn Nov 28, 2022

Bromeon Nov 27, 2022

JonnyPtn Nov 28, 2022

Bromeon commented Nov 28, 2022

ChrisThrasher left a comment

JonnyPtn commented Mar 10, 2023

Bromeon commented Apr 3, 2023

ChrisThrasher commented Apr 3, 2023

binary1248 commented Apr 3, 2023

Extending Font and Text to support multiple textures #2282

Are you sure you want to change the base?

Extending Font and Text to support multiple textures #2282

Conversation

JonnyPtn commented Nov 22, 2022

codecov bot commented Nov 22, 2022 • edited

Codecov Report

JonnyPtn commented Nov 22, 2022

JonnyPtn commented Nov 23, 2022

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Nov 27, 2022 • edited

Choose a reason for hiding this comment

ChrisThrasher Nov 28, 2022 • edited

Choose a reason for hiding this comment

JonnyPtn Nov 28, 2022

Choose a reason for hiding this comment

Bromeon Nov 27, 2022 • edited

Choose a reason for hiding this comment

JonnyPtn Nov 28, 2022

Choose a reason for hiding this comment

Bromeon Nov 27, 2022

Choose a reason for hiding this comment

JonnyPtn Nov 28, 2022

Choose a reason for hiding this comment

Bromeon commented Nov 28, 2022

ChrisThrasher left a comment

Choose a reason for hiding this comment

JonnyPtn commented Mar 10, 2023

Bromeon commented Apr 3, 2023

ChrisThrasher commented Apr 3, 2023

binary1248 commented Apr 3, 2023

codecov bot commented Nov 22, 2022 •

edited

Bromeon Nov 27, 2022 •

edited

ChrisThrasher Nov 28, 2022 •

edited

Bromeon Nov 27, 2022 •

edited