note-spacing: stretch somewhat uniformly; issue 3304 (issue 36830045)

[mtsolo]

https://codereview.appspot.com/36830045/diff/20001/lily/note-spacing.cc
File lily/note-spacing.cc (right):

https://codereview.appspot.com/36830045/diff/20001/lily/note-spacing.cc#newcode117
lily/note-spacing.cc:117: ret.set_inverse_stretch_strength (base_space);
Looked at it, but I have no idea what any of this stuff means.
If you understand this stuff, could you put a comment in
lily/include/spring.hh as to what inverse_compress_strength and
inverse_stretch_strength are?

https://codereview.appspot.com/36830045/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

grob-property: if callback is independent of layout, call just once (issue 42190043)

[mtsolo]

https://codereview.appspot.com/42190043/diff/1/lily/unpure-pure-container.cc
File lily/unpure-pure-container.cc (right):

https://codereview.appspot.com/42190043/diff/1/lily/unpure-pure-container.cc#newcode45
lily/unpure-pure-container.cc:45:
Looking good.

The only thing is - how do we verify that this actually ignores start
and end?

For example, let's say that there is a scheme function with optional
arguments for start and end that is used as the pure and unpure
function.  It'll evaluate just fine for unpure cuz begin and end are
optional.  But begin and end may be useful when it is pure.  If this is
the only function passed to the constructor, it will return true for the
function above and yet perhaps be an operation that we don't want to
cache.

Conversely, it is possible to do:

(ly:make-unpure-pure-container foo foo)

that should be cached but it would return false for this function.

So, my proposal would be:

1) if there is a single function, verify that it takes only one argument
2) if there are two functions, verify that the second takes only one
argument

All of this is of course predicated, like I said above, on being able to
check the number of arguments a scheme function takes, which may or may
not be possible...

Or create a new type predicate for a new class of functions instead of
doing a boolean test (which seems like a pain...so I like the boolean
approach above).

https://codereview.appspot.com/42190043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows inner-markup spacing using skylines. (issue 13341044)

[mtsolo]

The point is to allow users to achieve the same snug skyline spacing
between stencils that is achieved between grobs.

I agree that it is inefficient as skylines are never cached, but as it
is not default beahvior anywhere in the code base, it allows people to
decide on the efficiency versus snugness-of-placement trade-off.  For
people creating complex markups, this is useful.


https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc
File lily/stencil.cc (right):

https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc#newcode271
lily/stencil.cc:271: add_at_edge (a, d, s, padding, false);
On 2013/08/28 21:17:20, dak wrote:

That's nonsensical.  C++ knows defaults arguments for functions.


Done.

https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc#newcode313
lily/stencil.cc:313: offset = (*first_skyp)[d].distance
((*second_skyp)[-d]);
On 2013/08/28 21:17:20, dak wrote:

This is an expensive non-cached operation, and yet this calculates

skyline

_pairs_ and ignores half of them.


Made a Skyline-only function.

https://codereview.appspot.com/13341044/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/13341044/diff/1/scm/define-markup-commands.scm#newcode2038
scm/define-markup-commands.scm:2038: (ly:stencil-combine-at-edge m1 axis
dir m2 0.0 #t)))
On 2013/08/28 21:17:20, dak wrote:

Why use the same function in the first place when the last argument is

never

used as a variable?


What would be a better way to achieve this?

https://codereview.appspot.com/13341044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds dimension stencil command to correct with-dimension (issue 12957047)

[mtsolo]

https://codereview.appspot.com/12957047/diff/46001/lily/paper-system.cc
File lily/paper-system.cc (right):

https://codereview.appspot.com/12957047/diff/46001/lily/paper-system.cc#newcode55
lily/paper-system.cc:55: if (head == ly_symbol2scm ("skyline-stencil"))
On 2013/08/24 16:19:27, dak wrote:

It seems awkward and error-prone to go through a number of stencil

types here

just for fishing out footnotes.  Of course, this is just another layer

of

ugliness on top of existing ugliness, but it would seem that we

actually want a

separate _backend_ for fishing out footnotes, so that all the default
interpretations of stencils are done correctly.


Agreed - it's a problem in general of markups being implemented
differently than grobs, which has plagued footnotes from the beginning.

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

https://codereview.appspot.com/12957047/diff/46001/lily/stencil-integral.cc#newcode915
lily/stencil-integral.cc:915: SCM skyline_stencil = scm_cadr (expr);
On 2013/08/24 16:19:27, dak wrote:

Why would this traverse the skyline_stencil rather than the real one?


Because we are using the skyline stencil as a substitute for the
dimensions of the real one.

https://codereview.appspot.com/12957047/diff/46001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/12957047/diff/46001/scm/define-markup-commands.scm#newcode730
scm/define-markup-commands.scm:730: (make-box-skyline-stencil
On 2013/08/24 16:19:27, dak wrote:

Stupid question: _iff_ you store a _whole_ replacement skyline anyway,

shouldn't

pad-around just shift the original left/right/up/down skylines
left/right/up/down by the given amount (don't ask me what to do about

the

corners, though)?



Of course this would be incompatible with previous behavior, but more

likely

matching the expectations?



I am not sure that replacement skylines are the right thing anyway,

but it seems

sort of pointless _if_ you propose they are to not use them here.


It's a good idea, but stencils don't have left/right/up/down, so it'd be
hard to figure out how to add this padding to the stencils without
additional plane-geometry functions.

https://codereview.appspot.com/12957047/diff/46001/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/12957047/diff/46001/scm/stencil.scm#newcode689
scm/stencil.scm:689: (let ((sur (make-filled-box-stencil x y)))
On 2013/08/24 16:19:27, dak wrote:

A filled-box stencil seems like serious overkill here when all you

want is

setting the dimensions.  A stencil operator for just setting

dimensions seems

less awkward.  There are two possibilities: have it produce an empty

stencil

expression but with dimensions, and then use make-skyline-stencil on

it.  Or

have it contain an inner expression anyway.



In that case, make-box-skyline-stencil (what an awkward name) does not

need to

call either make-filled-box-stencil nor contain skyline-stencil

anyway.


It means that one needs one more stencil primitive _if_ one wants to

support

make-skyline-stencil.  But it avoids juggling with an obscure

combination of

stencil operations if one doesn't.


The empty stencil wouldn't work, as stencil-integral.cc operates on the
contents of the stencil (lines, curves, etc.), not on the dimension.
So, it would read an empty stencil as empty and not take it into account
in the skyline, irrespective of the dimension.

I'm not exactly sure what you mean in the rest of the comment, nor do I
see why the filled-box-stencil is overkill.  We need to draw a box
around the stencil at some time, and this gets the job done pretty
efficiently (there's not much overhead in stencil creation).  I'm open
to other implementation suggestions, though.

https://codereview.appspot.com/12957047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds dimension stencil command to correct with-dimension (issue 12957047)

[mtsolo]

It was easier to implement the delayed stuff than I expected, so I got
it done.

https://codereview.appspot.com/12957047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds dimension stencil command to correct with-dimension (issue 12957047)

[mtsolo]

I had time to implement everything but the delayed markup stuff.  I'll
try to get to that soon...

https://codereview.appspot.com/12957047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

side-position-interface: use real positions of cross-staff grobs; issue 3363 (issue 9426044)

[mtsolo]

LGTM - just make sure to check this against the
dynamics-avoid-cross-staff-stem regtests.

https://codereview.appspot.com/9426044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Flags cross-staff semi ties. (issue 8857043)

[mtsolo]

On 2013/04/19 12:40:43, Neil Puttock wrote:

Hi Mike,



Generally LGTM, but since there's nothing in the code which requires

C++ code I

favour adding a callback to output-lib.scm:



(define-public (semi-tie::calc-cross-staff grob)
   (let* ((note-head (ly:grob-object grob 'note-head))
  (stem (ly:grob-object note-head 'stem)))
 (and (ly:grob? stem)
  (ly:grob-property stem 'cross-staff #f



BTW, this surprised me a bit while fiddling with the regression test:



<<
  \new Staff = "up" \relative c' {
  f8
  \change Staff = "down"
  c e\f %oops
  \change Staff = "up"
  f
  }
  \new Staff = "down" { \clef bass s2 }
>>



Cheers,
Neil


Thanks Neil!
I wonder why this is a problem in this test you're doing and not the
dynamics-avoid-cross-staff regtests...will investigate...

Cheers,
MS


https://codereview.appspot.com/8857043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Add callback factory grob::inherit-parent-property (issue 8726045)

[mtsolo]

Hey, I hadn't seen this. I just finished writing an equivalent patch.
Yours is better, so keep it.  You can use this for the
inherit-X-parent-visibility and eliminate the
inherit-Y-parent-visibility callback, which is cruft and can be replaced
with the X one.

https://codereview.appspot.com/8726045/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add changes entry for Mike's work on skylines. (issue 8613043)

[mtsolo]

On 2013/04/16 07:51:46, dak wrote:

On 2013/04/16 07:40:19, Trevor Daniels wrote:
> On 2013/04/16 07:20:59, dak wrote:
> > On 2013/04/16 06:05:42, MikeSol wrote:
> >> I would add a note that, if there is too-snug spacing
> >> for an object, setting its vertical skylines to #'()
> >> will generally restore old behavior.
> >
> > I am not really enthused about telling people that,
> > since "too snug" is a bug and needs to get fixed rather
> > than worked around.  We'll get a proliferation of
> > LilyPond sources irretrievably tied into the bugs of
> > particular versions if we tell people to work around
> > bugs rather than report them.
>
> Indeed.  That is why we do not document bugs or add
> workarounds to the documentation (with a few exceptions,
> usually when it is agreed a fix is most unlikely to
> materialise, e.g. the grace timing problem.)  Bugs (and
> their workarounds) should be recorded in the Bug DB.



I hate it when I get last-minute realizations.  Here is another thing

we need to

do for the stable release: go through all problems of the "too snug"

kind and

work out defaults that avoid them.  It's ok to tell people in our

documentation

how to get stuff move closer in case of necessity, but if we aren't

talking

about running text and similar things, looser spacing than necessary

is _not_ a

bug.



Closer spacing than appropriate _is_ a bug.  One can publish a score

with loose

spacing, but one can't publish a score where things are sitting on

each other.


So for a stable release, we need conservative settings.  Settings that

won't

_force_ people to pepper their sources with overrides and tweaks just

to throw

them all out again when the next version arrives.


This is a great time to ping the user list.  People who have been using
the development version for a while now have had a chance to get used to
scores with this spacing, and if they have anything that they consider
"too tight", then we can use simple skylines for those things.  We've
already had some back-and-forth about text grobs but not much else.

I think it is a percentage game, more than anything else.  Meaning "what
percent of users need to consider spacing too close before it is
inappropriate as a default."  If 90% like the new spacing and 10% think
it is too close, then it is important to do something like Trevor is
talking about that gives them a way out.  However, if it is 50/50, then
it's better to err on the side of conservative.

Anyway, we'll know none of these things without consulting people making
real scores.  I prefer all the spacing improvements in my scores so no
complaints here.  But I'm willing to admit of course that I wouldn't
have done any of this work unless there were some internal motivation
stemming from my own aesthetic preferences.

https://codereview.appspot.com/8613043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add changes entry for Mike's work on skylines. (issue 8613043)

[mtsolo]

https://codereview.appspot.com/8613043/diff/19001/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/8613043/diff/19001/Documentation/changes.tely#newcode153
Documentation/changes.tely:153: @item
I would add a note that, if there is too-snug spacing for an object,
setting its vertical skylines to #'() will generally restore old
behavior.

https://codereview.appspot.com/8613043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: rewrite Self_alignment_interface (issue 7768043)

[mtsolo]

I'd recommend adding a sort of padding property to the
self-alignment-interface to get it completely there.  That is, imagine
that we right align to a grob and we want to be padded by 0.1  We should
be able to do that.  That'd allow self-alignment-interface to be used
for grobs like InstrumentName.


https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver.cc
File lily/new-fingering-engraver.cc (right):

https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver.cc#newcode291
lily/new-fingering-engraver.cc:291: // don't overwrite offset property
if it was overridden by the user
Consider chaining an offset callback here.  See chain_callback and
chain_offset_callback.

https://codereview.appspot.com/7768043/diff/46001/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7768043/diff/46001/lily/self-alignment-interface.cc#newcode152
lily/self-alignment-interface.cc:152:
This would be the place to tack on padding.

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm#newcode143
scm/define-grobs.scm:143: (X-extent . (-0.1 . 0.1)) ;UGH! how to get
ambitusline extent from its stencil?
Why can't you use the standard X extent function given in grob.cc?

https://codereview.appspot.com/7768043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: reorganize self_alignment_interface (issue 7768043)

[mtsolo]

In general, I see a lot of reassigning of parents.  What is the goal
with this (sorry if you've explained this already)?


https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc
File lily/fingering-engraver.cc (right):

https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc#newcode142
lily/fingering-engraver.cc:142:
Self_alignment_interface::aligned_on_x_parent (fingerings_[i]->self_scm
()));
This needs to be a chained offset procedure. See chain_offset_callback
elsewhere in the code.

https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc
File lily/paper-column.cc (right):

https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc#newcode222
lily/paper-column.cc:222: /*
Good work!

https://codereview.appspot.com/7768043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Removes outside-staff-priority from dynamic-related objects in Dynamic context (issue 7655045)

[mtsolo]

On 2013/03/27 21:50:02, janek wrote:

I see that these overrides solve the problem, but it would be nice to

know why

this works.  Can you write an explanation?  What was wrong with the

skylines

that caused this bug?  Maybe this is just one instance of a more

general

problem.


Objects that are placed inside the staff were being placed outside the
staff.  Conceptually, dynamics are "inside" a staff in Dynamics
contexts, even if there is no staff.



https://codereview.appspot.com/7655045/diff/2001/input/regression/dynamics-outside-staff-priority.ly

File input/regression/dynamics-outside-staff-priority.ly (right):



https://codereview.appspot.com/7655045/diff/2001/input/regression/dynamics-outside-staff-priority.ly#newcode18

input/regression/dynamics-outside-staff-priority.ly:18: >>
I suggest to minimize this example to



<<
   \new Staff = "Test" {
 \tempo "Andante" c'1
   }
   \new Dynamics \with { alignAboveContext = "Test" } {
 s1\f
   }
>>


Done

https://codereview.appspot.com/7655045/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Eliminates side poisition calculations for system-start-text grobs. (issue 7574048)

[mtsolo]

On 2013/03/24 19:59:38, dak wrote:

On 2013/03/24 19:06:49, mike7 wrote:
> On 24 mars 2013, at 14:59, mailto:d...@gnu.org wrote:
>
> >
> >
>


https://codereview.appspot.com/7574048/diff/5001/input/regression/instrument-name-x-align.ly

> > File input/regression/instrument-name-x-align.ly (right):
> >
> >
>


https://codereview.appspot.com/7574048/diff/5001/input/regression/instrument-name-x-align.ly#newcode33

> > input/regression/instrument-name-x-align.ly:33: \set Staff .
> > instrumentName = \markup \right-column {
> > Make no mistake: the previous code looked suspiciously asymmetric.

 Is

> > your change a cosmetic one, or is it related to other strange

behaviors

> > like the \left-column/\column mismatch we had noticed elsewhere?
> >
> > https://codereview.appspot.com/7574048/
>
> The latter - I noticed it clashed and have run into problems with

right column

> in the past and decided to get rid of it.
> I can put it in a different commit before pushing this one.



In general, separate issues should be separate commits even if you

don't want to

start a tracker issue of its own for it.



Since one wants to be able to pinpoint stuff by bisection, the regtest

change

should be in a commit before the rest.


Will do - I'll push this as two separate commits.

Cheers,
MS

https://codereview.appspot.com/7574048/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Page numbering: first page format only on first page of a book (issue 7974046)

[mtsolo]

LGTM

https://codereview.appspot.com/7974046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[mtsolo]

Hey all,

Following up to my comment on the tracker, I'd like to not push this
until everyone has had a chance to think through David's e-mails about
2.18.  I anticipate this needing 2-3 months of user testing to catch and
fix any bugs.  Given that this is my first big patch since August, it'll
likely take me another 6-7 months to do the next phase on top of this
patch. Comfortably after 2.18 (hopefully).

I am confident in the patch and think it is a step in the right
direction for numerous reasons, but it's obviously not worth pushing it
if people want to get 2.18 out in the next couple months.  If I did push
it, I'd send an e-mail to lilypond-user once the next development
version came out encouraging people to compile big scores and report
back any anomalies.  I haven't seen any in my scores.

Cheers,
MS

https://codereview.appspot.com/7185044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: reorganize self_alignment_interface (issue 7768043)

[mtsolo]

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode128
lily/self-alignment-interface.cc:128: /* LilyPond positions every grob
relative to its parent in respective axis.
In general, try to avoid using the word LilyPond in comments, as it is
in the LilyPond code base.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode133
lily/self-alignment-interface.cc:133:
I don't see staff space being looked up anywhere in the function.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode144
lily/self-alignment-interface.cc:144:
With respect to my last review, this function should accept a pointer to
a vector of grob pointers:

vector &hims

so that you can do the note column stuff for lyrics.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode144
lily/self-alignment-interface.cc:144:
I don't have the energy to trigger another comment flame war, but a
comment like this may wind up diverging from what the code is doing (it
already does with the staff space thing).  Can you write the bare
minimum and comment the code below in places?  The code is already
clear: I can tell what you're doing and why you're doing it, so it
speaks for itself.

https://codereview.appspot.com/7768043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: don't abort aligning when grob's parent is a PaperColumn (issue 7564044)

[mtsolo]

https://codereview.appspot.com/7564044/diff/9001/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7564044/diff/9001/lily/self-alignment-interface.cc#newcode130
lily/self-alignment-interface.cc:130: // TODO: should this function be
defined someplace else, e.g. in note-column.cc?
I like where you're going with this.
I would recommend creating a function algined_on_grobs that accepts a
grob, an vector of grobs to align to, and an axis.  Then
algined_on_parent becomes a subset of this where the vector is of length
one and is the parent.
Then, you can create a function
Self_alignment_interface::lyrics_algined_on_note_columns that invokes
aligned_on_grobs with a vector of note columns returned using the search
method in this function.  This function could be implemented in
paper-column.cc as get_generic_interface_extent such that it recurses
through a paper columns' element list, finds all grobs implementing
interface X and returns them.

https://codereview.appspot.com/7564044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: report a programming error when trying to align on empty parent (issue 7533046)

[mtsolo]

LGTM

https://codereview.appspot.com/7533046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows slurs to break at barlines. (issue 7424049)

[mtsolo]

The most recent patch set copies direction from SlurEvents and
PhrasingSlurEvents, but this doesn't seem to work as intended (it fails
silently).  Everything else is operational.

https://codereview.appspot.com/7424049/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds Ferneyhough hairpins to LilyPond. (issue 7615043)

[mtsolo]

On 2013/03/13 21:38:59, thomasmorley65 wrote:

Hi Mike,



sorry to be such an inch pincher.



https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm
File scm/output-lib.scm (right):



https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1061

scm/output-lib.scm:1061: (thick (* thick (layout-line-thickness

grob)))

At first sight I was surprised about `layout-line-thickness´. Than I

remembered,

it is defined in bar-line.scm
How about making it public, moving it to lily-library.scm?
It could be used then in local custom-definitions.
Same with `layout-blot-diameter´.
I know, it's another topic, though, what do you think about it?


Great idea!


https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1078

scm/output-lib.scm:1078:
What do you think about predefining sth like
ferneyhoughHairpinOn/Off
constanteHairpinOn/Off
in /lyproperty-init.ly?
i.e.
ferneyhoughHairpinOn =
\override Hairpin #'stencil = #ferneyhough-hairpin


I'm not against this idea at all - I'd like to see it proposed in a
different patch set, if possible.  I am teerible at judging UI
issues because I hack so much.  It is true that shortcuts help, but
there are many, many overrides that are shortcut-worthy.  I'm not sure
what standard determines which overrides should get shortcuts and which
shouldn't, so I leave that to a future (and important!) conversation.


https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1082

scm/output-lib.scm:1082: (define-public constante-hairpin
I'm not happy with constante-hairpin.



Look at the output from:



dimMolto =
#(make-dynamic-script (markup #:normal-text #:italic "dim. molto"))



\relative c'' {
   \dynamicUp
   \override Hairpin #'stencil = #constante-hairpin
   a1 a4\dimMolto\> a a a\!
}



The "hook" is placed at the left pointing up.



Wouldn't it be better to have the hook _always_ at the right?
Perhaps by adding a boolean variable to `my-c-p-s´. Doing scaling only

while

(and scalable? decresc?).



Is it possible to make the hook-direction depending on the position of

the

Hairpin i.e below or above the staff?


The constante indication means that the dynamic should not change at
all, so the decision to use a hairpin that grows in a given direction is
admittedly a hack. The idea of dimMolto (or crescMolto) with constante
is a contradiction (get very quiet while not changing dynamic).  Insofar
as a hairpin represents a change in dynamics, constante should, in
theory, be a different grob.  My solution is not ideal, but I think it
is an OK middle-ground short of the creation of a separate grob.  If
people think it is too hackish, I will propose a new patch with a
Constante grob (or make the hairpin grow-direction = #CENTER default to
constante, which'd be difficult but doable).

https://codereview.appspot.com/7615043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows minimum-length to work for end-of-line spanners. (issue 7453046)

[mtsolo]

Reviewers: lemzwerg, dak, mike7,

Message:
On 2013/03/11 10:18:59, dak wrote:

On 2013/03/10 00:32:43, mike7 wrote:



> >> > Why is this override needed for the regtest?  The other

overrides

> > are
> >> > obvious user-accessible overrides for triggering the tested
> >> > functionality.
> >> >
> >> > But should _this_ override not be the default?
> >> >
> >> > https://codereview.appspot.com/7453046/
> >
> >> Perhaps open a tracker issue for this?
> >> The question is not only valid for text spanners but also

hairpins,

> > glissandos,
> >> etc.
> >
> > Last time I looked, this issue purportedly "Allows minimum-length

to

> > work for end-of-line spanners."  And according to the regtest, it

does

> > not do the job.  Without additional messing with springs-and-rods

it

> > does not allow minimum-length to work for end-of-line-spanners.
> >
>
[...]



> Additional messing with springs and rods is because minimum-length
> is currently implemented by four different interfaces (lyric-hyphen,
> multi-measure-rest, ottava-bracket and spanner) and is also looked
> up in lyric-extender in a way that does not correspond to its
> docstring.  So, certain uses of minimum length require the
> additional override whereas others don't.  I do not think this is
> ideal, which is why I proposed a few weeks ago standardizing
> property names across interfaces.  It seems like the issues you are
> raising above and below have less to do with this patch and more to
> do with the multiple implementation of minimum-length and the use of
> the springs-and-rods property.



This is a typical case of "Somebody Else's Problem".  If there is a
party and you place a chair in the worst possible place, like
somewhere in a doorway, people will walk around it, squeeze themselves
through, get annoyed again and again.



That's our current state.  Now someone wants to do a polonaise and
since the chair is in the way, he proposes putting a plank across it.



Now we are moving from ridiculous to absurd, and it becomes harder to
do the right thing.



Yes, I am fully aware that you are not responsible for the chair in
the way of your polonaise.



Bit it is time to move it aside rather than taking it into account in
our planning and make it even harder to get into a proper state.  In
particular since your coding style requires a lot of reverse
engineering in order to figure out the hidden assumptions and
dependencies.  So that makes it doubly desirable to not add further
dependencies on broken behavior but first clean up the foundations.



Yes, that's not a problem caused by your patch, but the consequences
are acerbated by it.



> > The only thing more frustrating than missing functionality is
> > purportedly available functionality that needs
> > non-user-comprehensible trickery to actually work.
>
> I do not have a problem with the current need to set the
> springs-and-rods separately.



Of course you don't, and nobody keeps you from applying this sort of
patch to your own private code base without doing the necessary
cleanup before.



But you are not the metric for what goes into LilyPond's own
repository and what not.  The functionality that goes into LilyPond
has to work for the average user encountering that problem space.  The
solutions have to have a meaningful correspondence in complexity to
the problem and not require "don't think about it" steps.



> I'm positive it would because of the way that minimum-length is
> multiply defined.  That is why this patch is intended for the "some
> layout objects" discussed above like the TextSpanner.  I agree that
> the multiple use of the minimum-length property should be changed,
> but this seems like the business of another patch.



Sure.  But that patch will be harder to do once this patch _relies_ on
the broken behavior, and people write code _relying_ on this patch.



> If the regtest is bothering you that much, I can just eliminate it
> from this patch.



There is no point in hiding the symptoms of a problem away.  That only
makes things even harder in future.


I don't think this is a problem blocking the current patch.  You are
right that the current multiple functions of minimum-length are
problematic.  I'm not arguing that this is someone else's problem - I am
arguing that this (like all bugs) is the community's problem.  Yes, the
regtest that I've proposed uses an override that corresponds to that
used in the Documentation which is, as you have pointed out, a
suboptimal way of doing things.  It may take months or years to sort out
the multiple naming of properties.  This shouldn't block this patch from
being pushed.

So when you say "But this patch will be harder to do once this _relies_
on the broken behavior.", all this patch will rely on is an override
that you are arguing should be the default.

This patch fixes behavior that would exist even if the springs-and-rods
override became the default.

Imagine it this way.  Patients are taking a suboptimal drug for a
dise

Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)

[mtsolo]

Thanks for the review!


https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):

https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly#newcode5
input/regression/skyline-point-extent.ly:5: even though the
@code{NoteHead} stencils are empty. The @code{Stem_engraver}
On 2013/03/08 14:24:11, dak wrote:

This talks about the NoteHead stencil being empty, but the actual code

uses a

point-stencil (which is _not_ empty).


Done.

https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc
File lily/separation-item.cc (right):

https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc#newcode148
lily/separation-item.cc:148: Interval extra_width = robust_scm2interval
(elts[i]->get_property ("extra-spacing-width"),
On 2013/03/08 14:24:11, dak wrote:

This is not related to this patch, but isn't it complete nonsense to

use an

Interval here rather than a Drul_array or other form of offset pair?



The LEFT and RIGHT values don't form an interval as far as I can see.


you're right

https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc#newcode649
lily/skyline.cc:649: return ret;
On 2013/03/08 14:24:11, dak wrote:

Why not just
return *this;
here?  The function does not return a reference, so a copy is

constructed

anyway.  There is no point in doing yet another copy, is there?


Done.

https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode472
scm/output-lib.scm:472: 1000))
On 2013/03/08 14:24:11, dak wrote:

Don't we have some constants for the full range START and END values?


I don't think we have one for integers. I made one in lily-library.scm

https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode478
scm/output-lib.scm:478: (define-public
pure-from-neighbor-interface::unobtrusive-height
On 2013/03/08 14:24:11, dak wrote:

"unobtrusive" is a bit obscure: I suspected the only-a-bit-empty

interval at

first.  "height-if-pure" would be quite more descriptive.


Done.

https://codereview.appspot.com/7310075/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Adds Ferneyhough hairpins to LilyPond. (issue 7615043)

[mtsolo]

https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm#newcode929
scm/output-lib.scm:929: form. @code{x} is the portion of the width
consumed for a given line
On 2013/03/08 06:26:29, lemzwerg wrote:

Please use two spaces after a full stop.


Done.

https://codereview.appspot.com/7615043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses only unpure-pure containers to articulate unpure-pure relationships. (issue 7377046)

[mtsolo]

Some code from my patch for issue 3161 is copied over to this patch.
The fact that Grob::pure_height returns Interval (0,0) as default causes
point stencils to be placed in the skyline, potentially effecting
vertical spacing.

Separation_item::boxes uses this (0,0) extent to add extra spacing
height to certain grobs.  To allow this to still happen, we give the
grobs without pure height a small empty extent so that the (almost) full
compliment of extra spacing height is taken into account.  This is why
small-empty-interval exists in lily-library.scm.

https://codereview.appspot.com/7377046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Make ly:make-unpure-pure-container accept a single callback (issue 7382046)

[mtsolo]

LGTM, with one suggestion: the difference between unpure and pure
functions in LilyPond is that unpure functions accept n arguments
whereas pure functions accept n+2. This shortcut you're proposing only
works when n=1, which you document, but it'd be nice if it worked for an
arbitrary number of input arguments.  It'd only be possible to do this,
though, if there were a way in C++ to determine the number of arguments
a Scheme function took.

https://codereview.appspot.com/7382046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing with empty contexts; issue 1669 (issue 4515158)

[mtsolo]

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode222
lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2011/06/09 02:55:55, Keith wrote:

On 2011/06/07 04:48:01, joeneeman wrote:
> If pure is true, there may be some staves that are going to be

removed.

However,
> these staves won't be removed until after line-breaking.



... and, as you say, these staves are not yet removed when

Y-extent-estimates

for the page breaker are calculated, so this patch messes up the page

breaker.


If the page breaker depends on empty, but living, staves disappearing

for

vertical spacing purposes, then maybe empty staves should be treated

that way

consistently. That would be tantamount to giving all empty-able

contexts the

hara-kiri engraver, as you suggested much earlier.



The obvious alternative would be to have the empty staves decide

whether to

suicide before page breaking, but my suspicion is that is awkward, or

it would

have been done that way at first.



Having minimum_distances() change behavior based on whether 'pure' is

true is

too tricky for me. (how do we know that any staff that is empty we are

pure will

suicide later)



I'll think on it, but mark the patch as abandoned for now.


Couldn't there just be a vector ignore_me that contains the result
request_suicide for all elements (when pure)?  Then, the for loop would
be continued for these elements and they wouldn't have to be removed in
get_skylines.

https://codereview.appspot.com/4515158/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)

[mtsolo]

New patch set coming after I finish running the regtests.


https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):

https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly#newcode7
input/regression/skyline-point-extent.ly:7: }
On 2013/02/10 03:08:04, Keith wrote:

... The accents should follow the descending melody line, even though

the

note-head stencils are empty."}
{ \override NoteHead #'stencil = #point-stencil
   c'2.-> b8-- a-- g1-> }
#(ly:set-option 'debug-skylines)


Regtest changed.  I've removed the Stem_engraver in the regtest so that
the only possible side support element is the note head.

https://codereview.appspot.com/7310075/diff/10/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/7310075/diff/10/lily/grob.cc#newcode488
lily/grob.cc:488: Interval iv = robust_scm2interval (iv_scm, Interval
());
On 2013/02/10 03:08:04, Keith wrote:

I assume you will apply this change and the change in

separation-item.cc in a

separate commit, the "empty extents in pure-heights" part of the patch

set.

I'm not sure if these changes can be separated from the skyline stuff
without causing regtest havoc.

I can do this, but it'd take me more time (figure out what changes to
isolate, run regtests, etc.).  Is it necessary?

https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc
File lily/separation-item.cc (right):

https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcode153
lily/separation-item.cc:153: x[LEFT] += extra_width[LEFT];
On 2013/02/10 03:08:04, Keith wrote:

// The conventional empty extent is (+inf.0 . -inf.0)
//  but (-inf.0 . +inf.0) is used as extra-spacing-height
//  on items that must not overlap other note-columns.
// If these two uses of inf combine, leave the empty extent.



if (!isinf(x[LEFT]) x[LEFT] += extra_width[LEFT];
if (!isinf(x[RIGHT]) x[RIGHT] += extra_width[RIGHT];


Done.

https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcode159
lily/separation-item.cc:159: // empty interval with infinity at either
end
On 2013/02/10 03:08:04, Keith wrote:

Other than the addition above, what other 'operation' can produce a

NaN ?

Comment eliminated with change above.

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode59
lily/skyline.cc:59: /* If we start including very thin buildings,
numerical accuracy errors can
On 2013/02/10 03:08:04, Keith wrote:

Did you find where these numerical accuracy errors occured?   I don't

see

anything obvious.  The most likely seems to be the way we step through

the two

skylines in internal_merge_skylines();


No idea...

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode61
lily/skyline.cc:61: than epsilon wide. In merges, we omit them.
On 2013/02/10 03:08:04, Keith wrote:

Any such buildings are created in merge_skyline() are omitted from the

merged

result.


Done.

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode104
lily/skyline.cc:104: Interval start_end = fatten_skinny_buildings
(start, end);
On 2013/02/10 03:08:04, Keith wrote:

The old-fashioned way would be
   // Buildings all have width at least EPS
   end = min(end, start + EPS);
and the same in other constructors, but I know how you hate

code-duplication.

But this adds the EPS arbitrarily to the right instead of adding an
equal amount to the right and left...

And I hate code duplication.

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode119
lily/skyline.cc:119:
On 2013/02/10 03:08:04, Keith wrote:

// Buildings all have width at least EPS
end = min(end, start + EPS);


See above.

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode497
lily/skyline.cc:497: Boxes should have fatness in the horizon_axis,
otherwise they are ignored.
On 2013/02/10 03:08:04, Keith wrote:

This comment would no longer belong here


Changed to represent use of EPS as minimum fatness.

https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode519
lily/skyline.cc:519: Buildings should have fatness in the horizon_axis,
otherwise they are ignored.
On 2013/02/10 03:08:04, Keith wrote:

This comment would no longer belong here


Changed to represent use of EPS as minimum fatness.

https://codereview.appspot.com/7310075/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[mtsolo]

Hey all,

After letting this newest round sit for a few days, I'm pretty confident
that the patch is ready for review so I'd ask people to review it.
First, if anyone needs more comments in the code to let them know what's
going on, lemme know where and I'll add it.  Second, if people have
performance concerns, lemme know - I haven't noticed a slow down but I
haven't done rigorous testing.  Third, I'm interested to see if people
like this approach.  The long-term goal is to get rid of translate_axis
as much as possible, as using it destroys downstream possibilities to do
pure height calculations on grobs.

Cheers,
MS

https://codereview.appspot.com/7185044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[mtsolo]

People can review this if they have time...it is a lot of code, but it
is more a progress report than anything else.

To resume a bit of what's been said before: once translate_axis is
called, the dim_cache of grobs is set and it is very difficult to work
with approximations.  This makes cross-staff grobs very difficult to
deal with in vertical placement.  The goal is to eliminate this property
call for outside-staff placement so that pure skylines can be used in a
first pass without setting the Y-offset.  Then, in the second pass, real
skylines will be used and the Y-offset will be set.

https://codereview.appspot.com/7185044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Eliminates the Hara_kiri_engraver. (issue 7061062)

[mtsolo]

"sneaky callback" is not a real concern...it's a pedantic concern.
If we're following the model to a T, grob properties should not be read
in engravers, just set.  The idea is that the engravers are creating the
grobs and fixing them up with properties if need be.  I prefer
duplicating a context and grob property rather than just having the grob
property, but people seem against this.  It is a trivial fix if people
become for it.

I think Keith's comment is fixed in this patch set (although I must
admit that I don't completely follow it either...this is a new corner of
the code for me).

https://codereview.appspot.com/7061062/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Eliminates the Hara_kiri_engraver. (issue 7061062)

[mtsolo]

Reviewers: ,

Message:
This doesn't crash in the test case Keith proposed:

\new StaffGroup \with { \RemoveEmptyStaves
 }  << { b1 b1 b1}
   { b1 b1 \break b1 } >>

Description:
Eliminates the Hara_kiri_engraver.

Uses a haraKiri context property in the Axis_group_engraver to
acheive the same functionality.

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

Affected files:
  M lily/axis-group-engraver.cc
  D lily/hara-kiri-engraver.cc
  D lily/include/axis-group-engraver.hh
  M lily/include/lily-proto.hh
  M ly/context-mods-init.ly
  M ly/engraver-init.ly
  M scm/define-context-properties.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Checks for recursive element behavior (issue 6943072)

[mtsolo]

Reviewers: ,

Message:
Hey all,

I'm ok w/ this on the countdown but can someone check out David's
suspicion that this slows stuff down by O(n^3)?  I definitely won't push
this if it slows LilyPond down to a crawl.

Cheers,
MS

Description:
Checks for recursive element behavior

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

Affected files:
  M lily/engraver.cc
  M lily/include/pointer-group-interface.hh
  M lily/pointer-group-interface.cc


Index: lily/engraver.cc
diff --git a/lily/engraver.cc b/lily/engraver.cc
index  
db1303d63ce3b918f95e5d9f254c589a84bf89ea..11d080eb6e061a26c23356a825c89e4b8d434731  
100644

--- a/lily/engraver.cc
+++ b/lily/engraver.cc
@@ -123,7 +123,6 @@ Engraver::internal_make_grob (SCM symbol,

   SCM handle = scm_sloppy_assq (ly_symbol2scm ("meta"), props);
   SCM klass = scm_cdr (scm_sloppy_assq (ly_symbol2scm ("class"), scm_cdr  
(handle)));

-
   if (klass == ly_symbol2scm ("Item"))
 grob = new Item (props);
   else if (klass == ly_symbol2scm ("Spanner"))
Index: lily/include/pointer-group-interface.hh
diff --git a/lily/include/pointer-group-interface.hh  
b/lily/include/pointer-group-interface.hh
index  
e265bedc79d0cf29d72debc26af7c7eaa0d242b2..1970b1073fbddea5d63d6e02118bcd5ad0c906d2  
100644

--- a/lily/include/pointer-group-interface.hh
+++ b/lily/include/pointer-group-interface.hh
@@ -34,6 +34,7 @@ public:
   static void set_ordered (Grob *, SCM, bool);
   static Grob_array *get_grob_array (Grob *, SCM);
   static Grob *find_grob (Grob *, SCM, bool (*pred) (Grob *));
+  static bool has_in_element_chain (Grob *needle, Grob *hay);
 };

 vector const &internal_extract_grob_array (Grob const *elt, SCM  
symbol);

Index: lily/pointer-group-interface.cc
diff --git a/lily/pointer-group-interface.cc  
b/lily/pointer-group-interface.cc
index  
045563d457b9acd572a203788fb5dea3d4dab336..11deb42bee9f12e0d166b0db749d6d111d9cabb6  
100644

--- a/lily/pointer-group-interface.cc
+++ b/lily/pointer-group-interface.cc
@@ -21,6 +21,7 @@

 #include "grob-array.hh"
 #include "grob.hh"
+#include "international.hh"

 int
 Pointer_group_interface::count (Grob *me, SCM sym)
@@ -68,9 +69,32 @@ Pointer_group_interface::find_grob (Grob *me, SCM sym,  
bool (*pred) (Grob *))

   return 0;
 }

+bool
+Pointer_group_interface::has_in_element_chain (Grob *hay, Grob *needle)
+{
+  if (!needle || !hay)
+return false;
+
+  if (needle == hay)
+return true;
+  extract_grob_set(hay, "elements", haystack);
+  for (vsize i = 0; i < haystack.size (); i++)
+{
+  if (has_in_element_chain (haystack[i], needle))
+return true;
+}
+  return false;
+}
+
 void
 Pointer_group_interface::add_grob (Grob *me, SCM sym, Grob *p)
 {
+  if (sym == ly_symbol2scm ("elements") && has_in_element_chain (p, me))
+{
+  me->programming_error (_f ("Cowardly refusing to add %s to the  
elements list of %s, which would result in recursive behavior.", p->name  
().c_str (), me->name ().c_str ()));

+  return;
+}
+
   Grob_array *arr = get_grob_array (me, sym);
   arr->add (p);
 }



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Better alignment of MetronomeMark to MultiMeasureRest (issue 6972044)

[mtsolo]

Reviewers: Keith,

Message:
You're right...it was mostly out of laziness and desperation that I did
the quick fix...it worked for a piece I was typesetting.  I have a long
plane ride on the 28th to fix this correctly.


https://codereview.appspot.com/6972044/diff/1/lily/metronome-engraver.cc
File lily/metronome-engraver.cc (right):

https://codereview.appspot.com/6972044/diff/1/lily/metronome-engraver.cc#newcode134
lily/metronome-engraver.cc:134: has_note_column_ = true;
On 2012/12/20 02:40:11, Keith wrote:

Why not just
\override Score.MetronomeMark #'non-break-align-symbols =
#'(note-column-interface   multi-measure-rest-interface)


Good call...will do.

Description:
Better alignment of MetronomeMark to MultiMeasureRest

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

Affected files:
  M lily/break-alignment-interface.cc
  M lily/metronome-engraver.cc


Index: lily/break-alignment-interface.cc
diff --git a/lily/break-alignment-interface.cc  
b/lily/break-alignment-interface.cc
index  
242e39eacc7c35692d113ef7ea97d946f0a8da97..4a3ac4966213054b90f292cdff61c4e5f5c5a370  
100644

--- a/lily/break-alignment-interface.cc
+++ b/lily/break-alignment-interface.cc
@@ -265,7 +265,15 @@ Break_alignable_interface::self_align_callback (SCM  
grob)

   Grob *me = unsmob_grob (grob);
   Item *alignment = dynamic_cast (me->get_parent (X_AXIS));
   if (!Break_alignment_interface::has_interface (alignment))
-return scm_from_int (0);
+{
+  // If we're aligning to a break aligned, then we
+  // give up and shift as to-the-right as possible of its break  
alignment.

+  // If there is none, we give up entirely.
+  if (Break_alignment_interface::has_interface (alignment->get_parent  
(X_AXIS)))
+return scm_from_double (alignment->get_parent (X_AXIS)->extent  
(alignment->get_parent (X_AXIS), X_AXIS)[RIGHT]);

+  else
+return scm_from_int (0);
+}

   SCM symbol_list = me->get_property ("break-align-symbols");
   vector elements = Break_alignment_interface::ordered_elements  
(alignment);

Index: lily/metronome-engraver.cc
diff --git a/lily/metronome-engraver.cc b/lily/metronome-engraver.cc
index  
b4cd2ce328b89707ac7497ed0cff6ca18a6ab1ec..84567b6575a275efcc3b70b5281d9223f0cf02a0  
100644

--- a/lily/metronome-engraver.cc
+++ b/lily/metronome-engraver.cc
@@ -37,6 +37,8 @@ class Metronome_mark_engraver : public Engraver
   Item *text_;
   Grob *support_;
   Grob *bar_;
+  Grob *break_alignment_;
+  bool has_note_column_;
   Stream_event *tempo_ev_;

 public:
@@ -49,6 +51,7 @@ protected:
   DECLARE_ACKNOWLEDGER (break_aligned);
   DECLARE_ACKNOWLEDGER (break_alignment);
   DECLARE_ACKNOWLEDGER (grob);
+  DECLARE_ACKNOWLEDGER (note_column);

   DECLARE_TRANSLATOR_LISTENER (tempo_change);
 };
@@ -59,6 +62,8 @@ Metronome_mark_engraver::Metronome_mark_engraver ()
   support_ = 0;
   bar_ = 0;
   tempo_ev_ = 0;
+  break_alignment_ = 0;
+  has_note_column_ = false;
 }

 IMPLEMENT_TRANSLATOR_LISTENER (Metronome_mark_engraver, tempo_change);
@@ -106,6 +111,8 @@ Metronome_mark_engraver::acknowledge_break_alignment  
(Grob_info info)

   && support_
   && dynamic_cast (g))
 text_->set_parent (g, X_AXIS);
+
+  break_alignment_ = g;
 }

 void
@@ -122,15 +129,22 @@ Metronome_mark_engraver::acknowledge_grob (Grob_info  
info)

 }

 void
+Metronome_mark_engraver::acknowledge_note_column (Grob_info /* info */)
+{
+  has_note_column_ = true;
+}
+
+void
 Metronome_mark_engraver::stop_translation_timestep ()
 {
   if (text_)
 {
   if (text_->get_parent (X_AXIS)
   && text_->get_parent (X_AXIS)->internal_has_interface  
(ly_symbol2scm ("multi-measure-rest-interface"))

-  && bar_)
+  && (bar_ || break_alignment_))
+//text_->set_parent (break_alignment_ ? break_alignment_ : bar_,  
X_AXIS);

 text_->set_parent (bar_, X_AXIS);
-  else if (!support_)
+  if (!support_ && has_note_column_)
 {
   /*
 Gardner Read "Music Notation", p.278
@@ -150,7 +164,9 @@ Metronome_mark_engraver::stop_translation_timestep ()
   support_ = 0;
   bar_ = 0;
   tempo_ev_ = 0;
+  break_alignment_ = 0;
 }
+  has_note_column_ = false;
 }

 void
@@ -172,6 +188,7 @@ Metronome_mark_engraver::process_music ()
 ADD_ACKNOWLEDGER (Metronome_mark_engraver, break_aligned);
 ADD_ACKNOWLEDGER (Metronome_mark_engraver, break_alignment);
 ADD_ACKNOWLEDGER (Metronome_mark_engraver, grob);
+ADD_ACKNOWLEDGER (Metronome_mark_engraver, note_column);

 ADD_TRANSLATOR (Metronome_mark_engraver,
 /* doc */



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Various clean-ups in stems and beams. (issue 6584045)

[mtsolo]

Reviewers: aleksandr.andreev,

Message:
I think a user should be able to use Kievan notation and normal
stems/flags/beams if she so chooses. I'll define something like

startKievanNotation = {
   %% Set glyph styles.
 \override NoteHead #'style = #'kievan
 \override Rest #'style = #'mensural
 \override Accidental #'glyph-name-alist =
#alteration-kievan-glyph-name-alist
 \override Dots #'style = #'kievan
 \override Slur #'stencil = ##f

 %% There are beams in Kievan notation, but they are invoked manually
 \set autoBeaming = ##f
}

stopKievanNotation = {
 \revert NoteHead #'style
 \revert Rest #'style
 \revert Accidental #'glyph-name-alist
 \revert Dots #'style
 \revert Slur #'stencil

 \unset autoBeaming

}

and then use it in the documentation.

Description:
Various clean-ups in stems and beams.

*) Eliminates code dups for Kievan work.
*) Transfers functions that are called a lot to C++ to speed things up.
*) Eliminates unused variables.

No regtest changes.

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

Affected files:
  A input/regression/kievan-notation.ly
  M lily/include/beam.hh
  M lily/include/stem.hh
  M lily/stem.cc
  M ly/engraver-init.ly
  M scm/define-grobs.scm
  M scm/output-lib.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Caches grob properties before evaluation. (issue 6573066)

[mtsolo]

Reviewers: ,

Message:
This slows down LilyPond - I haven't done comprehensive tests of how
much.  I'm pretty sure it works (the regtest works as expected).
Irrespective of how multiple passes are done, this seems like a
necessary first step.

Note that this patch would not have a time impact if it used properties
stashed in immutable_properties_alist_. If all side effects were
eliminated from LilyPond (which would be awesome), then this could be
done.  However, this would be very difficult - for example, tweaks are
given to grobs after their immutable_properties_alist_ is fixed (see the
tweak engraver).

Description:
Caches grob properties before evaluation.

We cannot simply use the original value stored in the
immutable_properties_alist_ because simple closures may be created
at the engraver stage via chain_offset_callback and related functions.
Instead, we grab the property data at the moment of evaluation and cache
it.

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

Affected files:
  A input/regression/restore-property-to-original-value.ly
  M lily/grob-property.cc
  M lily/grob-scheme.cc
  M lily/grob-smob.cc
  M lily/grob.cc
  M lily/include/grob.hh
  M lily/include/lily-guile-macros.hh


Index: input/regression/restore-property-to-original-value.ly
diff --git a/input/regression/restore-property-to-original-value.ly  
b/input/regression/restore-property-to-original-value.ly

new file mode 100644
index  
..1a7c1df1e1e7bd190f844ad5b6e4d7068147e775

--- /dev/null
+++ b/input/regression/restore-property-to-original-value.ly
@@ -0,0 +1,24 @@
+\version "2.17.4"
+
+\header {
+  texidoc = "Properties can be restored to their original values.
+Below, the @code{NoteHead} stencil should print but its extent
+should not be taken into account in horizontal spacing, as horizontal
+spacing happens between calls to @code{before-line-breaking} and
+@code{after-line-breaking}.
+"
+}
+
+\relative c'' {
+  \override NoteHead #'before-line-breaking =
+#(lambda (grob)
+   ; trigger cache
+   (ly:grob-property grob 'stencil)
+   ; change the property
+   (ly:grob-set-property! grob 'stencil #f))
+  \override NoteHead #'after-line-breaking =
+#(lambda (grob)
+   ; restore the property
+   (ly:grob-restore-property-to-original-value! grob 'stencil))
+  a
+}
Index: lily/grob-property.cc
diff --git a/lily/grob-property.cc b/lily/grob-property.cc
index  
3afe182c0ef07793ecf1fd0d09a5ea147c9bd4e1..826b1e3f11d19d4aaaca7e13de65f20326e1b9d7  
100644

--- a/lily/grob-property.cc
+++ b/lily/grob-property.cc
@@ -164,6 +164,15 @@ SCM
 Grob::internal_get_property (SCM sym) const
 {
   SCM val = get_property_data (sym);
+  // if this is our first lookup, set the cache
+  SCM cached_handle = scm_sloppy_assq (sym,
+   cached_mutable_property_alist_);
+  if (cached_handle == SCM_BOOL_F)
+{
+  Grob *me = ((Grob *)this);
+  me->internal_set_value_on_alist (&me->cached_mutable_property_alist_,
+   sym, val);
+}

 #ifndef NDEBUG
   if (val == ly_symbol2scm ("calculation-in-progress"))
@@ -211,6 +220,21 @@ Grob::internal_get_maybe_pure_property (SCM sym, bool  
pure, int start, int end)
   return pure ? internal_get_pure_property (sym, start, end) :  
internal_get_property (sym);

 }

+void
+Grob::internal_restore_property_to_original_value (SCM sym)
+{
+  SCM handle = scm_sloppy_assq (sym, cached_mutable_property_alist_);
+  // We could issue warning if handle == SCM_BOOL_F, but it is possible
+  // that one wants to restore an original property before the property
+  // has been evaluated.  By definition, this means that the original
+  // property is still there.
+
+  if (handle != SCM_BOOL_F)
+internal_set_value_on_alist (&mutable_property_alist_,
+ sym, scm_cdr (handle));
+
+}
+
 SCM
 Grob::try_callback_on_alist (SCM *alist, SCM sym, SCM proc)
 {
Index: lily/grob-scheme.cc
diff --git a/lily/grob-scheme.cc b/lily/grob-scheme.cc
index  
81ff7864ff36b4b4ff972bb0fc3051d8700f60e5..e144be6f6a0884acc07f7f7408e59834731314e5  
100644

--- a/lily/grob-scheme.cc
+++ b/lily/grob-scheme.cc
@@ -179,6 +179,20 @@ LY_DEFINE (ly_grob_set_object_x, "ly:grob-set-object!",
   return SCM_UNSPECIFIED;
 }

+LY_DEFINE  
(ly_grob_restore_property_to_original_value_x, "ly:grob-restore-property-to-original-value!",

+   2, 0, 0, (SCM grob, SCM sym),
+   "Restore the property for property @var{sym} of @var{grob}."
+   " to its original value.")
+{
+  Grob *sc = unsmob_grob (grob);
+
+  LY_ASSERT_SMOB (Grob, grob, 1);
+  LY_ASSERT_TYPE (ly_is_symbol, sym, 2);
+
+  sc->restore_property_to_original_value (sym);
+  return SCM_UNSPECIFIED;
+}
+
 /* TODO: make difference between scaled and unscalead variable in
calling (i.e different funcs.) */
 LY_DEFINE (ly_grob_layout, "ly:grob-layout",
Index: lily/grob-smob.cc
diff --git a/lily/grob-s

Re: Corrects beamed cross-staff stems' pure heights (issue 6543046)

[mtsolo]

Reviewers: Keith,

Message:
I'm gonna hold off on posting to this one as it evolves constantly with
my work on issue 2801, which is revealing several issues in cross-staff
stem calculations.  Irrespective of how issue 2801 pans out, I'll take
all the relevant code from stem.cc and either push it as a separate
commit or include it in this Rietveld if my patch for 2801 isn't LGTM'd.

Description:
Corrects beamed cross-staff stems' pure heights

An incorrect subtraction made it such that the pure heighs were
too long for stems on extremal staves and too short on interior
staves.

Please review this at http://codereview.appspot.com/6543046/

Affected files:
  M lily/stem.cc


Index: lily/stem.cc
diff --git a/lily/stem.cc b/lily/stem.cc
index  
8069a456c814e48cf24bddec96bf9d8fae2279ac..43894123d7fc329bde7f9694edba29f042a8d298  
100644

--- a/lily/stem.cc
+++ b/lily/stem.cc
@@ -323,6 +323,7 @@ Stem::internal_pure_height (Grob *me, bool calc_beam)
   vector heights;
   vector my_stems;
   extract_grob_set (beam, "normal-stems", normal_stems);
+  // find the internal pure heights of all stems pointing in this  
direction

   for (vsize i = 0; i < normal_stems.size (); i++)
 if (get_grob_direction (normal_stems[i]) == dir)
   {
@@ -332,23 +333,27 @@ Stem::internal_pure_height (Grob *me, bool calc_beam)
   heights.push_back (iv);
 my_stems.push_back (normal_stems[i]);
   }
-  //iv.unite (heights.back ());
-  // look for cross staff effects
+
   vector coords;
   Grob *common = common_refpoint_of_array (my_stems, me, Y_AXIS);
   Real min_pos = infinity_f;
   Real max_pos = -infinity_f;
+
+  // find the offset of various staves as well as the minimum and  
maximum offset

   for (vsize i = 0; i < my_stems.size (); i++)
 {
   coords.push_back (my_stems[i]->pure_relative_y_coordinate  
(common, 0, INT_MAX));

   min_pos = min (min_pos, coords[i]);
   max_pos = max (max_pos, coords[i]);
 }
+  // Tack on the difference of the current stem's minimum-translation  
and the
+  // lowest or highest minimum translation to get the stem up to the  
axis

+  // group where the beam lies
   for (vsize i = 0; i < heights.size (); i++)
 {
   heights[i][dir] += dir == DOWN
- ? coords[i] - max_pos
- : coords[i] - min_pos;
+ ? min_pos - coords[i]
+ : max_pos - coords[i];
 }

   for (vsize i = 0; i < heights.size (); i++) iv.unite (heights[i]);



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: spacing-spanner: rods for non-adjacent paper-columns; issue 1700 (issue 6489107)

[mtsolo]

LGTM - looking forward to the skyline version, as that'll more
accurately reflect where columns are overhanging.

http://codereview.appspot.com/6489107/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

[mtsolo]

Reviewers: dak,


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode83
lily/chord-name-engraver.cc:83: || ly_is_procedure
(chord_name_->get_property ("text")));
On 2012/09/06 08:50:40, dak wrote:

If it is a procedure, shouldn't it be called with the calculated

value?

Right you are - I should have used get_property_data.  Will fix.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149: && ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 08:50:40, dak wrote:

If one is doing the chord calculation manually, you can't make the

decision of

whether a chord changed based on the automatic calculation.  For

better or

worse, you need to compare the computed chord versions/text.


To respond to your points above, I don't throw away the values above
because they're used here.  As for the present point, that is an
interesting conundrum...I'll  drum up some logic for that.  That may
eliminate the need for guarding the values above having to do with chord
changes, in which point the if else statement will be able to be
simplified.

Description:
Allows user to set ChordName text

Please review this at http://codereview.appspot.com/6496085/

Affected files:
  A input/regression/chord-name-override-text.ly
  M lily/chord-name-engraver.cc


Index: input/regression/chord-name-override-text.ly
diff --git a/input/regression/chord-name-override-text.ly  
b/input/regression/chord-name-override-text.ly

new file mode 100644
index  
..4acd549394eff095065c0b9605473424ef13c649

--- /dev/null
+++ b/input/regression/chord-name-override-text.ly
@@ -0,0 +1,13 @@
+\version "2.17.2"
+
+\header {
+  texidoc = "Users can override the @code{text} property of
+@code{ChordName}.
+"
+}
+
+\new ChordNames \chordmode {
+  a b c:7
+  \once \override ChordName #'text = #"foo"
+  d
+}
\ No newline at end of file
Index: lily/chord-name-engraver.cc
diff --git a/lily/chord-name-engraver.cc b/lily/chord-name-engraver.cc
index  
eab4d94d075c52ef95f0ae09449d6d9d184585c5..6aad475ebb5a0f61292a1d58c8fe68aafe64c1c6  
100644

--- a/lily/chord-name-engraver.cc
+++ b/lily/chord-name-engraver.cc
@@ -76,7 +76,14 @@ Chord_name_engraver::process_music ()
   SCM inversion = SCM_EOL;
   SCM pitches = SCM_EOL;

-  if (rest_event_)
+  chord_name_ = make_item ("ChordName",
+   rest_event_ ? rest_event_->self_scm () :  
notes_[0]->self_scm ());

+
+  bool make_markup = !(Text_interface::is_markup  
(chord_name_->get_property ("text"))
+   || ly_is_procedure (chord_name_->get_property  
("text")));

+
+  if (rest_event_ && !make_markup) { }
+  else if (rest_event_)
 {
   SCM no_chord_markup = get_property ("noChordSymbol");
   if (!Text_interface::is_markup (no_chord_markup))
@@ -125,17 +132,17 @@ Chord_name_engraver::process_music ()
   pitches = scm_sort_list (pitches, Pitch::less_p_proc);

   SCM name_proc = get_property ("chordNameFunction");
-  markup = scm_call_4 (name_proc, pitches, bass, inversion,
-   context ()->self_scm ());
+  if (make_markup)
+markup = scm_call_4 (name_proc, pitches, bass, inversion,
+ context ()->self_scm ());
 }
   /*
 Ugh.
   */
   SCM chord_as_scm = scm_cons (pitches, scm_cons (bass, inversion));

-  chord_name_ = make_item ("ChordName",
-   rest_event_ ? rest_event_->self_scm () :  
notes_[0]->self_scm ());

-  chord_name_->set_property ("text", markup);
+  if (make_markup)
+chord_name_->set_property ("text", markup);

   SCM chord_changes = get_property ("chordChanges");
   if (to_boolean (chord_changes) && scm_is_pair (last_chord_)



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[mtsolo]

The speed problem was twofold - some cruft in callbacks coupled with the
fact that I wasn't doing make cleans, so something about the way that
gcc was putting together the old .o files was slowing things down. I
learned my lesson: always do make clean before testing a patch.

A full make clean brings compilation time on Keith's test case down to:

PATCH

real0m7.324s
user0m6.944s
sys 0m0.192s

CURRENT MASTER


real0m7.407s
user0m6.828s
sys 0m0.304s

Usually about a 3% increase for scores with lots of cross staff slurs,
almost nothing otherwise.

Note that this is with an optimized build that is not debugging or
profiled enabled.  All of those flags add about 4ish%.

http://codereview.appspot.com/6498077/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[mtsolo]

On 2012/09/03 13:46:33, mike7 wrote:

On 3 sept. 2012, at 07:07, mailto:mts...@gmail.com wrote:



> On 2012/09/02 20:38:28, Keith wrote:
>> On 2012/09/02 06:25:58, MikeSol wrote:
>
>>> It's not a copy of the original slur because it is using
>>> pure heights and offsets.
>
>> I saw you interrogating SlurStub regarding its purity, but did not
> notice that
>> SlurStub took any different shape based on 'pure' estimates.
>
>> The SlurStubs in the regtest were shaped just like the printed

Slurs,

> but now I
>> see the difference in the Chopin prelude, with
>
>> \layout { \context { \Score
>>   \override SlurStub #'color = #green
>>   \override SlurStub #'transparent = ##f
>>   \override Slur #'color = #darkgreen
>>   \override PhrasingSlur #'color = #darkgreen }}
>
>> The SlurStubs are not sufficiently accurate to help, and they cost

me

> time.
>
>> Chopin op45:   patch   'skylines'  2.16
>>real0m21.666s   0m16.245s   0m12.862s
>>user0m20.814s   0m15.573s   0m12.232s
>>sys 0m0.240s0m0.254s0m0.217s
>
> I'm working on a new version of the patch.  There is no way to

improve

> accuracy of the curve, but the Y-offset is often wrong and that can

be

> improved.
>
> The time hike seems awfully expensive - there must be a way to

optimize

> it.  I'll post something that works when I have it and then we can
> figure out how to optimize it.
>
> http://codereview.appspot.com/6498077/
>
> ___
> lilypond-devel mailing list
> mailto:lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel



New version posted.  I'm also getting a 25% increase on the Op. 45

test case.

I am getting severe hikes across the board, though, even in scores

without cross

staff slurs.  What's odd is that the hikes are happening in the

"Drawing

systems" phase (I think), although I'm positive via prints to the

command line

that the SlurStub's control points and vertical skylines are never

being

calculated.



Lemme know if you see something slipping through the cracks - gprof

shows a hike

in stencil integral and skyline code for my patch, so it must be

generating

extra skylines somewhere.



Cheers,
MS


After doing some digging, it looks like that even if not a single new
grob is created (meaning if I comment out all SlurStub stuff in the
engraver), the time increase is still there.  It is tough to see where
this is coming from using profiling tools, but it seems like the hike is
in the Drawing Systems phase.  I'm guessing that it may have to do with
all of the maybe_pure stuff.  If anyone has time to do profiling I'd
appreciate help on this patch - it leads to an appreciable improvement
in several scores I've checked out w/ cross-staff slurs, but it
obviously can't be put on a countdown until the performance hit gets
taken care of.

http://codereview.appspot.com/6498077/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[mtsolo]

On 2012/09/02 20:38:28, Keith wrote:

On 2012/09/02 06:25:58, MikeSol wrote:



> It's not a copy of the original slur because it is using
> pure heights and offsets.



I saw you interrogating SlurStub regarding its purity, but did not

notice that

SlurStub took any different shape based on 'pure' estimates.



The SlurStubs in the regtest were shaped just like the printed Slurs,

but now I

see the difference in the Chopin prelude, with



  \layout { \context { \Score
\override SlurStub #'color = #green
\override SlurStub #'transparent = ##f
\override Slur #'color = #darkgreen
\override PhrasingSlur #'color = #darkgreen }}



The SlurStubs are not sufficiently accurate to help, and they cost me

time.


Chopin op45:patch   'skylines'  2.16
 real   0m21.666s   0m16.245s   0m12.862s
 user   0m20.814s   0m15.573s   0m12.232s
 sys0m0.240s0m0.254s0m0.217s


I'm working on a new version of the patch.  There is no way to improve
accuracy of the curve, but the Y-offset is often wrong and that can be
improved.

The time hike seems awfully expensive - there must be a way to optimize
it.  I'll post something that works when I have it and then we can
figure out how to optimize it.

http://codereview.appspot.com/6498077/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Dynamics do not unnecessarily horizontal shift for stems. (issue 6493073)

[mtsolo]

Reviewers: dak,

Message:
Thanks for the review!


http://codereview.appspot.com/6493073/diff/1/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

http://codereview.appspot.com/6493073/diff/1/lily/self-alignment-interface.cc#newcode213
lily/self-alignment-interface.cc:213: vector_sort (vais, less ());
On 2012/09/02 16:06:30, dak wrote:

Seriously?  If dir is UP, you are interested in the minimum, and if it

is DOWN,

you are interested in the maximum.  And you create a vector and sort

it for

that?



Totally inefficient as well as incomprehensible.


Good catch - changed to Interval_t

Description:
Dynamics do not unnecessarily horizontal shift for stems.

Please review this at http://codereview.appspot.com/6493073/

Affected files:
  A input/regression/dynamics-avoid-cross-staff-stem-2.ly
  M lily/self-alignment-interface.cc


Index: input/regression/dynamics-avoid-cross-staff-stem-2.ly
diff --git a/input/regression/dynamics-avoid-cross-staff-stem-2.ly  
b/input/regression/dynamics-avoid-cross-staff-stem-2.ly

new file mode 100644
index  
..e549646f75df457d18513c7b0bc5f9f24adcad1b

--- /dev/null
+++ b/input/regression/dynamics-avoid-cross-staff-stem-2.ly
@@ -0,0 +1,23 @@
+\version "2.17.2"
+
+\header {
+  texidoc = "Dynamics do not horizontally shift when attached to
+an axis-group extremal cross staff grob that's extremal side
+(UP or DOWN) is the same as its direction.
+"
+}
+
+\new PianoStaff <<
+  \new Staff = "up" {
+s1 |
+  }
+  \new Staff = "down" {
+\clef bass
+\stemDown
+% keep staff alive
+8 [ 8_\f
+\change Staff = "up"
+g' g' ]
+r2 |
+  }
+>>
Index: lily/self-alignment-interface.cc
diff --git a/lily/self-alignment-interface.cc  
b/lily/self-alignment-interface.cc
index  
a37b5871007eec8ef9368976958997b6a4c84a98..2a033340ff27c283350c43cb3fc7ffba7cedf2d6  
100644

--- a/lily/self-alignment-interface.cc
+++ b/lily/self-alignment-interface.cc
@@ -199,7 +199,24 @@ Self_alignment_interface::avoid_colliding_grobs (Grob  
*me, Axis a, Real offset)


   Interval iv = me->extent (me, a) + offset;
   for (vsize i = 0; i < colls.size (); i++)
-ivs.push_back (colls[i]->extent (refp, a));
+{
+  int my_vai = Grob::get_vertical_axis_group_index (colls[i]);
+  Direction dir = get_grob_direction (colls[i]);
+  // if coll is cross staff but extremal and poiting in the
+  // direction of the extrema, we don't take it into consideration
+  if (Grob *beam = unsmob_grob (colls[i]->get_object ("beam")))
+{
+  Interval_t vais;
+  extract_grob_set (beam, "normal-stems", stems);
+  for (vsize j = 0; j < stems.size (); j++)
+vais.add_point (Grob::get_vertical_axis_group_index  
(stems[j]));

+  // ugh...up and down are different for VerticalAxisGroup order...
+  if ((my_vai == vais[DOWN] && dir == UP)
+  || (my_vai == vais[UP] && dir == DOWN))
+continue;
+}
+  ivs.push_back (colls[i]->extent (refp, a));
+}

   Interval_minefield minefield (Interval (iv.center (), iv.center ()),  
iv.length ());

   for (vsize i = 0; i < ivs.size (); i++)



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[mtsolo]

Thanks for the review!


http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc#newcode419
lily/axis-group-interface.cc:419: */
On 2012/09/02 15:59:00, dak wrote:

See above.


Removed - was old code I needed to read to make sure I waas getting
stuff right.  Forgot it waas there.

http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc#newcode1004
lily/page-layout-problem.cc:1004: if (is_spaceable (staves[i]))
On 2012/09/02 15:59:00, dak wrote:

This indentation is simply wrong and does not match the program

structure.

Cruft. Fixed.

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119
lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) ==
slur_stubs_.end ())
On 2012/09/02 15:59:00, dak wrote:

It is quite nonsensical to have slur_stubs be a map indexed via

slurs_[j] rather

than just an array indexed through j.



That is inefficient use of time, memory, and complexity.


It is sensical because:
--) It avoids having two vectors (slur_stubs_ and end_slur_stubs_ would
be needed), thus making the code easier to read and maintain.
--) It has a "find" method built into it.

Unless people are crunching scores with thousands of slurs on Strong
Bad's original Tandy 400, I'm not too worried about the tradeoff.

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332
lily/phrasing-slur-engraver.cc:332: map vag_to_slur;
On 2012/09/02 15:59:00, dak wrote:

Again: why a map here?


find method, see above.
I know that find exists for vectors in algorithm, but I think this is
easier to read and more consistent.

http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc
File lily/script-column.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc#newcode60
lily/script-column.cc:60: if (unsmob_grob (i1->get_object ("slur")) &&
unsmob_grob (i2->get_object ("slur")))
On 2012/09/02 15:59:00, dak wrote:

If both scripts have a field "slur" that is a grob, return #t is the

avoid-slur

property of the second script is "outside" or "around" while that of

the first

is neither.



Why don't you write this in a comment?  Why this complex code?  And

what is the

ultimate purpose?


I didn't put it in a comment because it can be understood by reading it,
as you explain with your eloquent prose :-)

This exists to avoid the situation where a grob has a lower slur
priority than another but is typeset outside a slur whereas the other is
put inside, leading to a contradiction. Priority is given to the slur
directive.

http://codereview.appspot.com/6498077/diff/21/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/slur.cc#newcode623
lily/slur.cc:623: "thickness "
On 2012/09/02 15:59:00, dak wrote:

No entry for "surrogate"?


scm/define-grob-interfaces.scm

http://codereview.appspot.com/6498077/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[mtsolo]

Reviewers: Keith,

Message:
On 2012/09/01 23:58:37, Keith wrote:

I might have a test case for you at
http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776



It seems you copy each slur into a "slur-stub", and from those keep

only the

ones with "cross-staff" set. Then when figuring system skylines you

insert all

Grobs with the slur-stub-interface into the skylines for each staff.



Why not insert the original Slur into the skylines, if it has

"cross-staff"

instead of the SlurStub.  For that matter, why not insert all Grobs

marked

"cross-staff"?



all the cross-staff


It's not a copy of the original slur because it is using pure heights
and offsets.  The original slur can't be inserted into the
VerticalAxisGroup skylines because its height and offset depends on the
distance between axis groups, which is not known at the time the skyline
is created.  So we use the pure version (the stub) to build the system
skylines in page-layout-problem.cc. Note that we don't use it for the
minimum-translations (I made a minimum-translation-vertical-skylines
property for that) because we don't want cross-staff grobs to affect the
distance between two axis groups in the same system.  We want them just
to appear in the aggregate of all vertical axis groups smooshed into one
system, created in the function
Page_layout_problem::build_system_skyline.

The test case shows that the patch is not ready - I need to make it
overshoot less.  But the general idea will remain the same.

Description:
Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

The approximation is almost always an overshoot for the time being.
A more robust strategy would be to create SlurStubs for every
VerticalAxisGroup on which the cross-staff slur encompassed a
note column. Then, the skyline added would be the union of
all these skylines where the top skyline acted as a top bound
and the low skyline acted as a lower bound, so if intermediary
skylines went over these bounds, those parts of the skyline would
be thrown out. This can be done in a separate commit: it'd require
making a Skyline::crop function that crops one skyline by
another and would require creating multiple SlurStubs in the
engraver as well as figuring out the right amount to shift
them for each vertical axis group.

Please review this at http://codereview.appspot.com/6498077/

Affected files:
  A input/regression/cross-staff-slur-vertical-spacing.ly
  M lily/align-interface.cc
  M lily/axis-group-interface.cc
  M lily/figured-bass-position-engraver.cc
  M lily/grob.cc
  M lily/include/axis-group-interface.hh
  M lily/include/grob.hh
  M lily/include/slur-scoring.hh
  M lily/include/slur.hh
  M lily/melody-engraver.cc
  M lily/phrasing-slur-engraver.cc
  M lily/slur-engraver.cc
  M lily/slur-scoring.cc
  M lily/slur.cc
  M lily/tab-tie-follow-engraver.cc
  M scm/define-grob-interfaces.scm
  M scm/define-grob-properties.scm
  M scm/define-grobs.scm
  M scm/output-lib.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Ledger-line-spanner: symmetric extents; issue 2493 (issue 6490043)

[mtsolo]

http://codereview.appspot.com/6490043/diff/7001/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/6490043/diff/7001/lily/ledger-line-spanner.cc#newcode50
lily/ledger-line-spanner.cc:50: < Paper_column::get_rank
(previous_column)))
I'm having trouble following this naming convention.  It seems that
previous_column should have a rank falling before current column, not
after.

Also, it seems like if the variables are called this, then the order
should be correct when they are passed into the function (meaning the
one supposed to come before the other comes before the other).
Otherwise a programming error should be raised.  What would be the case
where the ranks of the columns would be reversed and what would justify
that happening?

http://codereview.appspot.com/6490043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

bar-line interface part 2/2 (issue 6498052)

[mtsolo]

LGTM

http://codereview.appspot.com/6498052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Thanks for the performance tests!  I have no problem changing the
function avoid_outside_staff_collisions to be faster - it's just that I
haven't wrapped my head around how distance alone can do it.


http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode945
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/18 02:42:37, Keith wrote:

On 2012/08/17 17:16:25, MikeSol wrote:
> On 2012/08/17 08:12:56, Keith wrote:
> If you comment out the rider business, the vertical-skylines will

not be

correct
> for axis-group-interface.



That is very subtle, very minor, changes only the debug output, not

what would

normally be printed from that example, and is different from the

comment

indicates.


\relative c'' {
   \override TupletBracket #'outside-staff-priority = #1
   \override TupletNumber #'font-size = #5
   \times 2/3 { a4\trill a\trill^"foo" a\trill }
}

I added this as a regtest.  If you comment out the rider business,
you'll see that foo is printed on top of the tuplet number.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode697
lily/axis-group-interface.cc:697: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance ((*pair)[-d]));
On 2012/08/18 02:42:37, Keith wrote:

When d = dir, the direction we intend to move, this tells us how far

we need to

move, but when d = -dir, this tells us by how far we have encroached

into the

next obstacle, which we will eventually need to hop over.



What you really want to put on the bumps list is how far we need to

move to hope

over this next obstacle.  But we only tested the relevant skylines for
'intersection' ... if only we had stached them distance().


I'm not sure what you mean when you say "we only tested the relevant
skylines for intersection ... if only we had stached them distance()" -
one because I don't know what stached means in this context (from urban
dictionary: The art of placing a ficticious mustache on an individual or
oneself, drawing laughter and humor to this occurance.) and two because
every intersection call is followed by a distance call, so there is
bumping going on in this case.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode701
lily/axis-group-interface.cc:701: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance (to_flip));
On 2012/08/18 02:42:37, Keith wrote:

This finds the distance to move such that the top skyline of the new

object just

encloses, Russian egg doll style, one of the already-placed skylines.

We never

want that to be the final position, so don't put it on the bumps list.


That is a possible scenario, but Russian egg doll style enclosing is
tested for below, so even if this does happen it'd be rectified on the
next pass.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode732
lily/axis-group-interface.cc:732: pair->raise (min_bump);
On 2012/08/18 02:42:37, Keith wrote:

   printf("bumping %.3f\n", min_bump);
shows that for a simple case
   \relative f {f'^"mouse" c'' f,,^"K" d''}
we bump around quite a lot:
"mouse"
bumping 0.567
bumping 1.894
"EEEK"
bumping 0.443
bumping 0.048
bumping 0.095
bumping 0.191
bumping 0.382
bumping 0.459
bumping 1.344
bumping 0.161
bumping 0.322
bumping 0.578


Agreed.  I'd love for this to go faster.
I am for getting this into current master ASAP so that people can work
on speeding it up - I'm not at all adverse to the idea of using distance
the way you're talking about, but I don't completely understand how it'd
work yet and it seems like something that could be done in a separate
commit.  What'd be nice about having this in master is that it'd be
easier to spot in the regtests if efficiency changes to the algorithm
have a layout impact.  Of course, one could run regtests with this patch
as a baseline, but it is a pain.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

I've forgotten why I added the horizontal skyline stuff so I've taken it
out in a new version that I'll post after regtests.


http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: are avoided
On 2012/08/17 11:19:26, joeneeman wrote:

Have you measured the costs of not having exempt?


I've decided to scrap exempt in a new version - will upload soon.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode780
lily/axis-group-interface.cc:780: while (dirty);
On 2012/08/17 08:12:56, Keith wrote:

I am beginning to understand the new code. Would you consider again

using

distance() instead of the repeated calls to intersects() in the

while(dirty)

loop ?  It might look simpler now that you've been away from the code

for a

while.



The Skyline::distance()s have all the information you want.  If you

are trying

to place, by moving UP, an object with skylines 'pair' among a set of

skylines

'forest[j]' then the distance you need to move has several constraints



floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
ceilings[j] = forest[j][DOWN]~distance~pair[UP]



You know the solution will be to move an amount equal to one of the

floors[j],

and you want to find the smallest that fits, so make
   trials = sort (floors)
and find the smallest among trials that satisfies
trials[n] > 0
&&  for all j (trials[n] > floors[j]
|| trials[n] < ceilings[j] )



Somebody might complain that this is quadratic in the number of

objects to

place, but the problem placing N objects while looking for holes left

in earlier

placements is a quadratic task.  But now the inner loop is simple

comparisons,

rather than re-checking of skyline intersections and distances with

the

while(dirty) method.



In any case, the number N seems to be sufficiently limited in practice

(thanks

maybe to 'exempt') based on reported score compilation times.


Hey Keith,

I am not adverse to using distance if possible - my trouble case is the
following.

A

B

Image that object A above is placed first and LilyPond is now
considering whether it should leave B where it is or place it above A.
The distance function would dictate that it needs to be shifted above A
because its DOWN skyline is below A's UP.  However, it never intersects,
so there's no need to shift it.  This is why I'm looking for
intersections. Maybe I'm thinking of it incorrectly, though?

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode945
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/17 08:12:56, Keith wrote:

If I remove all additions to the 'riders' array,
'tuplet-number-outside-staff-priority.ly' stays the same.



I tried, but failed, to create a case where keeping track of 'riders'

helps.

   \relative c'' {
 \override TupletBracket #'outside-staff-priority = #1
 \times 2/3 { a8\trill a\trill a\trill } }
What case did you use ?


\version "2.17.0"
#(ly:set-option 'debug-skylines #t)
 \relative c'' {
   \override TupletBracket #'outside-staff-priority = #1
   \times 2/3 { a4\trill a\trill a\trill } }

If you comment out the rider business, the vertical-skylines will not be
correct for axis-group-interface. Comment it back in and they will.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode778
lily/axis-group-interface.cc:778: Note we only use 75% of padding and
apply the full compliment only if
On 2012/08/14 04:21:33, Keith wrote:

It looks like you add 50% padding here, then pass to
avoid_outside_staff_collision() which removes 37.5% of padding away.

Probably

not what you meant to do.



The verbs 'use' and 'apply' are vague enough that the comment didn't

help me see

what you meant to do.


Sorry, I see the problem now (I missed the 0.75).
I brought back the logic of the comment above & it looks like it
obviates the need for this whole 50%-of-padding thing.  Running regtests
now.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Thanks for the review!


http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly
File input/regression/text-script-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly#newcode5
input/regression/text-script-vertical-skylines.ly:5: kerning.
On 2012/08/14 04:21:33, Keith wrote:

It is not obvious we want this for TextScripts,
   {b'^"Élever" b'^"brusquement"}
so don't enshrine the requirement in a regtest.


Done.

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly
File input/regression/volta-bracket-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly#newcode4
input/regression/volta-bracket-vertical-skylines.ly:4: texidoc = "Volta
brackets are vertically kerned to objects below them.
On 2012/08/14 04:21:33, Keith wrote:

Good, but "fit" not "kerned"
To me, 'to kern' includes making many aesthetic judgments to come up

with

individual adjustments for each fitting pair.


Done.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode629
lily/axis-group-interface.cc:629: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:

I don't understand the comment, but what I needed to figure out was :
Given pre-padded vertical skylines 'pair' for an outside-staff Grob,

this

figures out the vertical position of the Grob.  It raises the skylines

in 'pair'

and shifts (along the skyline) 'horizontal_skylines' by the same

amount.


Using horizontal_skylines (with the buildings pointing horizontally)

to

determine vertical position, is a new concept worth mentioning.



I'm still figuring out 'exempt', '*_forest', etc.


Better comment added.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: check for horizontal edges jinnying up
On 2012/08/14 04:21:33, Keith wrote:

I learned the local dialect 200 miles north, so I don't know what this

means. Is

this the case where the edges lie side a by each, or kitty corner ?


Elaine Gould uses the phrase "jinnying up" in behind bars, and given
that we are using that as one of our base texts, I'm ok using the phrase
here.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode728
lily/axis-group-interface.cc:728: We need to take the padding away now
that it has been shifted.  Otherwise
On 2012/08/14 04:21:33, Keith wrote:

I can't find where the padding was ever added.


The call to grow in add_grobs_of_one_priority.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode751
lily/axis-group-interface.cc:751: edge of the just-placed grob).
Otherwise, we skip it until the next pass.
On 2012/08/14 04:21:33, Keith wrote:

This comment describes the old way of solving the problem you saw with
'midi-dynamics.pdf' but you removed the old code.  Maybe you want to

use the old

method, but make code and comment agree one way or another.


Excellent observation - I'd completely forgotten this. Perhaps this
takes care of the midi-dynamics.ly problem altogether...will
investigate.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode778
lily/axis-group-interface.cc:778: Note we only use 75% of padding and
apply the full compliment only if
On 2012/08/14 04:21:33, Keith wrote:

It looks like you add 50% padding here, then pass to
avoid_outside_staff_collision() which removes 37.5% of padding away.

Probably

not what you meant to do.



The verbs 'use' and 'apply' are vague enough that the comment didn't

help me see

what you meant to do.


I think that both remove 50%, no?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode843
lily/axis-group-interface.cc:843: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:

Does this part of the comment apply to the code below ?


The comment's blech...I'll redo it in a later patchset (remind me if I
don't).

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode890
lily/axis-group-interface.cc:890: feature present in one regrest :(
On 2012/08/14 04:21:33, Keith wrote:

You tease us.  Which regression test ?


Done.

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc#newcode1368
lily/beam.cc:1368: shift += 0.01;
On 2012/08/14 04:21:33, Keith wrote:

* beamdir ?


Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc
File lily/skyline-pair.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc#newcode103
lily/skyline-pair.cc:103: // other[UP] then we will not interse

Re: Set indent based on instrument name (issue 6457049)

[mtsolo]

Hey Phil!

First and foremost, congrats on this work.  I'm thrilled to see you
venturing into the C++ side.  You're tackling an issue that, while just
a few lines of code, uses a lot of advanced LilyPond structures, so it's
not easy.  My suggestions don't have to do with the math (all of it is
fine) but rather a way to restructure your work so that it fits better
into how LilyPond code works.

At the engraver level, it is generally not a good idea to touch layout
information.  When this happens, it is usually to kill grobs or set
properties.  Avoid measuring extents when engraving is happening because
they could be dependent on other callbacks which could trigger many
layout decisions before engraving is finished.

Here, what I would do is use the Pointer_group_interface to add two grob
arrays to the root system of instrument names - one that contains all
InstrumentName grobs with instrumentName and one that contains all
InstrumentName grobs with shortInstrumentName.  You can stash these in
two separate vectors and then loop through the vectors, invoking
Pointer_group_interface::add_grob in a finalize method for the engraver.
 Then, create two properties of the system grob called
instrument-name-length and short-instrument-name-length and two
callbacks that look up the appropriate grob array and do the X-extent
calculations you do in the engraver.  To avoid code-duping, they can
likely use an internal function for most of their work.  Properties, in
LilyPond, are the best way to stash information.  You almost never want
to create global variables or class member variables to do this.

Then, in paper-score.cc, create functions
  Real Paper_score::get_instrument_name_length ()
and
  Real Paper_score::get_short_instrument_name_length ()
These function will check to see if there is one or many systems and, if
so, glean the instrument name length or short instrument name length
from that.  Otherwise, it will return 0.

Then, for line_dimensions_int (Output_def *def, int n), make two extra
arguments where you pass in the results of these functions.  These will
replace the global variables you're creating in output-def.cc.  It works
because line_dimensions_int is only ever called when a Paper_score is
available, so you can always interrogate the Paper_score for the
appropriate indent values before making the function call.

These changes will make the code more maintainable and less prone to
kick up bugs in other areas.  Let me know if you have any questions!

Cheers,
MS


http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc
File lily/instrument-name-engraver.cc (right):

http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode116
lily/instrument-name-engraver.cc:116: SCM Text_scheme =
Text_interface::interpret_markup (layout->self_scm (),
Ditto for variable names - use lowercase.

http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode121
lily/instrument-name-engraver.cc:121: Real Text_len =
Text_int[MAX]-Text_int[MIN];
Real text_len = text_int.length ();

http://codereview.appspot.com/6457049/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode292
lily/skyline.cc:292: Real p1 = left->end_ * left->slope_ +
left->y_intercept_;
On 2012/07/14 14:01:08, joeneeman wrote:

use left->height (left_end_) instead of calculating it by hand


Done.

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode293
lily/skyline.cc:293: Real p2 = i->start_ * i->slope_ + i->y_intercept_;
On 2012/07/14 14:01:08, joeneeman wrote:

i->height (i->start_)


Done.

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302
lily/skyline.cc:302: while (dirty);
On 2012/07/14 14:01:08, joeneeman wrote:

I don't understand this do while (dirty) part. It seems to me that

unless you

have two empty segments in a row, you only need one pass. On the other

hand, if

you have two empty segments in a row then this function will fail

anyway because

you will end up setting center->slope_ and center->y_intercept_ both

to

-infinity, which is bound to start creating NaN sooner or later.


I'll rewrite this so that it combines all interior skylines with an
infinite y intercept and then only does one pass.

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh
File lily/include/skyline.hh (right):

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newcode58
lily/include/skyline.hh:58: NOT_ENOUGH_INFO
On 2012/07/14 14:01:08, joeneeman wrote:

I wish I could convince you to think of a skyline as a region instead

of just

the boundary of that region. Once you think of it that way, it becomes

clear

that this information can be easily obtained from the Skyline_pair,

using the

existing distance function.



Suppose s and t are Skyline_pairs.
max(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) = -infinity
means the objects don't overlap horizontally at all, so it's

meaningless to talk

about which one is higher (I think this is what you're calling

NOT_ENOUGH_INFO).


min(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) > 0
means that the objects intersect.



s[UP].distance(t[DOWN]) <= 0 and s[DOWN].distance(t[UP]) > 0
means that s is below t



t[UP].distance(s[DOWN]) <= 0 and t[DOWN].distance(s[UP]) > 0
means that t is below s


This logic uses two calls to distance, which is expensive, whereas using
the e-num gets this info from one call to distance.  I can use the
method you propose, but don't you think that'd cause a performance hit?

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc#newcode796
lily/skyline.cc:796: */
On 2012/07/14 14:01:08, joeneeman wrote:

Since padding doesn't seem to be involved in this function, you should

delete

this comment.


Done.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode693
lily/skyline.cc:693: Skyline::padded (Real horizon_padding) const
On 2012/07/01 23:04:17, Keith wrote:

The horizon_padding built in to Skyline::Skyline had nice beveled

corners; does

this result in nice beveled corners ?


It did - I want to get Joe's feedback first before moving back large
chunks of the old code, as he was the one who created a lot of this new
Skyline stuff and I want to make sure I'm understanding it correctly.

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode699
lily/skyline.cc:699: if (last_end > -infinity_f)
On 2012/07/01 23:04:17, Keith wrote:

I don't see any assignments to 'last_end' so this seems to be always

false.

True - that's one of the problems.  The other is that height is always
an empty interval.  I've rewritten it as:


Skyline
Skyline::padded (Real horizon_padding) const
{
  vector pad_boxes;
  Real last_end = -infinity_f;
  for (list::const_iterator i = buildings_.begin (); i !=
buildings_.end (); ++i)
{
  if (last_end > -infinity_f)
{
  // Add the box that pads the left side of the current
building.
  Real start = last_end - horizon_padding;
  Real height = i->height (last_end);
  if (height > -infinity_f)
pad_boxes.push_back (Box (Interval (start, last_end),
  Interval (min (height - 0.01,
height), max (height - 0.01, height;
}

  if (i->end_ < infinity_f)
{
  // Add the box that pads the right side of the current
building.
  Real start = i->end_;
  Real end = start + horizon_padding;
  Real height = i->height (start);
  if (height > -infinity_f)
pad_boxes.push_back (Box (Interval (start, end),
  Interval (min (height - 0.01,
height), max (height - 0.01, height;
}
}

  // We created pad_boxes assuming that sky_ is UP.  To get padded to
use
  // the UP side of the boxes, we need to create it pointing UP even if
  // it doesn't really.
  Skyline padded (pad_boxes, X_AXIS, UP);
  padded.sky_ = sky_;

  padded.merge (*this);
  return padded;
}

and still no result as expected...

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

It's back...

The only thing that doesn't the work the way I'd expect it is
Skyline::padded.  It seems to not be adding the padding correctly (Joe -
any ideas)?  This causes tighter than desired spacing in
alignment-order.ly.  Other than that it is all reviewable!  For those
who were keeping score before, files like size16.ly, size20.ly etc. were
a major nuisance and are now kept at bay.

Cheers,
MS

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: add general_alignment (issue 2613) (issue 6308093)

[mtsolo]

http://codereview.appspot.com/6308093/diff/10001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/6308093/diff/10001/scm/define-grob-properties.scm#newcode984
scm/define-grob-properties.scm:984: (Y-alignment ,list? "3-element list
specifying how an object is
Consider using a vector for this property - it is more in keeping w/
other 3-element structures in the code base (break alignment, etc.).

http://codereview.appspot.com/6308093/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Issue 1320: Scheme bar line interface (issue 6305115)

[mtsolo]

Good work - two overall things.

1) In Scheme, try to avoid `do'.  Use map and reduce.
2) Have you tested performance hits on large scores?  It may be worth it
to leave bar-line.cc as the default and have your script as a Scheme
implementation, not unlike the way we do bezier curves.

Cheers,
MS


http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc#newcode31
lily/bar-line.cc:31: #include 
Is it worth it to keep this file?  I'd just move what's left over to
Scheme at that point.

http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc
File lily/span-bar.cc (right):

http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc#newcode36
lily/span-bar.cc:36: Pointer_group_interface::add_grob (me,
ly_symbol2scm ("elements"), b);
Ditto - you can likely get rid of this as well.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode80
scm/bar-line.scm:80: (cons (- height half-staff) (+ height half-staff))
(interval-widen half-staff height)

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode94
scm/bar-line.scm:94: (stencil empty-stencil))
As you use let*, you can move all the set! to redefinitions of stencil
in the let*. It seems slightly more in keeping w/ coding practice (could
be wrong, though).

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode118
scm/bar-line.scm:118:
To be more scheme-ic, try a map to get an array of stencils w/ the
correct offsets and then using reduce on the list of stencils to smoosh
them together via ly:stencil-add.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode150
scm/bar-line.scm:150: stencil)
Ditto - as a general rule, avoid `do'. Map and reduce.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode214
scm/bar-line.scm:214: X RIGHT colon-stil kern))
Can you turn the above into the result of a call to a recursive function
that uses ly:stencil-combine-at-edge?

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode495
scm/bar-line.scm:495: ;; bulky side. Rewritten by Han-Wen. Ported from
c++ to Scheme by Marc Hohl.
Can you get this down to the 80 character line width?

http://codereview.appspot.com/6305115/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Treat accidentals parentheses as cautionary (issue 6310065)

[mtsolo]

LGTM

http://codereview.appspot.com/6310065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issues warning for negative-spanning line spanner. (issue 6302097)

[mtsolo]

Reviewers: Keith,

Message:
Thanks for the review!


http://codereview.appspot.com/6302097/diff/1/lily/line-spanner.cc
File lily/line-spanner.cc (right):

http://codereview.appspot.com/6302097/diff/1/lily/line-spanner.cc#newcode373
lily/line-spanner.cc:373: me->warning (_ ("Line spanner's left point is
to the right of its right point."));
On 2012/06/20 05:31:50, Keith wrote:

if 'me' supports me->origin()->warning()
it will tell the user which line spanner(s)


::origin is only defined for stream events, music, context defs and
books.

Description:
Issues warning for negative-spanning line spanner.

Please review this at http://codereview.appspot.com/6302097/

Affected files:
  M lily/line-spanner.cc


Index: lily/line-spanner.cc
diff --git a/lily/line-spanner.cc b/lily/line-spanner.cc
index  
2d43a71fa4f0006943990e89e390fcdf4c71b28b..f42628820e6e64b6a0ba019d93f0bf8666125472  
100644

--- a/lily/line-spanner.cc
+++ b/lily/line-spanner.cc
@@ -21,6 +21,7 @@
 #include "axis-group-interface.hh"
 #include "font-interface.hh"
 #include "grob-interface.hh"
+#include "international.hh"
 #include "item.hh"
 #include "lily-proto.hh"
 #include "line-interface.hh"
@@ -368,6 +369,8 @@ Line_spanner::print (SCM smob)
 arrows[LEFT],
 arrows[RIGHT]));
 }
+  else
+me->warning (_ ("Line spanner's left point is to the right of its  
right point."));


   line.translate (Offset (-me->relative_coordinate (commonx, X_AXIS),
   simple_y ? 0.0 : -me->relative_coordinate  
(my_common_y, Y_AXIS)));




___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 13:43:09, dak wrote:

Here is a code example from beam.cc:



   Interval weights (1 - multiplier, multiplier);



   if (feather_dir != LEFT)
 weights.swap ();



This is _hogwash_.  weights is not an _interval_ here, but a pair of

numbers.

Swapping the bounds of an interval does not even make _sense_.  Where

is the

point in type abstraction if one creates total conceptual chaos

without inherent

relation to the invariants of the mathematical model underlying the

type?


swap never should have been a public member function (it is used

internally when

mirroring intervals).



If a type is named "Interval", it needs to be employed as an Interval,

not as

something totally different that relies on implementation details.



Otherwise type abstraction makes code _less_ maintainable rather than

more,

since you always need to take all side-effects into consideration.


Dunno - it sounds like time for clean up.

Y-position of beams were stored as intervals for years (they may still
be - I forget).

This sounds like a pretty major task, so as always, I'd touch base w/
Han-Wen to see what Intervals were supposed to be at their inception and
then evaluate what they've grown into.

We can then firm up an Interval API and write a strongly-worded comment
in interval.hh and interval.tcc NOT to touch either of these files.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 13:22:10, dak wrote:

On 2012/06/12 12:54:40, MikeSol wrote:
> On 2012/06/12 12:49:45, dak wrote:
> > http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
> > File lily/grob.cc (right):
> >
> >

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

> > lily/grob.cc:472: real_ext[d] += offset;
> > On 2012/06/12 12:32:37, dak wrote:
> > > I don't understand this.  The only way to get a nan from adding

an offset

to
> > > infinity is by adding another nan or an infinite offset with

different

sign.
> > >
> > > What case is this supposed to catch?
> >
> > So what you actually meant to say was
> >   if (!real_ext.is_empty ())
> > real_ext.translate (offset);
> >
> > If that's what you mean, why don't you write it instead of some

puzzle?

>
> This is not what I mean.  An empty interval is, in LilyPond, an

interval whose

> left is greater than it's right.



I disagree.  An empty interval is an interval not containing any

point.  The

details are not important.



> So (3 . 2) is empty, but (3 . 2) can be
> translated by an infinite value in either direction and not result

in nan.


What meaning is there in "translating" an "empty" interval and

afterwards

getting an interval no longer being empty since its lower bound no

longer is

smaller than its upper bound?



Are you, by chance, confusing an _interval_ with a tuple of points?

You can

_implement_ a closed interval as a tuple of points, but that does not

lend

meaning to "shifting an empty interval".



> So just checking for emptiness isn't enough.



For mimicking all side effects of the current implementation, it

isn't.  But if

some code relies on those side effects, it is broken in my opinion.

If you want

to manipulate a two-dimensional vector without inherent relation

between its two

elements, use a Drul, not an Interval.



> > And frankly, shouldn't we rather put this into

flower/include/interval.hh

> > instead of trying to catch this in arbitrary places in our code?
>
> I had toyed with this idea - this is a bit out of my league, as I

don't know

if
> LilyPond will take a performance hit if we add extra operations to

something

as
> elemental as translate or is_empty.



Every piece of inherent safety takes a performance hit.  Personally I

think that

the only case where we have is_empty true is for (+inf, -inf).  And I

don't mean

that we should change the test: the test is fine.  There just isn't a
combination of interval bounds that makes more sense.  We could

equally well

define the empty interval as (0, -1) and get consistent behavior from

that.  But

while the interval is empty, its bounds should not be interpreted as

conveying

meaning.


If all empty intervals are the same then, by design, we need to make
sure that all of them equal (+inf.0 . -inf.0).  This means that:

--) Interval should have a method set_to_empty () that sets it to
(+inf.0 . -inf.0) (maybe it already does).
--) Anytime an interval is set to its left being greater than its right
such that it is not infinitely empty, a programming error should be
issued.
--) Perhaps all math done on empty intervals should raise programming
errors.

I'm all for making the code more coherent.  It sounds like we're talking
about a much deeper problem than this patch, and perhaps it's wiser to
come up w/ a solution to that first before pushing this.

Cheers,
MS


http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 12:49:45, dak wrote:

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):



http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

lily/grob.cc:472: real_ext[d] += offset;
On 2012/06/12 12:32:37, dak wrote:
> I don't understand this.  The only way to get a nan from adding an

offset to

> infinity is by adding another nan or an infinite offset with

different sign.

>
> What case is this supposed to catch?



So what you actually meant to say was
   if (!real_ext.is_empty ())
 real_ext.translate (offset);



If that's what you mean, why don't you write it instead of some

puzzle?

This is not what I mean.  An empty interval is, in LilyPond, an interval
whose left is greater than it's right.  So (3 . 2) is empty, but (3 . 2)
can be translated by an infinite value in either direction and not
result in nan.  So just checking for emptiness isn't enough.

An infinite value in an interval should not be translated by another
infinite value.  It either does nothing (if they both have the same
sign) or results in a nan.


And frankly, shouldn't we rather put this into

flower/include/interval.hh

instead of trying to catch this in arbitrary places in our code?


I had toyed with this idea - this is a bit out of my league, as I don't
know if LilyPond will take a performance hit if we add extra operations
to something as elemental as translate or is_empty.  If this is
appropriate, then the check can go there.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 12:32:37, dak wrote:

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):



http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

lily/grob.cc:472: real_ext[d] += offset;
I don't understand this.  The only way to get a nan from adding an

offset to

infinity is by adding another nan or an infinite offset with different

sign.


What case is this supposed to catch?


Sometimes there is an infinite offset (either positive or negative)
which, when added to an empty interval of (+inf.0 . -inf.0), will create
nan (or -nan) in one of the two directions.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/11 07:48:34, dak wrote:

On 2012/06/11 07:24:33, MikeSol wrote:
> On 2012/06/11 07:10:48, dak wrote:
>
> > Personally, I consider it an accident waiting to happen if downing

the

stencil
> > is not enough to suppress its consideration.
> >
> > _Iff_ there is a situation where it is really required to have a

non-zero

> > height, then setting the stencil to ##f is the wrong measure: it

should be a

> > point-stencil instead.
>
> So when stencil is #f for stems, the height should always be empty?

In this

> case, patch set 2 was the correct approach, and the fact that it

failed

regtests
> reveals a hidden bug in the handling of beamed rests under tuplets.



Not necessarily a hidden bug, but rather a hidden expectation.  If

LilyPond

will, when setting the stencil to #f for some reason of its own,

accompany this

with consistent height data, then the behavior might be consistent and

not

exhibit problems.



Passing information with that sort of secret contract, however, is a

bad idea

when users and/or other programmers are expected to be able to

unstencil grobs.

In that case, passing information in the height (or otherwise

manipulating it)

without taking the nonexistence of the stencil into consideration is a

rather

bad idea.



> If you think this is the best way to tackle it, then I'll do that.

It'll

likely
> mean a larger patch as it'll unearth some bugs that didn't surface

with the

stem
> work in 2.15.



As I said: these may not necessarily bugs but rather larvae: things

that are

rather certain to grow into bugs under reasonably natural conditions.



I think it would be worth the trouble to get rid of them.  In case we

have code

that relies on passing information through the Y-height of a stencil

even when

the stencil has been set to #f by the user, this will require finding

a

different solution.


My compromise solution in the most recent patch set is to use Keith's
idea but to leave a comment so that people know that beam, stencil-less
stems are treated as having a height of 0 at the point of the beam
whereas unbeamed stems are heightless.  The former is the case because
otherwise the beam doesn't know where to go - it seems like a reasonable
exception.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/11 07:10:48, dak wrote:

On 2012/06/11 05:34:23, MikeSol wrote:



> This shows a case where stem height cannot be determined by stem

stencil

> presence.  The first version of the patch works under the assumption

that

> information about height cannot be gleaned from the stencil and must

be made

> explicit through overrides.  I think that, even though this requires
redundancy
> (specifying no stencil and an empty height), if LilyPond treats

cases where

> there is no stencil and a non-empty height (like

beam-stem-stemlet.ly), then

the
> method in the first patch set is better.



What do you mean with "treats cases"?  That it implements some useful

behavior

based on the exact given height?  Or merely that it catches the case?



Personally, I consider it an accident waiting to happen if downing the

stencil

is not enough to suppress its consideration.



_Iff_ there is a situation where it is really required to have a

non-zero

height, then setting the stencil to ##f is the wrong measure: it

should be a

point-stencil instead.


So when stencil is #f for stems, the height should always be empty?  In
this case, patch set 2 was the correct approach, and the fact that it
failed regtests reveals a hidden bug in the handling of beamed rests
under tuplets.

If you think this is the best way to tackle it, then I'll do that.
It'll likely mean a larger patch as it'll unearth some bugs that didn't
surface with the stem work in 2.15.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Returns an empty interval for stencilless Stem height (issue 6303065)

[mtsolo]

On 2012/06/11 03:44:32, Keith wrote:

http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc
File lily/stem.cc (right):



http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc#newcode703

lily/stem.cc:703: if (calc_beam && !unsmob_stencil (me->get_property
("stencil")))
beam-stem-stemlet.ly shows that LilyPond produces no stencil for the

invisible

stems on beamed rests, yet she depends on stem extents for tuplet

brackets.

Maybe
   if (calc_beam && !beam && !stenxil) ?


This shows a case where stem height cannot be determined by stem stencil
presence.  The first version of the patch works under the assumption
that information about height cannot be gleaned from the stencil and
must be made explicit through overrides.  I think that, even though this
requires redundancy (specifying no stencil and an empty height), if
LilyPond treats cases where there is no stencil and a non-empty height
(like beam-stem-stemlet.ly), then the method in the first patch set is
better.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

Reviewers: ,

Message:
Another way of going about this would be changing the Stem::height
function so that it returned an empty interval for stencil-less stems.
Then the overrides wouldn't be necessary.  It's a design question more
than anything else.

Description:
Sets TabVoice Stem height to ##f

Please review this at http://codereview.appspot.com/6303065/

Affected files:
  M lily/grob.cc
  M ly/engraver-init.ly
  M ly/property-init.ly


Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index  
cc39c979f0e9cf146b40975b01f82ed9441d08bc..828ae7f07b40687457f8031475f0a42d9992dc67  
100644

--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -466,7 +466,10 @@ Grob::extent (Grob *refp, Axis a) const
   ((Grob *)this)->dim_cache_[a].extent_ = new Interval (real_ext);
 }

-  real_ext.translate (offset);
+  // We never want nan, so we avoid shifting infinite values.
+  for (LEFT_and_RIGHT (d))
+if (!isinf (real_ext[d]))
+  real_ext[d] += offset;

   return real_ext;
 }
Index: ly/engraver-init.ly
diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly
index  
9bb731883cba5d9754fdc7c2030508b49bde06f4..2c28af1f23f0472b34887bed7e3cbf3fa577d330  
100644

--- a/ly/engraver-init.ly
+++ b/ly/engraver-init.ly
@@ -825,6 +825,9 @@ context."
   %% after all, the stubs of the stems may still be visible, so ...
   \override Stem #'stencil = ##f
   \override Flag #'stencil = ##f
+  %% and they certainly don't have heights...
+  \override Stem #'Y-extent = ##f
+  \override Flag #'Y-extent = ##f
   %% automatic beams should be suppressed for similar reasons ...
   autoBeaming = ##f
   %% remove beams, dots and rests ...
Index: ly/property-init.ly
diff --git a/ly/property-init.ly b/ly/property-init.ly
index  
c0b352254ad0f195cb20d99465180ca5c706440a..edd117518a458c76460a277b48982c24cd048f2d  
100644

--- a/ly/property-init.ly
+++ b/ly/property-init.ly
@@ -451,6 +451,8 @@ tabFullNotation = {
   \revert TabVoice.Stem #'details
   \revert TabVoice.Stem #'stencil
   \revert TabVoice.Flag #'stencil
+  \revert TabVoice.Stem #'Y-extent
+  \revert TabVoice.Flag #'Y-extent
   \override TabVoice.Stem #'stencil =  
#tabvoice::draw-double-stem-for-half-notes
   \override TabVoice.Stem #'X-extent =  
#tabvoice::make-double-stem-width-for-half-notes

   \set TabVoice.autoBeaming = ##t



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Footnotes correctly printed on grobs at first timestep. (issue 6306064)

[mtsolo]

On 2012/06/10 16:09:03, dak wrote:

On 2012/06/10 15:58:25, MikeSol wrote:
> This fixes Issue 2574 but also deals with the

footnote-break-visibility

regtest,
> which currently does not register the property change (this may have

something

> to do with the footnote engraver being on the Score level).



I'll readily admit that the footnote engraver being at Score level may

be a

source for new problems.  However, automatic footnotes get a number

for each

time they hit an engraver, and footnotes may occur at pretty much any

level.  So

I don't really see a way around having the engraver registered at

Score level by

default.



Moving it to lower levels may be a user-level option, but it comes at

the cost

of some elements no longer being available for footnoting.  So it

would be good

if we could get the Score-level footnote engraver working well.


It's worth mentioning in the change log and perhaps a convert-ly
NOT_SMART rule.  A once-over of the footnote regtests wouldn't hurt
either if you have a bit of time just to make sure they're doing what
they claim to be doing.

http://codereview.appspot.com/6306064/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Footnotes correctly printed on grobs at first timestep. (issue 6306064)

[mtsolo]

Reviewers: ,

Message:
This fixes Issue 2574 but also deals with the footnote-break-visibility
regtest, which currently does not register the property change (this may
have something to do with the footnote engraver being on the Score
level).

Cheers,
MS

Description:
Footnotes correctly printed on grobs at first timestep.

Please review this at http://codereview.appspot.com/6306064/

Affected files:
  M input/regression/footnote-break-visibility.ly
  M lily/system.cc


Index: input/regression/footnote-break-visibility.ly
diff --git a/input/regression/footnote-break-visibility.ly  
b/input/regression/footnote-break-visibility.ly
index  
0f88e2df276599a66ff346c58cdd521e31e1dec6..4626553982a90a4808ac1b7807ae09fb0c9525e3  
100644

--- a/input/regression/footnote-break-visibility.ly
+++ b/input/regression/footnote-break-visibility.ly
@@ -18,7 +18,7 @@ This behavior can be overridden.
 \time 3/4
 \break \pageBreak
 c2.
-\once \override Staff . FootnoteItem #'break-visibility = ##(#f #f #t)
+\once \override Score . FootnoteItem #'break-visibility = ##(#f #f #t)
 \footnote "foo" #'(0 . 2) #'TimeSignature "bar" \default
 \time 4/4
 \break \pageBreak
Index: lily/system.cc
diff --git a/lily/system.cc b/lily/system.cc
index  
fdcc2b133d1b056f78482087218aac7abbc559ff..5d4cae8a57b8f86333bc49da7e267d2f7e8a81b6  
100644

--- a/lily/system.cc
+++ b/lily/system.cc
@@ -254,6 +254,8 @@ System::get_footnote_grobs_in_range (vsize start, vsize  
end)


   if (Item *item = dynamic_cast(at_bat))
 {
+  end_of_line_visible = item->break_status_dir () == LEFT;
+
   if (!Item::break_visible (item))
 continue;
   // safeguard to bring down the column rank so that end of line  
footnotes show up on the correct line
@@ -275,6 +277,8 @@ System::get_footnote_grobs_in_range (vsize start, vsize  
end)

 continue;
   if (!at_bat->is_live ())
 continue;
+  if (find (out.begin (), out.end (), at_bat) != out.end ())
+continue;

   out.push_back (at_bat);
 }



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Corrects typesetting for tuplet bracket with inside-slur scripts (issue 6282044)

[mtsolo]

Reviewers: Keith,

Message:
On 2012/06/04 06:36:27, Keith wrote:

Works good for me, and seems to make sense.



Of course, this means the original tuplet/script collision never was

fixed for

for scripts under slurs, and now we do not even try.


Exactly - it's a large chunk to chew.  The tuplet bracket position
depends on scripts, which depend on slurs, which depend on tuplet
brackets.  I'm sure there are rules we could put in place to break the
cycle, but it'd require rethinking Y-offset callbacks for some of these
grobs.

~Mike

Description:
Corrects typesetting for tuplet bracket with inside-slur scripts

Please review this at http://codereview.appspot.com/6282044/

Affected files:
  M lily/slur.cc
  M lily/tuplet-bracket.cc


Index: lily/slur.cc
diff --git a/lily/slur.cc b/lily/slur.cc
index  
a41aada7a7e5c31ef9e0aaadefe74a79d9486ad3..7e2cb1d1b02b4b82859f813eb043b3afbe6669a1  
100644

--- a/lily/slur.cc
+++ b/lily/slur.cc
@@ -400,6 +400,12 @@ Slur::auxiliary_acknowledge_extra_object (Grob_info  
const &info,


   Grob *e = info.grob ();
   SCM avoid = e->get_property ("avoid-slur");
+  Grob *slur;
+  if (end_slurs.size () && !slurs.size ())
+slur = end_slurs[0];
+  else
+slur = slurs[0];
+
   if (Tie::has_interface (e)
   || avoid == ly_symbol2scm ("inside"))
 {
@@ -407,16 +413,12 @@ Slur::auxiliary_acknowledge_extra_object (Grob_info  
const &info,

 add_extra_encompass (slurs[i], e);
   for (vsize i = end_slurs.size (); i--;)
 add_extra_encompass (end_slurs[i], e);
+  if (slur)
+e->set_object ("slur", slur->self_scm ());
 }
   else if (avoid == ly_symbol2scm ("outside")
|| avoid == ly_symbol2scm ("around"))
 {
-  Grob *slur;
-  if (end_slurs.size () && !slurs.size ())
-slur = end_slurs[0];
-  else
-slur = slurs[0];
-
   if (slur)
 {
   chain_offset_callback (e, outside_slur_callback_proc, Y_AXIS);
Index: lily/tuplet-bracket.cc
diff --git a/lily/tuplet-bracket.cc b/lily/tuplet-bracket.cc
index  
d566027d2af5d2e82dc89c12e6e85cef7e0f4494..d211dcae38f55b39c91cb9095791816eb07396cc  
100644

--- a/lily/tuplet-bracket.cc
+++ b/lily/tuplet-bracket.cc
@@ -650,10 +650,7 @@ Tuplet_bracket::calc_position_and_height (Grob  
*me_grob, Real *offset, Real *dy)


   // assume that if a script is avoiding slurs, it should not get  
placed

   // under a tuplet bracket
-  SCM avoid = scripts[i]->get_property ("avoid-slur");
-  if (unsmob_grob (scripts[i]->get_object ("slur"))
-  && (avoid == ly_symbol2scm ("outside")
-  || avoid == ly_symbol2scm ("around")))
+  if (unsmob_grob (scripts[i]->get_object ("slur")))
 continue;

   Interval script_x (scripts[i]->extent (commonx, X_AXIS));



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows lyrics to slide under TimeSignature when OctaveEight present. (issue 6201068)

[mtsolo]

On 2012/05/11 08:11:18, dak wrote:

"m...@apollinemike.com"  writes:



> On 11 mai 2012, at 09:09, mailto:d...@gnu.org wrote:
>
>>
>>


http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc

>> File lily/pure-from-neighbor-engraver.cc (right):
>>
>>


http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56

>> lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1,

Grob

>> *g2)
>> On 2012/05/11 05:23:27, MikeSol wrote:
>>> On 2012/05/10 21:40:48, janek wrote:
 That's problably the most stupid question ever, but why this

doesn't

>> it begin
 with Pure_from_neighbor_engraver:: ?  I don't see it being

special.

>>
>>> It doesn't need to be a class method - it'll never be called

outside

>> of this
>>> file.  It is just a small helper function.
>>
>> Then it should be declared static, right?
>>
>
> Why would it be part of the class?  It's just a small helper

function

> - architecturally, I don't see it as being part of a class.  There

are

> small helper functions like this all over the code base.



I did not say it should be declared a class function.  I said that it
should be declared static since it is not referenced outside of the
source file.



--
David Kastrup


Hey David,

I don't completely understand the hows and whys of it, and there are
many functions in the source code that are only used in one file but are
not declared static.  If all of these should be declared static, I'd
recommend opening a tracker issue describing the general problem.

Cheers,
MS

http://codereview.appspot.com/6201068/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows lyrics to slide under TimeSignature when OctaveEight present. (issue 6201068)

[mtsolo]

Reviewers: janek, carl.d.sorensen_gmail.com,


http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc
File lily/pure-from-neighbor-engraver.cc (right):

http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56
lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob
*g2)
On 2012/05/10 21:40:48, janek wrote:

That's problably the most stupid question ever, but why this doesn't

it begin

with Pure_from_neighbor_engraver:: ?  I don't see it being special.


It doesn't need to be a class method - it'll never be called outside of
this file.  It is just a small helper function.

http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode119
lily/pure-from-neighbor-engraver.cc:119:
Pointer_group_interface::add_grob
(need_pure_heights_from_neighbors[pos[j]][k], ly_symbol2scm
("neighbors"), pure_relevants_[i]);
On 2012/05/10 21:40:48, janek wrote:

could you reduce line width, please?  This one is way over regular 80

chars

limit.


Done.

Description:
Allows lyrics to slide under TimeSignature when OctaveEight present.

Please review this at http://codereview.appspot.com/6201068/

Affected files:
  A input/regression/lyric-octave-eight.ly
  M lily/pure-from-neighbor-engraver.cc


Index: input/regression/lyric-octave-eight.ly
diff --git a/input/regression/lyric-octave-eight.ly  
b/input/regression/lyric-octave-eight.ly

new file mode 100644
index  
..d3ed333bf54f4a7d5a3aa512cc4d5d090d75bdff

--- /dev/null
+++ b/input/regression/lyric-octave-eight.ly
@@ -0,0 +1,16 @@
+\version "2.15.37"
+
+\header {
+  texidoc = "Lyrics should still slide under @code{TimeSignature} when an
+@code{OctaveEight} is present.
+"
+}
+
+\new Staff {
+  \clef "treble_8"
+  b
+}
+\addlyrics {
+  \set stanza = "1."
+  aaa
+}
Index: lily/pure-from-neighbor-engraver.cc
diff --git a/lily/pure-from-neighbor-engraver.cc  
b/lily/pure-from-neighbor-engraver.cc
index  
a6e7b5f32fe4539867f3ddbabe136357b8ba9335..413bfe5215d2b5c29e8b08e7799981071aa32bab  
100644

--- a/lily/pure-from-neighbor-engraver.cc
+++ b/lily/pure-from-neighbor-engraver.cc
@@ -52,6 +52,17 @@ Pure_from_neighbor_engraver::acknowledge_item (Grob_info  
i)

 pure_relevants_.push_back (i.item ());
 }

+bool
+in_same_column (Grob *g1, Grob *g2)
+{
+  return (g1->spanned_rank_interval ()[LEFT]
+  == g2->spanned_rank_interval ()[LEFT])
+ && (g1->spanned_rank_interval ()[RIGHT]
+ == g2->spanned_rank_interval ()[RIGHT])
+ && (g1->spanned_rank_interval ()[LEFT]
+ == g1->spanned_rank_interval ()[RIGHT]);
+}
+
 void
 Pure_from_neighbor_engraver::acknowledge_pure_from_neighbor (Grob_info i)
 {
@@ -80,8 +91,10 @@ Pure_from_neighbor_engraver::finalize ()
   temp.push_back (need_pure_heights_from_neighbors_[l]);
   for (;
(l < need_pure_heights_from_neighbors_.size () - 1
-&&  
(need_pure_heights_from_neighbors_[l]->spanned_rank_interval ()[LEFT]
-== need_pure_heights_from_neighbors_[l +  
1]->spanned_rank_interval ()[LEFT]));

+&& ((need_pure_heights_from_neighbors_[l]
+ ->spanned_rank_interval ()[LEFT])
+== (need_pure_heights_from_neighbors_[l + 1]
+->spanned_rank_interval ()[LEFT])));
l++)
 temp.push_back (need_pure_heights_from_neighbors_[l + 1]);
   need_pure_heights_from_neighbors.push_back (temp);
@@ -99,15 +112,24 @@ Pure_from_neighbor_engraver::finalize ()
 {
   while (pos[1] < (int) need_pure_heights_from_neighbors.size ()
  && (pure_relevants_[i]->spanned_rank_interval ()[LEFT]
- >  
need_pure_heights_from_neighbors[pos[1]][0]->spanned_rank_interval  
()[LEFT]))

+ > (need_pure_heights_from_neighbors[pos[1]][0]
+->spanned_rank_interval ()[LEFT])))
 {
   pos[0] = pos[1];
   pos[1]++;
 }
   for (int j = 0; j < 2; j++)
-if (pos[j] >= 0 && pos[j] < (int)  
need_pure_heights_from_neighbors.size ())
-  for (vsize k = 0; k <  
need_pure_heights_from_neighbors[pos[j]].size (); k++)
-Pointer_group_interface::add_grob  
(need_pure_heights_from_neighbors[pos[j]][k], ly_symbol2scm ("neighbors"),  
pure_relevants_[i]);

+if (pos[j] >= 0 && pos[j]
+< (int) need_pure_heights_from_neighbors.size ())
+  for (vsize k = 0;
+   k < need_pure_heights_from_neighbors[pos[j]].size ();
+   k++)
+if  
(!in_same_column(need_pure_heights_from_neighbors[pos[j]][k],

+pure_relevants_[i]))
+  Pointer_group_interface::add_grob
+(need_pure_heights_from_neighbors[pos[j]][k],
+ ly_symbol2scm ("neighbors"),
+ pure_relevants_[i]);
 }

   need_pure_heights_from_neighbors_.clear ();

30 day webathon for kickstarter support (issue 6068045)

[mtsolo]

Reviewers: ,

Message:
Hey all,

My ensemble is launching a Kickstarter project in a day or two to
support our tour in France and Ireland.

We have a sweet plug in the project video for GNU LilyPond and I was
wondering if I could strike up a partnership with LilyPond to put a link
to the project on the LilyPond front page for the duration of the
Kickstarter fundraising drive (30 days).  In return, I'd be glad to fix
a couple bounty items (my lack of development time in recent months has
come from the fact that I've been spending all my time on composing and
fundraising).

This patch is more or less what I'd need.  I can change the layout based
on whatever people think is best.  Currently, the spot for the ensemble
is towards the top of the page and the downloads are horizontally
aligned with the news.

This partnership would be a huge boost for my ensemble and I don't think
it'd divert any € that'd otherwise be going to LilyPond.  I'll certainly
do my best to make sure that the tour promotes the software.

Cheers,
MS


Description:
30 day webathon for kickstarter support

Please review this at http://codereview.appspot.com/6068045/

Affected files:
  M Documentation/css/lilypond-website.css
  M Documentation/web.texi


Index: Documentation/css/lilypond-website.css
diff --git a/Documentation/css/lilypond-website.css  
b/Documentation/css/lilypond-website.css
index  
e5bf19280dd47d3533d0566051b95ebfe9b11206..806efe58687ea948f857680badce0be58e2760bb  
100644

--- a/Documentation/css/lilypond-website.css
+++ b/Documentation/css/lilypond-website.css
@@ -473,7 +473,16 @@ div.news-item {

 div#latestVersion {
   position: absolute;
-  top: 12.4em;
+  top: 16em;
+  right: 0;
+  width: 12em;
+  text-align: center;
+  border-left: 1px solid #5b7f64;
+}
+
+div#ensembleCinq {
+  position: absolute;
+  top: 0.0em;
   right: 0;
   width: 12em;
   text-align: center;
@@ -488,6 +497,14 @@ div#latestVersion {
   margin: 0;
 }

+#ensembleCinq .subheading {
+  background: #5b7f64;
+  color: #fff;
+  text-align: center;
+  padding: 0 0.5em;
+  margin: 0;
+}
+
 /* this might not work in certain browsers */
 a[name="Stable"] + h4 {
   background: #bdee9d url(../pictures/color1-bg.png) repeat-x top left;
Index: Documentation/web.texi
diff --git a/Documentation/web.texi b/Documentation/web.texi
index  
b0e2b8311ad0794e729b2f3bda1a64956140364b..daa73b88d68267176ed5e59db9c2f08271f2ca02  
100644

--- a/Documentation/web.texi
+++ b/Documentation/web.texi
@@ -150,6 +150,18 @@ Read more in our @ref{Introduction}!
 @end ifclear
 @ifset web_version
   @c make the box:
+@divId{ensembleCinq}
+@subheading Ensemble 101
+
+The Ensemble 101 is going on a European tour where they'll sing
+music typeset using LilyPond.  Click @uref{http://www.ensemble101.fr, here}
+to learn more and to help them out on Kickstarter!
+
+@divEnd
+@end ifset
+
+@ifset web_version
+  @c make the box:
 @divId{latestVersion}
 @subheading Quick links



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Triggers X-extent calculation for NoteColumn before setting stem-begin-position (issue 5934050)

[mtsolo]

http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc#newcode131
lily/stem.cc:131: Real stem_beg = internal_calc_stem_begin_position (me,
false);
On 2012/04/02 07:42:55, Keith wrote:

I'm assuming something within calc_stem_begin_position (me,

calc_beam=false)

triggers note-collision resolution before setting the stem-begin

position.

Yup.  It allows calc_stem_position to skip all of the beam business and
get to the setting of note positions.

http://codereview.appspot.com/5934050/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Creates pure conversion for Flag::calc_y_offset (issue 5934050)

[mtsolo]

Reviewers: Keith,

Message:
There is a nasty bug in LilyPond that this patch only exacerbates - as a
rule of thumb, X positioning functions should not set Y positioning
variables and vice versa.  note-collision.cc, which works from an X_AXIS
callback, sets the stem attachment, which is a both X and Y axis value.
This is a recipe for trouble - there even if it means logic / code dup,
these should be separated out into two functions: one that sets the X
value and one that sets the Y value.  As a result, I have to trigger the
X extent in Stem::internal_stem_begin_position to get the Y position set
correctly.  It may be worth it to fix this now: it'd delay 2.16 further,
but it seems like a sound architecture decision that'd prevent this
sorta problem from happening in the future.

Description:
Creates pure conversion for Flag::calc_y_offset

Please review this at http://codereview.appspot.com/5934050/

Affected files:
  A input/regression/stem-begin-position.ly
  M lily/stem.cc


Index: input/regression/stem-begin-position.ly
diff --git a/input/regression/stem-begin-position.ly  
b/input/regression/stem-begin-position.ly

new file mode 100644
index  
..30bd3ff1e3865c53f50a3ab7e03630b62fff5e1f

--- /dev/null
+++ b/input/regression/stem-begin-position.ly
@@ -0,0 +1,10 @@
+\version "2.15.36"
+
+\header {
+  texidoc = "Stems reach correct begin points of merged noteheads.
+"
+}
+
+<< { \aikenHeads f'8 }  \\ { \aikenHeads f'8 } >>
+<< { \aikenHeads f'4:32 }  \\ { \aikenHeads f' } >>
+<< { \aikenHeads e'8 f' s4 }  \\ { \aikenHeads e'8 f' s4 } >>
Index: lily/stem.cc
diff --git a/lily/stem.cc b/lily/stem.cc
index  
f662f3cf1dec75637680e3c85f9bcd9fd31a158c..0e8c13cdaa1ff7edc67ffe930c4496fbd841b243  
100644

--- a/lily/stem.cc
+++ b/lily/stem.cc
@@ -787,6 +787,14 @@ Stem::internal_calc_stem_begin_position (Grob *me,  
bool calc_beam)

   if (Grob *head = support_head (me))
 {
   Interval head_height = head->extent (head, Y_AXIS);
+  // trigger positioning of stems in note column in case of merge
+  if (me->get_parent (X_AXIS)
+  && me->get_parent (X_AXIS)->get_parent (X_AXIS))
+(void) me->get_parent (X_AXIS)
+ ->extent (me->get_parent (X_AXIS)
+ ->get_parent (X_AXIS), X_AXIS);
+  else
+me->programming_error ("Stem doesn't belong to NoteColumn or  
PaperColumn.");

   Real y_attach = Note_head::stem_attachment_coordinate (head, Y_AXIS);

   y_attach = head_height.linear_combination (y_attach);



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Autobeaming works properly with tuplet recheck (Issue 2408) (issue 5844052)

[mtsolo]

http://codereview.appspot.com/5844052/diff/3001/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5844052/diff/3001/lily/beaming-pattern.cc#newcode361
lily/beaming-pattern.cc:361: }
My comment has nothing to do with your patch but with this function - it
seems like there's a memory leak with *dur (I know very little about
memory management, so sorry in advance if I'm totally off...).

http://codereview.appspot.com/5844052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Axis group interface ignores column rank for pure-from-neighbor-interface (issue 5843063)

[mtsolo]

Reviewers: Keith, mike_apollinemike.com,

Message:
Running regtests.  Will post new patch once it gets a clean bill of
health.


http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly
File input/regression/pure-from-neighbor-interface-pure-height.ly
(right):

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly#newcode1
input/regression/pure-from-neighbor-interface-pure-height.ly:1: \version
"2.15.34"
On 2012/03/21 05:58:42, Keith wrote:

.35
The title makes less sense now.  "lyrics-spanbarstub.ly" ?


Done.

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly#newcode4
input/regression/pure-from-neighbor-interface-pure-height.ly:4: texidoc
= "@code{axis-group-interface::pure-height} does not take spanned
On 2012/03/21 05:58:42, Keith wrote:

The description no longer fits.  Maybe
"empty measures do not confuse SpanBarStub.  These lyrics should

remain clear of

the span bars."


Done.

http://codereview.appspot.com/5843063/diff/7/lily/item.cc
File lily/item.cc (right):

http://codereview.appspot.com/5843063/diff/7/lily/item.cc#newcode245
lily/item.cc:245: cache_pure_height (Grob::pure_height (this, 0,
INT_MAX));
On 2012/03/21 05:58:42, Keith wrote:

This commit :


http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=9fe7859a8592a080413b744e3768db41059dbfe3

depended on having 'start' and 'end' passed through, so we should put

this back.


Caching in this way assumes that the pure_height of an Item is

independent of

where the line-breaks are.



Anything that gets pure_height from its neighbors would break that

assumption,

but now it seems there are no such Items, as they all use
pure-from-neighbors-interface to set their extra-spacing-height.



Tied accidentals break that assumption as well, but that's not our

fault.  I'll

write a cautionary comment in a separate commit.


Done.

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm#newcode1883
scm/define-grobs.scm:1883: (Y-extent . (1 . -1))
On 2012/03/21 05:58:42, Keith wrote:

I suggest (Y-extent . #f)
because (1 . -1) looks so much like (-1 . 1), and it also makes

suspicious

people like me think it is an uncommented magic number.


Done.

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm#newcode437
scm/output-lib.scm:437: (let* ((height (ly:grob-pure-height grob grob 0
1000))
On 2012/03/21 05:58:42, Keith wrote:

The C code uses INT_MAX to represent the end of the whole score, so it

would be

nice to use the same here, if possible.


There'd have to be a constant defined for INT_MAX in guile.  I believe
that INT_MAX can change between computers (not sure if it's the compiler
or system that does this).  Probably worth doing in a separate commit
where INT-MAX is defined in the same place as all of the PI constants
and then plugged anywhere where there's a large integer.

Description:
Axis group interface ignores column rank for
pure-from-neighbor-interface

Please review this at http://codereview.appspot.com/5843063/

Affected files:
  A input/regression/pure-from-neighbor-interface-pure-height.ly
  M lily/item.cc
  M lily/pure-from-neighbor-engraver.cc
  M scm/define-grobs.scm
  M scm/output-lib.scm


Index: input/regression/pure-from-neighbor-interface-pure-height.ly
diff --git a/input/regression/pure-from-neighbor-interface-pure-height.ly  
b/input/regression/pure-from-neighbor-interface-pure-height.ly

new file mode 100644
index  
..3e7515dfea1cc991fef736e76c9566ffca728c64

--- /dev/null
+++ b/input/regression/pure-from-neighbor-interface-pure-height.ly
@@ -0,0 +1,18 @@
+\version "2.15.34"
+
+\header {
+  texidoc = "@code{axis-group-interface::pure-height} does not take spanned
+rank interval into account for @code{pure-from-neighbor-interface} grobs.
+"
+}
+
+\new StaffGroup <<
+  \new Staff { \repeat unfold 8 { R1 e'1 } }
+  \addlyrics {
+Worked twice...
+and then
+I continued...
+working... correctly.
+  }
+  \new Staff { R1*16 }
+>>
Index: lily/item.cc
diff --git a/lily/item.cc b/lily/item.cc
index  
6ea5643a5445e7d791a119479e73d54a5fe7aead..19c9926c3b755ae660e3a698da441498d755d5e1  
100644

--- a/lily/item.cc
+++ b/lily/item.cc
@@ -242,7 +242,7 @@ Item::pure_height (Grob *g, int start, int end)
   if (cached_pure_height_valid_)
 return cached_pure_height_ + pure_relative_y_coordinate (g, start,  
end);


-  cache_pure_height (Grob::pure_height (this, start, end));
+  cache_pure_height (Grob::pure_height (this, 0, INT_MAX));
   return cached_pure_height_ + pure_relative_y_coordinate (g, start, end);
 }

Index: lily/pure-from-neighbor-engraver.cc
diff --git a/lily/pure-from-neighbor

Turns off TupletBracket collision avoidance if Script avoids slur (issue 5821051)

[mtsolo]

Reviewers: ,

Message:
Meh...not my best work, but it is a first step towards fixing this
problem.  A full solution would do tuplet avoidance for scripts in the
same way they're done for slurs.  Takers?

Description:
Turns off TupletBracket collision avoidance if Script avoids slur

Please review this at http://codereview.appspot.com/5821051/

Affected files:
  M lily/tuplet-bracket.cc


Index: lily/tuplet-bracket.cc
diff --git a/lily/tuplet-bracket.cc b/lily/tuplet-bracket.cc
index  
40e0c728c5ef2c353de391bd5da025386bce7007..8e4a3c017ce4ee6f0a3b2089810396063f8aa55b  
100644

--- a/lily/tuplet-bracket.cc
+++ b/lily/tuplet-bracket.cc
@@ -669,6 +669,13 @@ Tuplet_bracket::calc_position_and_height (Grob  
*me_grob, Real *offset, Real *dy)
   if (scm_is_number (scripts[i]->get_property  
("outside-staff-priority")))

 continue;

+  // assume that if a script is avoiding slurs, it should not get  
placed

+  // under a tuplet bracket
+  SCM avoid = scripts[i]->get_property ("avoid-slur");
+  if (avoid == ly_symbol2scm ("outside")
+  || avoid == ly_symbol2scm ("around"))
+continue;
+
   Interval script_x (scripts[i]->extent (commonx, X_AXIS));
   Interval script_y (scripts[i]->extent (commony, Y_AXIS));




___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)

[mtsolo]

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc
File lily/page-breaking.cc (right):

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584
lily/page-breaking.cc:584: }
Just a C++ question - do these lines implicitly call the copy
constructor to create footnote_stencil so that footnote_stencil is a
copy of foot?  I'm not familiar with this syntax for copying, so I just
wanted to be sure that this is what these lines were doing.

http://codereview.appspot.com/5755058/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses derived_mark to avoid segfault (issue 5729051)

[mtsolo]

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode35
lily/span-bar-stub-engraver.cc:35: these four contexts.
On 2012/03/04 21:35:57, Keith wrote:

You said later that only two SpanBarStubs are created, so
"creation of SpanBarStubs will be contemplated in each of these four

contexts."

Fixed.

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160
lily/span-bar-stub-engraver.cc:160: // removes all unused contexts
On 2012/03/06 12:19:04, dak wrote:

On 2012/03/04 10:47:17, Neil Puttock wrote:
> Is it feasible to listen for RemoveContext instead?  At least then

you only

need
> to rebuild axis_groups_ when contexts die.



I'd second this suggestion.  It should give more reliable lifetimes,

and also

cause a better correlation of "parsed object should be dead" messages

with

realities.


Unfortunately this is not possible.  I tried, but it looks like
listeners in the way you're talking about are not possible in engravers.

http://codereview.appspot.com/5729051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses derived_mark to avoid segfault (issue 5729051)

[mtsolo]

On 2012/03/04 20:59:22, Keith wrote:

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc

File lily/span-bar-stub-engraver.cc (right):



http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode46

lily/span-bar-stub-engraver.cc:46: be engraved in contexts where

BarLines are

engraved.
So, in the example with the piano staff earlier in the comment,
Four SpanBarStubs are /created/ at each barline,
Two are /engraved/ (those in the Lyrics and Dynamics context which

have the

Pure_from_neightbor_engraver, but not those in the Staffs which have

the

Bar_line_engraver) and
Zero are printed.



Right?


Close.
Two are created, two are engraved, and 0 are printed.  The
Pure_from_neighbor_engraver would catch them in Staves too, but they are
not sent to staves or anything else that has BarLines, as BarLines
fulfill the role of SpanBarStubs in contexts that have them.  The line
that makes sure that contexts with barlines do not get SpanBarStubs is:

find (bar_axis_indices.begin (), bar_axis_indices.end (), j) ==
bar_axis_indices.end ()

Or, in other words, only create the grob (and thus do the engraving) if
the context's index is not in the list of indices of contexts that have
bar lines (== bar_axis_indices.end () is another way of saying that it
is not in the vector, so find returns a pointer to the vector's end).

http://codereview.appspot.com/5729051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses derived_mark to avoid segfault (issue 5729051)

[mtsolo]

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode71
lily/span-bar-stub-engraver.cc:71: axis_groups_ = scm_cons (scm_cons
(i.grob ()->self_scm (), i.context ()->self_scm ()), axis_groups_);
On 2012/03/04 07:26:30, dak wrote:

axis_groups_ never gets cleared out again: this keeps grobs and

contexts alive

at least until the engraver is collected.


This is the point.  I was wrong to suggest before that it be cleared
out.
Unless the score is insane, a context like PianoStaff will only ever
house 10ish contexts and 10 vertical axis groups max.  So the size will
stay tiny.

I could clear this out, but the problem is that LilyPond doesn't provide
a mechanism to signal when VerticalAxisGroups are no longer being used.
If announce_end_spanner were called on them, I could acknowledge that in
the engraver.  A separate tracker issue could be opened for that.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode81
lily/span-bar-stub-engraver.cc:81: if (!vertical_alignment) // we are at
the beginning of a score, so no need for stubs
On 2012/03/04 07:26:30, dak wrote:

If "we are at the beginning of the score" is a valid state here,

scm_caar

(axis_groups_) is likely to throw an exception.


This is not possible because process_acknowledged is called after all
other process calls, so the list will be populated.  However, I agree
that an extra check here couldn't hurt.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode127
lily/span-bar-stub-engraver.cc:127: for (vsize j = 0; j <
affected_contexts.size (); j++)
On 2012/03/04 07:26:30, dak wrote:

While you are at it: how about throwing out all of the reverses and

instead

letting the loop run backwards?


This is vestigial code from when I was doing sorting on the span bar
vector.  It is no longer necessary, so removed.

http://codereview.appspot.com/5729051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Uses derived_mark to avoid segfault (issue 5729051)

[mtsolo]

Reviewers: ,

Message:
Hey all,

I don't have time to test this patch tonight (David sent me to bed) but
it should do the trick.  The map was overkill - a simple list of pairs
does the trick w/o anymore overhead and allows the use of derived_mark.

Cheers,
MS

Description:
Uses derived_mark to avoid segfault

Please review this at http://codereview.appspot.com/5729051/

Affected files:
  M lily/span-bar-stub-engraver.cc


Index: lily/span-bar-stub-engraver.cc
diff --git a/lily/span-bar-stub-engraver.cc b/lily/span-bar-stub-engraver.cc
index  
7b26ddbd1e68ed1d0892471d3f465650661e1bdf..35a47207fa5f353519889393ca1a8e948883ac2f  
100644

--- a/lily/span-bar-stub-engraver.cc
+++ b/lily/span-bar-stub-engraver.cc
@@ -17,7 +17,6 @@
   along with LilyPond.  If not, see .
 */

-#include 
 #include 

 #include "align-interface.hh"
@@ -38,7 +37,7 @@
 class Span_bar_stub_engraver : public Engraver
 {
   vector spanbars_;
-  map axis_groups_;
+  SCM axis_groups_;

 public:
   TRANSLATOR_DECLARATIONS (Span_bar_stub_engraver);
@@ -46,10 +45,18 @@ protected:
   DECLARE_ACKNOWLEDGER (span_bar);
   DECLARE_ACKNOWLEDGER (hara_kiri_group_spanner);
   void process_acknowledged ();
+  virtual void derived_mark () const;
 };

 Span_bar_stub_engraver::Span_bar_stub_engraver ()
 {
+  axis_groups_ = SCM_EOL;
+}
+
+void
+Span_bar_stub_engraver::derived_mark () const
+{
+  scm_gc_mark (axis_groups_);
 }

 void
@@ -61,7 +68,7 @@ Span_bar_stub_engraver::acknowledge_span_bar (Grob_info i)
 void
 Span_bar_stub_engraver::acknowledge_hara_kiri_group_spanner (Grob_info i)
 {
-  axis_groups_[i.grob ()] = i.context ();
+  axis_groups_ = scm_cons (scm_cons (i.grob ()->self_scm (), i.context  
()->self_scm ()), axis_groups_);

 }

 void
@@ -70,12 +77,10 @@ Span_bar_stub_engraver::process_acknowledged ()
   if (!spanbars_.size ())
 return;

-  Grob *vertical_alignment = Grob::get_root_vertical_alignment  
((*axis_groups_.begin ()).first);
+  Grob *vertical_alignment = Grob::get_root_vertical_alignment  
(unsmob_grob (scm_caar (axis_groups_)));
   if (!vertical_alignment) // we are at the beginning of a score, so no  
need for stubs

 return;

-  extract_grob_set (vertical_alignment, "elements", elts);
-
   for (vsize i = 0; i < spanbars_.size (); i++)
 {
   extract_grob_set (spanbars_[i], "elements", bars);
@@ -89,8 +94,14 @@ Span_bar_stub_engraver::process_acknowledged ()
   vector affected_contexts;
   vector y_parents;
   vector keep_extent;
-  for (vsize j = 0; j < elts.size (); j++)
+  for (SCM s = axis_groups_; scm_is_pair (s); s = scm_cdr (s))
 {
+  Context *c = unsmob_context (scm_cdar (s));
+  Grob *g = unsmob_grob (scm_caar (s));
+  if (!c || !g)
+continue;
+
+  vsize j = Grob::get_vertical_axis_group_index (g);
   if (j > bar_axis_indices[0]
   && j < bar_axis_indices.back ()
   && find (bar_axis_indices.begin (), bar_axis_indices.end (),  
j) == bar_axis_indices.end ())

@@ -101,12 +112,11 @@ Span_bar_stub_engraver::process_acknowledged ()
   break;

   k--;
-  keep_extent.push_back (to_boolean (bars[k]->get_property  
("allow-span-bar")));


-  Context *c = axis_groups_[elts[j]];
   if (c && c->get_parent_context ())
 {
-  y_parents.push_back (elts[j]);
+  keep_extent.push_back (to_boolean (bars[k]->get_property  
("allow-span-bar")));

+  y_parents.push_back (g);
   affected_contexts.push_back (c);
 }
 }
@@ -122,7 +132,7 @@ Span_bar_stub_engraver::process_acknowledged ()
   gi.rerouting_daddy_context_ = affected_contexts[j];
   announce_grob (gi);
   if (!keep_extent[j])
-it->suicide ();//it->set_property ("Y-extent", ly_interval2scm  
(Interval (infinity_f, -infinity_f)));

+it->suicide ();
 }
 }
   spanbars_.clear ();



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

On 2012/02/23 13:35:29, dak wrote:

On 2012/02/23 12:37:04, MikeSol wrote:
>

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc

> File lily/stencil-integral.cc (right):
>
>


http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#newcode932

> lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
> simple_vertical_skylines_from_possibly_transparent_stencil, 1);
> On 2012/02/23 12:29:06, dak wrote:
> > Why do we need this?  I think that a _transparent_ stencil is not

supposed

to
> > create a different skyline.  That's the point of setting

transparency rather

> > than just #f-ing the stencil.
>
> The problem is TabStaff and TabVoice.  In both contexts, many grobs

are set to

> transparent as a default.  These are grobs that are never supposed

to factor

> into a vertical skyline.  I'd be interested in seeing what happens

if these

were
> set to ##f instead of transparent, but that'd be a new project.



a) the behavior is expected and consistent
b) http://code.google.com/p/lilypond/issues/detail?id=2085 is fixed

and verified


Setting that stuff to "transparent" is merely an ugly workaround used

for

historical reasons.  We should not create inconsistent and complex

semantics to

half-heartedly support it.


So, just to be clear, you're saying that the TabVoice/TabStaff
everything-is-transparent-when-it-should-be-##f issue is blocking this
patch?  If so, I'll open up an issue for that and mark this as blocked.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#newcode932
lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
simple_vertical_skylines_from_possibly_transparent_stencil, 1);
On 2012/02/23 12:29:06, dak wrote:

Why do we need this?  I think that a _transparent_ stencil is not

supposed to

create a different skyline.  That's the point of setting transparency

rather

than just #f-ing the stencil.


The problem is TabStaff and TabVoice.  In both contexts, many grobs are
set to transparent as a default.  These are grobs that are never
supposed to factor into a vertical skyline.  I'd be interested in seeing
what happens if these were set to ##f instead of transparent, but that'd
be a new project.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393
lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis
horizon_axis, Direction sky)
On 2012/02/22 09:34:43, joeneeman wrote:

This isn't quite what I had in mind (for one thing, it means that the

caller has

to be aware of buildings, calculating their slope, etc.)



what about
Skyline::Skyline (vector > const& segments, Axis,

Direction)?


it works similarly to Skyline::Skyline(vector...) except that the

resulting

skyline shows the outline of the given set of line segments.


Done.

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647
lily/skyline.cc:647: out.merge (to_merge);
On 2012/02/22 09:34:43, joeneeman wrote:

merge is linear, so this loop is quadratic.


It should now be n*log(n).

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362
lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f,
-infinity_f, iv[LEFT] - 2 * horizon_padding));
On 2012/02/23 11:11:47, joeneeman wrote:

push_back is constant time for STL lists. No need to push_front and

then

reverse.


I'm not not in favor of this, but why is there a reverse in the other
non_overlapping_skyline function?  I tried to copy it as closely as
possible.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc#newcode659
lily/axis-group-interface.cc:659: vector *riders)
On 2012/02/16 08:09:10, joeneeman wrote:

I don't understand why riders are necessary. Is it because of this

cyclic

dependence stuff?


Not cyclical, but imagine that a grob (say tuplet bracket, for example)
has its outside staff priority set whereas one of its Y_AXIS children
(say tuplet number, for example), doesn't.  If the tuplet number's
skyline is added to the skyline_pair using add boxes before its parent
is shifted, it will be placed too low in the skyline.  Thus, it must be
added to the skyline only after its parent's outside-staff-priority has
been set to SCM_BOOL_F.  This is why riders are used.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode467
lily/skyline.cc:467: Strips all old sloped material, adds new.
On 2012/02/16 08:09:10, joeneeman wrote:

You're assuming that all sloped material came from skyline padding.

That's true

right now, but there's no reason that it will continue to be true.

It's probably

better to avoid adding padding at creation time altogether, and

instead to add

it when calling Skyline::distance.


Right you are...I agree that this is entirely dependent on the
implementation of skylines at the moment.  I know that I always respond
to you that it's something that can be done in a future patch, but I'd
put this in that category as well, as it seems like a separate problem
that doesn't need to be tacked on to this patch to be solved.  That
said, if I forget, nag me!

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
On 2012/02/16 08:09:10, joeneeman wrote:

This is linear in the number of buildings, but it should be constant.


How would one do this?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
On 2012/02/16 08:09:10, joeneeman wrote:

Ditto


Ditto

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc#newcode543
lily/stencil-integral.cc:543: SCM glyph_info = scm_hashq_ref (font_list,
scm_string_to_symbol (glyph), SCM_EOL);
On 2012/02/16 08:09:10, joeneeman wrote:

There isn't much error-checking here. What if the user substitutes an

unofficial

font which isn't in the list?



By the way, lilypond's fonts embed extra data in font tables (look for

LILC and

LILY in open-type-font.cc). That may be a better way to embed this

information

than by putting it in a scm file. For example, it would allow

unofficial fonts

to support integrals by embedding an extra table.


I think this is a good idea...I'll have a look at it.  Question, though
- aren't font tables done as alists?  I think it's important to use a
hash table here, as there is a lot of data getting thrown around and
hash table look ups are in constant time.  I'll investigate.

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile
File mf/GNUmakefile (right):

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile#newcode231
mf/GNUmakefile:231: $(LILYPOND_BINARY) --verbose
$(top-src-dir)/ly/generate-font-integrals > $@
On 2012/02/15 10:26:17, Graham Percival wrote:

This looks good, but not immediately relevant to the rest of the

vertical

skylines stuff.  Could you make this a separate patch so that it can

be pushed

sooner?


Sorry for not responding to this comment inlined...the response is that
it'd be hard as it requires new files and/or changes to old ones to go
along w/ it.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Sorry for whitespace problems.

After receiving several good general comments regarding the structure of
this patch, I have incorporated them all into my work and am now putting
it on Reitveld for more technical review.

Please run:

make clean
./autogen.sh --disable-optimising
make all

To make sure that it works.

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Should have added before: the most recent patch set is not bug free.
I'm fixing all of the regtest issues, but what I need most from other
people who have a few minutes are benchmarks.

Cheers,
MS

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Hey all,

Could people please try this patch out on a couple real world scores and
do some benchmarking compared to current master?  It adds a lot of
calculations to lilypond and a lot of them happen in Scheme, so I wanna
make sure lilypond doesn't take a large processing hit.

Cheers,
MS

http://codereview.appspot.com/5626052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Reviewers: ,

Message:
Sorry in advance for the whitespace errors.

This patch provides a generic framework for building vertical skylines
out of boxes by traversing through a stencil.  It currently only
implements skylines for three stencil types (draw-line, named-glyph, and
glyph-string) and can do more for other glyphs.  It is modular, so as
skyline estimation improve for a given stencil, that function can be
worked on alone w/o touching the other parts of stencil-integral.scm.

The implementation for glyph-string is ugly and slightly incorrect for y
heights of these stencils...it would be infinitely easier if the stencil
could contain information about y extents, but unfortunately,
PangoGlyphGeometry does not carry height information.  This error does
not have a bearing on the visual results in this patch, but it'd be good
to find a way to make the dimensions exact.

I'm also not sure that line thickness is tacked on correctly for the
draw-line boxes, but it seems to yield correct results.

Cheers,
MS

Description:
Gets vertical skylines from grob stencils

Please review this at http://codereview.appspot.com/5626052/

Affected files:
  A input/regression/tuplet-bracket-vertical-skylines.ly
  A input/regression/volta-bracket-vertical-skylines.ly
  M lily/axis-group-interface.cc
  A lily/box-scheme.cc
  M lily/include/box.hh
  M lily/include/skyline.hh
  M lily/skyline-pair.cc
  A lily/skyline-scheme.cc
  M lily/skyline.cc
  M lily/stencil-scheme.cc
  M lily/tuplet-bracket.cc
  M lily/volta-bracket.cc
  M scm/define-grobs.scm
  M scm/lily.scm
  A scm/stencil-integral.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix tuplet subdivision (issue 2243) (issue 5623051)

[mtsolo]

Haven't had a chance to test it, but the code looks good.

Have you tested this out yet with nested tuplets?

Cheers,
MS


http://codereview.appspot.com/5623051/diff/1/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5623051/diff/1/lily/beaming-pattern.cc#newcode281
lily/beaming-pattern.cc:281: infos_[i].rhythmic_importance_ = -1;
Could you include a comment indicating what the numbers mean in rhythmic
importance?

http://codereview.appspot.com/5623051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Overhauls broken bound coordinate checking (issue 5602054)

[mtsolo]

Hey all,

The newest version of this misses up some of the hairpin regtests
(hairpin-barline-break.ly, for example).  Still not sure why - I don't
have time to check it out for a bit, but if anyone has intuition as to
why, lemme know!

Cheers,
MS

http://codereview.appspot.com/5602054/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Overhauls broken bound coordinate checking (issue 5602054)

[mtsolo]

Reviewers: Neil Puttock,


http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly
File input/regression/metronome-mark-broken-bound.ly (right):

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode1
input/regression/metronome-mark-broken-bound.ly:1: \version "2.15.27"
On 2012/02/01 15:04:47, Neil Puttock wrote:

2.15.28


Fixed.

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode4
input/regression/metronome-mark-broken-bound.ly:4: texidoc = "A broken
spanner must not interfere with @code{\\tempo}
On 2012/02/01 15:04:47, Neil Puttock wrote:

Surely you mean break-alignable grobs shouldn't interfere with broken

spanners?

You have this back-to-front.


I may or may not have copied and pasted this text with out reading it.

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode12
input/regression/metronome-mark-broken-bound.ly:12: \tempo \markup {
"fo" } 4 = 90
On 2012/02/01 15:04:47, Neil Puttock wrote:

You might add



\override Score.MetronomeMark #'break-visibility = #all-visible



otherwise you're only checking the left-broken spanners.


Done.

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode22
input/regression/metronome-mark-broken-bound.ly:22: \ottava #1 \times
1/1 { e'8\<\startTextSpan\startTrillSpan\glissando\episemInitium
On 2012/02/01 15:04:47, Neil Puttock wrote:

If you want to test episema, you need to add the engraver (it's not

worth it imo

since you wouldn't use it outside a VaticanaVoice and it would never

be broken).


The code's a bit messy here


Fair nuf - removed.

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode22
input/regression/metronome-mark-broken-bound.ly:22: \ottava #1 \times
1/1 { e'8\<\startTextSpan\startTrillSpan\glissando\episemInitium
On 2012/02/01 15:04:47, Neil Puttock wrote:

If you want to test episema, you need to add the engraver (it's not

worth it imo

since you wouldn't use it outside a VaticanaVoice and it would never

be broken).


The code's a bit messy here


Fair nuf - omitted.  What'd be the proper formatting here?

http://codereview.appspot.com/5602054/diff/2001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5602054/diff/2001/scm/define-grobs.scm#newcode1420
scm/define-grobs.scm:1420: (bound-alignment-forbidden-interfaces .
(metronome-mark-interface))
On 2012/02/01 15:04:47, Neil Puttock wrote:

I question whether you need this property.  Would be better just to

exclude

break-alignable-interface in axis-group-interface.cc (so it includes
RehearsalMark and BarNumber - note that these will also interfere with

bounds if

you leave them out here)


If one could never conceive of a case where a bound would need to
include these things, then I could hard code it, but I tend to favor
using properties in case there is a scenario I can't imagine where a
person would want these to be included in NonMusicalPaperColumn bounds.

Description:
Overhauls broken bound coordinate checking

Please review this at http://codereview.appspot.com/5602054/

Affected files:
  A input/regression/metronome-mark-broken-bound.ly
  M lily/axis-group-interface.cc
  M lily/beam.cc
  M lily/lyric-hyphen.cc
  M lily/ottava-bracket.cc
  M lily/tuplet-bracket.cc
  M scm/define-grob-properties.scm
  M scm/define-grobs.scm


Index: input/regression/metronome-mark-broken-bound.ly
diff --git a/input/regression/metronome-mark-broken-bound.ly  
b/input/regression/metronome-mark-broken-bound.ly

new file mode 100644
index  
..5ba5064065ab8e3340aca7861f55881ac2fdbb3e

--- /dev/null
+++ b/input/regression/metronome-mark-broken-bound.ly
@@ -0,0 +1,36 @@
+\version "2.15.28"
+
+\header {
+texidoc = "A @code{MetronomeMark}, @code{RehearsalMark} and  
@code{BarNumber}

+should not effect the starting point of spanners.
+"
+}
+
+<<
+ \new Staff {
+   e'1 \time 4/4 \break |
+   \tempo \markup { "fo" } 4 = 90
+   e'1 |
+   e'1 |
+ }
+
+ \new Staff {
+   \override Score.MetronomeMark #'break-visibility = #all-visible
+   \override TupletBracket #'breakable = ##t
+   \override Beam #'breakable = ##t
+   \override Glissando #'breakable = ##t
+
+   \ottava #1 \times 1/1 { e'8\<\startTextSpan\startTrillSpan\glissando
+ [ \override NoteColumn #'glissando-skip = ##t\repeat unfold 22 e'8
+   \revert NoteColumn #'glissando-skip  
e'8\!\stopTextSpan\stopTrillSpan ] } |

+ }
+ \addlyrics { ah __ \repeat unfold 21 { \skip 4 } _ rrgh }
+ \addlyrics { ah --  \repeat unfold 21 { \skip 4 } _ rrgh }
+>>
+
+\layout {
+ \context {
+   \Voice
+   \remove "Forbid_line_break_engraver"
+ }
+}
Index: lily/axis-group-interface.cc
diff --git a/lily/axis-group-interface.cc b/lily/axis-group-interface.cc
index  
8857de109d6b3cb6ec438fae

Creates a MIDI note length formatter (issue 5576062)

[mtsolo]

Reviewers: ,

Message:
The idea here is to create a generic framework that allows for
modifications to note lengths (i.e. swing) in the MIDI without having a
typographical impact on the score.

Description:
Creates a MIDI note length formatter

Please review this at http://codereview.appspot.com/5576062/

Affected files:
  M lily/note-performer.cc
  M scm/define-context-properties.scm


Index: lily/note-performer.cc
diff --git a/lily/note-performer.cc b/lily/note-performer.cc
index  
01086620ef5e3f6ea23ce554926c52e7eb8ee338..c22a17199bc2470aa47eeac96ca8623e132aa052  
100644

--- a/lily/note-performer.cc
+++ b/lily/note-performer.cc
@@ -53,6 +53,7 @@ Note_performer::process_music ()

   Pitch transposing;
   SCM prop = get_property ("instrumentTransposition");
+  SCM length_formatter = get_property ("midiNoteLengthFormatter");
   if (unsmob_pitch (prop))
 transposing = *unsmob_pitch (prop);

@@ -78,6 +79,13 @@ Note_performer::process_music ()
 }

   Moment len = get_event_length (n, now_mom ());
+  if (ly_is_procedure (length_formatter))
+{
+  len = *unsmob_moment (scm_call_3 (length_formatter,
+ len.smobbed_copy (),
+ get_property  
("measureLength"),
+ get_property  
("measurePosition")));

+}

   Audio_note *p = new Audio_note (*pitp, len,
   tie_event, transposing.negated  
());

@@ -129,7 +137,10 @@ ADD_TRANSLATOR (Note_performer,
 "",

 /* read */
-"",
+"instrumentTransposition "
+"measureLength "
+"measurePosition "
+"midiNoteLengthFormatter ",

 /* write */
 ""
Index: scm/define-context-properties.scm
diff --git a/scm/define-context-properties.scm  
b/scm/define-context-properties.scm
index  
43079859c638b83acba705060a6eba9413a85d2f..bae534e7a965eb0b4f329fbe8f41313ac00b267f  
100644

--- a/scm/define-context-properties.scm
+++ b/scm/define-context-properties.scm
@@ -368,6 +368,7 @@ is used for ottava brackets.")
  (middleCPosition ,number? "The place of the middle C, measured in
 half staff-spaces.  Usually determined by looking at
 @code{middleCClefPosition} and @code{middleCOffset}.")
+ (midiChannelMapping ,symbol? "How to map MIDI channels: per  
@code{instrument} (default), @code{staff} or @code{voice}.")

  (midiInstrument ,string? "Name of the MIDI instrument to use.")
  (midiMergeUnisons ,boolean? "If true, output only one MIDI note-on
 event when notes with the same pitch, in the same MIDI-file track,  
overlap.")
@@ -375,7 +376,10 @@ event when notes with the same pitch, in the same  
MIDI-file track, overlap.")

 @code{midiMinimumVolume}.")
  (midiMinimumVolume ,number? "Set the minimum loudness for MIDI.
 Ranges from 0 to@tie{}1.")
- (midiChannelMapping ,symbol? "How to map MIDI channels: per  
@code{instrument} (default), @code{staff} or @code{voice}.")

+ (midiNoteLengthFormatter ,procedure? "A procedure that takes three
+arguments: the length of a note, the length of the measure in which the  
note
+falls, and the position of the note in the measure.  The function must  
return

+a moment that corresponds to the actual length of the note.")
  (minimumFret ,number? "The tablature auto string-selecting
 mechanism selects the highest string with a fret at least
 @code{minimumFret}.")



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Doc: NR 5.5.5 Adv tweaks - Unpure-pure containers (issue 5569050)

[mtsolo]

Looks good!  Much more user-understandable than anything I could have
ever come up with :)


http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely
File Documentation/notation/changing-defaults.itely (right):

http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely#newcode3714
Documentation/notation/changing-defaults.itely:3714: evaluated
correctly, usually returning a value of @var{0} or
It's not that the function is not evaluated correctly - it is not
evaluated at all, and lilypond subs in a dummy value.

http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely#newcode3717
Documentation/notation/changing-defaults.itely:3717:
The definition of a pure function is that it does not result in
properties or objects being set or grob suicides.

http://codereview.appspot.com/5569050/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implements DOM-id property for grobs. (issue 5504106)

[mtsolo]

Reviewers: Patrick McCarty, dak, mike_apollinemike.com, janek,


http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode177
lily/grob.cc:177: SCM expr = scm_list_3 (ly_symbol2scm ("id"),
On 2012/01/21 18:15:25, janek wrote:

isn't 'id' a scheme-thingy already?  I mean, in line 174, it is

declared as SCM,

so why convert it with ly_symbol2scm?


We checked previously for a grob property called "id" that needs to be a
string.  Here, we are creating a stencil.  All stencils are represented
internally by lists where the first element is the name of the stencil
in symbol form.

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode178
lily/grob.cc:178: id,
On 2012/01/21 18:15:25, janek wrote:

why store id two times?


The first id is the symbol id.
The second is the variable "id" that contains something else (foobar or
whatever).

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode179
lily/grob.cc:179: retval.expr ());
On 2012/01/21 18:15:25, janek wrote:

do i understand correcly that retval stands for "return value" and is

some kind

of an object?


Yup.  It should always be another stencil.  This is a convention int he
code base.

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode181
lily/grob.cc:181: retval = Stencil (retval.extent_box (), expr);
On 2012/01/21 18:15:25, janek wrote:

Do we overwrite the retval that was set earlier (in other if's)?

Why?

This is a way of saying "the retval is equal to the old retval wrapped
in something new".  Here, the new wrapping is an id.  Earlier in
grob.cc, it's a color.

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc
File lily/stencil-interpret.cc (right):

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode81
lily/stencil-interpret.cc:81: (*func) (func_arg, scm_list_2
(ly_symbol2scm ("start-enclosing-id-node"), id));
On 2012/01/21 18:15:25, janek wrote:

this line is longer than 80 chars, do we care about it?


I don't think so.

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode82
lily/stencil-interpret.cc:82: interpret_stencil_expression (scm_caddr
(expr), func, func_arg, o);
On 2012/01/21 18:15:25, janek wrote:

i understand that we extract actual stencil here and interpret it

again, but

what these func's do?


Depends what the func is.  It's a way to pass a function as an argument.
 Look for interpret_stencil_expression calls and you'll see what's
passed in & why.

Description:
Implements DOM-id property for grobs.

Please review this at http://codereview.appspot.com/5504106/

Affected files:
  A input/regression/id.ly
  M lily/grob.cc
  M lily/stencil-interpret.cc
  M scm/define-grob-properties.scm
  M scm/define-stencil-commands.scm
  M scm/output-ps.scm
  M scm/output-svg.scm


Index: input/regression/id.ly
diff --git a/input/regression/id.ly b/input/regression/id.ly
new file mode 100644
index  
..10c628f3a8a549c286c94d50e2deaf32660e7fee

--- /dev/null
+++ b/input/regression/id.ly
@@ -0,0 +1,9 @@
+\version "2.15.27"
+
+\header {
+  texidoc = "Shows the id property of a grob being set.  This should have
+no effect in the PS backend.
+"
+}
+
+{ \override NoteHead #'id = #"foo" c }
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index  
6c2eba1710fb15a283f5ecd50249f3ee7f11320f..ed96f1d13a9b1386a4c64912c2d823684b397097  
100644

--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -170,6 +170,17 @@ Grob::get_print_stencil () const
 = *unsmob_stencil (scm_call_1 (ly_lily_module_constant  
("stencil-whiteout"),

retval.smobbed_copy ()));
 }
+
+  SCM id = get_property ("id");
+  if (scm_is_string (id))
+{
+  SCM expr = scm_list_3 (ly_symbol2scm ("id"),
+ id,
+ retval.expr ());
+
+  retval = Stencil (retval.extent_box (), expr);
+}
+
 }

   return retval;
@@ -784,6 +795,7 @@ ADD_INTERFACE (Grob,
"cause "
"color "
"cross-staff "
+   "id "
"extra-X-extent "
"extra-Y-extent "
"extra-offset "
Index: lily/stencil-interpret.cc
diff --git a/lily/stencil-interpret.cc b/lily/stencil-interpret.cc
index  
1d89e032ba2559051bc8d489fc339eacc7450df2..8214af5dcc8e40a427dcd80212a0931a74bcef6f  
100644

--- a/lily/stencil-interpret.cc
+++ b/lily/stencil-interpret.cc
@@ -74,6 +74,16 @@ interpret_stencil_expression (SCM expr,

   return;
 }
+  else if (head == ly_symbol2scm ("id"))
+{
+  SCM id = scm_cadr (expr);
+
+  (*func) (func_arg, scm_list_2 (ly_symbol2scm  
("start-enclosing-id-node"), id));
+  interpret_stencil_expression (scm_caddr (expr), func, func_arg,  
o);
+ 

Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)

[mtsolo]

On 2012/01/20 15:33:07, dak wrote:


Should I be forking the rhythmic-music-iterator stuff into the
separate review


I'd say leave it with this - it's easy to forget that two patches need
to be tested in concert.

http://codereview.appspot.com/5440084/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)

[mtsolo]

http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc
File lily/rhythmic-music-iterator.cc (right):

http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc#newcode62
lily/rhythmic-music-iterator.cc:62: if (scm_is_true
ly_lily_module_constant should only be called for things that don't
exist in C++.  Otherwise, it's better to use the C++ function.  In this
case:
ly_is_listened_event_class
(in translator.cc)

http://codereview.appspot.com/5440084/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel