Skip to content
This repository has been archived by the owner on Apr 17, 2022. It is now read-only.

Replace quesoglc with harfbuzz #4507

Closed
wzdev-ci opened this issue Sep 15, 2016 · 18 comments
Closed

Replace quesoglc with harfbuzz #4507

wzdev-ci opened this issue Sep 15, 2016 · 18 comments

Comments

@wzdev-ci
Copy link
Contributor

resolution_fixed type_patch (an actual patch, not a request for one) | by Vincent


This patch replaces quesoglc with direct harfbuzz + freetype usage.
In addition to sharper text the addition of harfbuzz means that some scripts like arabic
or hindic can be properly supported since harfbuzz provides ligature substitution.

The rendering uses a stripped down iv_drawImage function so that it will work with any
graphic API that implements iv_drawImage.

I'm developping with Visual Studio 2015, I updated configure.ac and tested that it works
on bash for Ubuntu but I think some testing on Linux and Mac are welcomed.


Issue migrated from trac:4507 at 2022-04-16 12:36:57 -0700

@wzdev-ci
Copy link
Contributor Author

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

@wzdev-ci
Copy link
Contributor Author

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

@wzdev-ci
Copy link
Contributor Author

Vincent commented


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

@wzdev-ci
Copy link
Contributor Author

Cyp changed blocking which not transferred by tractive

@wzdev-ci
Copy link
Contributor Author

Cyp changed blockedby which not transferred by tractive

@wzdev-ci
Copy link
Contributor Author

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

@wzdev-ci
Copy link
Contributor Author

Per uploaded file fixfont.diff (1.4 KiB)

This fixes the patch to work on my system

@wzdev-ci
Copy link
Contributor Author

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.

@wzdev-ci
Copy link
Contributor Author

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

Remove assert() and change font path

@wzdev-ci
Copy link
Contributor Author

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.

@wzdev-ci
Copy link
Contributor Author

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

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 2, 2016

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.

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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.

1 similar comment
@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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.

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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

@wzdev-ci
Copy link
Contributor Author

wzdev-ci commented Oct 4, 2016

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

@wzdev-ci wzdev-ci closed this as completed Oct 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant