Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Font fixes for various things #1703

Merged
9 commits merged into from

4 participants

jmarshallnz ronie Black09 HitcherUK
jmarshallnz
Owner

The primary intention of this is to fix #11015, an issue where a font is just the right size that it defies our (lack of) checking for texture bounds, leading to DirectX not doing the transfer of a letter to the texture. GL would have had issues as well, involving writing to memory that (potentially) doesn't exist.

The secondary intention is to fix #10549, where we don't align outlined fonts correctly, thus causing the border texture to be incorrect for some characters. In fixing this, we must align the baselines between fonts, which essentially means a slight change in vertical alignment for all labels that don't align centrally. From a quick look at a couple of skins I didn't notice anything out of place, but skinners should look more careful (basically any label control that isn't centered vertically).

EDIT: Also fixes #11855.

Lastly, there's various cleanups to make the code (hopefully) clearer to others.

Jonathan Mar... added some commits
Jonathan Marshall [fonts] make m_cellHeight actually reference the cell height, and hav…
…e the padded version be generated by a function
dbd786f
Jonathan Marshall [fonts] remove unused, local m_cellWidth variable d3b11cb
Jonathan Marshall [fonts] protect against accessing memory or textures outside of their…
… bounds - fixes #11015
3e3cad7
Jonathan Marshall [fonts] no need to clamp ch->offsetY at 0 now that the real culprit i…
…s found
1e2d14e
Jonathan Marshall [fonts] remove commented logging statement 590498e
Jonathan Marshall [fonts] always offset by the border size, so that we get correct alig…
…nment of font and bordered font in the cases where the border has a hight ascender. Closes #10549
6f229e9
Jonathan Marshall [fonts] rewrite the computations for m_cellHeight and m_cellBaseLine …
…so that they ensure no truncation takes place. Fixes #11855.
81e4c67
Jonathan Marshall [fonts] get rid of unused constant of 72/72 in font sizing calculations 158267e
Jonathan Marshall [fonts] make the space between characters in the texture a static con…
…stant and reduce the gap vertically now that the truncation issues have been fixed
a55c567
ronie
Collaborator

thx!

i can confirm this fixes issue #11015

Deleted user

i can't really comment much on the implementation, but if you put your balls on the table..

Deleted user ghost merged commit f849887 into from
Black09

Any chance that this could be post-Frodo? Otherwise I would have to fix the vertical alignment in almost every label in my skin. A proper fix for correct center alignment for every font would also be ok but I guess that's tricky.

jmarshallnz
Owner

They're already merged, as this the alignment one.

Black09

All my (vertically) centered labels have a different alignment now, 1 or 2px different as before this fix. Is that intended? Because it is a lot of work to fix everything now.

jmarshallnz
Owner

No vertically centered labels should have changed by more than a single pixel (due to rounding). It is intended as before the font height was incorrectly rounded to smaller than it should be.

Black09

Ok, I made another test and it seems fine now with your fix... I corrected the valign of my font and everything is perfect now, thanks.

HitcherUK

@jmarshallnz As well as making all my centrally aligned labels 1 pixel off it's really messed up my textboxes (anything from 6 to 30 pixels off depending on font size).

Black09

jmarshall is correct that the font height was calculated smaller than it's real height before so the font was aligned 1px wrong in most cases, I did some testing yesterday. If it's now wrongly aligned, the font is the problem.

HitcherUK

On further inspection all the textbox fonts are using <linespacing>...</linespacing>

Could this be the issue?

jmarshallnz
Owner

The lineheight wasn't altered at all. @HitcherUK mind posting in the forums what the issue is with which skin so I can take a look?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 31, 2012
  1. [fonts] make m_cellHeight actually reference the cell height, and hav…

    Jonathan Marshall authored
    …e the padded version be generated by a function
  2. [fonts] remove unused, local m_cellWidth variable

    Jonathan Marshall authored
  3. [fonts] protect against accessing memory or textures outside of their…

    Jonathan Marshall authored
    … bounds - fixes #11015
  4. [fonts] no need to clamp ch->offsetY at 0 now that the real culprit i…

    Jonathan Marshall authored
    …s found
  5. [fonts] remove commented logging statement

    Jonathan Marshall authored
  6. [fonts] always offset by the border size, so that we get correct alig…

    Jonathan Marshall authored
    …nment of font and bordered font in the cases where the border has a hight ascender. Closes #10549
  7. [fonts] rewrite the computations for m_cellHeight and m_cellBaseLine …

    Jonathan Marshall authored
    …so that they ensure no truncation takes place. Fixes #11855.
  8. [fonts] make the space between characters in the texture a static con…

    Jonathan Marshall authored
    …stant and reduce the gap vertically now that the truncation issues have been fixed
This page is out of date. Refresh to see the latest.
106 xbmc/guilib/GUIFontTTF.cpp
View
@@ -84,7 +84,7 @@ class CFreeTypeLibrary
if (FT_New_Face( m_library, CSpecialProtocol::TranslatePath(filename).c_str(), 0, &face ))
return NULL;
- unsigned int ydpi = GetDPI();
+ unsigned int ydpi = 72; // 72 points to the inch is the freetype default
unsigned int xdpi = (unsigned int)MathUtils::round_int(ydpi * aspect);
// we set our screen res currently to 96dpi in both directions (windows default)
@@ -124,11 +124,6 @@ class CFreeTypeLibrary
FT_Stroker_Done(stroker);
}
- unsigned int GetDPI() const
- {
- return 72; // default dpi, matches what XPR fonts used to use for sizing
- };
-
private:
FT_Library m_library;
};
@@ -196,7 +191,7 @@ void CGUIFontTTFBase::ClearCharacterCache()
m_maxChars = CHAR_CHUNK;
// set the posX and posY so that our texture will be created on first character write.
m_posX = m_textureWidth;
- m_posY = -(int)m_cellHeight;
+ m_posY = -(int)GetTextureLineHeight();
m_textureHeight = 0;
}
@@ -234,44 +229,53 @@ bool CGUIFontTTFBase::Load(const CStdString& strFilename, float height, float as
if (!m_face)
return false;
- // grab the maximum cell height and width
- unsigned int m_cellWidth = m_face->bbox.xMax - m_face->bbox.xMin;
- m_cellHeight = std::max<unsigned int>(m_face->bbox.yMax - m_face->bbox.yMin, m_face->ascender - m_face->descender);
- m_cellBaseLine = std::max<unsigned int>(m_face->bbox.yMax, m_face->ascender);
+ /*
+ the values used are described below
+
+ XBMC coords Freetype coords
+
+ 0 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ bbox.yMax, ascender
+ A \
+ A A |
+ A A |
+ AAAAA pppp cellAscender
+ A A p p |
+ A A p p |
+ m_cellBaseLine _ _A_ _A_ pppp_ _ _ _ _/_ _ _ _ _ 0, base line.
+ p \
+ p cellDescender
+ m_cellHeight _ _ _ _ _ p _ _ _ _ _ _/_ _ _ _ _ bbox.yMin, descender
+
+ */
+ int cellDescender = std::min<int>(m_face->bbox.yMin, m_face->descender);
+ int cellAscender = std::max<int>(m_face->bbox.yMax, m_face->ascender);
+
+ /*
+ add on the strength of any border - we do this in non-bordered cases
+ as bordered fonts are done by first rendering the border and then the
+ main font - thus m_cellBaseLine needs to align in both cases
+ */
+ FT_Pos strength = FT_MulFix( m_face->units_per_EM, m_face->size->metrics.y_scale) / 12;
+ if (strength < 128)
+ strength = 128;
+
+ cellDescender -= strength;
+ cellAscender += strength;
if (border)
{
m_stroker = g_freeTypeLibrary.GetStroker();
-
- FT_Pos strength = FT_MulFix( m_face->units_per_EM, m_face->size->metrics.y_scale) / 12;
- if (strength < 128)
- strength = 128;
- m_cellHeight += 2*strength;
-
if (m_stroker)
FT_Stroker_Set(m_stroker, strength, FT_STROKER_LINECAP_ROUND, FT_STROKER_LINEJOIN_ROUND, 0);
}
+ // scale to pixel sizing, rounding so that maximal extent is obtained
+ float scaler = height / m_face->units_per_EM;
+ cellDescender = MathUtils::round_int(cellDescender * scaler - 0.5f); // round down
+ cellAscender = MathUtils::round_int(cellAscender * scaler + 0.5f); // round up
- unsigned int ydpi = g_freeTypeLibrary.GetDPI();
- unsigned int xdpi = (unsigned int)MathUtils::round_int(ydpi * aspect);
-
- m_cellWidth *= (unsigned int)(height * xdpi);
- m_cellWidth /= (72 * m_face->units_per_EM);
-
- m_cellHeight *= (unsigned int)(height * ydpi);
- m_cellHeight /= (72 * m_face->units_per_EM);
-
- m_cellBaseLine *= (unsigned int)(height * ydpi);
- m_cellBaseLine /= (72 * m_face->units_per_EM);
-
- // increment for good measure to give space in our texture
- m_cellWidth++;
- m_cellHeight+=2;
- m_cellBaseLine++;
-
-// CLog::Log(LOGDEBUG, "%s Scaled size of font %s (%f): width = %i, height = %i, lineheight = %li",
-// __FUNCTION__, strFilename.c_str(), height, m_cellWidth, m_cellHeight, m_face->size->metrics.height / 64);
+ m_cellBaseLine = cellAscender;
+ m_cellHeight = cellAscender - cellDescender;
m_height = height;
@@ -295,7 +299,7 @@ bool CGUIFontTTFBase::Load(const CStdString& strFilename, float height, float as
// set the posX and posY so that our texture will be created on first character write.
m_posX = m_textureWidth;
- m_posY = -(int)m_cellHeight;
+ m_posY = -(int)GetTextureLineHeight();
// cache the ellipses width
Character *ellipse = GetCharacter(L'.');
@@ -326,7 +330,7 @@ void CGUIFontTTFBase::DrawTextInternal(float x, float y, const vecColors &colors
// calculate sizing information
float startX = 0;
- float startY = (alignment & XBFONT_CENTER_Y) ? -0.5f*(m_cellHeight-2) : 0; // vertical centering
+ float startY = (alignment & XBFONT_CENTER_Y) ? -0.5f*m_cellHeight : 0; // vertical centering
if ( alignment & (XBFONT_RIGHT | XBFONT_CENTER_X) )
{
@@ -433,7 +437,7 @@ float CGUIFontTTFBase::GetCharWidthInternal(character_t ch)
float CGUIFontTTFBase::GetTextHeight(float lineSpacing, int numLines) const
{
- return (float)(numLines - 1) * GetLineHeight(lineSpacing) + (m_cellHeight - 2); // -2 as we increment this for space in our texture
+ return (float)(numLines - 1) * GetLineHeight(lineSpacing) + m_cellHeight;
}
float CGUIFontTTFBase::GetLineHeight(float lineSpacing) const
@@ -443,6 +447,13 @@ float CGUIFontTTFBase::GetLineHeight(float lineSpacing) const
return 0.0f;
}
+unsigned int CGUIFontTTFBase::spacing_between_characters_in_texture = 1;
+
+unsigned int CGUIFontTTFBase::GetTextureLineHeight() const
+{
+ return m_cellHeight + spacing_between_characters_in_texture;
+}
+
CGUIFontTTFBase::Character* CGUIFontTTFBase::GetCharacter(character_t chr)
{
wchar_t letter = (wchar_t)(chr & 0xffff);
@@ -570,14 +581,14 @@ bool CGUIFontTTFBase::CacheCharacter(wchar_t letter, uint32_t style, Character *
if (m_posX + bitGlyph->left + bitmap.width > (int)m_textureWidth)
{ // no space - gotta drop to the next line (which means creating a new texture and copying it across)
m_posX = 0;
- m_posY += m_cellHeight;
+ m_posY += GetTextureLineHeight();
if (bitGlyph->left < 0)
m_posX += -bitGlyph->left;
- if(m_posY + m_cellHeight >= m_textureHeight)
+ if(m_posY + GetTextureLineHeight() >= m_textureHeight)
{
// create the new larger texture
- unsigned int newHeight = m_posY + m_cellHeight;
+ unsigned int newHeight = m_posY + GetTextureLineHeight();
// check for max height
if (newHeight > g_Windowing.GetMaxTextureSize())
{
@@ -607,7 +618,7 @@ bool CGUIFontTTFBase::CacheCharacter(wchar_t letter, uint32_t style, Character *
// set the character in our table
ch->letterAndStyle = (style << 16) | letter;
ch->offsetX = (short)bitGlyph->left;
- ch->offsetY = (short)max((short)m_cellBaseLine - bitGlyph->top, 0);
+ ch->offsetY = (short)m_cellBaseLine - bitGlyph->top;
ch->left = (float)m_posX + ch->offsetX;
ch->top = (float)m_posY + ch->offsetY;
ch->right = ch->left + bitmap.width;
@@ -617,9 +628,14 @@ bool CGUIFontTTFBase::CacheCharacter(wchar_t letter, uint32_t style, Character *
// we need only render if we actually have some pixels
if (bitmap.width * bitmap.rows)
{
- CopyCharToTexture(bitGlyph, ch);
+ // ensure our rect will stay inside the texture (it *should* but we need to be certain)
+ unsigned int x1 = max(m_posX + ch->offsetX, 0);
+ unsigned int y1 = max(m_posY + ch->offsetY, 0);
+ unsigned int x2 = min(x1 + bitmap.width, m_textureWidth);
+ unsigned int y2 = min(y1 + bitmap.rows, m_textureHeight);
+ CopyCharToTexture(bitGlyph, x1, y1, x2, y2);
}
- m_posX += 1 + (unsigned short)max(ch->right - ch->left + ch->offsetX, ch->advance);
+ m_posX += spacing_between_characters_in_texture + (unsigned short)max(ch->right - ch->left + ch->offsetX, ch->advance);
m_numChars++;
m_textureScaleX = 1.0f / m_textureWidth;
8 xbmc/guilib/GUIFontTTF.h
View
@@ -112,7 +112,7 @@ class CGUIFontTTFBase
void ClearCharacterCache();
virtual CBaseTexture* ReallocTexture(unsigned int& newHeight) = 0;
- virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character *ch) = 0;
+ virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, unsigned int x1, unsigned int y1, unsigned int x2, unsigned int y2) = 0;
virtual void DeleteHardwareTexture() = 0;
// modifying glyphs
@@ -126,6 +126,12 @@ class CGUIFontTTFBase
int m_posX; // current position in the texture
int m_posY;
+ /*! \brief the height of each line in the texture.
+ Accounts for spacing between lines to avoid characters overlapping.
+ */
+ unsigned int GetTextureLineHeight() const;
+ static unsigned int spacing_between_characters_in_texture;
+
color_t m_color;
Character *m_char; // our characters
10 xbmc/guilib/GUIFontTTFDX.cpp
View
@@ -259,7 +259,7 @@ CBaseTexture* CGUIFontTTFDX::ReallocTexture(unsigned int& newHeight)
return pNewTexture;
}
-bool CGUIFontTTFDX::CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character* ch)
+bool CGUIFontTTFDX::CopyCharToTexture(FT_BitmapGlyph bitGlyph, unsigned int x1, unsigned int y1, unsigned int x2, unsigned int y2)
{
FT_Bitmap bitmap = bitGlyph->bitmap;
@@ -271,12 +271,8 @@ bool CGUIFontTTFDX::CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character* ch)
texture->GetSurfaceLevel(0, &target);
RECT sourcerect = { 0, 0, bitmap.width, bitmap.rows };
- RECT targetrect;
- targetrect.top = m_posY + ch->offsetY;
- targetrect.left = m_posX + bitGlyph->left;
- targetrect.bottom = targetrect.top + bitmap.rows;
- targetrect.right = targetrect.left + bitmap.width;
-
+ RECT targetrect = { x1, y1, x2, y2 };
+
HRESULT hr = D3DXLoadSurfaceFromMemory( target, NULL, &targetrect,
bitmap.buffer, D3DFMT_LIN_A8, bitmap.pitch, NULL, &sourcerect,
D3DX_FILTER_NONE, 0x00000000);
2  xbmc/guilib/GUIFontTTFDX.h
View
@@ -46,7 +46,7 @@ class CGUIFontTTFDX : public CGUIFontTTFBase
protected:
virtual CBaseTexture* ReallocTexture(unsigned int& newHeight);
- virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character *ch);
+ virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, unsigned int x1, unsigned int y1, unsigned int x2, unsigned int y2);
virtual void DeleteHardwareTexture();
CD3DTexture *m_speedupTexture; // extra texture to speed up reallocations when the main texture is in d3dpool_default.
// that's the typical situation of Windows Vista and above.
8 xbmc/guilib/GUIFontTTFGL.cpp
View
@@ -201,16 +201,16 @@ CBaseTexture* CGUIFontTTFGL::ReallocTexture(unsigned int& newHeight)
return newTexture;
}
-bool CGUIFontTTFGL::CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character* ch)
+bool CGUIFontTTFGL::CopyCharToTexture(FT_BitmapGlyph bitGlyph, unsigned int x1, unsigned int y1, unsigned int x2, unsigned int y2)
{
FT_Bitmap bitmap = bitGlyph->bitmap;
unsigned char* source = (unsigned char*) bitmap.buffer;
- unsigned char* target = (unsigned char*) m_texture->GetPixels() + (m_posY + ch->offsetY) * m_texture->GetPitch() + m_posX + bitGlyph->left;
+ unsigned char* target = (unsigned char*) m_texture->GetPixels() + y1 * m_texture->GetPitch() + x1;
- for (int y = 0; y < bitmap.rows; y++)
+ for (unsigned int y = y1; y < y2; y++)
{
- memcpy(target, source, bitmap.width);
+ memcpy(target, source, x2-x1);
source += bitmap.width;
target += m_texture->GetPitch();
}
2  xbmc/guilib/GUIFontTTFGL.h
View
@@ -46,7 +46,7 @@ class CGUIFontTTFGL : public CGUIFontTTFBase
protected:
virtual CBaseTexture* ReallocTexture(unsigned int& newHeight);
- virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, Character *ch);
+ virtual bool CopyCharToTexture(FT_BitmapGlyph bitGlyph, unsigned int x1, unsigned int y1, unsigned int x2, unsigned int y2);
virtual void DeleteHardwareTexture();
};
Something went wrong with that request. Please try again.