LilyIssues / Issues / #4005 Patch: Set X-parent of TextScript to NoteColumn instead of PaperColumn

Google Importer - 2014-07-10

Originally posted by: janek.li...@gmail.com

(No comment was entered for this change.)

Owner: janek.li...@gmail.com

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: dak@gnu.org

Patchy the autobot says: Fails make check

Labels: -Patch-new Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: janek.li...@gmail.com

Hmm. Any hints on what's failing? It seems to work ok on my test cases.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: dak@gnu.org

dak@lola:/usr/local/tmp/lilypond$ out/bin/lilypond input/regression/completion-rest.ly
GNU LilyPond 2.19.10
Processing `input/regression/completion-rest.ly'
Parsing...
Interpreting music...[8][16]
Preprocessing graphical objects...Segmentation fault (core dumped)
#0 Grob::extent (this=0x0, refp=refp@entry=0x86a5be8, a=a@entry=X_AXIS)
    at grob.cc:455
#1 0x081b4a6c in Note_column::calc_main_heads_extent (me=me@entry=0x86a5be8)
    at note-column.cc:167
#2 0x08252b39 in Self_alignment_interface::aligned_on_parent (me=0x86a56f0,
    a=a@entry=X_AXIS) at self-alignment-interface.cc:120
#3 0x08252ed7 in Self_alignment_interface::aligned_on_x_parent (
    smob=0xb6adef20) at self-alignment-interface.cc:93
#4 0xb7ef625c in scm_dapply () from /usr/lib/libguile.so.17
#5 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#6 0xb7efd390 in scm_call_1 () from /usr/lib/libguile.so.17
#7 0x08138f49 in Grob::try_callback_on_alist (this=this@entry=0x86a56f0,
    alist=alist@entry=0x86a5720, sym=sym@entry=0xb690e6e0,
    proc=proc@entry=0xb72cb458) at grob-property.cc:241
#8 0x08139146 in Grob::internal_get_property (this=this@entry=0x86a56f0,
    sym=sym@entry=0xb690e6e0) at grob-property.cc:189
#9 0x08144721 in get_offset (a=X_AXIS, a@entry=(unknown: 3062949600),
    this=this@entry=0x86a56f0) at grob.cc:409
#10 Grob::relative_coordinate (this=this@entry=0x86a56f0, refp=0x86a5340,
    a=a@entry=X_AXIS) at grob.cc:338
#11 0x08146e69 in relative_coordinate (a=X_AXIS, refp=0x86a5340,
    this=0x86a56f0) at grob.cc:333
#12 Grob::extent (this=this@entry=0x86a56f0, refp=0x86a5340, a=a@entry=X_AXIS)
    at grob.cc:453
#13 0x082574fd in boxes (left=0x0, me=<optimized out>)
    at separation-item.cc:161
#14 Separation_item::calc_skylines (smob=0xb6adf398) at separation-item.cc:91
#15 0xb7ef625c in scm_dapply () from /usr/lib/libguile.so.17
#16 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#17 0xb7efd390 in scm_call_1 () from /usr/lib/libguile.so.17
#18 0x08138f49 in Grob::try_callback_on_alist (this=this@entry=0x86a5340,
    alist=alist@entry=0x86a5370, sym=sym@entry=0xb566ca80,
    proc=proc@entry=0xb72cd070) at grob-property.cc:241
#19 0x08139146 in Grob::internal_get_property (this=0x86a5340, sym=0xb566ca80)
    at grob-property.cc:189
#20 0x081fd035 in Paper_column::minimum_distance (left=left@entry=0x869da40,
    right=right@entry=0x86a5340) at paper-column.cc:173
#21 0x082a201f in Staff_spacing::get_spacing (me=me@entry=0x86a5d48,
    right_col=0x86a5340, situational_space=1) at staff-spacing.cc:198
#22 0x0828f4c8 in Spacing_spanner::breakable_column_spacing (
    me=me@entry=0x8685d10, l=l@entry=0x869da40, r=r@entry=0x86a5340,
    options=options@entry=0xbfffcb88) at spacing-spanner.cc:502
#23 0x0828fa78 in Spacing_spanner::generate_pair_spacing (
    me=me@entry=0x8685d10, left_col=left_col@entry=0x869da40,
    right_col=right_col@entry=0x86a5340, after_right_col=0x86a6358,
    options=options@entry=0xbfffcb88) at spacing-spanner.cc:210
#24 0x0828fc35 in Spacing_spanner::generate_springs (me=me@entry=0x8685d10,
    cols=..., options=options@entry=0xbfffcb88) at spacing-spanner.cc:300
#25 0x0829026f in Spacing_spanner::set_springs (smob=0xb6b07480)
    at spacing-spanner.cc:73
#26 0xb7ef625c in scm_dapply () from /usr/lib/libguile.so.17
#27 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#28 0xb7efd390 in scm_call_1 () from /usr/lib/libguile.so.17
#29 0x08138f49 in Grob::try_callback_on_alist (this=this@entry=0x8685d10,
    alist=alist@entry=0x8685d40, sym=sym@entry=0xb690e970,
    proc=proc@entry=0xb72d98b8) at grob-property.cc:241
#30 0x08139146 in Grob::internal_get_property (this=this@entry=0x8685d10,
    sym=0xb690e970) at grob-property.cc:189
#31 0x082d8cb4 in System::pre_processing (this=0x8670500) at system.cc:533
#32 0x08201c63 in Paper_score::process (this=0x866fc30) at paper-score.cc:140
#33 0x081299c7 in ly_format_output (context=context@entry=0xb6b59cc0)
    at global-context-scheme.cc:45
#34 0x08248ed9 in Score::book_rendering (this=this@entry=0x84f1790, layoutbook=
    0x84f1928, default_def=default_def@entry=0x8575048) at score.cc:156
#35 0x080ae044 in Book::process_score (this=this@entry=0x84f1830,
    s=s@entry=0xb6b79428, output_paper_book=output_paper_book@entry=0x85246f0,
    layout=layout@entry=0x8575048) at book.cc:236
#36 0x080ae28c in Book::process (this=this@entry=0x84f1830,
    default_paper=default_paper@entry=0x8550920,
    default_layout=default_layout@entry=0x8575048,
    parent_part=parent_part@entry=0x0) at book.cc:302
#37 0x080ae357 in Book::process (this=0x84f1830, default_paper=0x8550920,
    default_layout=0x8575048) at book.cc:207
#38 0x080aaaf3 in ly_book_process (book_smob=0xb6b9d6b0,
    default_paper=0xb6e33e88, default_layout=0xb6c55518, output=0xb6827010)
    at book-scheme.cc:76
#39 0xb7f10409 in scm_gsubr_apply () from /usr/lib/libguile.so.17
#40 0xb7ef6420 in scm_dapply () from /usr/lib/libguile.so.17
#41 0xb7ef72e9 in ?? () from /usr/lib/libguile.so.17
#42 0xb7efef05 in scm_primitive_eval () from /usr/lib/libguile.so.17
#43 0x08204530 in internal_ly_eval_scm (ps=0xbfffd110) at parse-scm.cc:108
#44 ly_eval_scm (form=form@entry=0xb6b9e3a0, i=..., safe=safe@entry=false,
    parser=parser@entry=0x852c248) at parse-scm.cc:182
#45 0x08329b55 in Lily_lexer::eval_scm (this=this@entry=0x84e9cc8,
    readerdata=readerdata@entry=0xb6b9e3a0, hi=...,
    extra_token=extra_token@entry=35 '#') at lexer.ll:1081
#46 0x0833fde0 in Lily_lexer::eval_scm_token (this=0x84e9cc8, sval=0xb6b9e3a0,
    w=...) at ./include/lily-lexer.hh:58
#47 0x0833db0e in yyparse (parser=parser@entry=0x852c248,
    retval=retval@entry=0xbfffe1ac) at parser.yy:434
#48 0x0833fd3f in Lily_parser::do_yyparse (this=this@entry=0x852c248)
    at parser.yy:3648
#49 0x0816e438 in Lily_parser::parse_file (this=this@entry=0x852c248,
    init=..., name=..., out_name=...) at lily-parser.cc:126
#50 0x0816bc5f in ly_parse_file (name=0xb6741f60) at lily-parser-scheme.cc:121
#51 0xb7ef82b7 in ?? () from /usr/lib/libguile.so.17
#52 0xb7ef6874 in scm_dapply () from /usr/lib/libguile.so.17
#53 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#54 0xb7efd2bb in scm_call_0 () from /usr/lib/libguile.so.17
#55 0xb7f597ee in scm_body_thunk () from /usr/lib/libguile.so.17
#56 0xb7f59e4b in scm_c_catch () from /usr/lib/libguile.so.17
#57 0xb7f5a082 in scm_catch_with_pre_unwind_handler ()
   from /usr/lib/libguile.so.17
#58 0xb7f10409 in scm_gsubr_apply () from /usr/lib/libguile.so.17
#59 0xb7ef6420 in scm_dapply () from /usr/lib/libguile.so.17
#60 0xb7ef72e9 in ?? () from /usr/lib/libguile.so.17
#61 0xb7ef745e in ?? () from /usr/lib/libguile.so.17
#62 0xb7ef6874 in scm_dapply () from /usr/lib/libguile.so.17
#63 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#64 0xb7efd390 in scm_call_1 () from /usr/lib/libguile.so.17
#65 0xb7fd5ddc in scm_srfi1_for_each ()
   from /usr/lib/libguile-srfi-srfi-1-v-3.so
#66 0xb7ef8562 in ?? () from /usr/lib/libguile.so.17
#67 0xb7ef745e in ?? () from /usr/lib/libguile.so.17
#68 0xb7ef823c in ?? () from /usr/lib/libguile.so.17
#69 0xb7ef6874 in scm_dapply () from /usr/lib/libguile.so.17
#70 0xb7efcb78 in scm_apply () from /usr/lib/libguile.so.17
#71 0xb7efd390 in scm_call_1 () from /usr/lib/libguile.so.17
#72 0x08181448 in main_with_guile () at main.cc:533
#73 0xb7f14554 in ?? () from /usr/lib/libguile.so.17
#74 0xb7ee6eb0 in ?? () from /usr/lib/libguile.so.17
#75 0xb7f59e4b in scm_c_catch () from /usr/lib/libguile.so.17
#76 0xb7ee750f in scm_i_with_continuation_barrier ()
   from /usr/lib/libguile.so.17
#77 0xb7ee75e5 in scm_c_with_continuation_barrier ()
   from /usr/lib/libguile.so.17
#78 0xb7f57de4 in scm_i_with_guile_and_parent () from /usr/lib/libguile.so.17
#79 0xb7f57e3d in scm_with_guile () from /usr/lib/libguile.so.17
#80 0xb7f14725 in scm_boot_guile () from /usr/lib/libguile.so.17
#81 0x080542c3 in main (argc=2, argv=0xbffff104, envp=0xbffff110)
    at main.cc:808

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: janek.li...@gmail.com

don't segfault when first head is NULL

http://codereview.appspot.com/106640043

Labels: -Patch-needs_work Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: janek.li...@gmail.com

This was very helpful, thanks!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-10

Originally posted by: pkx1...@gmail.com

Passes make, make check and a full make doc.

Reg test diff attached.

This looks like it needs more work still.

Labels: -Patch-new Patch-needs_work

Screenshot.png

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-20

Originally posted by: janek.li...@gmail.com

Text_engraver: store events and grobs separately

http://codereview.appspot.com/106640043

Labels: -Patch-needs_work Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-20

Originally posted by: janek.li...@gmail.com

Set TextScript's X-parent to rest if applicable.

http://codereview.appspot.com/106640043

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-20

Originally posted by: pkx1...@gmail.com

Patchy the autobot says: passes tests. Includes a full make doc

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-21

Originally posted by: janek.li...@gmail.com

Patch on countdown for July 24th

Labels: -Patch-review Patch-countdown

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-21

Originally posted by: dak@gnu.org

Well, I've played around a bit with
$@(append-map
   (lambda (ev)
     (map (lambda (ass)
        #{
          <<
          \new Staff = "high" { \time 4/2 \key fis\major \repeat volta 2 $ev c'2 b a1 }
          \new Lyrics \with $ass
          \lyricmode { \times 2/3 { In2 splen4 -- do4. -- ri8 -- bus sanct --
                        o2. -- rum. } A2-- _ __ men. }
          >>
          #})
      (list #{ \with {} #} #{ \with { associatedVoice = "high" } #})))
   (list #{ cis'\breve #} #{ r\breve #} #{ R\breve #} #{ s\breve #}))

In general, the alignment appears a bit more satisfactory, in particular not overrunning the repeat bar. However, line 2 is ridiculous (basically a cis'\breve with an associatedVoice pointing to it). That's not an actual showstopper for this patch as line 2 is exactly as ridiculous before the patch. But it seems like something worth thinking about.

As another note: I don't like warning message. It is both too verbose and too technical. A user will have no idea whatsoever what a PaperColumn is, and I don't think there is a sensible way to figure out from the docs.

This is now
gaga.ly:8:41: warning: Aligning lyric syllables to notes was requested, but there are no notes in the specified context. Syllable will be attached to a PaperColumn instead.
          \lyricmode { \times 2/3 {
                                        In2 splen4 -- do4. -- ri8 -- bus sanct --

and previously it was
/tmp/gaga.ly:8:41: warning: Lyric syllable does not have note. Use \lyricsto or associatedVoice.
          \lyricmode { \times 2/3 {
                                        In2 splen4 -- do4. -- ri8 -- bus sanct --

Now the previous warning also was obviously ludicrous because it comes about _exactly_ when using \lyricsto or associatedVoice.

If we warn, something like "no note to associate with this syllable" should be sufficient.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-21

Originally posted by: janek.li...@gmail.com

Interesting case. However, isn't this actually related to issue 3254, rather than this one?

I agree that the warning is bad. I'll eiher reword it or remove completely; before deciding on this i'll wait for the outcome of devnull vs nullvoice discussion (http://lists.gnu.org/archive/html/lilypond-devel/2014-07/msg00104.html).

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-22

Originally posted by: k-ohara5...@oco.net

The "PaperColumn" warning message was added to address issue 248, now moot after issue 3254, so we can remove it.

The context Devnull will be easier to use without that message.
Users who want exactly the same alignment as they would get to a Voice, including ties and slurs and centering on differently-size noteheads, will need NullVoice. But its desire to exactly parallel the behavior with note-heads seems to make NullVoice necessarily kludgy, so making Devnull also work smoothly is valuable.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-22

Originally posted by: janek.li...@gmail.com

I'll remove the warning. Shall i push the patch directly to staging (assuming that it will pass patchy) or does this qualify for a full review?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-24

Originally posted by: pkx1...@gmail.com

Patch counted down - please push. If you want another test before hand then set it to patch-new and Patchy will test it.

Labels: -Patch-countdown Patch-push

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-25

Originally posted by: janek.li...@gmail.com

Pushed as

commit [r2371d6ba3b62d4d6dc349ab50fa0d76eadfba044]
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Sun Jun 29 18:25:04 2014 +0200

    Issue 4005: Set X-parent of TextScript to NoteColumn instead of PaperColumn

    This makes TextScripts consistent with DynamicTexts and LyricTexts.
    This is a follow-up to commit 59a842eba0f7ad78289a58a (Issue 2245).

    Setting TextScript.cross-staff property to #f is required to ensure
    that there are no collisions between TextScripts and cross-staff notes:

    \context PianoStaff <<
      \new Staff = "up" {
        b8[
        \change Staff="down"
        d'] ^"text"
      }
      \new Staff = "down" {
        \clef bass
        s4
      }
    >>

    (see also beam-cross-staff-auto-knee.ly)

    As far as I can see, we don't want TextScript.cross-staff to be true
    in any situation, because it would result in unwanted collisions.

    Why it worked before: cross-staff property in this example evaluated to #f,
    but only because of a bug in Script_interface::calc_cross_staff. That
    function should have marked the TextScript as cross-staff if the stem
    of the note to which TextScript was attached was cross-staff, but it
    didn't work correctly because it expected the parent of the TextScript
    to be a NoteColumn, while it actually was a PaperColumn. When I changed
    the parent to be a NoteColumn, the function started working correctly
    and marked the TextScript as cross-staff, so I had to change the default
    value of the property.

Thanks!

Labels: -Patch-push Fixed_2_19_11
Status: Fixed

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-26

Originally posted by: janek.li...@gmail.com

@David: i have opened issue 4030 for your report.
@Keith: i'm removing the warning in issue 4029.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-27

Originally posted by: janek.li...@gmail.com

Keith identified a problem with the patch; it may be necessary to revert it.

Labels: -Fixed_2_19_11
Status: Accepted

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-09

Originally posted by: janek.li...@gmail.com

(No comment was entered for this change.)

Labels: Janek_alignment_changes

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Patch: Set X-parent of TextScript to NoteColumn instead of PaperColumn

Issue Tracker for LilyPond

Searches

Help

#4005 Patch: Set X-parent of TextScript to NoteColumn instead of PaperColumn

Discussion