LilyIssues / Issues / #1031 constantly-changing input/regression/rest-collision-beam-note.ly

Google Importer - 2010-03-04

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

the latter is a bug, but AFAIK, it is a regression.

It would be good to know when this started.

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

Google Importer - 2010-03-05

Originally posted by: percival.music.ca@gmail.com

Ok, bumping priority.

Summary: constantly-changing input/regression/rest-collision-beam-note.ly
Labels: -Priority-High Priority-Critical

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

Google Importer - 2010-03-24

Originally posted by: percival.music.ca@gmail.com

Issue 1047 has been merged into this issue.

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

Google Importer - 2010-05-08

Originally posted by: pnorcks@gmail.com

IIRC, in the past I've seen this happen with
input/regression/dot-rest-beam-trigger.ly too.

The latest example I can find is the comparison between 2.13.11 and 2.13.12:

http://lilypond.org/test/v2.13.12-1/compare-v2.13.11-1/v2.13.11-1/dot-rest-beam-trigger.png
http://lilypond.org/test/v2.13.12-1/compare-v2.13.11-1/v2.13.12-1/dot-rest-beam-trigger.compare.jpeg

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

Google Importer - 2010-05-22

Originally posted by: pnorcks@gmail.com

I've managed to reproduce the incorrect behavior (with luck) while processing
multiple regtests (including rest-collision-beam-note.ly) through GDB, and I found
the location where data differs for correct and incorrect results.

The initialization of `discrete_y' in rest-collision.cc on line 275 looks like this:

    int discrete_y = dir * int (ceil (y / (0.5 * dir * staff_space)));

When correct results are produced, the value -4 is assigned to `discrete_y'.
For incorrect results, the value -8 is assigned instead.

These differing values stem from the assignment on line 233 of the same file:

    Interval restdim = rcol->extent (common, Y_AXIS);

The interval *length* is always the same, namely "1.870006", but the intervals
themselves differ:

Correct interval:   (-3.050006 . -1.18)
Incorrect interval: (-1.050006 . 0.82)

Since these intervals are the Y-extents for the NoteColumns (with rests attached),
somehow the extents are incorrectly computed.

Unfortunately, it's not easy to reproduce this bug, though I've never encountered it
while compiling *only* this regtest.

Still investigating...

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

Google Importer - 2010-05-23

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

have you tried running it through valgrind? It might turn up a subtly uninitialized
variable issue.

Another thing to look for is places where we sort vector<Foo*> arrays by pointer
address. We should probably put deterministically determined numbers (eg. enumerate
all grobs using a global counter) in the Grob and use that to sort things.

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

Google Importer - 2010-05-24

Originally posted by: pnorcks@gmail.com

Thank you for the helpful suggestions, Han-Wen.

Your intuition is correct: the problem is with the sorting vectors of pointers to
objects.

For the case with rest-collision-beam-note.ly, the specific sort that is problematic
is in Axis_group_interface::skyline_spacing() on axis-group-interface.cc:637

vector_sort (elements, staff_priority_less);

For a solution, are you suggesting that we add a new member to the Grob class that
records a unique Grob ID? I'm not I understand correctly what you refer to.

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

Google Importer - 2010-05-24

Originally posted by: pnorcks@gmail.com

Hmm... in this case, I guess we are not actually sorting by pointer address, so the
problem might be elsewhere.

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

Google Importer - 2010-05-24

Originally posted by: pnorcks@gmail.com

(Ignore my last comment.)

There are certain cases where pointer addresses are compared in staff_priority_less.
If I comment out these two lines of code, the problem seems to be fixed, though I
haven't run `make check' to verify.

/* if neither grob has an outside-staff priority, the ordering will have no
     effect -- we just need to choose a consistent ordering. We do this to
     avoid the side-effect of calculating extents. */
if (isinf (priority_1))
    return g1 < g2;

Is this the likely cause of the inconsistency?

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

Google Importer - 2010-05-24

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

We should try to remove all of these comparisons for the sake of reproducibility. It
does not fix the actual problem though, since for one choice of ordering (assuming we
can do it deterministically), we are resolve the collision incorrectly, so there is
another collision bug there.

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

Google Importer - 2010-05-27

Originally posted by: pnorcks@gmail.com

Update:

I have discovered the culprit function call. See below. The result of this function
call is that -4 is added to the dim_cache_[Y_AXIS].offset_ field of the Rest grob,
resulting in a incorrect rest offset. This call is located in
Beam::rest_collision_callback() on line 1475. There is also a TODO, so I'll list it
here.

/*
    TODO: this is dubious, because this call needs the info we're
    computing right now.
   */
Interval rest_extent = rest->extent (common_y, Y_AXIS);

I can't work on this anymore tonight, but I'm not sure how to proceed. Any hints on
how to fix this?

Owner: pnorcks
Status: Started

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

Google Importer - 2010-05-27

Originally posted by: pnorcks@gmail.com

I also wanted to mention that I *do* have a reproducible test case, discovered
through a guess-and-check approach, but it might not work for anyone else. It also
does not work with latest git, so I reverted to commit 8d15677 to continue my work on
this issue.

My test case for incorrect results:

$ lilypond quoted-tie.ly \
             ragged-bottom-one-page.ly \
             rest-collision-beam.ly \
             rest-collision-beam-note.ly

And for correct results:

$ lilypond rest-collision-beam-note.ly

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

Google Importer - 2010-06-05

Originally posted by: pnorcks@gmail.com

New info:

If I remove these two lines from rest-collision.cc, the bug seems to go away.

67 if (scm_is_number (offset))
68 rest_grob->translate_axis (scm_to_double (offset), Y_AXIS);

I'm currently checking the regtests.

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

Google Importer - 2010-06-05

Originally posted by: pnorcks@gmail.com

After checking the regtests, my suggested change from comment 13 creates other
unwanted regressions.

I'm very close to figuring this out though, so that's good. :-)

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

Google Importer - 2010-06-14

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

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

Thanks to Patrick's investigations, here is a patch that seems to fix the issue. The problem is a cyclic dependence between Beam::rest_collision_callback and Rest_collision::calc_positioning_done. The patch resolves the dependency by making Beam::rest_collision_callback not depend on Rest_collision::calc_positioning_done.

Labels: Patch

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

Google Importer - 2010-06-14

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

Oops, try this instead:
http://codereview.appspot.com/1694041

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

Google Importer - 2010-06-17

Originally posted by: pnorcks@gmail.com

(No comment was entered for this change.)

Owner: joeneeman

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

Google Importer - 2010-06-19

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

(No comment was entered for this change.)

Labels: fixed_2_13_25
Status: Fixed

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

Google Importer - 2010-06-29

Originally posted by: n.putt...@gmail.com

Joe, have you seen what this does to lyric-combine-switch-voice.ly?

programming error: cyclic dependency: calculation-in-progress encountered for #'Y-extent (NoteColumn)

[r8] f g
backtrace:
0: (#<Grob RestCollision > positioning-done #<primitive-procedure ly:rest-collision::calc-positioning-done>)
1: (#<Grob Rest > Y-offset #<simple-closure #<simple-closure (#<primitive-procedure ly:rest-collision::force-shift-callback-rest> #<simple-closure (#<primitive-procedure ly:rest::y-offset-callback>) >) > >)
2: (#<Grob NoteColumn > Y-extent #<primitive-procedure ly:axis-group-interface::height>)
3: (#<Grob TupletBracket > positions #<primitive-procedure ly:tuplet-bracket::calc-positions>)
4: (#<Grob TupletBracket > control-points #<primitive-procedure ly:tuplet-bracket::calc-control-points>)
5: (#<Grob TupletBracket > stencil #<primitive-procedure ly:tuplet-bracket::print>)
6: (#<Grob TupletBracket > Y-extent #<primitive-procedure ly:grob::stencil-height>)
7: (#<Grob VerticalAxisGroup > vertical-skylines #<primitive-procedure ly:hara-kiri-group-spanner::calc-skylines>)

I don't know why this doesn't show up in the test results between 2.13.24 and 25, but it's popped up at random over the last few local regtest checks I've done.

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

Google Importer - 2010-06-29

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

hm, I can't reproduce it. Do you have a command line that gets the problem consistently? If not, could you get me a gdb backtrace too?

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

Google Importer - 2010-06-29

Originally posted by: n.putt...@gmail.com

It happens consistently via the command line or running inside Frescobaldi (naturally, with --disable-optimising for the binary.)

I'll try doing a backtrace shortly.

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

Google Importer - 2010-06-29

Originally posted by: pnorcks@gmail.com

I am getting the same results as Neil, running with

$ lilypond lyric-combine-switch-voice.ly

Attached is the backtrace I am seeing.

cyclic-dependency.backtrace.log

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

Google Importer - 2010-06-29

Originally posted by: n.putt...@gmail.com

Cheers, Patrick. Yours looks the same as mine. :)

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

Google Importer - 2010-06-30

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

Does it help if you change "rcol" in rest-collision.cc:214 to "rest"?

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

Google Importer - 2010-06-30

Originally posted by: n.putt...@gmail.com

Yes, thanks.

It's confusingly named, don't you think? I originally read it as short for "rest collision", but it's actually "rest column".

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

constantly-changing input/regression/rest-collision-beam-note.ly

Issue Tracker for LilyPond

Searches

Help

#1031 constantly-changing input/regression/rest-collision-beam-note.ly

Discussion