tablature: ties and harmonics (issue1669041)

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

tablature: ties and harmonics (issue1669041)

Neil Puttock





http://codereview.appspot.com/1669041/diff/1/2
File input/regression/tablature-tie-harmonic.ly (right):

http://codereview.appspot.com/1669041/diff/1/2#newcode3
input/regression/tablature-tie-harmonic.ly:3: \header{ texidoc =
"Harmonics in tablature follow the rules for
\header {
   texidoc = "Harmonics in tablature follow the rules for
fret numbers in tie situations."
}

http://codereview.appspot.com/1669041/diff/1/3
File ly/engraver-init.ly (right):

http://codereview.appspot.com/1669041/diff/1/3#newcode748
ly/engraver-init.ly:748: %% the HarmonicParenthesesItem engraver has to
listen to the
HarmonicParenthesesItem grob

http://codereview.appspot.com/1669041/diff/1/3#newcode750
ly/engraver-init.ly:750: \override HarmonicParenthesesItem
#'after-line-breaking =
#harmonic-parentheses-item::apply-tab-note-head-visibility
It'd be simpler to use 'transparent (see my comments concerning
'whiteout vs 'transparent below)

http://codereview.appspot.com/1669041/diff/1/4
File scm/define-grobs.scm (right):

http://codereview.appspot.com/1669041/diff/1/4#newcode871
scm/define-grobs.scm:871: (angularity . 1.5)
These (angularity, white-padding & width) need documenting in
define-grob-properties.scm and adding to an interface in
define-grob-interfaces.scm (either a new interface or
parentheses-interface.)

http://codereview.appspot.com/1669041/diff/1/4#newcode872
scm/define-grobs.scm:872: (half-thickness . 0.05)
This should come from line-thickness.

http://codereview.appspot.com/1669041/diff/1/4#newcode878
scm/define-grobs.scm:878: (width . 0.5)
could be more specific

http://codereview.appspot.com/1669041/diff/1/5
File scm/output-lib.scm (right):

http://codereview.appspot.com/1669041/diff/1/5#newcode505
scm/output-lib.scm:505: (let* ((lp (draw-angled-bracket-stencil LEFT
grob))
indent

http://codereview.appspot.com/1669041/diff/1/6
File scm/tablature.scm (right):

http://codereview.appspot.com/1669041/diff/1/6#newcode146
scm/tablature.scm:146: (let* ((articulations (ly:event-property
(event-cause grob) 'articulations))
let

http://codereview.appspot.com/1669041/diff/1/6#newcode150
scm/tablature.scm:150: (if (eq? 'harmonic-event (ly:event-property art
'class))
(ly:in-event-class? art 'harmonic-event)

http://codereview.appspot.com/1669041/diff/1/6#newcode157
scm/tablature.scm:157: (half-thickness 0.05) ;; urg, hard-coded values
Hmm, tricky.  It'll be difficult (rather convoluted) to get from
TabNoteHead -> HarmonicParenthesesItem.

(not half-thickness though, since this should come from line-thickness
and ly:output-def-lookup)

http://codereview.appspot.com/1669041/diff/1/6#newcode161
scm/tablature.scm:161: (dirp (ly:stencil-aligned-to
could do with a more descriptive name

http://codereview.appspot.com/1669041/diff/1/6#newcode291
scm/tablature.scm:291: (display "\ntransparent: ")(display
tab-note-head-transparent)
remove debug code

http://codereview.appspot.com/1669041/diff/1/6#newcode293
scm/tablature.scm:293: ;; we simply remove the stencil, so we don't have
to care about 'whiteout
I've thought about this for a while, and it's silly that 'whiteout and
'transparent aren't mutually exclusive: if something's transparent, its
whiteout shouldn't be visible.

(see grob.cc)

http://codereview.appspot.com/1669041/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
[hidden email] schrieb:

>
>
>
>
>
> http://codereview.appspot.com/1669041/diff/1/2
> File input/regression/tablature-tie-harmonic.ly (right):
>
> http://codereview.appspot.com/1669041/diff/1/2#newcode3
> input/regression/tablature-tie-harmonic.ly:3: \header{ texidoc =
> "Harmonics in tablature follow the rules for
> \header {
>   texidoc = "Harmonics in tablature follow the rules for
> fret numbers in tie situations."
> }
Done.

>
> http://codereview.appspot.com/1669041/diff/1/3
> File ly/engraver-init.ly (right):
>
> http://codereview.appspot.com/1669041/diff/1/3#newcode748
> ly/engraver-init.ly:748: %% the HarmonicParenthesesItem engraver has to
> listen to the
> HarmonicParenthesesItem grob
>
> http://codereview.appspot.com/1669041/diff/1/3#newcode750
> ly/engraver-init.ly:750: \override HarmonicParenthesesItem
> #'after-line-breaking =
> #harmonic-parentheses-item::apply-tab-note-head-visibility
> It'd be simpler to use 'transparent (see my comments concerning
> 'whiteout vs 'transparent below)
Ok, done. But at the moment, 'whiteout and 'transparent can be
used independently - if you compile the regression file, you see
white spots. What is the right way to solve this?

>
> http://codereview.appspot.com/1669041/diff/1/4
> File scm/define-grobs.scm (right):
>
> http://codereview.appspot.com/1669041/diff/1/4#newcode871
> scm/define-grobs.scm:871: (angularity . 1.5)
> These (angularity, white-padding & width) need documenting in
> define-grob-properties.scm and adding to an interface in
> define-grob-interfaces.scm (either a new interface or
> parentheses-interface.)
Ok, done.
>
> http://codereview.appspot.com/1669041/diff/1/4#newcode872
> scm/define-grobs.scm:872: (half-thickness . 0.05)
> This should come from line-thickness.
Done.
>
> http://codereview.appspot.com/1669041/diff/1/4#newcode878
> scm/define-grobs.scm:878: (width . 0.5)
> could be more specific
I replaced it by "bracket-width" globally.
>
> http://codereview.appspot.com/1669041/diff/1/5
> File scm/output-lib.scm (right):
>
> http://codereview.appspot.com/1669041/diff/1/5#newcode505
> scm/output-lib.scm:505: (let* ((lp (draw-angled-bracket-stencil LEFT
> grob))
> indent
Sorry, done. (I moved the code into the helper function and didn't check
the indentation)
>
> http://codereview.appspot.com/1669041/diff/1/6
> File scm/tablature.scm (right):
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode146
> scm/tablature.scm:146: (let* ((articulations (ly:event-property
> (event-cause grob) 'articulations))
> let
Yes, of course.
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode150
> scm/tablature.scm:150: (if (eq? 'harmonic-event (ly:event-property art
> 'class))
> (ly:in-event-class? art 'harmonic-event)
Done. Is this function documented somewhere? I didn't find it.
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode157
> scm/tablature.scm:157: (half-thickness 0.05) ;; urg, hard-coded values
> Hmm, tricky.  It'll be difficult (rather convoluted) to get from
> TabNoteHead -> HarmonicParenthesesItem.
I don't know whether it is worth the effort (probably no-one wil
touch the harmonic angle brackets), but in general it is not
very elegant to store the values in one place while not being able
to use them globally.
>
> (not half-thickness though, since this should come from line-thickness
> and ly:output-def-lookup)
Done.
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode161
> scm/tablature.scm:161: (dirp (ly:stencil-aligned-to
> could do with a more descriptive name
Done.
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode291
> scm/tablature.scm:291: (display "\ntransparent: ")(display
> tab-note-head-transparent)
> remove debug code
Done.
>
> http://codereview.appspot.com/1669041/diff/1/6#newcode293
> scm/tablature.scm:293: ;; we simply remove the stencil, so we don't have
> to care about 'whiteout
> I've thought about this for a while, and it's silly that 'whiteout and
> 'transparent aren't mutually exclusive: if something's transparent, its
> whiteout shouldn't be visible.
>
> (see grob.cc)
You are right - can this behavior be changed in grob.cc to be valid
for all grobs in lilypond? If yes, how?

Thank you

Marc

http://codereview.appspot.com/1669041/show


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
Marc Hohl schrieb:

> [hidden email] schrieb:
> [...]
>>
>> http://codereview.appspot.com/1669041/diff/1/6#newcode293
>> scm/tablature.scm:293: ;; we simply remove the stencil, so we don't have
>> to care about 'whiteout
>> I've thought about this for a while, and it's silly that 'whiteout and
>> 'transparent aren't mutually exclusive: if something's transparent, its
>> whiteout shouldn't be visible.
>>
>> (see grob.cc)
> You are right - can this behavior be changed in grob.cc to be valid
> for all grobs in lilypond? If yes, how?
Sorry, silly question. I have uploaded a separate patch
(http://codereview.appspot.com/1681043); if it is accepted, I will continue
on this issue and simplify parts of scm/tablature.scm.

Marc

>
> http://codereview.appspot.com/1669041/show
>


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
Marc Hohl schrieb:
> Marc Hohl schrieb:
> [...]
> if it is accepted, I will continue
> on this issue and simplify parts of scm/tablature.scm.
>
Ok, done.

Now I *must* overlook the obvious, because the fret numbers still disappear,
even if they are not tied together.

Please have a look at this simple input file:

---

\version "2.13.25"

test = \relative c {
  < c\harmonic >2  < c\harmonic >
}

\context StaffGroup <<
  \context Staff {
     \clef "G_8"
     \test
  }
  \context TabStaff {
  \test
  }
 >>

---

The fret number will be invisible, and since no tie function in
scm/tablature.scm
is called during this compilation, I think the error must be buried
somewhere in
scm/output-lib.scm, but during the tests for the changes in this file,
everything
worked correctly ...

Marc

http://codereview.appspot.com/1669041/show



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Neil Puttock
In reply to this post by Neil Puttock
On 2010/06/21 19:06:52, marc wrote:

> Now I *must* overlook the obvious, because the fret numbers still
disappear,
> even if they are not tied together.

Remember what I said about 'layer?

Both TabNoteHead and HarmonicParenthesesItem have the same default layer
(1), but the latter is likely to be printed over the notehead since its
engraver depends on acknowledging a notehead first.

I think you'll have to set TabNoteHead's layer to 2 to ensure it's
always printed over the harmonic brackets.

Cheers,
Neil

http://codereview.appspot.com/1669041/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
[hidden email] schrieb:
> On 2010/06/21 19:06:52, marc wrote:
>
>> Now I *must* overlook the obvious, because the fret numbers still
> disappear,
>> even if they are not tied together.
>
> Remember what I said about 'layer?
Yes, of course, but since the HarmonicParenthesesItem *did* work before
without coping with layers and I only restructured the stencil building
process, I don't understand why this behavior has changed.

I just created a separate branch, loaded the untouched scm/output-lib.scm
in my editor, replaced the old code with the new (added the property values
by hand, because they are not reachable in this branch), and the tab number
appears. So these changes are obviously not the reason for the tab number
to dissappear.
>
> Both TabNoteHead and HarmonicParenthesesItem have the same default layer
> (1), but the latter is likely to be printed over the notehead since its
> engraver depends on acknowledging a notehead first.
I understand this, but why did it work before then?
>
> I think you'll have to set TabNoteHead's layer to 2 to ensure it's
> always printed over the harmonic brackets.
Ok, done.

Thanks,

Marc

http://codereview.appspot.com/1669041/show



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Carl.D.Sorensen
In reply to this post by Neil Puttock
Looks pretty good.  I have a few comments about things that seem to be
unnecessarily specific to harmonics.

Thanks,

Carl



http://codereview.appspot.com/1669041/diff/26001/27003
File scm/define-grob-interfaces.scm (right):

http://codereview.appspot.com/1669041/diff/26001/27003#newcode91
scm/define-grob-interfaces.scm:91: 'harmonic-parentheses-interface
Calling this "parenthesis-interface" would allow its use for other
applications of parentheses and would be a good idea, in my opinion.

http://codereview.appspot.com/1669041/diff/26001/27004
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/1669041/diff/26001/27004#newcode56
scm/define-grob-properties.scm:56: (angularity ,number? "Angularity of a
bracket.")
"angle bracket" instead of "bracket"?

http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
the harmonic angle bracket.")
Why do we need bracket-width?  Why can't we just use the pre-existing
width property?

http://codereview.appspot.com/1669041/diff/26001/27004#newcode850
scm/define-grob-properties.scm:850: (white-padding ,number? "Amount of
padding surrounding a harmonic
Why make this specific for harmonics?  Why not just "Amount of white
space boundary in a whiteout" or something similar?

http://codereview.appspot.com/1669041/diff/26001/27007
File scm/tablature.scm (right):

http://codereview.appspot.com/1669041/diff/26001/27007#newcode155
scm/tablature.scm:155: (define (draw-harmonic-stencil dir grob)
Why not use parenthesize-stencil (see scm/stencil.scm)?  This seems like
duplicated code, and we should probably avoid that if possible.

http://codereview.appspot.com/1669041/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
[hidden email] schrieb:

> Looks pretty good.  I have a few comments about things that seem to be
> unnecessarily specific to harmonics.
>
> Thanks,
>
> Carl
>
>
>
> http://codereview.appspot.com/1669041/diff/26001/27003
> File scm/define-grob-interfaces.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27003#newcode91
> scm/define-grob-interfaces.scm:91: 'harmonic-parentheses-interface
> Calling this "parenthesis-interface" would allow its use for other
> applications of parentheses and would be a good idea, in my opinion.
Ok, done.
>
> http://codereview.appspot.com/1669041/diff/26001/27004
> File scm/define-grob-properties.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode56
> scm/define-grob-properties.scm:56: (angularity ,number? "Angularity of a
> bracket.")
> "angle bracket" instead of "bracket"?
Done.
>
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
> the harmonic angle bracket.")
> Why do we need bracket-width?  Why can't we just use the pre-existing
> width property?
Hm, Neil proposed to use a more descriptive name for this property, IIUC.
Done.
>
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode850
> scm/define-grob-properties.scm:850: (white-padding ,number? "Amount of
> padding surrounding a harmonic
> Why make this specific for harmonics?  Why not just "Amount of white
> space boundary in a whiteout" or something similar?
Done.
>
> http://codereview.appspot.com/1669041/diff/26001/27007
> File scm/tablature.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27007#newcode155
> scm/tablature.scm:155: (define (draw-harmonic-stencil dir grob)
> Why not use parenthesize-stencil (see scm/stencil.scm)?  This seems like
> duplicated code, and we should probably avoid that if possible.
>
I just copied the code from scm/output-lib.scm similar to the parenthesizing
routine within scm/tablature.scm. I thought I could call the newly
introduced
helper routine itself, but this is not possible due to the limitation
that I can't
(at least easily?) get the current property values of the
HarmonicParenthesesItem
grob within the tie callback.
parenthesize-stencil (in scm,/stencil.scm) does not take the whiteout
into account,
and at the moment, the whiteout handling is not done properly throughout
scm/tablature.scm. Sometimes it is hardcoded, sometimes it takes the
'whiteout property into account. I think the brackets and parentheses
should follow the TabNoteHead #'whiteout more consequently in this respect.


I'll shut down my computer for now - today he seems to hate me ;-)
I had to redo my patch sets and did a partial revert of a former patch,
no idea why this happened ...

http://codereview.appspot.com/1669041/show


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Carl Sorensen
On 6/27/10 3:19 AM, "Marc Hohl" <[hidden email]> wrote:

> [hidden email] schrieb:
>> http://codereview.appspot.com/1669041/diff/26001/27003
>> File scm/define-grob-interfaces.scm (right):
>>
>> http://codereview.appspot.com/1669041/diff/26001/27003#newcode91
>> scm/define-grob-interfaces.scm:91: 'harmonic-parentheses-interface
>> Calling this "parenthesis-interface" would allow its use for other
>> applications of parentheses and would be a good idea, in my opinion.
> Ok, done.
>>
>> http://codereview.appspot.com/1669041/diff/26001/27004
>> File scm/define-grob-properties.scm (right):
>>
>> http://codereview.appspot.com/1669041/diff/26001/27004#newcode56
>> scm/define-grob-properties.scm:56: (angularity ,number? "Angularity of a
>> bracket.")
>> "angle bracket" instead of "bracket"?
> Done.
>>
>> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
>> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
>> the harmonic angle bracket.")
>> Why do we need bracket-width?  Why can't we just use the pre-existing
>> width property?
> Hm, Neil proposed to use a more descriptive name for this property, IIUC.
> Done.

Well, if we want to have this be part of the parenthesis-interface, then we
may want to go ahead with bracket-width.

>>
>> http://codereview.appspot.com/1669041/diff/26001/27007#newcode155
>> scm/tablature.scm:155: (define (draw-harmonic-stencil dir grob)
>> Why not use parenthesize-stencil (see scm/stencil.scm)?  This seems like
>> duplicated code, and we should probably avoid that if possible.
>>
> I just copied the code from scm/output-lib.scm similar to the parenthesizing
> routine within scm/tablature.scm. I thought I could call the newly
> introduced
> helper routine itself, but this is not possible due to the limitation
> that I can't
> (at least easily?) get the current property values of the
> HarmonicParenthesesItem
> grob within the tie callback.
> parenthesize-stencil (in scm,/stencil.scm) does not take the whiteout
> into account,
> and at the moment, the whiteout handling is not done properly throughout
> scm/tablature.scm. Sometimes it is hardcoded, sometimes it takes the
> 'whiteout property into account. I think the brackets and parentheses
> should follow the TabNoteHead #'whiteout more consequently in this respect.

If instead of making the whiteout and the stencil different stencils, we
combined them, then parenthesize-stencil would work great.  But this might
not be possible if we need to have the stencil and the whiteout on separate
layers.  As an alternative, parenthesize-stencil could be modified to take
white-padding as an argument.  There's nothing that says we can't modify
scm/stencil.scm so that it works better for tablature.  It's much better,
IMO, to have one general-purpose piece of code than to have two separate
special-purpose code chunks.

Thanks,

Carl


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
Carl Sorensen schrieb:

> [...]
>>> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
>>> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
>>> the harmonic angle bracket.")
>>> Why do we need bracket-width?  Why can't we just use the pre-existing
>>> width property?
>>>      
>> Hm, Neil proposed to use a more descriptive name for this property, IIUC.
>> Done.
>>    
>
> Well, if we want to have this be part of the parenthesis-interface, then we
> may want to go ahead with bracket-width.
>  
Ok, done.

>  
> [...]
> If instead of making the whiteout and the stencil different stencils, we
> combined them, then parenthesize-stencil would work great.  But this might
> not be possible if we need to have the stencil and the whiteout on separate
> layers.  As an alternative, parenthesize-stencil could be modified to take
> white-padding as an argument.  There's nothing that says we can't modify
> scm/stencil.scm so that it works better for tablature.  It's much better,
> IMO, to have one general-purpose piece of code than to have two separate
> special-purpose code chunks.
>  
So I tried to extend the parenthesize-stencil function in scm/stencil.scm.
It compiles without error, but the regression file is cluttered up.

I feel that this is not the right approach, either. I still don't understand
why I have to shift the TabNoteHead #'layer at all. As far as I see it,
HarmonicParenthesesItem #'stencils is a list of stencils, containing the
left and the right angle bracket, which are placed either side of the
TabNoteHead #'stencil. So the order in which the stancils are placed
should not change anything at all, but if the TabNoteHead #'stencil is set
*before* the HarmonicParenthesesItem #'stencils, it seems that some
whitespace occurs between the angle brackets, overriding the
already placed TabNoteHead.
If on the other hand the whiteout would not appear, we could just use the
parenthesize-tab-note-head function defined in scm/tablature.scm.
This function could detect whether we have a harmonic and in this case
would place the parentheses just further apart.

Furthermore, I have a bad feeling to misuse your time reviewing my patch
for error seeking issues, but I am stuck here. I like the general idea of
not defining the same functionality over and over again, but on the
other hand,
the parenthesize-stencil function in scm/stencil.scm does not seem to
be used anywhere else ...

Thanks in advance,

Marc



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Carl Sorensen



On 6/30/10 1:48 AM, "Marc Hohl" <[hidden email]> wrote:

> Carl Sorensen schrieb:
>> [...]
>>>> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
>>>> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
>>>> the harmonic angle bracket.")
>>>> Why do we need bracket-width?  Why can't we just use the pre-existing
>>>> width property?
>>>>      
>>> Hm, Neil proposed to use a more descriptive name for this property, IIUC.
>>> Done.
>>>    
>>
>> Well, if we want to have this be part of the parenthesis-interface, then we
>> may want to go ahead with bracket-width.
>>  
> Ok, done.
>>  
>> [...]
>> If instead of making the whiteout and the stencil different stencils, we
>> combined them, then parenthesize-stencil would work great.  But this might
>> not be possible if we need to have the stencil and the whiteout on separate
>> layers.  As an alternative, parenthesize-stencil could be modified to take
>> white-padding as an argument.  There's nothing that says we can't modify
>> scm/stencil.scm so that it works better for tablature.  It's much better,
>> IMO, to have one general-purpose piece of code than to have two separate
>> special-purpose code chunks.
>>  
> So I tried to extend the parenthesize-stencil function in scm/stencil.scm.
> It compiles without error, but the regression file is cluttered up.

Can you tell me what you mean by "the regression file is cluttered up"?  Can
you tell me which examples fail the regression test for graphical output?
(The changes that indicate different amounts of memory don't bother me a
bit).

>
> I feel that this is not the right approach, either. I still don't understand
> why I have to shift the TabNoteHead #'layer at all. As far as I see it,
> HarmonicParenthesesItem #'stencils is a list of stencils, containing the
> left and the right angle bracket, which are placed either side of the
> TabNoteHead #'stencil. So the order in which the stancils are placed
> should not change anything at all, but if the TabNoteHead #'stencil is set
> *before* the HarmonicParenthesesItem #'stencils, it seems that some
> whitespace occurs between the angle brackets, overriding the
> already placed TabNoteHead.

Have you looked at the list of stencils created by
parenthesis-item::calc-parentehesis-stencils grob

and

map stencil-whiteout (parenthesis-item::calc-parentehesis-stencils grob) to
see how they differ?

Sometimes lists get reversed during LilyPond processing.  Perhaps this is
happening to this list.  You might see what happens if you reverse the list
of stencils....



> If on the other hand the whiteout would not appear, we could just use the
> parenthesize-tab-note-head function defined in scm/tablature.scm.
> This function could detect whether we have a harmonic and in this case
> would place the parentheses just further apart.
>
> Furthermore, I have a bad feeling to misuse your time reviewing my patch
> for error seeking issues, but I am stuck here.

I don't view it as a misuse of time.  But right now I'm very busy, so I may
not get to it very quickly....

> I like the general idea of
> not defining the same functionality over and over again, but on the
> other hand,
> the parenthesize-stencil function in scm/stencil.scm does not seem to
> be used anywhere else ...

Parenthesize-stencil will be used for the new chord namer, so we need to
keep it in stencil.scm and keep the current functionality.


Thanks,

Carl


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Neil Puttock
In reply to this post by Neil Puttock
On 2010/06/30 07:48:12, marc wrote:

> So I tried to extend the parenthesize-stencil function in
scm/stencil.scm.
> It compiles without error, but the regression file is cluttered up.

I can't test this.  Would you mind uploading another version here which
includes all the changes since the first patch set?

> I feel that this is not the right approach, either. I still don't
understand
> why I have to shift the TabNoteHead #'layer at all. As far as I see
it,
> HarmonicParenthesesItem #'stencils is a list of stencils, containing
the
> left and the right angle bracket, which are placed either side of the
> TabNoteHead #'stencil. So the order in which the stancils are placed
> should not change anything at all, but if the TabNoteHead #'stencil is
set
> *before* the HarmonicParenthesesItem #'stencils, it seems that some
> whitespace occurs between the angle brackets, overriding the
> already placed TabNoteHead.

You added the default for 'whiteout to HarmonicParenthesesItem in your
first patch.  It applies the whiteout to the total extent: the two
brackets plus the space between.

I don't know why grobs with the same layer sometimes swap places, but
there's nothing we can do about it apart from change 'layer to ensure
visibility, unless you remove 'whiteout for the angle brackets.

Cheers,
Neil

http://codereview.appspot.com/1669041/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
In reply to this post by Carl Sorensen
Carl Sorensen schrieb:

>
> On 6/30/10 1:48 AM, "Marc Hohl" <[hidden email]> wrote:
>
>  
>> Carl Sorensen schrieb:
>>    
>>> [...]
>>>      
>>>>> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
>>>>> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
>>>>> the harmonic angle bracket.")
>>>>> Why do we need bracket-width?  Why can't we just use the pre-existing
>>>>> width property?
>>>>>      
>>>>>          
>>>> Hm, Neil proposed to use a more descriptive name for this property, IIUC.
>>>> Done.
>>>>    
>>>>        
>>> Well, if we want to have this be part of the parenthesis-interface, then we
>>> may want to go ahead with bracket-width.
>>>  
>>>      
>> Ok, done.
>>    
>>>  
>>> [...]
>>> If instead of making the whiteout and the stencil different stencils, we
>>> combined them, then parenthesize-stencil would work great.  But this might
>>> not be possible if we need to have the stencil and the whiteout on separate
>>> layers.  As an alternative, parenthesize-stencil could be modified to take
>>> white-padding as an argument.  There's nothing that says we can't modify
>>> scm/stencil.scm so that it works better for tablature.  It's much better,
>>> IMO, to have one general-purpose piece of code than to have two separate
>>> special-purpose code chunks.
>>>  
>>>      
>> So I tried to extend the parenthesize-stencil function in scm/stencil.scm.
>> It compiles without error, but the regression file is cluttered up.
>>    
>
> Can you tell me what you mean by "the regression file is cluttered up"?  Can
> you tell me which examples fail the regression test for graphical output?
> (The changes that indicate different amounts of memory don't bother me a
> bit).
>  
Oh, sorry, I meant the newly introduced
input/regression/tablature-tie-harmonic.ly.
With the patch set before, it worked as expected, now the angle brackets
surround
invisible fret numbers again, and after line breaks, the harmonic
brackets overlap
the parentheses construct (while erasing the fret number :-( )

>> I feel that this is not the right approach, either. I still don't understand
>> why I have to shift the TabNoteHead #'layer at all. As far as I see it,
>> HarmonicParenthesesItem #'stencils is a list of stencils, containing the
>> left and the right angle bracket, which are placed either side of the
>> TabNoteHead #'stencil. So the order in which the stancils are placed
>> should not change anything at all, but if the TabNoteHead #'stencil is set
>> *before* the HarmonicParenthesesItem #'stencils, it seems that some
>> whitespace occurs between the angle brackets, overriding the
>> already placed TabNoteHead.
>>    
>
> Have you looked at the list of stencils created by
> parenthesis-item::calc-parentehesis-stencils grob
>
> and
>
> map stencil-whiteout (parenthesis-item::calc-parentehesis-stencils grob) to
> see how they differ?
>
> Sometimes lists get reversed during LilyPond processing.  Perhaps this is
> happening to this list.  You might see what happens if you reverse the list
> of stencils....
>  
I think I understand now - Neil has given me the clue to this.
> [...]
>> Furthermore, I have a bad feeling to misuse your time reviewing my patch
>> for error seeking issues, but I am stuck here.
>>    
>
> I don't view it as a misuse of time.  But right now I'm very busy, so I may
> not get to it very quickly....
>  
Thanks for your kind words - you do a great job in motivating frustrated
people like me ;-)

>  
>> I like the general idea of
>> not defining the same functionality over and over again, but on the
>> other hand,
>> the parenthesize-stencil function in scm/stencil.scm does not seem to
>> be used anywhere else ...
>>    
>
> Parenthesize-stencil will be used for the new chord namer, so we need to
> keep it in stencil.scm and keep the current functionality.
>  
Ok.

Thank you

Marc


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: tablature: ties and harmonics (issue1669041)

Marc Hohl
In reply to this post by Neil Puttock
[hidden email] schrieb:
> On 2010/06/30 07:48:12, marc wrote:
>
>> So I tried to extend the parenthesize-stencil function in
> scm/stencil.scm.
>> It compiles without error, but the regression file is cluttered up.
>
> I can't test this.  Would you mind uploading another version here which
> includes all the changes since the first patch set?
Of course! I hope I did it right - at least it looks like I have catched
everything.

>
>> I feel that this is not the right approach, either. I still don't
> understand
>> why I have to shift the TabNoteHead #'layer at all. As far as I see
> it,
>> HarmonicParenthesesItem #'stencils is a list of stencils, containing
> the
>> left and the right angle bracket, which are placed either side of the
>> TabNoteHead #'stencil. So the order in which the stancils are placed
>> should not change anything at all, but if the TabNoteHead #'stencil is
> set
>> *before* the HarmonicParenthesesItem #'stencils, it seems that some
>> whitespace occurs between the angle brackets, overriding the
>> already placed TabNoteHead.
>
> You added the default for 'whiteout to HarmonicParenthesesItem in your
> first patch.  It applies the whiteout to the total extent: the two
> brackets plus the space between.
Ah, that makes everything clearer, thanks for this explanation!
>
> I don't know why grobs with the same layer sometimes swap places, but
> there's nothing we can do about it apart from change 'layer to ensure
> visibility, unless you remove 'whiteout for the angle brackets.
Now the whole #'layer stuff makes a lot more sense to me ...

Thank you

Marc


---
----
Join the Frogs!