Add a font supporting more characters #9

tulior · 2017-04-12T22:26:02Z

Changed the font from Arial to Liberation Sans, which is licensed under the SIL Open Font License.

The tool fontbuilder was used to generate the following character map from the aforementioned font.

This is the whole of printable Extended ASCII.

Also, when matching for the character's id, the type has been changed to unsigned so that charcodes larger than 0x0F could be used.

JayFoxRox · 2017-04-12T23:36:52Z

Please have all information in the first post (= merge your second post into the first post and delete the second post).

Why isn't there a screenshot of the text in action?

One font is probably fine for now. It's more important that the exact details used for the fonts creation are documented (e.g. have the fontmap, list of fonts [possibly with download link, so preferably fonts which are easy to license], font size, texture size etc. in the wiki)

Also see andryblack/fontbuilder#19 and
andryblack/fontbuilder#4 . If those would exist one could just store a config file to generate these fonts more easily. This is beyond the scope of the PR though.

I haven't reviewed the code yet. I'll probably do so during the next couple of days. Please remind me if it doesn't happen :)
(Reviews by others appreciated! @anodium ?)

anodium

Why is Arial Black still in the codebase, if Liberation Mono does its job and more?

anodium · 2017-04-13T04:22:46Z

main.cpp

@@ -66,7 +66,8 @@ int main(int argc, char* argv[]) {

  std::shared_ptr<Texture> texture = std::make_shared<Texture>("data/font/main");

-  std::shared_ptr<Text> text = std::make_shared<Text>("data/font/arial_black_regular_65");
+  //std::shared_ptr<Text> text = std::make_shared<Text>("data/font/arial_black_regular_65");


Why was this commented out instead of edited directly?

anodium · 2017-04-13T04:24:04Z

text.cpp

@@ -91,7 +91,7 @@ class Text {
      }

      auto isMatchingSymbol = [&](const Symbol& symbol) -> bool {
-        return symbol.id == static_cast<uint32_t>(c);
+          return symbol.id == static_cast<unsigned char>(c);


What purpose does changing the type have?

Yes. Also if the type is unsigned here it should probably also be like that in the zengl struct (which is currently int32_t).
However, I don't know how unicode works in C++ and if zengl files even use unicode (which I kind of hope).

anodium · 2017-04-13T04:25:53Z

main.cpp

@@ -466,7 +467,17 @@ int main(int argc, char* argv[]) {

    // Test text
    glColor3f(0.1f, 0.2f, 0.5f);
-    text->draw(std::string("boats: ") + std::to_string(boats.size()) + std::string("\nsparks: ") + std::to_string(sparks.size()) + std::string("\nus/F: ") + std::to_string(uspf.count()));
+


I see ninja refactoring. 👀

If you're going to ninja @JayFoxRox, at least clean up your whitespace. 😉

JayFoxRox · 2017-04-13T20:28:20Z

text.cpp

@@ -91,7 +91,7 @@ class Text {
      }

      auto isMatchingSymbol = [&](const Symbol& symbol) -> bool {
-        return symbol.id == static_cast<uint32_t>(c);
+          return symbol.id == static_cast<unsigned char>(c);


Whitespace change not necessary

tulior · 2017-04-22T01:42:08Z

Pushed new commit. Also, the list of supported characters has been greatly reduced, since we are not doing Unicode. The new character set is shown below.

JayFoxRox · 2017-04-22T08:15:08Z

main.cpp

-        std::string("\n\xB5/F: ") + //'\xB5' being the micro symbol
-            std::to_string(uspf.count());
-    text->draw(debuggingText);
+    //'\xB5' = µ = micro sign (in case your editor's font can't handle it)


nit: Ideally this should have been in a variable: char symbolMicro = '\xB5'; or something.

JayFoxRox · 2017-04-22T08:15:57Z

text.cpp

-          return symbol.id == static_cast<unsigned char>(c);
+        /* c is probably char_t here, so ...
+          casting first to unsigned char is necessary, because if we
+          cast to int32_t directly, the machine bit extends, meaning


*sign extends

JayFoxRox · 2017-04-22T08:16:05Z

text.cpp

@@ -91,7 +91,16 @@ class Text {
      }

      auto isMatchingSymbol = [&](const Symbol& symbol) -> bool {
-          return symbol.id == static_cast<unsigned char>(c);
+        /* c is probably char_t here, so ...


JayFoxRox · 2017-04-22T08:16:56Z

text.cpp

+          that the match would fail because of the type's sign.
+          we could also do uint32_t, since I think zengl supports utf-32
+          but I don't think that's necessary because we are not support 
+          unicode anyway (i.e. we never go beyond 0xFF)


depending on what zengl uses we should fix the zengl struct and also this cast. I stole my zengl struct from the font tool source and changed some datatypes to stdint iirc.

JayFoxRox · 2017-04-22T08:17:58Z

Only some smaller issues left, I will test this later and hopefully we can integrate it in a couple of hours

tulior · 2017-04-23T16:44:00Z

Hopefully everything is OK now.
This is what the last version looks like.

JayFoxRox · 2017-04-23T17:00:02Z

text.cpp

@@ -91,7 +91,16 @@ class Text {
      }

      auto isMatchingSymbol = [&](const Symbol& symbol) -> bool {
-        return symbol.id == static_cast<uint32_t>(c);
+        /* c is char_t here, so ...


isn't it just a char ?

forgot about this one? @doTulio
It's a nit, so if you don't change it I'll just merge anyway in about 10 minutes or so.

JayFoxRox · 2017-04-23T17:00:23Z

text.cpp

+        /* c is char_t here, so ...
+          casting first to unsigned char is necessary, because if we
+          cast to int32_t directly, the machine sign extends, meaning
+          that the match would fail because of the type's sign.


JayFoxRox · 2017-04-23T17:01:18Z

text.cpp

+          unicode anyway (i.e. we never go beyond 0xFF)
+        */
+        unsigned char chUnsigned = static_cast<unsigned char>(c);
+        return symbol.id == static_cast<int32_t>(chUnsigned);


symbol_id is uint32_t, so this should be uint32_t too

JayFoxRox · 2017-04-23T17:01:49Z

text.cpp

+          casting first to unsigned char is necessary, because if we
+          cast to int32_t directly, the machine sign extends, meaning
+          that the match would fail because of the type's sign.
+          we could also do uint32_t, since I think zengl supports utf-32


We already do a 32 bit conversion here, so I'm not sure why you say "we could" - we already do. (also we can't - c is a char and we won't change that. So information has already been lost)

JayFoxRox · 2017-04-23T17:06:19Z

Okay, please fix what's mentioned in the remaining comments, squash up and make sure to add the character map to the wiki.

Reviewed and tested: LGTM aside from what's mentioned

- Deleted extra font files. We need only one. Later it can be decided whether we use a more appropriate, prettier, gamelier looking font. - Generated Sans Bold. Bold just looks better the way font rendering currently is done. - Improved casting and explanation in text.cpp.

JayFoxRox · 2017-04-23T19:26:40Z

Thanks!

(Please add the charmap and other settings you used + a link to the font (as ttf or w/e) to the wiki)

anodium reviewed Apr 13, 2017

View reviewed changes

JayFoxRox mentioned this pull request Apr 13, 2017

Add a font supporting more characters #8

Closed

JayFoxRox reviewed Apr 13, 2017

View reviewed changes

JayFoxRox mentioned this pull request Apr 21, 2017

Add hacky checkpoints #14

Merged

JayFoxRox reviewed Apr 22, 2017

View reviewed changes

JayFoxRox reviewed Apr 23, 2017

View reviewed changes

JayFoxRox merged commit 2a228ad into JayFoxRox:master Apr 23, 2017

tulior deleted the ExtraCharactersSupport branch April 23, 2017 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a font supporting more characters #9

Add a font supporting more characters #9

tulior commented Apr 12, 2017 •

edited

Loading

JayFoxRox commented Apr 12, 2017

anodium left a comment

anodium Apr 13, 2017

anodium Apr 13, 2017

JayFoxRox Apr 13, 2017

anodium Apr 13, 2017 •

edited

Loading

JayFoxRox Apr 13, 2017

tulior commented Apr 22, 2017 •

edited

Loading

JayFoxRox Apr 22, 2017

JayFoxRox Apr 22, 2017

JayFoxRox Apr 22, 2017

JayFoxRox Apr 22, 2017

JayFoxRox commented Apr 22, 2017

tulior commented Apr 23, 2017

JayFoxRox Apr 23, 2017

JayFoxRox Apr 23, 2017

JayFoxRox Apr 23, 2017

JayFoxRox Apr 23, 2017

JayFoxRox Apr 23, 2017 •

edited

Loading

JayFoxRox commented Apr 23, 2017

JayFoxRox commented Apr 23, 2017 •

edited

Loading

Add a font supporting more characters #9

Add a font supporting more characters #9

Conversation

tulior commented Apr 12, 2017 • edited Loading

JayFoxRox commented Apr 12, 2017

anodium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anodium Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tulior commented Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayFoxRox commented Apr 22, 2017

tulior commented Apr 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayFoxRox Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

JayFoxRox commented Apr 23, 2017

JayFoxRox commented Apr 23, 2017 • edited Loading

tulior commented Apr 12, 2017 •

edited

Loading

anodium Apr 13, 2017 •

edited

Loading

tulior commented Apr 22, 2017 •

edited

Loading

JayFoxRox Apr 23, 2017 •

edited

Loading

JayFoxRox commented Apr 23, 2017 •

edited

Loading