Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GDI engine for font glyph rendering as a replacement for FreeType (take 2) #7572

Merged
merged 5 commits into from May 14, 2019

Conversation

@michicc
Copy link
Member

michicc commented May 4, 2019

Rebased PR #6980 which unfortunately can't be reopened. Might fix #7511.

Old description:

This PR supplies a native Windows GDI font rendering back-end. It drops FreeType as a required lib, OTOH it is one more code path to maintain while FreeType isn't really causing problems.

Building with FreeType is still possible and will take precedence over the GDI renderer, but
the project files don't include FreeType any more by default. Combining GDI rendering with ICU text layout is untested.

@michicc michicc force-pushed the michicc:pr/gdi_font_render branch from fcf50c5 to 2d4ba10 May 4, 2019
@YJSoft
Copy link

YJSoft commented May 4, 2019

Looks like loading font file from path not works at this build.

With ./fonts/NanumGothic.ttf (with or without font installed)

vmware_PXVEeQaeRJ

With NanumGothic (with font installed)

vmware_Y9qVgpGmlT

(I increased font size to see diff easily)

@glx22
Copy link
Contributor

glx22 commented May 4, 2019

But at least the text is correct this time, even if the font is not the expected one. That's already a progress.

@michicc
Copy link
Member Author

michicc commented May 5, 2019

Looks like loading font file from path not works at this build.

Try now?

@michicc michicc force-pushed the michicc:pr/gdi_font_render branch from 3dfae0e to 066bbf1 May 5, 2019
@michicc
Copy link
Member Author

michicc commented May 5, 2019

As the PR is now, I can principally get OpenTTD to load a NanumGothic.ttf without it being installed. However, if I select Korean as game language, it will not use the font for me because it is missing characters. As I just randomly googled for the font file, it is very likely I just have a bad file though.

@YJSoft
Copy link

YJSoft commented May 6, 2019

Tested it, works very well

okay

src/fontcache.cpp Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/gdi_font_render branch from 066bbf1 to eaaa754 May 6, 2019
@@ -41,8 +40,8 @@ the `static` versions, and OpenTTD currently needs the following dependencies:
To install both the x64 (64bit) and x86 (32bit) variants, you can use:

```ps
.\vcpkg install freetype:x64-windows-static liblzma:x64-windows-static libpng:x64-windows-static lzo:x64-windows-static zlib:x64-windows-static
.\vcpkg install freetype:x86-windows-static liblzma:x86-windows-static libpng:x86-windows-static lzo:x86-windows-static zlib:x86-windows-static

This comment has been minimized.

Copy link
@LordAro

LordAro May 9, 2019

Member

Do we want to update the CI to remove this as well?

This comment has been minimized.

Copy link
@nielsmh

nielsmh May 9, 2019

Contributor

Probably a good idea to check the CI can build without Freetype as well.

This comment has been minimized.

Copy link
@glx22

glx22 May 9, 2019

Contributor

CI should already build this PR without Freetype as it's removed from the project files, and I tried locally after removing Freetype via vcpkg and it builds fine. But mingw will still build with Freetype as the detection code is untouched.

This comment has been minimized.

Copy link
@michicc

michicc May 9, 2019

Author Member

Windows dependencies come in via OpenTTD/CompileFarm, can't change that in this PR.

… does not depend on Freetype and into one that does.

This makes it easier to add other TrueType font rendering engines.
@michicc michicc force-pushed the michicc:pr/gdi_font_render branch from 56aa7d2 to 86eb314 May 9, 2019
@michicc
Copy link
Member Author

michicc commented May 9, 2019

Added a change to config.lib to a899786, maybe @glx22 can have a look if it seems sane.

@glx22
Copy link
Contributor

glx22 commented May 9, 2019

Freetype properly skipped with MinGW.

Many warnings in fontcache.cpp with MinGW:

[SRC] Compiling fontcache.cpp
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp: In member function 'virtual const Sprite* Win32FontCache::InternalGetGlyph(GlyphID, bool)':
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:837:18: warning: operation on 'bmp' may be undefined [-Wsequence-point]
  byte *bmp = bmp = AllocaM(byte, size);

This one is fixable by removing one bmp =

D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp: In function 'void LoadWin32Font(FontSize)':
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfWidth' [-Wmissing-field-initializers]
  LOGFONT logfont = { 0 };
                        ^
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfEscapement' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfOrientation' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfWeight' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfItalic' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfUnderline' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfStrikeOut' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfCharSet' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfOutPrecision' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfClipPrecision' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfQuality' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfPitchAndFamily' [-Wmissing-field-initializers]
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:965:24: warning: missing initializer for member 'tagLOGFONTW::lfFaceName' [-Wmissing-field-initializers]

I have not tried to fix this one.

D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:972:41: warning: cast from type 'const void*' to type 'LPLOGFONT' {aka 'tagLOGFONTW*'} casts away qualifiers [-Wcast-qual]
   logfont = *(const LPLOGFONT)settings->os_handle;
                                         ^~~~~~~~~
D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:972:41: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers]

Maybe a different cast, but don't know which one yet.

D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:983:147: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PFNGETFONTRESOURCEINFO' {aka 'int (*)(const wchar_t*, long unsigned int*, void*, long unsigned int)'} [-Wcast-function-type]
    static PFNGETFONTRESOURCEINFO GetFontResourceInfo = (PFNGETFONTRESOURCEINFO)GetProcAddress(GetModuleHandle(_T("Gdi32")), "GetFontResourceInfoW");
                                                                                                                                                   ^

This one can be ignored, there are already similar warnings in win32.cpp and win32_v.cpp

D:/developpement/GitHub/glx22/OpenTTD/src/fontcache.cpp:1025:13: warning: format '%X' expects argument of type 'unsigned int', but argument 4 has type 'DWORD' {aka 'long unsigned int'} [-Wformat=]
   ShowInfoF("Unable to use '%s' for %s font, Win32 reported error 0x%X, using sprite font instead", settings->font, SIZE_TO_NAME[fs], GetLastError());
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                    ~~~~~~~~~~~~~~

Using %lX fixes it.

michicc added 4 commits Nov 25, 2018
… including FreeType.

Building with FreeType is still possible and will take precedence over the GDI renderer, but
the project files don't include FreeType anymore by default. Combining GDI rendering with ICU
text layout is untested.
… have one, instead of repeatedly guessing the font.
@michicc michicc force-pushed the michicc:pr/gdi_font_render branch from 86eb314 to 0dfc8ec May 9, 2019
@michicc
Copy link
Member Author

michicc commented May 9, 2019

I think I've addressed all the mentioned warnings.

@glx22
Copy link
Contributor

glx22 commented May 9, 2019

Yup all warnings fixed (except the function cast one, but it's not fixable I think)

@PeterN
Copy link
Member

PeterN commented May 13, 2019

I notice you add an #ifdef in uniscribe code for FreeType. Is it still possible to compile with FreeType on Windows? Is that desirable?

@michicc
Copy link
Member Author

michicc commented May 13, 2019

Yes, I didn't remove any FreeType stuff. After all, it is still needed for other platforms.

Whether it is desirable is debatable. Disabling it for Windows would complicate the #ifdef's because you'd have to explicitly exclude WIN32.

@orudge
Copy link
Contributor

orudge commented May 13, 2019

As an aside, I started work on a DirectWrite font renderer (based on this patch) with the intention that it could be used for an eventual UWP build, should we want to go down that route (it would primarily benefit ARM-based Windows 10 devices). FreeType is supported on UWP but GDI isn't, so retaining the possibility of FreeType support on Windows would have some benefit there. In general, I don't see that it would hurt to keep it anyway.

@PeterN
PeterN approved these changes May 14, 2019
@orudge orudge merged commit de73c8f into OpenTTD:master May 14, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190509.6 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@michicc michicc deleted the michicc:pr/gdi_font_render branch May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.