add general_alignment (issue 2613)

This adds a versatile method for specifying alignment of grobs, allowing parent reference point to be specified separately from grob reference point.

[janek, 2012-06-19 17:59:44]

A pdf showing what this patch does is attached to tracker issue:
http://code.google.com/p/lilypond/issues/detail?id=2613#c2

Please review!

[janek, 2012-06-19 18:03:08]

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

http://codereview.appspot.com/6308093/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:964: (X-alignment ,list? "eponymous")
i will write a better description ofc :)

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

http://codereview.appspot.com/6308093/diff/1/scm/define-grobs.scm#newcode1279
scm/define-grobs.scm:1279: (X-alignment . ((outer . 1) (outer . 1) 0))
(that's just for testing purposes)

[hanwenn, 2012-06-19 23:28:58]

This patch seems to be for solving a lyrics related problem. It would
be nice if this wouldn't require overhaul of the core data structures
of LilyPond. Each and every graphic object has a dimension-cache, so
you're adding a core-extent (as opposed to the normal extent) to each
object. Does that make sense?  I think it would be much better if you
could solve this problem while only touching the lyrics related code.

On Tue, Jun 19, 2012 at 2:59 PM,  <janek.lilypond@gmail.com> wrote:
> Reviewers: MikeSol, joeneeman,
>
> Message:
> A pdf showing what this patch does is attached to tracker issue:
> http://code.google.com/p/lilypond/issues/detail?id=2613#c2
>
> Please review!
>
> Description:
> [XY]-core-extent and general_alignment (issue 2613)
>
> This adds a versatile method for specifying alignment of grobs.
>
> Please review this at http://codereview.appspot.com/6308093/
>
> Affected files:
>  M lily/dimension-cache.cc
>  M lily/grob.cc
>  M lily/include/dimension-cache.hh
>  M lily/include/grob.hh
>  M lily/include/self-alignment-interface.hh
>  M lily/self-alignment-interface.cc
>  M scm/define-grob-properties.scm
>  M scm/define-grobs.scm
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[janek, 2012-06-20 03:20:07]

On Wed, Jun 20, 2012 at 1:28 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> This patch seems to be for solving a lyrics related problem. It would
> be nice if this wouldn't require overhaul of the core data structures
> of LilyPond. Each and every graphic object has a dimension-cache, so
> you're adding a core-extent (as opposed to the normal extent) to each
> object. Does that make sense?  I think it would be much better if you
> could solve this problem while only touching the lyrics related code.

Sorry, i didn't make myself clear.
First of all, this is a draft (i should've marked it patch-needs_work).
Secondly - yes, this will help solving 3 gsoc-lyrics issues.  But it
will also help with other things, and i'm pretty sure that a lot of
objects will benefit from having both extents and core-extents.
Please take a look at
http://lists.gnu.org/archive/html/lilypond-devel/2012-06/msg00230.html
- it shows an example of how markups will benefit.
I will elaborate and send more examples later today.

thanks for a good question!
Janek

[Keith, 2012-06-20 05:02:27]

On 2012/06/20 03:20:07, janek wrote:
> On Wed, Jun 20, 2012 at 1:28 AM, Han-Wen Nienhuys <mailto:hanwenn@gmail.com>
wrote:
> > It would
> > be nice if this wouldn't require overhaul of the core data 
> > structures of LilyPond. 

>  But it
> will also help with other things, and i'm pretty sure that a lot of
> objects will benefit from having both extents and core-extents.
> 

Other properties help with many objects, 'padding for example, but we do not
reserve space for the possible value of 'padding in every single graphical
object.  You do not need to fill my RAM with empty core-extent caches for each
one of my staccato dots.

Access your property directly in the self-alignment-interface.cc, as is done
with self-alignment-X, for example.

[joeneeman, 2012-06-20 09:45:25]

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

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc...
lily/self-alignment-interface.cc:206: grob_alignment = scm_to_double (scm_cdr
(grob_alignment_property));
Use robust_scm2double

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc...
lily/self-alignment-interface.cc:221: : me->core_extent (me, a);
What about:
if (which_grob_extent == ly_symbol2scm ("extent"))
  grob_extent = me->extent (me, a);
else
  grob_extent = ly_scm2interval(me->get_property (which_grob_extent);

Then you can get rid of all the core-extent-related caching code.

[janek, 2012-06-20 13:05:37]

Hi,

After rereading the code i see that you are totally right!  I don't know why i
wanted to use dim_cache so much.  It's scrapped now.

Joe, thanks to your suggestion it is now possible to read extent from any
property, including user-defined ones!

many thanks,
Janek

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

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc...
lily/self-alignment-interface.cc:206: grob_alignment = scm_to_double (scm_cdr
(grob_alignment_property));
On 2012/06/20 09:45:26, joeneeman wrote:
> Use robust_scm2double

Done.

http://codereview.appspot.com/6308093/diff/1/lily/self-alignment-interface.cc...
lily/self-alignment-interface.cc:221: : me->core_extent (me, a);
On 2012/06/20 09:45:26, joeneeman wrote:
> What about:
> if (which_grob_extent == ly_symbol2scm ("extent"))
>   grob_extent = me->extent (me, a);
> else
>   grob_extent = ly_scm2interval(me->get_property (which_grob_extent);
> 
> Then you can get rid of all the core-extent-related caching code. 

Done. (in a bit different way)

[janek, 2012-06-21 07:12:13]

Hi all,

i've found why it was breaking make - an interface for properties was missing. 
It is added now.

I've decided not to change any grob properties with this patch (i.e. only add
new method, not use it yet).  Thus, there should be no regressions at all.
Nevertheless, it's possible to override X-offset property on the ly code to use
this:
\override LyricText #'X-offset =
  #(ly:make-simple-closure (ly:make-simple-closure
     (list ly:self-alignment-interface::general-x-alignment)))

This is no longer a draft; let me know if it's ready to be added to the source.

cheers,
Janek

[MikeSol, 2012-06-21 07:26:31]

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.s...
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.).

[janek, 2012-06-21 07:36:31]

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.s...
scm/define-grob-properties.scm:984: (Y-alignment ,list? "3-element list
specifying how an object is
On 2012/06/21 07:26:31, MikeSol wrote:
> Consider using a vector for this property - it is more in keeping w/ other
> 3-element structures in the code base (break alignment, etc.).

but doesn't a vector need to have all its elements of the same type?  Here first
two elements are pairs, and the third one is a number.

[Keith, 2012-06-21 08:24:11]

Looks good; 
I hope to try it using your example-input on the bug-tracker issue 2613 within a
day or so.

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.s...
scm/define-grob-properties.scm:990: @tie{} {@code{-1}} means left edge of the
extent, @code{0} means center
  @code{DOWN = -1} for bottom, @code{CENTER = 0} or @code{UP = 1} for top, or
other numerical values.

[dak, 2012-06-21 19:57:09]

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.s...
scm/define-grob-properties.scm:969: be used (for example X@tie{}extent) and the
second value is a number
@tie{} is misused here.  You probably mean something like @samp{X-extent} or
@code{X-extent}.

http://codereview.appspot.com/6308093/diff/10001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:970: @tie{} {@code{-1}} means left edge of the
extent, @code{0} means center
@tie{} here is garbage.  The braces around @code{-1} are garbage as well.

http://codereview.appspot.com/6308093/diff/10001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:989: be used (for example X@tie{}extent) and the
second value is a number
See above.  This never could have passed "make".

[Keith, 2012-06-22 05:41:49]

This fails an assertion in scm_or_str2symbol, so I had to recompile with NDEBUG

On 2012/06/21 07:36:31, janek wrote:

> Nevertheless, it's possible to override X-offset property on the 
>  ly code to use this:
> \override LyricText #'X-offset =
>  #(ly:make-simple-closure (ly:make-simple-closure 
>    (list ly:self-alignment-interface::general-x-alignment)))

 .. = #ly:self-alignment-interface::general-x-alignment
I don t know Scheme, but I see no need to wrap in closures.

> This is no longer a draft; let me know if it's ready to 
> be added to the source.

The new property X-alignment will be awkward to use, because it is a complex
structure of unnamed conceptually-distinct objects.

As you have built it, a list of pairs, I can do things like
 \override LyricText #'X-alignment #'X-extent = #RIGHT
but I don't think that was intended.

The objects in X-alignment could be individually-named properties:
 self-alignment-X-extent-symbol
 self-alignment-X (the existing property)
 parent-alignment-X
 parent-self-alignment-X-extent-symbol
 extra-X-offset

Will all of these be independently useful ? 
I would like to see how this solves some engraving challenges, without
hand-calculation of the key numbers, before you push the C code.  There will be
a difficult convert-ly rule to write, if you have general_alignment() in the
2.16 source, but then need to change the interface.

[janek, 2012-06-22 15:21:24]

On Fri, Jun 22, 2012 at 7:41 AM,  <k-ohara5a5a@oco.net> wrote:
> This fails an assertion in scm_or_str2symbol, so I had to recompile with
> NDEBUG

I'm sorry, but i don't recognize what failing an assertion in
scm_or_str2symbol could mean here.  Was this related to bad
docstrings?  (they are fixed now)

>> Nevertheless, it's possible to override X-offset property on the
>>  ly code to use this:
>> \override LyricText #'X-offset =
>>  #(ly:make-simple-closure (ly:make-simple-closure
>>    (list ly:self-alignment-interface::general-x-alignment)))
>
>  .. = #ly:self-alignment-interface::general-x-alignment
> I don t know Scheme, but I see no need to wrap in closures.

Indeed!  Thank you.

> The new property X-alignment will be awkward to use, because it is a
> complex structure of unnamed conceptually-distinct objects.

Unnamed?  What do you mean?  How can i name them?
Concerning user-friendliness: i was going to suggest defining
"aliases" for most commonly used values - just like we can use LEFT
instead of -1.

> As you have built it, a list of pairs, I can do things like
>  \override LyricText #'X-alignment #'X-extent = #RIGHT
> but I don't think that was intended.

I don't understand - does that override work for you?  When i tried it, i got

  ERROR: In procedure ly:self-alignment-interface::general-x-alignment:
  ERROR: Wrong type (expecting real number): (X-extent . 1)

> The objects in X-alignment could be individually-named properties:
>  self-alignment-X-extent-symbol
>  self-alignment-X (the existing property)
>  parent-alignment-X
>  parent-self-alignment-X-extent-symbol
>  extra-X-offset
>
> Will all of these be independently useful ?

I don't think so, and they would horribly "clutter the view" (for
example, Frescobaldi hints what properties of a particular object can
be overridden - i use this sometimes).

> I would like to see how this solves some engraving challenges, without
> hand-calculation of the key numbers, before you push the C code.

Here you are:
- issue 2451: punctuation can be detected automatically, setting
X-core-extent accordingly,
- issue 2452: a special character could be used by the user to tell
the "prefix" apart from the core of the syllable (e.g. z&nim instead
of "z nim"), and with this information it'll be easy to automatically
calculate core-extent,
- issue 2245: it would be possible to easily choose whether objects
like dynamics should be aligned only to the "main" note column (i.e.
w/o suspended notes), or should the suspended notes be included - just
include the suspended heads in outer-extent, and not in core-extent
(easy to do automatically) and choose which extent you align to.
- alignment of bass clef ottavation:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705593/clef-ottavation.png
- alignment of music glyphs and text in markup:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705593/text-and-music-glyp...
- alignment of markups containing scores:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705593/align-score-markups...
- we had some problems with spanner bounds (issue 620, issue 2532): i
think my approach might help here (i haven't investigated it closely,
though).

I hope these examples are explained clear enough.

> There will be a difficult convert-ly rule to write, if you have
> general_alignment() in the 2.16 source, but then need to change the
> interface.

Ok, let's not push this until i actually use new alignment
possibilities in my lyrics issues and/or change existing code to use
it.

cheers,
Janek

[Keith, 2012-06-22 19:02:48]

On Fri, 22 Jun 2012 08:21:02 -0700, Janek Warchoł <janek.lilypond@gmail.com>
wrote:

> On Fri, Jun 22, 2012 at 7:41 AM,  <k-ohara5a5a@oco.net> wrote:
>> This fails an assertion in scm_or_str2symbol, so I had to recompile with
>> NDEBUG
>
> I'm sorry, but i don't recognize what failing an assertion in
> scm_or_str2symbol could mean here.

Neither did I.
Run your ..../configure with --disable-optimising so that when you 'make' you
will have assert() enabled so you can see these debugging checks.

Searching for the definition of scm_or_str2smbol(), it is in
lily-guile-macros.hh
   48 /* this lets us "overload" macros such as get_property to take
   49    symbols as well as strings */
   50 inline SCM
   51 scm_or_str2symbol (char const *c) { return scm_from_locale_symbol (c); }
   52
   53 inline SCM
   54 scm_or_str2symbol (SCM s)
   55 {
   56   assert (scm_is_symbol (s));
   57   return s;
   58 }
so it looks like your use of
   SCM which_grob_extent = SCM_EOL; ... get_property (which_grob_extent)
was not considered valid use of get_property when it was designed.

[Keith, 2012-06-22 20:13:35]

On Fri, 22 Jun 2012 08:21:02 -0700, Janek Warchoł <janek.lilypond@gmail.com>
wrote:

>> The new property X-alignment will be awkward to use, because it is a
>> complex structure of unnamed conceptually-distinct objects.
>
> Unnamed?  What do you mean?

If you change things as you have the patch now, when someone asks, over the
telephone,
"Why doesn't \once\override self-alignment-X work anymore ?"
you have to say "If you are using on lyrics, don't use self-alignment-X; instead
copy X-alignment from the define-grobs.scm and change the car of the second
element in the list to what you used to use for self-alignment-X".  I think I
might have gotten something wrong in that.

> I hope these examples are explained clear enough.
>
Yes. It takes some imagination to visualize an implementation of solutions,
though.

I see how it makes sense to have LyricsText determine its 'core' and then let
self-alignment-X (or, rather, the appropriate element in X-alignment) refer to
that core.

It is hard for me to predict if a "core extent" can be determined by
straightforward code for the various parent objects.   It worked as a
description for what you want in the cases you described.

The relationship between parent and child is asymmetric, though, so you might
not want the symmetric concept of parent and child each having a core.  Tou
might think of the child having a core extent and the parent having a choice of
alignment points (including its reference point which is our choice today).  It
depends on what you can reasonably extract from the parent objects.