Vincent uploaded file 0001-Use-harfbuzz.patch (36.4 KiB)

Vincent uploaded file 0001-Use-harfbuzz.2.patch (35.6 KiB)

Vincent commented

I sightly updated the patch to remove some default argument that were not useful.

Cyp changed blocking which not transferred by tractive

Cyp changed blockedby which not transferred by tractive

Cyp commented

Some initial testing of 5bc1b2cec399359465fbef25b22c4678645b6c1a on Linux, it doesn't compile unless I do:

diff --git a/lib/ivis_opengl/textdraw.cpp b/lib/ivis_opengl/textdraw.cpp
index d3c19150..365031c 100644
--- a/lib/ivis_opengl/textdraw.cpp
+++ b/lib/ivis_opengl/textdraw.cpp
@@ -46,7 +46,7 @@ static float font_colour[4] = {1.f, 1.f, 1.f, 1.f};
 
 #include "hb.h"
 #include "hb-ft.h"
-#include "ftglyph.h"
+#include <freetype/ftglyph.h>
 #include <unordered_map>
 #include <memory>
 
@@ -372,7 +372,7 @@ TextShaper &getShaper()
 #ifdef WZ_OS_WIN32
 #define FONT_PATH std::string(PHYSFS_getBaseDir()) + "fonts\\"
 #else
-#define FONT_PATH /usr/share/fonts/truetype/dejavu/
+#define FONT_PATH std::string("/usr/share/fonts/truetype/dejavu/")
 #endif
 
 FTFace &getFTFace(iV_fonts FontID)

It then crashes in the following assertion:

assert(!FT_New_Face(lib, fileName.c_str(), 0, &m_face));

Note that if FT_New_Face (or any other function) has any important side effects, it shouldn't be in an assert(), since it might get removed when compiling a non-debug build. (I think it's ok in ASSERT or ASSERT_OR_RETURN macros, though.)

Per uploaded file fixfont.diff (1.4 KiB)

This fixes the patch to work on my system

Per commented

The problem was, if that was not clear, that the path is incorrect, or at least not guaranteed to be correct. I think relying on system font paths is dubious, and we should be loading fonts from our data directory, or font files installed along side the executable, instead.

Vincent uploaded file 0001-Use-harfbuzz.3.patch (36.2 KiB)

Remove assert() and change font path

Vincent commented

I applied Per's change to my patch.

About the font path : what is the "official" data directory in a linux install ? I used the font path existing in bash for Ubuntu (which is the one on Ubuntu). Since WZ2100 use a single font (DejaVuSans and the Bold variation) which is shipped as part of the game it makes sense to use a local path ; on the other hand I fear that "make install" script may break font path or that distro may want to use the system DejaVu ttf file.

Vincent commented

I have an issue with +#include <freetype/ftglyph.h>, it seems distro dépendent.
On Ubuntu #include "ftglyph.h" works, but it's stored in "/usr/include/freetype2/ftglyph.h".
On the other hand on Fedora "#include <freetype/ftglyph.h> works".

Per commented

I like your new font rendering a great deal. Getting rid of QuesoGLC is huge, and the new system looks good.

I don't think we should load fonts from the system directories. Instead, now that we have greater control over the font system, we should treat fonts like any other data resource, and put them in our .wz files. I've implemented this (but when reading from our data system, you cannot do this through static variable initialization - which I think in general is a bad idea).

I've made some modifications to your patch, and pushed it to my repo. See
perim/warzone2100@8a224a6

If you think those changes are okay, then I'll push it to master.

(I'm not sure if I should merge the two commits, or not. And if I should merge, how. That is, I don't know what is the proper git-iquette for such things.)

What I would like to ask going forward, though, is that please try to write more straightforward and simple code. I know it is fun to write code as cleverly and optimized as possible, but that kind of code is a lot harder for other people to review, fix bugs in, and improve on later. Keep in mind that not everyone in the world is a c++11 wizard.

Vincent commented

I think you can push it to master. I'm glad font can be used as a resource like any others, it should make packaging more consistent between platforms.

Per Inge Mathisen <per.mathisen@...> changed status from new to closed

Per Inge Mathisen <per.mathisen@...> changed owner from `` to Per Inge Mathisen <per.mathisen@gmail.com>

Per Inge Mathisen <per.mathisen@...> changed resolution from `` to fixed

Per Inge Mathisen <per.mathisen@...> commented

In Warzone2100/warzone2100@cb02fcf:

#CommitTicketReference repository="" revision="cb02fcf7350161945128246e9abe2ab6bd825fab"
Use harfbuzz for font rendering, rather than QuesoGLC.

Changes from previous commit:
 * Do not use anonymous namespaces, unless absolutely necessary. They clutter
   the code and breaks existing code style.
 * Load fonts from base.wz, rather than try to find them in system folders.
   This is safer and bring fonts in line with other types of data that we use.
 * Remove unused function.
 * Remove QuesoGLC bug workaround in the code, measuring '-' rather than space.
 * Remove QuesoGLC from the linux build system. And from the repository.

Closes #4507