Improved code for PolygonStrokePrimitive2D::getB2DRange

For extended discussion & background information please refer
to the comments added to the code there.

Had to handle view-dependent parts like the hairline
different, do not buffer that case. It could be, but it's not
expensive and would require to remember and check against
the view-dependent part which was used to create the
B2DRange initially.

Change-Id: I10df46207990865c667d41f56aedb8f0956a1706
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/142114
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

1 files changed, 111 insertions, 27 deletions

diff --git a/drawinglayer/source/primitive2d/polygonprimitive2d.cxx b/drawinglayer/source/primitive2d/polygonprimitive2d.cxx
index 00ac96405b62..a91e87b38c1c 100644
--- a/drawinglayer/source/primitive2d/polygonprimitive2d.cxx
+++ b/drawinglayer/source/primitive2d/polygonprimitive2d.cxx
@@ -278,6 +278,7 @@ PolygonStrokePrimitive2D::PolygonStrokePrimitive2D(basegfx::B2DPolygon aPolygon,
     : maPolygon(std::move(aPolygon))
     , maLineAttribute(rLineAttribute)
     , maStrokeAttribute(std::move(aStrokeAttribute))
+    , maBufferedRange()
 {
     // MM01: keep these - these are no curve-decompposers but just checks
     // simplify curve segments: moved here to not need to use it
@@ -289,6 +290,7 @@ PolygonStrokePrimitive2D::PolygonStrokePrimitive2D(basegfx::B2DPolygon aPolygon,
                                                    const attribute::LineAttribute& rLineAttribute)
     : maPolygon(std::move(aPolygon))
     , maLineAttribute(rLineAttribute)
+    , maBufferedRange()
 {
     // MM01: keep these - these are no curve-decompposers but just checks
     // simplify curve segments: moved here to not need to use it
@@ -314,7 +316,11 @@ bool PolygonStrokePrimitive2D::operator==(const BasePrimitive2D& rPrimitive) con
 basegfx::B2DRange
 PolygonStrokePrimitive2D::getB2DRange(const geometry::ViewInformation2D& rViewInformation) const
 {
-    basegfx::B2DRange aRetval;
+    if (!maBufferedRange.isEmpty())
+    {
+        // use the view-independent, buffered B2DRange
+        return maBufferedRange;
+    }
 
     if (getLineAttribute().getWidth())
     {
@@ -334,6 +340,20 @@ PolygonStrokePrimitive2D::getB2DRange(const geometry::ViewInformation2D& rViewIn
             // the grow method below works perfectly for LineCap_ROUND since the grow is in
             // all directions and the rounded cap needs the same grow in all directions independent
             // from its orientation. Unfortunately this is not the case for drawing::LineCap_SQUARE
+
+            // NOTE: I thought about using [sqrt(2) * 0.5] a a factor for LineCap_SQUARE and not
+            // set bUseDecomposition. I even tried that it works. Then an auto-test failing showed
+            // not only that view-dependent stuff needs to be considered (what is done for the
+            // hairline case below), *BUT* also e.g. conversions to PNG/exports use the B2DRange
+            // of the geometry, so:
+            // - expanding by 1/2 LineWidth is OK for rounded
+            // - expanding by more (like sqrt(2) * 0.5 * LineWidth) immediately extends the size
+            //   of e.g. geometry converted to PNG, plus many similar cases that cannot be thought
+            //   of in advance.
+            // This means that converting those thoght-experiment examples in (4) will and do lead
+            // to bigger e.g. Bitmap conversion(s), not avoiding but painting the free space. That
+            // could only be done by correctly and fully decomposing the geometry, including
+            // stroke, and accepting the cost...
             bUseDecomposition = true;
         }
 
@@ -341,44 +361,108 @@ PolygonStrokePrimitive2D::getB2DRange(const geometry::ViewInformation2D& rViewIn
         {
             // get correct range by using the decomposition fallback, reasons see above cases
 
-            // ofz#947 to optimize calculating the range, turn any slow dashes into a solid line
-            // when just calculating bounds
-            attribute::StrokeAttribute aOrigStrokeAttribute = maStrokeAttribute;
-            const_cast<PolygonStrokePrimitive2D*>(this)->maStrokeAttribute
-                = attribute::StrokeAttribute();
-            aRetval = BufferedDecompositionPrimitive2D::getB2DRange(rViewInformation);
-            const_cast<PolygonStrokePrimitive2D*>(this)->maStrokeAttribute = aOrigStrokeAttribute;
+            // It is not a good idea to temporarily (re)set the PolygonStrokePrimitive2D
+            // to default. While it is understandable to use the speed advantage, it is
+            // bad for quite some reasons:
+            //
+            // (1) As described in include/drawinglayer/primitive2d/baseprimitive2d.hxx
+            //     a Primitive is "noncopyable to make clear that a primitive is a read-only
+            //     instance and copying or changing values is not intended". This is the base
+            //     assumption for many decisions for Primitive handling.
+            // (2) For example, that the decomposition is *always* re-usable. It cannot change
+            //     and is correct when it already exists since the values the decomposition is
+            //     based on cannot change.
+            // (3) If this *is* done (like it was here) and the Primitive is derived from
+            //     BufferedDecompositionPrimitive2D and thus buffers it's decomposition,
+            //     the risk is that in this case the *wrong* decomposition will be used by
+            //     other PrimitiveProcessors. Maybe not by the VclPixelProcessor2D/VclProcessor2D
+            //     since it handles this primitive directly - not even sure for all cases.
+            //     Sooner or later another PrimitiveProcessor will re-use this wrong temporary
+            //     decompositon, and as an error, a non-stroked line will be painted/used.
+            // (4) The B2DRange is not strictly defined as minimal bound for the geometry,
+            //     but it should be as small/tight as possible. Making it larger risks more
+            //     area to be invalidated (repaint) and processed (all geometric stuff,l may
+            //     include future and existing exports to other formats which are or will be
+            //     implemented as PrimitiveProcessor). It is easy to imagine cases with much
+            //     too large B2DRange - a line with a pattern that would solve to a single
+            //     small start-rectange and rest is empty, or a circle with a stroke that
+            //     makes only a quarter of it visible.
+            //
+            // The reason to do this is speed, what is a good argument. But speed should
+            // only be used if the pair of [correctness/speed] does not sacrifice the correctness
+            // over the speed.
+            // Luckily there are alternatives to solve this and to keep [correctness/speed]
+            // valid:
+            //
+            // (a) Reset the temporary decomposition after having const-casted and
+            //     changed maStrokeAttribute.
+            //     Disadvantage: More const-cast hacks, plus this temporary decomposition
+            //     will be potentially done repeatedly (every time
+            //     PolygonStrokePrimitive2D::getB2DRange is called)
+            // (b) Use a temporary, local PolygonStrokePrimitive2D here, with neutral
+            //     PolygonStrokePrimitive2D and call ::getB2DRange() at it. That way
+            //     the buffered decomposition will not be harmed.
+            //     Disadvantage: Same as (a), decomposition will be potentially done repeatedly
+            // (c) Use a temporary, local PolygonStrokePrimitive2D and buffer B2DRange
+            //     locally for this Prmitive. Due to (1)/(2) this cannot change, so
+            //     when calculated once it is totally legal to use it.
+            //
+            // Thus here I would use (c): It accepts the disadvantages of (4) over speed, but
+            // avoids the errors/problems from (1-4).
+            // Additional argument for this: The hairline case below *also* uses the full
+            // B2DRange of the polygon, ignoring an evtl. stroke, so (4) applies
+            if (!getStrokeAttribute().isDefault())
+            {
+                // only do this if StrokeAttribute is used, else recursion may happen (!)
+                const rtl::Reference<primitive2d::PolygonStrokePrimitive2D>
+                    aTemporaryPrimitiveWithoutStroke(new primitive2d::PolygonStrokePrimitive2D(
+                        getB2DPolygon(), getLineAttribute()));
+                maBufferedRange
+                    = aTemporaryPrimitiveWithoutStroke
+                          ->BufferedDecompositionPrimitive2D::getB2DRange(rViewInformation);
+            }
+            else
+            {
+                // fallback to normal decompose, that result can be used for visualization
+                // later, too. Still buffer B2DRange in maBufferedRange, so it needs to be
+                // merged into one B2DRange only once
+                maBufferedRange = BufferedDecompositionPrimitive2D::getB2DRange(rViewInformation);
+            }
         }
         else
         {
             // for all other B2DLINEJOIN_* get the range from the base geometry
-            // and expand by half the line width
-            aRetval = getB2DPolygon().getB2DRange();
-            aRetval.grow(getLineAttribute().getWidth() * 0.5);
+            // and expand by half the line width.
+            maBufferedRange = getB2DPolygon().getB2DRange();
+            maBufferedRange.grow(getLineAttribute().getWidth() * 0.5);
         }
+
+        return maBufferedRange;
     }
-    else
+
+    // It is a hairline, thus the line width is view-dependent. Get range of polygon
+    // as base size.
+    // CAUTION: Since a hairline *is* view-dependent,
+    // - either use maBufferedRange, additionally remember view-dependent
+    //   factor & reset if that changes
+    // - or do not buffer for hairline -> not really needed, the range is buffered
+    //   in the B2DPolygon, no decompostion is needed and a simple grow is cheap
+    basegfx::B2DRange aHairlineRange = getB2DPolygon().getB2DRange();
+
+    if (!aHairlineRange.isEmpty())
     {
-        // this is a hairline, thus the line width is view-dependent. Get range of polygon
-        // as base size
-        aRetval = getB2DPolygon().getB2DRange();
+        // Calculate view-dependent hairline width
+        const basegfx::B2DVector aDiscreteSize(
+            rViewInformation.getInverseObjectToViewTransformation() * basegfx::B2DVector(1.0, 0.0));
+        const double fDiscreteHalfLineWidth(aDiscreteSize.getLength() * 0.5);
 
-        if (!aRetval.isEmpty())
+        if (basegfx::fTools::more(fDiscreteHalfLineWidth, 0.0))
         {
-            // Calculate view-dependent hairline width
-            const basegfx::B2DVector aDiscreteSize(
-                rViewInformation.getInverseObjectToViewTransformation()
-                * basegfx::B2DVector(1.0, 0.0));
-            const double fDiscreteHalfLineWidth(aDiscreteSize.getLength() * 0.5);
-
-            if (basegfx::fTools::more(fDiscreteHalfLineWidth, 0.0))
-            {
-                aRetval.grow(fDiscreteHalfLineWidth);
-            }
+            aHairlineRange.grow(fDiscreteHalfLineWidth);
         }
     }
 
-    return aRetval;
+    return aHairlineRange;
 }
 
 // provide unique ID