Uses only unpure-pure containers to articulate unpure-pure relationships.

Uses only unpure-pure containers to articulate unpure-pure relationships.

Previously, LilyPond used several different lists in define-grobs.scm
to define relationships between unpure and pure functions.  This patch
eliminates these lists, using unpure-pure containers to articulate
these relationships.

The modifications required to implement this change are described below:

-) axis-group-interface.cc
A Scheme function is no longer needed to determine pure relevant grobs.
All grobs can have their Y-extents meaningfully pure-evaluated now. The
worst-case scenario is that they evaluate to false. Dead grobs, on the
other hand, are never pure relevant. The calls to is_live are the only
holdovers from the old pure-relevant? scheme function.

-) grob-closure.cc
We allow unpure-pure containers in simple closures.

-) grob-property.cc
call_pure_function no longer looks up a Scheme module. Because there are
no hard-coded lists in Scheme any more, the function is smaller and
writing it in C++ gets slight efficiency gains.

-) grob.cc
pure_stencil_height used to be a Scheme function in define-grobs.scm.
Because it is much simpler (it no longer makes references to lists defined
in Scheme), it can be implemented in C++.

-) pure-from-neighbor-engraver.cc
Similar to axis-group-interface.cc, the pure-relevant distinction is
no longer important.

-) side-position-interface.cc
In order to avoid issues with alterBroken, we only check pure properties
before line breaking.

-) simple-closure.cc
Simple closures were incorrectly evaluated when containing unpure-pure
containers. This rectifies that.

-) stencil-integral.cc
Several pure equivalent functions needed to be written for skylines.

-) define-grobs.scm
Multiple overrides must be changed to unpure-pure containers. Previous
hard-coded lists are all deleted and several functions moved to C++ (see
above).

-) output-lib.scm
Several common unpure-pure containers used in define-grobs.scm are
defined here. Several functions from define-grobs.scm pertaining to
grob offsets are moved to this file.

[MikeSol, 2013-02-22 20:54:56]

Fixes side-position-interface::y-aligned-side unpure-pure-container

[MikeSol, 2013-02-23 05:35:50]

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.

[dak, 2013-02-23 11:26:25]

On 2013/02/23 05:35:50, MikeSol wrote:
> 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.

Mike, you need to work on your problem solving strategies.  The
problem to solve is:

Problem: for some stencils, the pure values are not distinctive enough
to differentiate even between empty and non-empty results.  Adding
extra spacing height to empty intervals, however, has no effect.

Solution: mess with the internals of intervals to create a different
flavor of empty intervals (that the interval code is not actually
designed for supporting and that is total nonsense with regard to the
mathematical model underlying the Interval type), some of which will,
when given extra height above an arbitrary threshold, will mutate into
non-empty intervals that almost, but not quite, correspond to giving
extra height to a point stencil.

Seriously?

You stated previously that your programming workflow does not allow
writing comments in advance since you are not actually planning
anything, but the plan emerges while you code.

But then you should at least write the comments afterwards and check
whether the plan which emerged is not totally insane.

There are several obvious solutions for tackling this:

a) when extra-height is considered and not set to (), treat empty
heights as a point-stencil at (0,0) instead.

b) make sure that the pure stencils at least share the emptiness or
non-emptiness of unpure stencils by appropriately defining them.

Now if you bothered actually documenting your approaches instead of
just hoping nobody will notice (I discovered the
"small-empty-interval" junk more by accident than anything else and
assumed it was intended to save you from fixing you some bug rather
than suspecting that it was supposed to be an exploit of the current
Interval implementation saving you from fixing your design), the
probability that you leave such time bombs in the code would be lower.

And make no mistake: this is a time bomb.  It would be totally
reasonable to change the implementation of Interval to make sure that
offsetting an empty interval can never make it (by virtue of numerics)
become a point interval.  And it is quite probable that at some point
of time the internals will be changed along that line, just making the
offsetting of empty intervals a noop in the implementation,
corresponding to what the underlying concept behaves like.

[MikeSol, 2013-02-24 08:24:03]

Uses point pure heights for grobs that need them

[MikeSol, 2013-02-24 08:27:25]

Fixes error in last patch set

[Keith, 2013-02-27 06:22:12]

The cardinal cantle seems meldable, but first the cruft needs to be strained.

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

https://codereview.appspot.com/7377046/diff/11001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:1827: (Y-extent . ,grob::unpure-empty-pure-point-height)
  (Y-extent . ,grob::always-Y-extent-from-stencil)

This leaked in from another patch set.

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

https://codereview.appspot.com/7377046/diff/11001/scm/output-lib.scm#newcode108
scm/output-lib.scm:108: ;; decisions, so we make their unpure height #f.
This gives a way for an object to say: "Please leave space for me as if I were
just a dot, while I am still a virgin (before page-breaking) but once I have
carnal knowledge (during final page-layout) feel free to print other things
right on top of me."
Eeew.

[mike7, 2013-02-27 10:59:09]

On 27 févr. 2013, at 07:22, k-ohara5a5a@oco.net wrote:

> The cardinal cantle seems meldable, but first the cruft needs to be
> strained.
> 
> 
> https://codereview.appspot.com/7377046/diff/11001/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
https://codereview.appspot.com/7377046/diff/11001/scm/define-grobs.scm#newcod...
> scm/define-grobs.scm:1827: (Y-extent .
> ,grob::unpure-empty-pure-point-height)
>  (Y-extent . ,grob::always-Y-extent-from-stencil)

Are you sure we want this?  This isn't what was there previously.  Previously,
there was no height for these grobs save '(0 . 0) as a pure height.

> 
> This leaked in from another patch set.
> 
> https://codereview.appspot.com/7377046/diff/11001/scm/output-lib.scm
> File scm/output-lib.scm (right):
> 
>
https://codereview.appspot.com/7377046/diff/11001/scm/output-lib.scm#newcode108
> scm/output-lib.scm:108: ;; decisions, so we make their unpure height #f.
> This gives a way for an object to say: "Please leave space for me as if
> I were just a dot, while I am still a virgin (before page-breaking) but
> once I have carnal knowledge (during final page-layout) feel free to
> print other things right on top of me."
> Eeew.
> 

As a developer whose work is pure as snow, I can't even fathom what you mean. 
My brain is going into catatonic mode to protect my innocence.

Cheers,
MS

[MikeSol, 2013-02-27 11:38:43]

Incorporates Keith's suggestion for repeat ties

[dak, 2013-02-27 18:06:34]

https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
File input/regression/scheme-text-spanner.ly (right):

https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
input/regression/scheme-text-spanner.ly:129:
side-position-interface::y-aligned-side)
I really don't understand why you ask on the developer list about gratuitous
prefix changes because of a different implementation language when you propose
such changes right afterwards.

https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:261: if (!g->is_live ())
Shouldn't you check for liveness before anything else?

https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc
File lily/grob-property.cc (right):

https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc#newco...
lily/grob-property.cc:345: if (is_simple_closure (unpure))
This is not related to this patch and review, but can anybody explain why this
simple_closure $#!+ can't be replaced by a callable closure created using a smob
type made callable via scm_set_smob_apply?  Is it because of the delayed arg? 
Can't we deal with that by using a fluid (in Scheme, a parameter) in
evaluate_with_simple_closure?  These pseudo "simple" closures really make my
head ache, and being able to throw all of them out would be quite a boon.

https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc
File lily/grob-scheme.cc (right):

https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc#newcode454
lily/grob-scheme.cc:454: SCM_ASSERT_TYPE (ly_is_procedure (proc) ||
is_unpure_pure_container (proc), proc, SCM_ARG2, __FUNCTION__, "procedure or
unpure pure container");
Possibly worth a named type predicate of its own?

https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc#newcode866
lily/grob.cc:866: if (to_boolean (scm_object_property (me->get_property_data
("stencil"), ly_symbol2scm ("ly:stencil?"))))
Where is this object property being set?

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

https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode61
scm/output-lib.scm:61: (define-public (grob::wrap-in-unpure-pure-container fn)
After issue 3200 passed, this is just ly:make-unpure-pure-container.  Remove,
change all its callers.

https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode89
scm/output-lib.scm:89: (define-public
grob::always-vertical-skylines-from-stencil
I don't think that these are worth a definition of their own.  Better expand
them inline.  An unpure-pure container is cheap enough that having separate
containers for each use is not really a problem.

[mike7, 2013-02-27 23:00:48]

On 27 févr. 2013, at 19:06, dak@gnu.org wrote:

> 
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
> File input/regression/scheme-text-spanner.ly (right):
> 
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
> input/regression/scheme-text-spanner.ly:129:
> side-position-interface::y-aligned-side)
> I really don't understand why you ask on the developer list about
> gratuitous prefix changes because of a different implementation language
> when you propose such changes right afterwards.
> 

In my e-mail, I stated:
"I'd prefer if all native Scheme functions did not have the ly: prefix - it
helps to know what things are where."
side-position-interface::y-aligned-side above is a native Scheme function that
does not have the ly: prefix.

>
https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:261: if (!g->is_live ())
> Shouldn't you check for liveness before anything else?

The call to check the cross-staff property above could trigger the suicide.
In general, it is better to access properties first, suicide later.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc
> File lily/grob-property.cc (right):
> 
>
https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc#newco...
> lily/grob-property.cc:345: if (is_simple_closure (unpure))
> This is not related to this patch and review, but can anybody explain
> why this simple_closure $#!+ can't be replaced by a callable closure
> created using a smob type made callable via scm_set_smob_apply?  Is it
> because of the delayed arg?  Can't we deal with that by using a fluid
> (in Scheme, a parameter) in evaluate_with_simple_closure?  These pseudo
> "simple" closures really make my head ache, and being able to throw all
> of them out would be quite a boon.

Agreed.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc
> File lily/grob-scheme.cc (right):
> 
>
https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc#newcode454
> lily/grob-scheme.cc:454: SCM_ASSERT_TYPE (ly_is_procedure (proc) ||
> is_unpure_pure_container (proc), proc, SCM_ARG2, __FUNCTION__,
> "procedure or unpure pure container");
> Possibly worth a named type predicate of its own?

Possibly.  Could be in a separate patch.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc
> File lily/grob.cc (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc#newcode866
> lily/grob.cc:866: if (to_boolean (scm_object_property
> (me->get_property_data ("stencil"), ly_symbol2scm ("ly:stencil?"))))
> Where is this object property being set?

In define-grobs.scm.

> 
> https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm
> File scm/output-lib.scm (right):
> 
>
https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode61
> scm/output-lib.scm:61: (define-public
> (grob::wrap-in-unpure-pure-container fn)
> After issue 3200 passed, this is just ly:make-unpure-pure-container.
> Remove, change all its callers.

Ok. I'll try to remember for this patch set.

> 
>
https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode89
> scm/output-lib.scm:89: (define-public
> grob::always-vertical-skylines-from-stencil
> I don't think that these are worth a definition of their own.  Better
> expand them inline.  An unpure-pure container is cheap enough that
> having separate containers for each use is not really a problem.
> 

They reappear several times.  It is a useful heuristic so that if I need to
change how this works, I only need to change it in one place instead of
several.

Thanks for the review!

Cheers,
MS

[dak, 2013-02-28 05:48:18]

On 2013/02/27 23:00:48, mike7 wrote:
> On 27 févr. 2013, at 19:06, mailto:dak@gnu.org wrote:
> 
> > 
> >
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
> > File input/regression/scheme-text-spanner.ly (right):
> > 
> >
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
> > input/regression/scheme-text-spanner.ly:129:
> > side-position-interface::y-aligned-side)
> > I really don't understand why you ask on the developer list about
> > gratuitous prefix changes because of a different implementation language
> > when you propose such changes right afterwards.
> > 
> 
> In my e-mail, I stated:
> "I'd prefer if all native Scheme functions did not have the ly: prefix - it
> helps to know what things are where."

So why do you ask when you ignore the answer?

> side-position-interface::y-aligned-side above is a native Scheme function that
> does not have the ly: prefix.

You remove the prefix in this patch set.

> > https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc
> > File lily/grob.cc (right):
> > 
> > https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc#newcode866
> > lily/grob.cc:866: if (to_boolean (scm_object_property
> > (me->get_property_data ("stencil"), ly_symbol2scm ("ly:stencil?"))))
> > Where is this object property being set?
> 
> In define-grobs.scm.

I've looked very thoroughly without success.  I can't find a setting
of the ly:stencil? object property in your patch set, and I can't find
it in the current code base, either.

Care for a line number?

[MikeSol, 2013-02-28 10:20:51]

Fixes Stencil check

[mike7, 2013-02-28 10:22:06]

On 28 févr. 2013, at 06:48, dak@gnu.org wrote:

> On 2013/02/27 23:00:48, mike7 wrote:
>> On 27 févr. 2013, at 19:06, mailto:dak@gnu.org wrote:
> 
>> >
>> >
> 
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
>> > File input/regression/scheme-text-spanner.ly (right):
>> >
>> >
> 
>
https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-tex...
>> > input/regression/scheme-text-spanner.ly:129:
>> > side-position-interface::y-aligned-side)
>> > I really don't understand why you ask on the developer list about
>> > gratuitous prefix changes because of a different implementation
> language
>> > when you propose such changes right afterwards.
>> >
> 
>> In my e-mail, I stated:
>> "I'd prefer if all native Scheme functions did not have the ly: prefix
> - it
>> helps to know what things are where."
> 
> So why do you ask when you ignore the answer?
> 
>> side-position-interface::y-aligned-side above is a native Scheme
> function that
>> does not have the ly: prefix.
> 
> You remove the prefix in this patch set.

Sorry, but I still don't understand what you mean.
ly:side-position-interface::y-aligned-side is defined in
side-position-interface.cc
side-position-interface::y-aligned-side is defined in output-lib.scm
The function defined in C++ has the ly: prefix, whereas the function defined in
Scheme doesn't.

> 
>> > https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc
>> > File lily/grob.cc (right):
>> >
>> >
> https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc#newcode866
>> > lily/grob.cc:866: if (to_boolean (scm_object_property
>> > (me->get_property_data ("stencil"), ly_symbol2scm ("ly:stencil?"))))
>> > Where is this object property being set?
> 
>> In define-grobs.scm.
> 
> I've looked very thoroughly without success.  I can't find a setting
> of the ly:stencil? object property in your patch set, and I can't find
> it in the current code base, either.
> 
> Care for a line number?

Ah, OK, I misunderstood your question.
In your reasking it, I now see (a) exactly what you mean; and (b) that I screwed
up this test.  I fixed the test and posted a new patch set.

Cheers,
MS

[MikeSol, 2013-03-02 10:43:51]

Better pure height for span bars

[MikeSol, 2013-03-03 17:03:42]

Uses single-argumented ly:make-unpure-pure-container

[Keith, 2013-05-18 05:21:40]

https://codereview.appspot.com/7377046/diff/26003/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

https://codereview.appspot.com/7377046/diff/26003/lily/axis-group-interface.c...
lily/axis-group-interface.cc:494:
Axis_group_interface::internal_calc_pure_relevant_grobs (Grob *me, string
grob_set_name)
Sometimes VerticalAxisGroup gets an element that is parented to another VAG,
like in issue 3359.  Now it seems these are include in pure-relevant-grobs, and
that sets the pure-Y-common of the VAG to be the overall VerticalAlignment. 
Initial staff-spacing does an infinite recursion, because the pure-height of a
VAG tries to answer in terms of its placement in the VerticalAlignment.