Merge branch 'fix-custom-brush'

src/app/tools/ink_processing.h

@@ -1102,6 +1102,7 @@ public:
     m_height = m_brush->bounds().h;
     m_u = (m_brush->patternOrigin().x - loop->getCelOrigin().x) % m_width;
     m_v = (m_brush->patternOrigin().y - loop->getCelOrigin().y) % m_height;
+    m_transparentColor = loop->sprite()->transparentColor();
   }
 
   void prepareForPointShape(ToolLoop* loop, bool firstPoint, int x, int y) override {
@@ -1132,6 +1133,11 @@ private:
   const Image* m_brushMask;
   int m_opacity;
   int m_u, m_v, m_width, m_height;
+
+  // When we have a image brush from an INDEXED sprite, we need to know
+  // which is the background color in order to translate to transparent color
+  // in a RGBA sprite.
+  color_t m_transparentColor;
 };
 
 template<>
@@ -1144,20 +1150,36 @@ void BrushInkProcessing<RgbTraits>::processPixel(int x, int y) {
   switch (m_brushImage->pixelFormat()) {
     case IMAGE_RGB: {
       c = get_pixel_fast<RgbTraits>(m_brushImage, x, y);
+
+      // We blend the previous image brush pixel with a pixel from the
+      // image preview (*m_dstAddress). Yes, dstImage, in that way we
+      // can overlap image brush self printed areas (auto compose
+      // colors). Doing this, we avoid eraser action of the pixels
+      // with alpha <255 in the image brush.
+      c = rgba_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_INDEXED: {
+      // TODO m_palette->getEntry(c) does not work because the m_palette member is
+      // loaded the Graya Palette, NOT the original Indexed Palette from where m_brushImage belongs.
+      // This conversion can be possible if we load the palette pointer in m_brush when
+      // is created the custom brush in the Indexed Sprite.
       c = get_pixel_fast<IndexedTraits>(m_brushImage, x, y);
-      c = m_palette->getEntry(c);
+      if (m_transparentColor == c)
+        c = 0;
+      else
+        c = m_palette->getEntry(c);
+      c = rgba_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_GRAYSCALE: {
       c = get_pixel_fast<GrayscaleTraits>(m_brushImage, x, y);
-      // TODO review this line
+      c = rgba(graya_getv(c), graya_getv(c), graya_getv(c), graya_geta(c));
-      c = graya(m_palette->getEntry(c), graya_geta(c));
+      c = rgba_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_BITMAP: {
+      // TODO In which circuntance is possible this case?
       c = get_pixel_fast<BitmapTraits>(m_brushImage, x, y);
       c = c ? m_fgColor: m_bgColor;
       break;
@@ -1166,8 +1188,7 @@ void BrushInkProcessing<RgbTraits>::processPixel(int x, int y) {
       ASSERT(false);
       return;
   }
-
+  *m_dstAddress = c;
-  *m_dstAddress = rgba_blender_normal(*m_srcAddress, c, m_opacity);
 }
 
 template<>
@@ -1180,22 +1201,33 @@ void BrushInkProcessing<GrayscaleTraits>::processPixel(int x, int y) {
   switch (m_brushImage->pixelFormat()) {
     case IMAGE_RGB: {
       c = get_pixel_fast<RgbTraits>(m_brushImage, x, y);
-      c = graya(int(rgba_getr(c)) + int(rgba_getg(c)) + int(rgba_getb(c)) / 3,
+      c = graya(rgba_luma(c), rgba_geta(c));
-                rgba_geta(c));
+      c = graya_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_INDEXED: {
+      // TODO m_palette->getEntry(c) does not work because the
+      // m_palette member is loaded the Graya Palette, NOT the
+      // original Indexed Palette from where m_brushImage belongs.
+      // This conversion can be possible if we load the palette
+      // pointer in m_brush when is created the custom brush in the
+      // Indexed Sprite.
       c = get_pixel_fast<IndexedTraits>(m_brushImage, x, y);
-      c = m_palette->getEntry(c);
+      if (m_transparentColor == c)
-      c = graya(int(rgba_getr(c)) + int(rgba_getg(c)) + int(rgba_getb(c)) / 3,
+        c = 0;
-                rgba_geta(c));
+      else
+        c = m_palette->getEntry(c);
+      c = graya(rgba_luma(c), rgba_geta(c));
+      c = graya_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_GRAYSCALE: {
       c = get_pixel_fast<GrayscaleTraits>(m_brushImage, x, y);
+      c = graya_blender_normal(*m_dstAddress, c, m_opacity);
       break;
     }
     case IMAGE_BITMAP: {
+      // TODO In which circuntance is possible this case?
       c = get_pixel_fast<BitmapTraits>(m_brushImage, x, y);
       c = c ? m_fgColor: m_bgColor;
       break;
@@ -1204,8 +1236,7 @@ void BrushInkProcessing<GrayscaleTraits>::processPixel(int x, int y) {
       ASSERT(false);
       return;
   }
-
+  *m_dstAddress = c;
-  *m_dstAddress = graya_blender_normal(*m_srcAddress, c, m_opacity);
 }
 
 template<>
@@ -1234,6 +1265,7 @@ void BrushInkProcessing<IndexedTraits>::processPixel(int x, int y) {
       break;
     }
     case IMAGE_BITMAP: {
+      // TODO In which circuntance is possible this case?
       c = get_pixel_fast<BitmapTraits>(m_brushImage, x, y);
       c = c ? m_fgColor: m_bgColor;
       break;
@@ -1242,8 +1274,8 @@ void BrushInkProcessing<IndexedTraits>::processPixel(int x, int y) {
       ASSERT(false);
       return;
   }
-
+  if (c != m_transparentColor)
-  *m_dstAddress = c;
+    *m_dstAddress = c;
 }
 
 //////////////////////////////////////////////////////////////////////

src/app/tools/intertwiners.h

@@ -10,6 +10,15 @@
 namespace app {
 namespace tools {
 
+static void addPointsWithoutDuplicatingLastOne(int x, int y, Stroke* stroke)
+{
+  const gfx::Point newPoint(x, y);
+  if (stroke->empty() ||
+      stroke->lastPoint() != newPoint) {
+    stroke->addPoint(newPoint);
+  }
+}
+
 class IntertwineNone : public Intertwine {
 public:
 
@@ -59,45 +68,90 @@ public:
 };
 
 class IntertwineAsLines : public Intertwine {
+  Stroke m_lastPointPrinted;
+  Stroke m_pts;
+
+  void saveLastPointAndDoPointshape(ToolLoop* loop, const Stroke& stroke) {
+    m_lastPointPrinted = stroke;
+    doPointshapePoint(stroke[0].x, stroke[0].y, loop);
+  }
+
 public:
   bool snapByAngle() override { return true; }
 
-  void joinStroke(ToolLoop* loop, const Stroke& stroke) override
+  void joinStroke(ToolLoop* loop, const Stroke& stroke) override {
-  {
     if (stroke.size() == 0)
       return;
 
     if (stroke.size() == 1) {
-      doPointshapePoint(stroke[0].x, stroke[0].y, loop);
+      saveLastPointAndDoPointshape(loop, stroke);
+      return;
     }
     else if (stroke.size() >= 2) {
-      for (int c=0; c+1<stroke.size(); ++c) {
+      if (stroke.size() == 2 && stroke[0] == stroke[1]) {
-        int x1 = stroke[c].x;
+        if (m_lastPointPrinted.empty()) {
-        int y1 = stroke[c].y;
+          saveLastPointAndDoPointshape(loop, stroke);
-        int x2 = stroke[c+1].x;
+          return;
-        int y2 = stroke[c+1].y;
+        }
-
+        else {
-        doPointshapeLine(x1, y1, x2, y2, loop);
+          if (m_lastPointPrinted[0] != stroke[0] ||
+              loop->getTracePolicy() == TracePolicy::Last) {
+            saveLastPointAndDoPointshape(loop, stroke);
+            return;
+          }
+        }
       }
+      else {
+        for (int c=0; c+1<stroke.size(); ++c) {
+          algo_line_continuous(
+            stroke[c].x,
+            stroke[c].y,
+            stroke[c+1].x,
+            stroke[c+1].y,
+            (void*)&m_pts,
+            (AlgoPixel)&addPointsWithoutDuplicatingLastOne);
+        }
+
+        // Don't draw the first point in freehand tools (this is to
+        // avoid painting above the last pixel of a freehand stroke,
+        // when we use Shift+click in the Pencil tool to continue the
+        // old stroke).
+        // TODO useful only in the case when brush size = 1px
+        const int start = (loop->getController()->isFreehand() ? 1: 0);
+
+        for (int c=start; c<m_pts.size(); ++c)
+          doPointshapePoint(m_pts[c].x, m_pts[c].y, loop);
+
+        ASSERT(!m_lastPointPrinted.empty());
+        m_lastPointPrinted[0] = m_pts[m_pts.size()-1];
+      }
+      m_pts.reset();
     }
 
     // Closed shape (polygon outline)
-    if (loop->getFilled()) {
+    // Note: Contour tool was getting into the condition with no need, so
+    // we add the && !isFreehand to detect this circunstance.
+    // When this is missing, we have problems previewing the stroke of
+    // contour tool, with brush type = kImageBrush with alpha content and
+    // with not Pixel Perfect pencil mode.
+    if (loop->getFilled() && !loop->getController()->isFreehand()) {
       doPointshapeLine(stroke[0].x, stroke[0].y,
                        stroke[stroke.size()-1].x,
                        stroke[stroke.size()-1].y, loop);
     }
   }
 
-  void fillStroke(ToolLoop* loop, const Stroke& stroke) override
+  void fillStroke(ToolLoop* loop, const Stroke& stroke) override {
-  {
     if (stroke.size() < 3) {
       joinStroke(loop, stroke);
       return;
     }
 
-    // Contour
+    // Don't draw the contour to avoid double drawing the filled
-    joinStroke(loop, stroke);
+    // polygon and the contour when we use a custom brush and we use
+    // the alpha compositing ink with opacity < 255 or the custom
+    // brush has semi-transparent pixels.
+    //joinStroke(loop, stroke);
 
     // Fill content
     doc::algorithm::polygon(stroke.size(), (const int*)&stroke[0], loop, (AlgoHLine)doPointshapeHline);
@@ -108,8 +162,7 @@ public:
 class IntertwineAsRectangles : public Intertwine {
 public:
 
-  void joinStroke(ToolLoop* loop, const Stroke& stroke) override
+  void joinStroke(ToolLoop* loop, const Stroke& stroke) override {
-  {
     if (stroke.size() == 0)
       return;
 
@@ -318,8 +371,7 @@ public:
 class IntertwineAsBezier : public Intertwine {
 public:
 
-  void joinStroke(ToolLoop* loop, const Stroke& stroke) override
+  void joinStroke(ToolLoop* loop, const Stroke& stroke) override {
-  {
     if (stroke.size() == 0)
       return;
 
@@ -348,21 +400,17 @@ public:
     }
   }
 
-  void fillStroke(ToolLoop* loop, const Stroke& stroke) override
+  void fillStroke(ToolLoop* loop, const Stroke& stroke) override {
-  {
     joinStroke(loop, stroke);
   }
 };
 
 class IntertwineAsPixelPerfect : public Intertwine {
-  static void pixelPerfectLine(int x, int y, Stroke* stroke) {
+  // It was introduced to know if joinStroke function
-    gfx::Point newPoint(x, y);
+  // was executed inmediatelly after a "Last" trace policy (i.e. after the
-    if (stroke->empty() ||
+  // user confirms a line draw while he is holding down the SHIFT key), so
-        stroke->lastPoint() != newPoint) {
+  // we have to ignore printing the first pixel of the line.
-      stroke->addPoint(newPoint);
+  bool m_retainedTracePolicyLast = false;
-    }
-  }
-
   Stroke m_pts;
 
 public:
@@ -372,6 +420,7 @@ public:
 
   void prepareIntertwine() override {
     m_pts.reset();
+    m_retainedTracePolicyLast = false;
   }
 
   void joinStroke(ToolLoop* loop, const Stroke& stroke) override {
@@ -380,13 +429,18 @@ public:
     // new joinStroke() is like a fresh start.  Without this fix, the
     // first stage on LineFreehand will draw a "star" like pattern
     // with lines from the first point to the last point.
-    if (loop->getTracePolicy() == TracePolicy::Last)
+    if (loop->getTracePolicy() == TracePolicy::Last) {
+      m_retainedTracePolicyLast = true;
       m_pts.reset();
+    }
 
     if (stroke.size() == 0)
       return;
-    else if (m_pts.empty() && stroke.size() == 1) {
+    else if (stroke.size() == 1) {
-      m_pts = stroke;
+      if (m_pts.empty())
+        m_pts = stroke;
+      doPointshapePoint(stroke[0].x, stroke[0].y, loop);
+      return;
     }
     else {
       for (int c=0; c+1<stroke.size(); ++c) {
@@ -396,7 +450,7 @@ public:
           stroke[c+1].x,
           stroke[c+1].y,
           (void*)&m_pts,
-          (AlgoPixel)&IntertwineAsPixelPerfect::pixelPerfectLine);
+          (AlgoPixel)&addPointsWithoutDuplicatingLastOne);
       }
     }
 
@@ -411,6 +465,12 @@ public:
         ++c;
       }
 
+      // We must ignore to print the first point of the line after
+      // a joinStroke pass with a retained "Last" trace policy
+      // (i.e. the user confirms draw a line while he is holding
+      // the SHIFT key))
+      if (c == 0 && m_retainedTracePolicyLast)
+        continue;
       doPointshapePoint(m_pts[c].x, m_pts[c].y, loop);
     }
   }
@@ -421,8 +481,11 @@ public:
       return;
     }
 
-    // Contour
+    // Don't draw the contour to avoid double drawing the filled
-    joinStroke(loop, stroke);
+    // polygon and the contour when we use a custom brush and we use
+    // the alpha compositing ink with opacity < 255 or the custom
+    // brush has semi-transparent pixels.
+    //joinStroke(loop, stroke);
 
     // Fill content
     doc::algorithm::polygon(stroke.size(), (const int*)&stroke[0], loop, (AlgoHLine)doPointshapeHline);

src/app/tools/tool_loop_manager.cpp

@@ -77,6 +77,16 @@ void ToolLoopManager::pressButton(const Pointer& pointer)
 {
   TOOL_TRACE("ToolLoopManager::pressButton", pointer.point());
 
+  // A little patch to memorize initial Trace Policy in the
+  // current function execution.
+  // When the initial trace policy is "Last" and then
+  // changes to different trace policy at the end of
+  // this function, the user confirms a line draw while he
+  // is holding the SHIFT key.
+  bool tracePolicyWasLast = false;
+  if (m_toolLoop->getTracePolicy() == TracePolicy::Last)
+    tracePolicyWasLast = true;
+
   m_lastPointer = pointer;
 
   if (isCanceled())
@@ -102,7 +112,18 @@ void ToolLoopManager::pressButton(const Pointer& pointer)
   m_toolLoop->getController()->getStatusBarText(m_toolLoop, m_stroke, statusText);
   m_toolLoop->updateStatusBar(statusText.c_str());
 
-  doLoopStep(false);
+  // We evaluate if the trace policy has changed compared with
+  // the initial trace policy.
+  if (!(m_toolLoop->getTracePolicy() == TracePolicy::Last) &&
+        tracePolicyWasLast) {
+    // Do nothing. We do not need execute an additional doLoopStep
+    // (which it want to accumulate more points in m_pts in function
+    // joinStroke() from intertwiners.h)
+    // This avoid double print of a line while the user holds down
+    // the SHIFT key.
+  }
+  else
+    doLoopStep(false);
 }
 
 bool ToolLoopManager::releaseButton(const Pointer& pointer)
@@ -195,10 +216,18 @@ void ToolLoopManager::doLoopStep(bool lastStep)
 
   m_toolLoop->getInk()->prepareForStrokes(m_toolLoop, strokes);
 
-  // Invalidate destination image area.
+  // True when we have to fill
-  if (m_toolLoop->getTracePolicy() == TracePolicy::Last) {
+  const bool fillStrokes =
-    // Copy source to destination (reset the previous trace). Useful
+    (m_toolLoop->getFilled() &&
-    // for tools like Line and Ellipse (we keep the last trace only).
+     (lastStep || m_toolLoop->getPreviewFilled()));
+
+  // Invalidate the whole destination image area.
+  if (m_toolLoop->getTracePolicy() == TracePolicy::Last ||
+      fillStrokes) {
+    // Copy source to destination (reset all the previous
+    // traces). Useful for tools like Line and Ellipse (we keep the
+    // last trace only) or to draw the final result in contour tool
+    // (the final result is filled).
     m_toolLoop->invalidateDstImage();
   }
   else if (m_toolLoop->getTracePolicy() == TracePolicy::AccumulateUpdateLast) {
@@ -206,16 +235,24 @@ void ToolLoopManager::doLoopStep(bool lastStep)
     // freehand algorithm needs this trace policy to redraw only the
     // last dirty area, which can vary in one pixel from the previous
     // tool loop cycle).
-    m_toolLoop->invalidateDstImage(m_dirtyArea);
+    if (m_toolLoop->getBrush()->type() != kImageBrushType) {
+      m_toolLoop->invalidateDstImage(m_dirtyArea);
+    }
+    // For custom brush we revalidate the whole destination area so
+    // the whole trace is redrawn from scratch.
+    else {
+      m_toolLoop->invalidateDstImage();
+      m_toolLoop->validateDstImage(gfx::Region(m_toolLoop->getDstImage()->bounds()));
+    }
   }
 
   m_toolLoop->validateDstImage(m_dirtyArea);
 
   // Join or fill user points
-  if (!m_toolLoop->getFilled() || (!lastStep && !m_toolLoop->getPreviewFilled()))
+  if (fillStrokes)
-    m_toolLoop->getIntertwine()->joinStroke(m_toolLoop, main_stroke);
-  else
     m_toolLoop->getIntertwine()->fillStroke(m_toolLoop, main_stroke);
+  else
+    m_toolLoop->getIntertwine()->joinStroke(m_toolLoop, main_stroke);
 
   if (m_toolLoop->getTracePolicy() == TracePolicy::Overlap) {
     // Copy destination to source (yes, destination to source). In