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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@orudge orudge merged commit de73c8f into OpenTTD:master May 14, 2019
@michicc michicc deleted the pr/gdi_font_render branch May 22, 2019 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text rendering error under CJK system locale
7 participants