make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanwenn@gmail.com)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanwenn@gmail.com)

nine.fierce.ballads
I see I'm late to the party with this comment, but what are the error
bars on that 0.29%?  I'm not sure the decreased readability is worth it.

https://codereview.appspot.com/551730043/

Reply | Threaded
Open this post in threaded view
|

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
Reviewers: Dan Eble,

Message:
The percentages between different have looked fairly repeatable, but as
a less invasive measure is to just tweak the final 'if'

Description:
make_draw_bezier_boxes: save work if thickness == 0.0

This drop make_draw_bezier_boxes from 0.73% to 0.44% in the profile for
MSDM

Please review this at https://codereview.appspot.com/551730043/

Affected files (+24, -5 lines):
  M lily/stencil-integral.cc


Index: lily/stencil-integral.cc
diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
index 189cdcdc881ec0c1c417a4a783b4675446ec4207..e43bbee6cd14756bcf0b6e4a575c68b3abd4a136 100644
--- a/lily/stencil-integral.cc
+++ b/lily/stencil-integral.cc
@@ -480,12 +480,20 @@ make_draw_bezier_boxes (vector<Box> &boxes,
                            + (temp3 - temp2).length ())
                           / QUANTIZATION_UNIT);
 
-  Offset d0 = curve.dir_at_point (0.0);
-  Offset d1 = curve.dir_at_point (1.0);
+  Offset d0;
+  Offset d1;
+
+  Offset normal;
+  if (th > 0)
+    {
+      d0 = curve.dir_at_point (0.0);
+      normal = get_normal ((th / 2) * d0);
+    }
 
-  Offset normal = get_normal ((th / 2) * d0);
   for (DOWN_and_UP (d))
     {
+      if (th == 0.0 && d == UP)
+        break;
       points[d].push_back (
         scm_transform (trans, curve.control_[0] + d * normal));
     }
@@ -497,14 +505,23 @@ make_draw_bezier_boxes (vector<Box> &boxes,
 
       for (DOWN_and_UP (d))
         {
+          if (th == 0.0 && d == UP)
+            break;
           points[d].push_back (
             scm_transform (trans, curve.curve_point (pt) + d * norm));
         }
     }
 
-  normal = get_normal ((th / 2) * d1);
+  if (th > 0)
+    {
+      d1 = curve.dir_at_point (1.0);
+      normal = get_normal ((th / 2) * d1);
+    }
+
   for (DOWN_and_UP (d))
     {
+      if (th == 0.0 && d == UP)
+        break;
       points[d].push_back (
         scm_transform (trans, curve.control_[3] + d * normal));
     }
@@ -514,13 +531,15 @@ make_draw_bezier_boxes (vector<Box> &boxes,
       Box b;
       for (DOWN_and_UP (d))
         {
+          if (th == 0.0 && d == UP)
+            break;
           b.add_point (points[d][i]);
           b.add_point (points[d][i + 1]);
         }
       boxes.push_back (b);
     }
 
-  if (th >= 0)
+  if (th > 0)
     {
       // beg line cap
       create_path_cap (boxes, buildings, trans, curve.control_[0], th / 2, -d0);



Reply | Threaded
Open this post in threaded view
|

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanwenn@gmail.com)

David Kastrup
In reply to this post by nine.fierce.ballads

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc#newcode496
lily/stencil-integral.cc:496: break;
I think that is less readable than desirable.  Instead how about putting

if (th == 0.0)
   break;

behind the push?  That makes very obvious that only one point is being
pushed without further cleverness.

Either way you'll be checking thickness twice if its non-zero.  The
trivial way to avoid that would be to simply unfold the loop.  Like

points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
normal);
if (th != 0.0)
  points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
normal;

Written like that, it actually becomes far from trivial to see why this
optimisation would be valid, so maybe add a comment explaining it for
the sake of human readers?

https://codereview.appspot.com/551730043/

Reply | Threaded
Open this post in threaded view
|

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
On Fri, Apr 24, 2020 at 8:56 PM <[hidden email]> wrote:

>
>
> https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
>
> https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc#newcode496
> lily/stencil-integral.cc:496: break;
> I think that is less readable than desirable.  Instead how about putting
>
> if (th == 0.0)
>    break;
>
> behind the push?  That makes very obvious that only one point is being
> pushed without further cleverness.

Yeah, that's better.

> Either way you'll be checking thickness twice if its non-zero.  The
> trivial way to avoid that would be to simply unfold the loop.  Like
>
> points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
> normal);
> if (th != 0.0)
>   points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
> normal;

the UP/DOWN manipulation has been a frequent source of errors in cut &
paste code, so I'd like to keep the loops.

> Written like that, it actually becomes far from trivial to see why this
> optimisation would be valid, so maybe add a comment explaining it for
> the sake of human readers?

The thickness check is cheap. The expensive bit is calculating
directions and bezier points. I added a comment.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen