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

Implement proper font handling #1561

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@sapier
Copy link
Contributor

commented Aug 17, 2014

Adds on the fly font size changes
Adds interface for loading different fonts (e.g. Mono font in chat console)
Adds support for different font sizes

This is not supposed to result in absolutely identical menu as it's removing the current automatic gui scaling and replaces it by evaluation of parameter screen_dpi.
That parameter as well as font size setting are supposed to be added to GUI settings subtab to be implemented soon.

@sapier sapier force-pushed the minetest:master branch 2 times, most recently to c24e075 Aug 19, 2014

@sapier sapier force-pushed the sapier:font_engine branch 3 times, most recently Aug 19, 2014

@kahrl

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2014

I think this is good, but some comments:

  • A global variable shouldn't be called "fe"
  • getTextHeight()/getLineHeight() should cache the result if at all possible, or at least use a shorter unimportant string for performance reasons
  • I don't like the settings callback mechanism very much as using it is only possible with global state, as exemplified in font_setting_changed()
  • Using XML files that are 10 times as big as the actual font seems stupid to me.
@@ -176,10 +176,23 @@ void set_default_settings(Settings *settings)

settings->setDefault("fallback_font_shadow", "1");
settings->setDefault("fallback_font_shadow_alpha", "128");

std::stringstream fontsize;
fontsize << TTF_DEFAULT_FONT_SIZE;

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 17, 2014

Member

Can you use TOSTRING() here?

This comment has been minimized.

Copy link
@sapier

sapier Nov 23, 2014

Author Contributor

Fixed in next version
Edit1: For some reason TOSTRING(TTF_DEFAULT_FONT_SIZE) isn't same as fontsize.c_str(), any ideas why?

@ShadowNinja

View changes

src/fontengine.cpp Outdated
}

m_settings = main_settings;
m_env = env;

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 17, 2014

Member

You use an initializer list to set things like m_settings and m_env to NULL, and then set them to the arguments later. Why?

This comment has been minimized.

Copy link
@sapier

sapier Oct 10, 2014

Author Contributor

I'll check if those initializations can be done in the initializer list too.
Fixed in next version, all done by initializer list.

@ShadowNinja

View changes

src/settings.h Outdated
(*iter)();
}
}

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 17, 2014

Member
  1. This should use const references for strings.
  2. There shouldn't be extra empty lines after ifs.
  3. I think it would be useful to pass the setting name (as a const reference), so that one function can handle multiple callbacks.

This comment has been minimized.

Copy link
@sapier

sapier Nov 23, 2014

Author Contributor
  1. Fixed in nest version
  2. Fixed
  3. I don't see a usecase for this but as it's a minor change I'm gonna implement it that way anyway
@ShadowNinja

View changes

src/fontengine.h Outdated
SimpleMono,
MaxMode,
Unspecified
};

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Sep 17, 2014

Member

Enum's populate their containing namespace. These shouldn't be global. Prefix them with at least something like FM_.

This comment has been minimized.

Copy link
@sapier

sapier Nov 23, 2014

Author Contributor

Changed in next version

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

I don't like how this adds so many different font texture files. Can you load a big one and scale it down instead?

@ShadowNinja ShadowNinja force-pushed the minetest:master branch 4 times, most recently to 56195dc Sep 20, 2014

@sfan5 sfan5 added the Rebase needed label Oct 6, 2014

@ShadowNinja ShadowNinja force-pushed the minetest:master branch from cba038e to b98e8d6 Oct 7, 2014

@sapier

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2014

@kahrl those files contain >10 times more fonts so they're mot really bigger

@ShadowNinja Sorry there's no way to scale a font file in irrlicht, it's simply not supported. The only way would be merging (full) irrlicht font tool to minetest which imho would be quite insane.

I'll fix your comments and create a new pull containing zeframs formspec scaling fixes too.

@sapier sapier force-pushed the sapier:font_engine branch 2 times, most recently Nov 23, 2014

@sapier sapier removed the Rebase needed label Nov 23, 2014

@sapier sapier force-pushed the sapier:font_engine branch Nov 23, 2014

@Zeno- Zeno- force-pushed the minetest:master branch to a1db83e Nov 25, 2014

@Megaf

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2014

👍
Just tested here, fonts look much nicer now and they scale correctly. No regressions noticed.

@sapier sapier force-pushed the sapier:font_engine branch Nov 30, 2014

Zefram and others added some commits Aug 20, 2014

Zefram sapier
Scale form elements consistently
The ratios between the sizes of form elements, including text, is now
fixed, aside from variations caused by rounding.  This makes form layout
almost fully predictable, and particularly independent of player's
screen size.  The proportions of non-text elements are the traditional
proportions.

For compatibility, the way in which element positions and sizes are
specified remains unchanged, in all its baroqueness, with one exception.
The exception is that the position of a label[] element is now defined
in terms of the vertically center of the first line of the label,
rather than the bottom of the first line of the label.  This change
allows a label to be precisely aligned with button text or an edit box,
which are positioned in a centering manner.  Label positioning remains
consistent with the previous system, just more precisely defined.

Make multi-line label[] elements work properly.  Previously the code set
a bounding rectangle assuming that there would be only a single line,
and as a result a multi-line label would be cut somewhere in the middle
of the second line.  Now multi-line labels not only work, but have
guaranteed line spacing relative to inventory slots, to aid alignment.

Incidentally fix tabheader[] elements which were being constrained to
the wrong width.

Given an unusually large form, in variable-size mode, the form rendering
system now chooses a scale that will fit the entire form on the screen,
if that doesn't make elements too small.  Fixed-size forms, including the
main menu, are have their sizes fixed in inch terms.  The fixed size for
fixed-size forms and the preferred and minimum sizes for variable-size
forms all scale according to the gui_scaling parameter.
sapier sapier
Make hud use fontengine too
Fix non coding style conforming glb_fontengine to g_fontengine
Fix fonts never been deleted due to grabbed to often

@sapier sapier force-pushed the sapier:font_engine branch to 8921352 Nov 30, 2014

@@ -215,6 +215,8 @@
#directional_colored_fog = true
#tooltip_show_delay = 400
# Delay showing tooltips, in miliseconds
#screen_dpi = 72
# adjust dpi configuration to your screen (PC/MAC only) e.g. for 4k screens

This comment has been minimized.

Copy link
@sfan5

sfan5 Nov 30, 2014

Member
  1. A Mac is a PC.
  2. MAC is not an acronym. (at least not the Mac you mean)
  3. Why is this computer only?

This comment has been minimized.

Copy link
@sapier

sapier Nov 30, 2014

Author Contributor

Android does provide valid dpi information on it's own so we don't need the setting there. Maybe some time in future we will have that information on Desktop to then it's not required any longer.

@sapier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2014

@sapier sapier closed this Dec 1, 2014

@SmallJoker

This comment has been minimized.

Copy link
Member

commented on src/fontengine.cpp in dceb9f7 Dec 6, 2014

How about '.gif', '.jpg', '.JPG' or '.TTF'?
Please make those extentions case insensitive.
Would fix #1905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.