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

Clean up and fix freetype=false crashes #8641

Merged
merged 2 commits into from Aug 6, 2019

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented Jun 29, 2019

A IGUIFont of type bitmap/vector cannot be converted to CGUITTFont
Fixes various segfaults in gameplay
Shorter font cache code, cleaned up (?)

This was broken since colored text became a thing. Tracking down this bug took a horrible long time, but now it finally works.

Test 1: USE_FREETYPE=1, freetype = true
Compiled-in TTF support. Same functionality as in master.

  1. Modify font_path and/or mono_font_path to invalid paths
  2. Minetest loads the fallback path, or the setting default if everything else fails

Test 2 Bitmap: USE_FREETYPE=1, freetype = false
TTF support, but using images. Works in PR, does not in master.

  1. Get the font images from https://github.com/minetest/minetest/tree/25945dc5/fonts (removed in dceb9f7)
  2. Adjust font_path and mono_font_path
  3. Minetest starts, no text colors but it runs

Test 3 Vectors: USE_FREETYPE=1, freetype = false
TTF support, but using xml files. Works in PR, does not in master.

  1. font_path = fonts/mono_dejavu_sans.xml
  2. mono_font_path = fonts/mono_dejavu_sans.xml
  3. Minetest starts, no text colors but it runs

USE_FREETYPE=0
Requires many guiFormSpecMenu changes, thus it should be fixed in a later PR. There's yet no client option without freetype built-in.

@SmallJoker SmallJoker added the Bugfix 🐛 PRs that fix a bug label Jun 29, 2019
return;

if ((mode == FM_Simple) || (mode == FM_SimpleMono)) {
initSimpleFont(basesize, mode);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to getFont for a saner call structure.


std::string font_path = g_settings->get(font_config_prefix + "font_path");
std::string fallback_settings[] = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same functionality, just shorter code.

std::stringstream fontsize_plus_xml;
fontsize_plus_xml << basename << "_" << (size + offset) << ".xml";
// Find nearest matching font scale
// Does a "zig-zag motion" (positibe/negative), from 0 to MAX_FONT_SIZE_OFFSET
Copy link
Member Author

Choose a reason for hiding this comment

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

Also same functionality here. Positive offset it checked first, then goes from 0 to -MAX_FONT_SIZE_OFFSET and +MAX_FONT_SIZE_OFFSET

@@ -112,15 +112,6 @@ class FontEngine
/** current font engine mode */
FontMode m_currentMode = FM_Standard;

/** font mode of last request */
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced by replacing several m_font_cache[mode] lookups in getFont(). Roughly the same performance, I guess.

@SmallJoker SmallJoker added the WIP The PR is still being worked on by its author and not ready yet. label Jun 29, 2019
A IGUIFont of type bitmap/vector cannot be converted to CGUITTFont
Fixes various segfaults in gameplay
Shorter font cache code, cleaned up (?)
@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 30, 2019

Forgive my incompetence. Any usage scenario? MT is built with freetype support, but the user turns off freetype. What for? This should not work in principle.

@SmallJoker
Copy link
Member Author

SmallJoker commented Jun 30, 2019

@MoNTE48 Using bitmaps to render text gives you a few FPS more on old (real old) hardware. This PR is just one of the performance tweaks I wrote to make the game playable even on my worst rig.

PS: One other part was to disable the stars in the sky, it took more than 10ms to draw.

@SmallJoker SmallJoker removed the WIP The PR is still being worked on by its author and not ready yet. label Jun 30, 2019
@SmallJoker
Copy link
Member Author

SmallJoker commented Jun 30, 2019

Comparison: Freetype enabled on the right side.
GPU: Quadro FX 4600 (nouveau)
CPU: Core 2 Quad Q6700
Full render size: 1280x978 (image cut)

Statistics

The time spent for drawing is roughly proportional to the amount of characters drawn on screen.
DejaVuSans_15pt.png: 13ms (left side)

Arimo-Regular.ttf: 61ms (right side)
DroidSansFallbackFull.ttf: 55ms
Retron2000.ttf: 45ms

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 30, 2019

Wow, I didn’t know that Freetype reduced performance so much. Try this font, please. Just wondering
https://github.com/MultiCraftProject/MultiCraft/blob/DEV/fonts/Retron2000.ttf

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 30, 2019

Relative gains from using bitmaps is minimal with small font. By the way, why does MT use DroidSansFallbackFull.ttf? There is DroidSansFallback.ttf, which is less than 1 MB, but has no differences, as far as I could find in Google.
[offtop] Also MT could use the system font. Many fonts in Windows, Linux, Android are required. for example, MultiCraft uses the Android system font, which reduced the APK by 5 MB compared to Minetest. Never heard of any problems. [/offtop]

@SmallJoker
Copy link
Member Author

@MoNTE48 I updated the older post with some comparisons I made. What matters is not the file size, but the amount of characters drawn on screen.

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 30, 2019

@SmallJoker I understand, thank you.
The second part of my previous post was not about performance, but about file size. Why increase the size of the game if can not do this? Even if it does not affect performance. If I am not mistaken, MultiCraft for Android, along with analytics, advertising and other additional libs, took up less space than Minetest without all this.

@SmallJoker SmallJoker added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Jul 13, 2019
src/client/fontengine.cpp Outdated Show resolved Hide resolved
@SmallJoker SmallJoker merged commit 233cb86 into minetest:master Aug 6, 2019
@SmallJoker SmallJoker deleted the fix_no_freetype branch August 17, 2019 11:32
@@ -211,20 +176,17 @@ unsigned int FontEngine::getDefaultFontSize()
/******************************************************************************/
void FontEngine::readSettings()
{
#if USE_FREETYPE

Choose a reason for hiding this comment

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

@SmallJoker, by removing this #ifdef you broke possibility to build minetest without freetype and gettext:

Right now I'm trying to build the very minimal configuration and I've set ENABLE_FREETYPE=OFF and ENABLE_GETTEXT=OFF and I see the following error:

minetest\src\client\fontengine.cpp(184): error C3861: 'gettext': identifier not found

Copy link
Member Author

Choose a reason for hiding this comment

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

Please explain further. This preprocessor checks against USE_FREETYPE, not USE_GETTEXT.
There's a dummy function available for gettext() if gettext is not available:

#define gettext(String) String

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Thanks a lot, this fixes the build!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants