Implement make-bow-stencil, make-tie-stencil for use in markup-commands undertie and overtie

Implement make-bow-stencil, make-tie-stencil for use in markup-commands undertie and overtie

issue 3088

In a follow up it is planned to replace make-parenthesis-stencil with
an appropiate setting of make-bow-stencil and to partially rework the
parenthesize-markup-command

[thomasmorley651, 2015-11-04 00:14:33]

please review

[simon.albrecht, 2015-11-04 00:28:27]

Man, sorry, while not being up to your coding skills, I can make nitpicks…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode50
scm/stencil.scm:50: @var{bow-height} determines the heigth of the bow.
…height…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode54
scm/stencil.scm:54: Both variables are supplied to support independant usage.
…independent…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode56
scm/stencil.scm:56: The task is done by calculating a horizontal bow with
appropiate length first,
…appropriate…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode64
scm/stencil.scm:64: ;;;; (4) calculat control-points for a horizontal
bezier-curce, beginning
…curve…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode67
scm/stencil.scm:67: ;;;; (6) move rotated conrol-points to match `start'
…control-points… (both preceding lines)

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode101
scm/stencil.scm:101: ;;;; (4) calculate control-points for a horizontal
bezier-curce,
…curve…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode138
scm/stencil.scm:138: ;;;; (5) rotate conrol-points around '(0 . 0) to match
`stop'
…control-points…

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode148
scm/stencil.scm:148: ;;;; (6) move rotated conrol-points to match `start'
…same here…

[thomasmorley651, 2015-11-04 21:26:33]

fix typos and oversights

[thomasmorley651, 2015-11-04 21:27:33]

thanks for review

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode50
scm/stencil.scm:50: @var{bow-height} determines the heigth of the bow.
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …height…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode54
scm/stencil.scm:54: Both variables are supplied to support independant usage.
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …independent…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode56
scm/stencil.scm:56: The task is done by calculating a horizontal bow with
appropiate length first,
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …appropriate…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode64
scm/stencil.scm:64: ;;;; (4) calculat control-points for a horizontal
bezier-curce, beginning
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …curve…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode67
scm/stencil.scm:67: ;;;; (6) move rotated conrol-points to match `start'
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …control-points… (both preceding lines)

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode101
scm/stencil.scm:101: ;;;; (4) calculate control-points for a horizontal
bezier-curce,
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …curve…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode138
scm/stencil.scm:138: ;;;; (5) rotate conrol-points around '(0 . 0) to match
`stop'
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …control-points…

Done.

https://codereview.appspot.com/270640043/diff/1/scm/stencil.scm#newcode148
scm/stencil.scm:148: ;;;; (6) move rotated conrol-points to match `start'
On 2015/11/04 00:28:27, simon.albrecht wrote:
> …same here…

Done.

[dak, 2015-11-08 21:22:04]

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode76
scm/stencil.scm:76: (cons 0 0))
Why not '(0 . 0) here?  It does not look like the cons is ever changed, so why a
fresh cons for every use?

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode82
scm/stencil.scm:82: (interval-length (ordered-cons (car start) (car stop))))
you mean (abs (- (car start) (car stop))) here I presume.  The difference of two
x coordinates is not an "interval" and should not be treated as such.

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode85
scm/stencil.scm:85: (interval-length (ordered-cons (cdr start) (cdr stop))))
Same here.

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode88
scm/stencil.scm:88: (sqrt (+ (* width width) (* height height))))
Is there a reason you are actually taking the absolute values of width and
height?  You don't need them for calculating the length here.

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode100
scm/stencil.scm:100: (/ height length-to-print))))
This looks like a total mess.  Do you even need all this for something?  What
are you trying to do?

Perhaps express it in words and we'll try finding a direct representation that
does not need all this juggling with angles and stuff.

[thomasmorley651, 2015-11-08 23:22:51]

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode88
scm/stencil.scm:88: (sqrt (+ (* width width) (* height height))))
On 2015/11/08 21:22:04, dak wrote:
> Is there a reason you are actually taking the absolute values of width and
> height?  You don't need them for calculating the length here.

Not sure what you mean.
Isn't it simple Pythagoras?

https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode100
scm/stencil.scm:100: (/ height length-to-print))))
On 2015/11/08 21:22:04, dak wrote:
> This looks like a total mess.  Do you even need all this for something?  What
> are you trying to do?
> 
> Perhaps express it in words and we'll try finding a direct representation that
> does not need all this juggling with angles and stuff.

You ask for quadrant and angle-start/stop-X-axis, right?

I hoped I had put in enough comments to explain what's done and why, obviously
not sufficient.

In general I have to points P(x1 y1) and Q(x2 y2) and want to create a bow
connecting them.
Therefore I first boil down it, to create a bow from Zero(0 0) to a point on the
(positive-part) of X-axis with with the coordinates (direct-distance-P-Q 0),
because it's far easier to calculate.
After this is done the bow is rotated around Zero to have it the same
orientation like the direct connection of P and Q.
Last step is to move the bow from Zero to P.

For the rotating part I need the value of the angle which depends on the
quadrants of Q relative to P. Calculated using Zero and P moved to 'new-stop'

Hope this is clearer now.

In general I regard the section containing quadrant and angle-start/stop-X-axis
the weakest part of the code.
Any suggestion is highly welcome.

[dak, 2015-11-08 23:54:37]

On 2015/11/08 23:22:51, thomasmorley651 wrote:
> https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm
> File scm/stencil.scm (right):
> 
> https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode88
> scm/stencil.scm:88: (sqrt (+ (* width width) (* height height))))
> On 2015/11/08 21:22:04, dak wrote:
> > Is there a reason you are actually taking the absolute values of width and
> > height?  You don't need them for calculating the length here.
> 
> Not sure what you mean.
> Isn't it simple Pythagoras?
> 
> https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode100
> scm/stencil.scm:100: (/ height length-to-print))))
> On 2015/11/08 21:22:04, dak wrote:
> > This looks like a total mess.  Do you even need all this for something? 
What
> > are you trying to do?
> > 
> > Perhaps express it in words and we'll try finding a direct representation
that
> > does not need all this juggling with angles and stuff.
> 
> You ask for quadrant and angle-start/stop-X-axis, right?
> 
> I hoped I had put in enough comments to explain what's done and why, obviously
> not sufficient.
> 
> In general I have to points P(x1 y1) and Q(x2 y2) and want to create a bow
> connecting them.
> Therefore I first boil down it, to create a bow from Zero(0 0) to a point on
the
> (positive-part) of X-axis with with the coordinates (direct-distance-P-Q 0),
> because it's far easier to calculate.
> After this is done the bow is rotated around Zero to have it the same
> orientation like the direct connection of P and Q.
> Last step is to move the bow from Zero to P.
> 
> For the rotating part I need the value of the angle which depends on the
> quadrants of Q relative to P. Calculated using Zero and P moved to 'new-stop'
> 
> Hope this is clearer now.
> 
> In general I regard the section containing quadrant and
angle-start/stop-X-axis
> the weakest part of the code.
> Any suggestion is highly welcome.

What do you need angles and quadrants for?

Assuming that you create your bow as a bow from (0,0) to (1,0),
you can then transform a point (x0,y0) on this bow to one from P(x1 y1) to Q(x2
y2)
by using (x0*(x2-x1)-y0*(y2-y1)+x1, x0*(y2-y1)+y0*(x2-x1)+y1)
assuming that I did not make a mistake here.  No quadrants, no angles, no
trigonometry,
no square roots.

[thomasmorley651, 2015-11-10 20:30:12]

David's comments and other small rework

[thomasmorley651, 2015-11-10 20:34:23]

On 2015/11/08 23:54:37, dak wrote:
> On 2015/11/08 23:22:51, thomasmorley651 wrote:
> > https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm
> > File scm/stencil.scm (right):
> > 
> >
https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode88
> > scm/stencil.scm:88: (sqrt (+ (* width width) (* height height))))
> > On 2015/11/08 21:22:04, dak wrote:
> > > Is there a reason you are actually taking the absolute values of width and
> > > height?  You don't need them for calculating the length here.
> > 
> > Not sure what you mean.
> > Isn't it simple Pythagoras?

See bottom.

> > 
> >
https://codereview.appspot.com/270640043/diff/20001/scm/stencil.scm#newcode100
> > scm/stencil.scm:100: (/ height length-to-print))))
> > On 2015/11/08 21:22:04, dak wrote:
> > > This looks like a total mess.  Do you even need all this for something? 
> What
> > > are you trying to do?
> > > 
> > > Perhaps express it in words and we'll try finding a direct representation
> that
> > > does not need all this juggling with angles and stuff.
> > 
> > You ask for quadrant and angle-start/stop-X-axis, right?
> > 
> > I hoped I had put in enough comments to explain what's done and why,
obviously
> > not sufficient.
> > 
> > In general I have to points P(x1 y1) and Q(x2 y2) and want to create a bow
> > connecting them.
> > Therefore I first boil down it, to create a bow from Zero(0 0) to a point on
> the
> > (positive-part) of X-axis with with the coordinates (direct-distance-P-Q 0),
> > because it's far easier to calculate.
> > After this is done the bow is rotated around Zero to have it the same
> > orientation like the direct connection of P and Q.
> > Last step is to move the bow from Zero to P.
> > 
> > For the rotating part I need the value of the angle which depends on the
> > quadrants of Q relative to P. Calculated using Zero and P moved to
'new-stop'
> > 
> > Hope this is clearer now.
> > 
> > In general I regard the section containing quadrant and
> angle-start/stop-X-axis
> > the weakest part of the code.
> > Any suggestion is highly welcome.
> 
> What do you need angles and quadrants for?
> 
> Assuming that you create your bow as a bow from (0,0) to (1,0),
> you can then transform a point (x0,y0) on this bow to one from P(x1 y1) to
Q(x2
> y2)
> by using (x0*(x2-x1)-y0*(y2-y1)+x1, x0*(y2-y1)+y0*(x2-x1)+y1)
> assuming that I did not make a mistake here.  No quadrants, no angles, no
> trigonometry,
> no square roots.

New patch follows your suggestions, thanks a lot for it.
One problem remaining, for the length I still use square roots - don't know how
to do it different.

[dak, 2015-11-10 20:52:18]

https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode73
scm/stencil.scm:73: (width (- (car stop) (car start)))
I'd not use `width' (and `height') here but `dx' and `dy': the expectation for
variable names `width' and `height' is that they will generally be non-negative.

https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode81
scm/stencil.scm:81: ;;;;     beginning '(0 . 0), ending at (cons length-to-print
0)
Ah, here's the rub.  Why end at (length-to-print . 0) instead of (1 . 0)? 
Scaling by length at the start instead of after you are done makes things
unnecessarily complex and indeed requires Pythagoras.  If you instead work with
a "unit" bow, scaling and rotating the resulting points to match some actual
line does not require calculating the line length but just the equivalent of a
complex multiplication in cartesian coordinates.

https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode123
scm/stencil.scm:123: length-to-print)
See?  You've effectively multiplied everything with length-to-print at the
start, just to divide by it again here.

Total waste of effort.

[thomasmorley651, 2015-11-11 09:56:16]

On 2015/11/10 20:52:18, dak wrote:
> https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm
> File scm/stencil.scm (right):
> 
> https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode73
> scm/stencil.scm:73: (width (- (car stop) (car start)))
> I'd not use `width' (and `height') here but `dx' and `dy': the expectation for
> variable names `width' and `height' is that they will generally be
non-negative.
> 
> https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode81
> scm/stencil.scm:81: ;;;;     beginning '(0 . 0), ending at (cons
length-to-print
> 0)
> Ah, here's the rub.  Why end at (length-to-print . 0) instead of (1 . 0)? 
> Scaling by length at the start instead of after you are done makes things
> unnecessarily complex and indeed requires Pythagoras.  If you instead work
with
> a "unit" bow, scaling and rotating the resulting points to match some actual
> line does not require calculating the line length but just the equivalent of a
> complex multiplication in cartesian coordinates.
> 
> https://codereview.appspot.com/270640043/diff/40001/scm/stencil.scm#newcode123
> scm/stencil.scm:123: length-to-print)
> See?  You've effectively multiplied everything with length-to-print at the
> start, just to divide by it again here.
> 
> Total waste of effort.

Well, outer/inner-control rely on the user-settable variables bow-height and
thickness.
Intended is that the user may set/modify them depending on the final bow, not
the unit-bow.
Changing bow-height/thickness of the unit-bow only 0.01 will have a more heavy
impact for the
final bow than most users will expect and would be inconsistent with setting
thickness for Ties, etc.

I could hide this from the user by doing some division maybe with 10, although
the appropriate value 
would be `length-to-print', which would make you not happy again... lol

I'll think about it, though, any suggestion is highly welcome as well!

[dak, 2015-11-12 15:20:51]

On 2015/11/11 09:56:16, thomasmorley651 wrote:

> Well, outer/inner-control rely on the user-settable variables bow-height and
> thickness.

Ok, didn't notice that.  I'm not sure it's the best interface to have those in
semi-absolute dimensions but at least with thickness it would be sort-of
consistent with other uses.

I'd still consider it cleanest to create the basic shape in unit size (meaning
that one would scale down those dimensions by length-to-print).  Everything else
being equal, it would be nicest to write stuff in a manner where one does not
need to divide by length-to-print (or some similar dimension) at all so that
stuff does not get instable when the size happens to be zero.  But I don't see
that this can be achieved when we have something like a non-zero bow-height
since it requires a direction to put the non-zero-wide bow in, and (dx, dy) =
(0, 0) is not useful for that.

[thomasmorley651, 2015-11-13 21:23:34]

clean up/rework based on latest discussion

[thomasmorley651, 2015-11-13 21:31:31]

On 2015/11/12 15:20:51, dak wrote:
> On 2015/11/11 09:56:16, thomasmorley651 wrote:
> 
> > Well, outer/inner-control rely on the user-settable variables bow-height and
> > thickness.
> 
> Ok, didn't notice that.  I'm not sure it's the best interface to have those in
> semi-absolute dimensions but at least with thickness it would be sort-of
> consistent with other uses.
> 
> I'd still consider it cleanest to create the basic shape in unit size (meaning
> that one would scale down those dimensions by length-to-print).  

Done.

> Everything else
> being equal, it would be nicest to write stuff in a manner where one does not
> need to divide by length-to-print (or some similar dimension) at all so that
> stuff does not get instable when the size happens to be zero.  But I don't see
> that this can be achieved when we have something like a non-zero bow-height
> since it requires a direction to put the non-zero-wide bow in, and (dx, dy) =
> (0, 0) is not useful for that.

length-to-print is only zero if start/stop are equal. In this case an empty
stencil is returned. 
Ofcourse a bezier-curve starting and ending at the same point is thinkable,
though, I don't think we have any use for this.



One additional point: this patch also provides make-tie-stencil, which may
deserve a closer look as well.
For now I mostly c/p Jan's code here.

[dak, 2015-11-13 21:51:19]

Sorry for yet finding more stuff that, after all, could likely be improved.  At
least I did so pretty quickly this time.

https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
scm/define-markup-commands.scm:623: (direction DOWN)
Should this markup command be called "undertie" or should it rather be "tie",
with "undertie" explicitly overriding `direction'?

Because the rather explicit name '\undertie' seems a bit inconsistent with the
behavior of

{
  c'1^\markup \undertie hm
}

where the direction is determined by the direction specified for the TextScript.

https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode86
scm/stencil.scm:86: (interval-index '(0 . 1) offset-index))
This is just (+ 0.1 (* 0.3 angularity)) I think and right-control would then be
(- 1 left-control).

I think that with the points being based on (0 . 1), the combined
interval-index/offset-index calculation (which never was hotness itself) is not
exactly making things clearer.

[thomasmorley651, 2015-11-13 22:38:02]

On 2015/11/13 21:51:19, dak wrote:
> Sorry for yet finding more stuff that, after all, could likely be improved. 
At
> least I did so pretty quickly this time.

No problem!
The code became so much better during revision!
I have to work too much in my regular job to upload more than one patch-set per
working-day. Though, I made the experience looking at the code after a day or so
is sometimes very helpful.
Thanks a lot for your review.

> 
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> File scm/define-markup-commands.scm (right):
> 
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> scm/define-markup-commands.scm:623: (direction DOWN)
> Should this markup command be called "undertie" or should it rather be "tie",
> with "undertie" explicitly overriding `direction'?
> 
> Because the rather explicit name '\undertie' seems a bit inconsistent with the
> behavior of
> 
> {
>   c'1^\markup \undertie hm
> }
> 
> where the direction is determined by the direction specified for the
TextScript.

Hm, this was _not_ intended.
In this case a Tie _below_ the arg should be printed. For ties above the arg
\overtie was defined.

Though, thinking (a little) about it, we could go for
a)
two commands (under- and overtie) independant from direction-modifiers
or
b)
one tie-markup-command letting the user specify direction via \override
(direction . ...) and/or direction-modifiers

I'm undecided for now, opinions?

> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
> File scm/stencil.scm (right):
> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode86
> scm/stencil.scm:86: (interval-index '(0 . 1) offset-index))
> This is just (+ 0.1 (* 0.3 angularity)) I think and right-control would then
be
> (- 1 left-control).

great catch

> 
> I think that with the points being based on (0 . 1), the combined
> interval-index/offset-index calculation (which never was hotness itself) is
not
> exactly making things clearer.

[dak, 2015-11-13 23:01:39]

Aaand another one.

https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode66
scm/stencil.scm:66: (if (equal? start stop)
I think that's too optimistic.  If you have
start = (0 . 0)
and
stop = (1e-200 . 1e-200)
then your length calculation will still throw out 0.  I'd recommend doing this
"foolproof" by starting with the length calculation and going for the empty
stencil exactly when the length is zero (since exactly then dividing by it is a
bad idea).

That would make this foolproof.  Yes, the additional indentation level is a
nuisance.

One can additionally increase precision by letting
(length-to-print (magnitude (make-rectangular dx dy)))
which turns out to still work when dx and dy are 1e-200 and similar (meaning
that their square would underflow precision at least on my computer).

[thomasmorley651, 2015-11-13 23:13:21]

On 2015/11/13 22:38:02, thomasmorley651 wrote:
> On 2015/11/13 21:51:19, dak wrote:
> > Sorry for yet finding more stuff that, after all, could likely be improved. 
> At
> > least I did so pretty quickly this time.
> 
> No problem!
> The code became so much better during revision!
> I have to work too much in my regular job to upload more than one patch-set
per
> working-day. Though, I made the experience looking at the code after a day or
so
> is sometimes very helpful.
> Thanks a lot for your review.
> 
> > 
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > File scm/define-markup-commands.scm (right):
> > 
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > scm/define-markup-commands.scm:623: (direction DOWN)
> > Should this markup command be called "undertie" or should it rather be
"tie",
> > with "undertie" explicitly overriding `direction'?
> > 
> > Because the rather explicit name '\undertie' seems a bit inconsistent with
the
> > behavior of
> > 
> > {
> >   c'1^\markup \undertie hm
> > }
> > 
> > where the direction is determined by the direction specified for the
> TextScript.
> 
> Hm, this was _not_ intended.
> In this case a Tie _below_ the arg should be printed. For ties above the arg
> \overtie was defined.
> 
> Though, thinking (a little) about it, we could go for
> a)
> two commands (under- and overtie) independant from direction-modifiers
> or
> b)
> one tie-markup-command letting the user specify direction via \override
> (direction . ...) and/or direction-modifiers

c)
a tie-command, with under/overtie derived from it.
I think that was your proposal.

> 
> I'm undecided for now, opinions?
> 
> > 
> > https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
> > File scm/stencil.scm (right):
> > 
> >
https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode86
> > scm/stencil.scm:86: (interval-index '(0 . 1) offset-index))
> > This is just (+ 0.1 (* 0.3 angularity)) I think and right-control would then
> be
> > (- 1 left-control).
> 
> great catch
> 
> > 
> > I think that with the points being based on (0 . 1), the combined
> > interval-index/offset-index calculation (which never was hotness itself) is
> not
> > exactly making things clearer.

[thomasmorley651, 2015-11-13 23:15:22]

On 2015/11/13 23:01:39, dak wrote:
> Aaand another one.
> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
> File scm/stencil.scm (right):
> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode66
> scm/stencil.scm:66: (if (equal? start stop)
> I think that's too optimistic.  If you have
> start = (0 . 0)
> and
> stop = (1e-200 . 1e-200)
> then your length calculation will still throw out 0.  

Good catch, again

> I'd recommend doing this
> "foolproof" by starting with the length calculation and going for the empty
> stencil exactly when the length is zero (since exactly then dividing by it is
a
> bad idea).
> 
> That would make this foolproof.  Yes, the additional indentation level is a
> nuisance.
> 
> One can additionally increase precision by letting
> (length-to-print (magnitude (make-rectangular dx dy)))
> which turns out to still work when dx and dy are 1e-200 and similar (meaning
> that their square would underflow precision at least on my computer).

Will do.

[Trevor Daniels, 2015-11-13 23:37:12]

>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > > scm/define-markup-commands.scm:623: (direction DOWN)
> > > Should this markup command be called "undertie" or should it rather be
> "tie",
> > > with "undertie" explicitly overriding `direction'?
> > > 
> > > Because the rather explicit name '\undertie' seems a bit inconsistent with
> the
> > > behavior of
> > > 
> > > {
> > >   c'1^\markup \undertie hm
> > > }
> > > 
> > > where the direction is determined by the direction specified for the
> > TextScript.
> > 
> > Hm, this was _not_ intended.
> > In this case a Tie _below_ the arg should be printed. For ties above the arg
> > \overtie was defined.
> > 
> > Though, thinking (a little) about it, we could go for
> > a)
> > two commands (under- and overtie) independant from direction-modifiers
> > or
> > b)
> > one tie-markup-command letting the user specify direction via \override
> > (direction . ...) and/or direction-modifiers
> 
> c)
> a tie-command, with under/overtie derived from it.
> I think that was your proposal.
> 
> > 
> > I'm undecided for now, opinions?

In the example above: 
    c'1^\markup \undertie hm
the direction modifier should indicate the placement of the markup relative to
the staff, surely.  The placement of the tie relative to the text needs to be
specified independently of the direction modifier, so \undertie and \overtie are
needed for this.  I don't like \tie; \undertie is consistent with \underline. 
(OK, so we don't have \overline, but we could.)

[thomasmorley651, 2015-11-14 21:22:25]

make under/overtie robust against direction-modifiers, better foolproof for
bow-stencil, more clean-up

[thomasmorley651, 2015-11-14 21:25:11]

On 2015/11/13 21:51:19, dak wrote:
> Sorry for yet finding more stuff that, after all, could likely be improved. 
At
> least I did so pretty quickly this time.
> 
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> File scm/define-markup-commands.scm (right):
> 
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> scm/define-markup-commands.scm:623: (direction DOWN)
> Should this markup command be called "undertie" or should it rather be "tie",
> with "undertie" explicitly overriding `direction'?
> 
> Because the rather explicit name '\undertie' seems a bit inconsistent with the
> behavior of
> 
> {
>   c'1^\markup \undertie hm
> }
> 
> where the direction is determined by the direction specified for the
TextScript.

Changed property direction to text-direction, which is in use already.
Now under/overtie should be robust against direction-modifiers

> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
> File scm/stencil.scm (right):
> 
> https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode86
> scm/stencil.scm:86: (interval-index '(0 . 1) offset-index))
> This is just (+ 0.1 (* 0.3 angularity)) I think and right-control would then
be
> (- 1 left-control).

Done.

[thomasmorley651, 2015-11-14 21:25:42]

On 2015/11/13 23:15:22, thomasmorley651 wrote:
> On 2015/11/13 23:01:39, dak wrote:
> > Aaand another one.
> > 
> > https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm
> > File scm/stencil.scm (right):
> > 
> >
https://codereview.appspot.com/270640043/diff/60001/scm/stencil.scm#newcode66
> > scm/stencil.scm:66: (if (equal? start stop)
> > I think that's too optimistic.  If you have
> > start = (0 . 0)
> > and
> > stop = (1e-200 . 1e-200)
> > then your length calculation will still throw out 0.  
> 
> Good catch, again
> 
> > I'd recommend doing this
> > "foolproof" by starting with the length calculation and going for the empty
> > stencil exactly when the length is zero (since exactly then dividing by it
is
> a
> > bad idea).
> > 
> > That would make this foolproof.  Yes, the additional indentation level is a
> > nuisance.
> > 
> > One can additionally increase precision by letting
> > (length-to-print (magnitude (make-rectangular dx dy)))
> > which turns out to still work when dx and dy are 1e-200 and similar (meaning
> > that their square would underflow precision at least on my computer).
> 
> Will do.

Done.

[dak, 2015-11-16 15:57:49]

On 2015/11/14 21:25:11, thomasmorley651 wrote:
> On 2015/11/13 21:51:19, dak wrote:
> > Sorry for yet finding more stuff that, after all, could likely be improved. 
> At
> > least I did so pretty quickly this time.
> > 
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > File scm/define-markup-commands.scm (right):
> > 
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > scm/define-markup-commands.scm:623: (direction DOWN)
> > Should this markup command be called "undertie" or should it rather be
"tie",
> > with "undertie" explicitly overriding `direction'?
> > 
> > Because the rather explicit name '\undertie' seems a bit inconsistent with
the
> > behavior of
> > 
> > {
> >   c'1^\markup \undertie hm
> > }
> > 
> > where the direction is determined by the direction specified for the
> TextScript.
> 
> Changed property direction to text-direction, which is in use already.

Uh, that's for left/right direction changes.  I'd strongly recommend against
that.  If we are sure we'll never need to change the direction based on the
markup direction (like, say, over/under actual note glyphs?), I'd just implement
commands undertie/overtie that call some internal function with the direction to
use directly.

If there may be a use case for inheriting direction, some directable markup
command may be useful.  Either it is called (with an appropriate explicit
direction override) by undertie/overtie, or all three call the same internal
function.

[thomasmorley651, 2015-11-18 22:56:22]

let over/undertie call an internal procedure

[thomasmorley651, 2015-11-18 23:01:20]

On 2015/11/16 15:57:49, dak wrote:
> On 2015/11/14 21:25:11, thomasmorley651 wrote:
> > On 2015/11/13 21:51:19, dak wrote:
> > > Sorry for yet finding more stuff that, after all, could likely be
improved. 
> > At
> > > least I did so pretty quickly this time.
> > > 
> > >
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > > File scm/define-markup-commands.scm (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/270640043/diff/60001/scm/define-markup-command...
> > > scm/define-markup-commands.scm:623: (direction DOWN)
> > > Should this markup command be called "undertie" or should it rather be
> "tie",
> > > with "undertie" explicitly overriding `direction'?
> > > 
> > > Because the rather explicit name '\undertie' seems a bit inconsistent with
> the
> > > behavior of
> > > 
> > > {
> > >   c'1^\markup \undertie hm
> > > }
> > > 
> > > where the direction is determined by the direction specified for the
> > TextScript.
> > 
> > Changed property direction to text-direction, which is in use already.
> 
> Uh, that's for left/right direction changes.  I'd strongly recommend against
> that.  If we are sure we'll never need to change the direction based on the
> markup direction (like, say, over/under actual note glyphs?), I'd just
implement
> commands undertie/overtie that call some internal function with the direction
to
> use directly.

Done this way. Right now I can't imagine any use-case for relying on
direction-modifiers.
If there is use for it sometime in the future we may change this again,
ofcourse.

> 
> If there may be a use case for inheriting direction, some directable markup
> command may be useful.  Either it is called (with an appropriate explicit
> direction override) by undertie/overtie, or all three call the same internal
> function.

[Trevor Daniels, 2015-11-19 15:46:07]

LGTM, apart from an oversight, although this is based on just eye-balling.

https://codereview.appspot.com/270640043/diff/100001/scm/define-markup-comman...
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/270640043/diff/100001/scm/define-markup-comman...
scm/define-markup-commands.scm:623: (text-direction UP)
I don't think text-direction is used now.

https://codereview.appspot.com/270640043/diff/100001/scm/define-markup-comman...
scm/define-markup-commands.scm:654: (text-direction UP)
ditto

[thomasmorley651, 2015-11-19 21:24:15]

let under- and overtie rely on the generic tie-markup-command

[thomasmorley651, 2015-11-19 21:29:41]

On 2015/11/18 23:01:20, thomasmorley651 wrote:
> On 2015/11/16 15:57:49, dak wrote:

> > If we are sure we'll never need to change the direction based on the
> > markup direction (like, say, over/under actual note glyphs?), I'd just
> implement
> > commands undertie/overtie that call some internal function with the
direction
> to
> > use directly.
> 
> Done this way. Right now I can't imagine any use-case for relying on
> direction-modifiers.
> If there is use for it sometime in the future we may change this again,
> ofcourse.
> 
> > 
> > If there may be a use case for inheriting direction, some directable markup
> > command may be useful.  Either it is called (with an appropriate explicit
> > direction override) by undertie/overtie, or all three call the same internal
> > function.

Changed my mind, because I found a not uncommon use-case for a tie-command
relying on direction-modifiers and \voiceXxx:

m = {
  c''1 \prall -\tweak text \markup \tie "131" -1
}

{ \voiceOne \m \voiceTwo \m }

This is used in scores for guitar quite often and I seem to remember having seen
similiar in piano-scores indicating silent finger changes.

@James
sorry for the additional work

[thomasmorley651, 2015-11-19 21:36:51]

On 2015/11/19 21:29:41, thomasmorley651 wrote:
> On 2015/11/18 23:01:20, thomasmorley651 wrote:
> > On 2015/11/16 15:57:49, dak wrote:
> 
> > > If we are sure we'll never need to change the direction based on the
> > > markup direction (like, say, over/under actual note glyphs?), I'd just
> > implement
> > > commands undertie/overtie that call some internal function with the
> direction
> > to
> > > use directly.
> > 
> > Done this way. Right now I can't imagine any use-case for relying on
> > direction-modifiers.
> > If there is use for it sometime in the future we may change this again,
> > ofcourse.
> > 
> > > 
> > > If there may be a use case for inheriting direction, some directable
markup
> > > command may be useful.  Either it is called (with an appropriate explicit
> > > direction override) by undertie/overtie, or all three call the same
internal
> > > function.
> 
> Changed my mind, because I found a not uncommon use-case for a tie-command
> relying on direction-modifiers and \voiceXxx:
> 
> m = {
>   c''1 \prall -\tweak text \markup \tie "131" -1
> }
> 
> { \voiceOne \m \voiceTwo \m }

see png on the tracker
https://sourceforge.net/p/testlilyissues/issues/3088/?page=2

> 
> This is used in scores for guitar quite often and I seem to remember having
seen
> similiar in piano-scores indicating silent finger changes.
> 
> @James
> sorry for the additional work