dynamics collide more often with other systems

Issue Tracker for LilyPond

Brought to you by: fedelibre, hahnjo, philholmes, tdanielsmusic

#3503 dynamics collide more often with other systems

Status: Verified

Owner: nobody

Labels: fixed_2_17_26 (3) Regression (247)

Needs:

Patch:

Type: Ugly

Updated: 2013-09-09

Created: 2013-08-16

Creator: Anonymous

Private: No

Originally created by: *anonymous

Originally created by: k-ohara5...@oco.net
Originally owned by: k-ohara5...@oco.net

The last patch for issue 2527 changed the placement of dynamics around cross-staff beams. The dynamics avoid the beam in the less-common case where the beams are forced in the unusual direction, but increase collisions in the more-common case (first score below).

\score {<<
\new Staff = "up" {\tempo"Vivo Molto" s1 | }
\new Staff = "down" { \clef bass
   <c,, c,>8 [ <c,, c,>8\f
   \change Staff = "up" g' g'\! ] [r2] | }>>}
\score {<<
\new Staff = "up" {\tempo"Andantino" s1 | }
\new Staff = "down" { \clef bass
   \stemDown
   <c,, c,>8 [ <c,, c,>8\f
   \change Staff = "up" g' g'\! ] [r2] | }>>}
\score {b'1}

1 Attachments

3503.png

Related

Issues: ~~#2527~~
Issues: #4182

Discussion

Google Importer - 2013-08-17

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

Removing the obviously-relevant changes from issue 2527, the 'cross-staff' mark from NoteColumns that contain a cross-staff stem, causes problems like that in the image for issue 3385.

Before issue 2527, such NoteColumns were not marked 'cross-staff', and side-positioning would refer to the extents of the NoteColumn to avoid the note-heads and normal stems and beams, but ignore cross-staff stems and beams. Now, however, the side-positioning uses skylines, and getting even an estimate of the NoteColumn's skyline requires the stem directions to be set.

It was nice to have outside-staff objects be placed before staff-spacing, so these objects were in place and get space during staff-spacing. Getting that behavior back and working with the new side-positioning code is not as simple as I hoped.

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

Google Importer - 2013-08-22

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

I posted the combined set of patches for issue 3363 issue 3501 and this one, because they are so intertwined that it makes more sense to review the proposed final state, and if one of them fails, I'll drop all of them and start working on a revert of 2527.

One test fails, input/regression/dynamics-avoid-cross-staff-stem-2.ly
because that is the functionality we are removing. The middle system in the image at the top of the report is that test, and it goes back to the 2.16.2 output.

http://codereview.appspot.com/13167043

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

Google Importer - 2013-08-22

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

(No comment was entered for this change.)

Labels: Patch-new

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

Google Importer - 2013-08-22

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

Ok with your proposed solution and reverting back to the 2.16.2 behavior, as the fix caused even worse behavior.

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

Google Importer - 2013-08-22

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

Passes Make, make check and make doc. Reg test diff attached

Labels: -Patch-new Patch-review
Owner: k-ohara5...@oco.net

Screenshot%20from%202013-08-22%2011%3A46%3A43.png

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

Google Importer - 2013-08-22

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

(No comment was entered for this change.)

Labels: -Patch-review Patch-new

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

Google Importer - 2013-08-22

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

Patchy the autobot says: passes tests.

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 - 2013-08-22

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

Adding one more related change, that came out of the discussion of issue 2527. It caused no regression-test changes for me, but if anyone can run the patch script to confirm this, I think that would be worthwhile.
http://codereview.appspot.com/13167043

Labels: -Patch-review Patch-new

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

Google Importer - 2013-08-22

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

I'm getting a bunch of "old chunk mismatch" on the Ritveld issue in the side-by-side difs. But in general it looks good. So the basic idea is that grouping grobs should not be cross-staff?

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

Google Importer - 2013-08-23

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

Re-uploaded and now side-by-side diffs are visible.

The idea is that the specific grouping-grob type NoteColumn should not be marked 'cross-staff' because that particular grob handled internally the complexity of having some of its parts being cross-staff. Going back to that method seems the safest way to have everything working, and still keep the good changes for issue 2527.

Having NoteColumn check which of its parts are cross-staff solves issue 3385 as well -- I forgot to put that in the patch description until just now.

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

Google Importer - 2013-08-23

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

Passes make, make check and a full make doc.

Reg test diff attached

Labels: -Patch-new Patch-review

Screenshot%20from%202013-08-23%2008%3A46%3A45.png

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

Google Importer - 2013-08-23

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

Patch on Countdown for August 27th - 06:00 GMT

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 - 2013-08-24

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

(No comment was entered for this change.)

Status: Started

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

Google Importer - 2013-08-26

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

Patch counted down, please push.

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 - 2013-08-27

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

commit [r7891600a5dd421c1f25776ea3b405c64f4f14752]

Labels: -Patch-push fixed_2_17_26
Status: Fixed

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

Google Importer - 2013-09-09

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

(No comment was entered for this change.)

Status: Verified

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