tdf#160430: Fix glyph bounds calculation, and use basegfx::B2DRectangle

... instead of tools::Rectangle.

Several problems were there:
1. First, a horizontal bounding rectangle was calculated, with due
rounding; and then the result was rotated, and after that, rounded
again. That made the resulting rotated rectangle coordinates very
imprecise.
2. Also, ceil/floor was applied without normalization; and in case
of rotated font, that meant, that sometimes the range could be not
expanded to cover partially covered pixels, but instead collapsed.
3. The rotation to angles other than 90 degree multiples was done
incorrectly, resulting in cut off parts of characters.
4. For 90 degrees, the imprecise result of sin/cos converted 0.0
into values like 3e-16, which then could be ceil'ed up to 1.

Using B2DRectangle and its transform allows to simplify and fix
the calculations easily, and avoids premature rounding. Render of
rotated text of small size is more stable with this change.

Change-Id: Idffd74b9937feb2418ab76a8d325fdaf4ff841b7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165553
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

6 files changed, 43 insertions, 49 deletions

diff --git a/vcl/source/font/LogicalFontInstance.cxx b/vcl/source/font/LogicalFontInstance.cxx
index 94e1e72353fb..56129199d3a8 100644
--- a/vcl/source/font/LogicalFontInstance.cxx
+++ b/vcl/source/font/LogicalFontInstance.cxx
@@ -26,6 +26,8 @@
 #include <font/LogicalFontInstance.hxx>
 #include <impfontcache.hxx>
 
+#include <basegfx/matrix/b2dhommatrixtools.hxx>
+
 LogicalFontInstance::LogicalFontInstance(const vcl::font::PhysicalFontFace& rFontFace,
                                          const vcl::font::FontSelectPattern& rFontSelData)
     : mxFontMetric(new FontMetricData(rFontSelData))
@@ -167,47 +169,39 @@ void LogicalFontInstance::IgnoreFallbackForUnicode(sal_UCS4 cChar, FontWeight eW
         maUnicodeFallbackList.erase(it);
 }
 
-bool LogicalFontInstance::GetGlyphBoundRect(sal_GlyphId nID, tools::Rectangle& rRect,
+bool LogicalFontInstance::GetGlyphBoundRect(sal_GlyphId nID, basegfx::B2DRectangle& rRect,
                                             bool bVertical) const
 {
+    // TODO/FIXME: bVertical handling here is highly suspicious. When it's true, it may
+    // return different rectangle, depending on if this glyph was cached already or not.
+
     if (mpFontCache && mpFontCache->GetCachedGlyphBoundRect(this, nID, rRect))
         return true;
 
     auto* pHbFont = const_cast<LogicalFontInstance*>(this)->GetHbFont();
     hb_glyph_extents_t aExtents;
-    bool res = hb_font_get_glyph_extents(pHbFont, nID, &aExtents);
+    if (!hb_font_get_glyph_extents(pHbFont, nID, &aExtents))
+        return false;
 
-    if (res)
-    {
-        double nXScale = 0, nYScale = 0;
-        GetScale(&nXScale, &nYScale);
+    double nXScale = 0, nYScale = 0;
+    GetScale(&nXScale, &nYScale);
 
-        double fMinX = aExtents.x_bearing;
-        double fMinY = aExtents.y_bearing;
-        double fMaxX = aExtents.x_bearing + aExtents.width;
-        double fMaxY = aExtents.y_bearing + aExtents.height;
+    double fMinX = aExtents.x_bearing * nXScale;
+    double fMinY = -aExtents.y_bearing * nYScale;
+    double fMaxX = (aExtents.x_bearing + aExtents.width) * nXScale;
+    double fMaxY = -(aExtents.y_bearing + aExtents.height) * nYScale;
+    rRect = basegfx::B2DRectangle(fMinX, fMinY, fMaxX, fMaxY);
 
-        tools::Rectangle aRect(std::floor(fMinX * nXScale), -std::ceil(fMinY * nYScale),
-                               std::ceil(fMaxX * nXScale), -std::floor(fMaxY * nYScale));
-        if (mnOrientation && !bVertical)
-        {
-            // Apply font rotation.
-            const double fRad = toRadians(mnOrientation);
-            const double fCos = cos(fRad);
-            const double fSin = sin(fRad);
-
-            rRect.SetLeft(fCos * aRect.Left() + fSin * aRect.Top());
-            rRect.SetTop(-fSin * aRect.Left() - fCos * aRect.Top());
-            rRect.SetRight(fCos * aRect.Right() + fSin * aRect.Bottom());
-            rRect.SetBottom(-fSin * aRect.Right() - fCos * aRect.Bottom());
-        }
-        else
-            rRect = aRect;
+    if (mnOrientation && !bVertical)
+    {
+        // Apply font rotation.
+        rRect.transform(basegfx::utils::createRotateB2DHomMatrix(-toRadians(mnOrientation)));
     }
 
-    if (mpFontCache && res)
+    if (mpFontCache)
         mpFontCache->CacheGlyphBoundRect(this, nID, rRect);
-    return res;
+
+    return true;
 }
 
 sal_GlyphId LogicalFontInstance::GetGlyphIndex(uint32_t nUnicode, uint32_t nVariationSelector) const
diff --git a/vcl/source/font/fontcache.cxx b/vcl/source/font/fontcache.cxx
index c0dba153502e..ce4ba6adf64c 100644
--- a/vcl/source/font/fontcache.cxx
+++ b/vcl/source/font/fontcache.cxx
@@ -252,7 +252,7 @@ void ImplFontCache::Invalidate()
     m_aBoundRectCache.clear();
 }
 
-bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, tools::Rectangle &rRect)
+bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, basegfx::B2DRectangle &rRect)
 {
     if (!pFont->GetFontCache())
         return false;
@@ -269,7 +269,7 @@ bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sa
     return false;
 }
 
-void ImplFontCache::CacheGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, tools::Rectangle &rRect)
+void ImplFontCache::CacheGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, basegfx::B2DRectangle &rRect)
 {
     if (!pFont->GetFontCache())
         return;
diff --git a/vcl/source/gdi/CommonSalLayout.cxx b/vcl/source/gdi/CommonSalLayout.cxx
index 61f2734f322e..fc3a2a2bf79c 100644
--- a/vcl/source/gdi/CommonSalLayout.cxx
+++ b/vcl/source/gdi/CommonSalLayout.cxx
@@ -534,12 +534,12 @@ bool GenericSalLayout::LayoutText(vcl::text::ImplLayoutArgs& rArgs, const SalLay
                     {
                         // We need glyph's advance, top bearing, and height to
                         // correct y offset.
-                        tools::Rectangle aRect;
+                        basegfx::B2DRectangle aRect;
                         // Get cached bound rect value for the font,
                         GetFont().GetGlyphBoundRect(nGlyphIndex, aRect, true);
 
-                        nXOffset = -(aRect.Top() / nXScale  + ( pHbPositions[i].y_advance
-                                    + ( aRect.GetHeight() / nXScale ) ) / 2.0 );
+                        nXOffset = -(aRect.getMinX() / nXScale  + ( pHbPositions[i].y_advance
+                                    + ( aRect.getHeight() / nXScale ) ) / 2.0 );
                     }
 
                 }
diff --git a/vcl/source/gdi/pdfwriter_impl.cxx b/vcl/source/gdi/pdfwriter_impl.cxx
index bb444b32cf3d..4d33471177d2 100644
--- a/vcl/source/gdi/pdfwriter_impl.cxx
+++ b/vcl/source/gdi/pdfwriter_impl.cxx
@@ -7091,7 +7091,7 @@ void PDFWriterImpl::drawLayout( SalLayout& rLayout, const OUString& rText, bool
     else if ( eAlign == ALIGN_TOP )
         aOffset.AdjustY(GetFontInstance()->mxFontMetric->GetAscent() );
 
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aRectangle;
     nIndex = 0;
     while (rLayout.GetNextGlyph(&pGlyph, aPos, nIndex, &pGlyphFont))
     {
@@ -7109,7 +7109,7 @@ void PDFWriterImpl::drawLayout( SalLayout& rLayout, const OUString& rText, bool
             else
             {
                 aAdjOffset = basegfx::B2DPoint(aOffset.X(), aOffset.Y());
-                aAdjOffset.adjustX(aRectangle.Left() + (aRectangle.GetWidth() - aEmphasisMark.GetWidth()) / 2 );
+                aAdjOffset.adjustX(aRectangle.getMinX() + (aRectangle.getWidth() - aEmphasisMark.GetWidth()) / 2 );
             }
 
             aAdjOffset = aRotScale.transform( aAdjOffset );
diff --git a/vcl/source/gdi/sallayout.cxx b/vcl/source/gdi/sallayout.cxx
index af281127ba86..d1a29de327ff 100644
--- a/vcl/source/gdi/sallayout.cxx
+++ b/vcl/source/gdi/sallayout.cxx
@@ -214,12 +214,15 @@ bool SalLayout::GetOutline(basegfx::B2DPolyPolygonVector& rVector) const
     return (bAllOk && bOneOk);
 }
 
+// No need to expand to the next pixel, when the character only covers its tiny fraction
+static double trimInsignificant(double n) { return std::round(n * 1e5) / 1e5; }
+
 bool SalLayout::GetBoundRect(tools::Rectangle& rRect) const
 {
     bool bRet = false;
-    rRect.SetEmpty();
 
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aUnion;
+    basegfx::B2DRectangle aRectangle;
 
     basegfx::B2DPoint aPos;
     const GlyphItem* pGlyph;
@@ -230,22 +233,19 @@ bool SalLayout::GetBoundRect(tools::Rectangle& rRect) const
         // get bounding rectangle of individual glyph
         if (pGlyph->GetGlyphBoundRect(pGlyphFont, aRectangle))
         {
-            if (!aRectangle.IsEmpty())
+            if (!aRectangle.isEmpty())
             {
-                aRectangle.AdjustLeft(std::floor(aPos.getX()));
-                aRectangle.AdjustRight(std::ceil(aPos.getX()));
-                aRectangle.AdjustTop(std::floor(aPos.getY()));
-                aRectangle.AdjustBottom(std::ceil(aPos.getY()));
-
+                aRectangle.transform(basegfx::utils::createTranslateB2DHomMatrix(aPos));
                 // merge rectangle
-                if (rRect.IsEmpty())
-                    rRect = aRectangle;
-                else
-                    rRect.Union(aRectangle);
+                aUnion.expand(aRectangle);
             }
             bRet = true;
         }
     }
+    rRect = tools::Rectangle(rtl::math::approxFloor(trimInsignificant(aUnion.getMinX())),
+                             rtl::math::approxFloor(trimInsignificant(aUnion.getMinY())),
+                             rtl::math::approxCeil(trimInsignificant(aUnion.getMaxX())),
+                             rtl::math::approxCeil(trimInsignificant(aUnion.getMaxY())));
 
     return bRet;
 }
diff --git a/vcl/source/outdev/font.cxx b/vcl/source/outdev/font.cxx
index 0b87c8ef8418..9416963b93b6 100644
--- a/vcl/source/outdev/font.cxx
+++ b/vcl/source/outdev/font.cxx
@@ -948,7 +948,7 @@ void OutputDevice::ImplDrawEmphasisMarks( SalLayout& rSalLayout )
     aOffset += Point( nEmphasisWidth2, nEmphasisHeight2 );
 
     basegfx::B2DPoint aOutPoint;
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aRectangle;
     const GlyphItem* pGlyph;
     const LogicalFontInstance* pGlyphFont;
     int nStart = 0;
@@ -968,7 +968,7 @@ void OutputDevice::ImplDrawEmphasisMarks( SalLayout& rSalLayout )
             else
             {
                 aAdjPoint = aOffset;
-                aAdjPoint.AdjustX(aRectangle.Left() + (aRectangle.GetWidth() - aEmphasisMark.GetWidth()) / 2 );
+                aAdjPoint.AdjustX(aRectangle.getMinX() + (aRectangle.getWidth() - aEmphasisMark.GetWidth()) / 2 );
             }
 
             if ( mpFontInstance->mnOrientation )