Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(487)

Issue 4643067: Fix issue 75: Allow multiple concurrent slurs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 9 months ago by Reinhold
Modified:
12 years, 8 months ago
Reviewers:
pkx166h, kieren_macmillan, carl.d.sorensen, Neil Puttock, reinhold
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix issue 75: Allow multiple concurrent slurs Rewrite the Slur_engraver and the Phrasing_slur_engraver to support multiple concurrent slurs. The default lilypond syntax using parentheses still supports only one slur at a time, but by adding a slur-id property to the (Phrasing)SlurEvent music expression, one can create multiple concurrent slurs, each with a different slur-id. This finally allows appoggiaturas and acciaccaturas (which both create a slur from the grace note the the next note) to be placed inside a slur.

Patch Set 1 #

Patch Set 2 : Fix documentation compilation #

Total comments: 1

Patch Set 3 : Renamed slur-id to spanner-id #

Total comments: 21

Patch Set 4 : Neil's code review (ly:export, robust_scm2string, for loop, spanner interface, startGraceSlur) #

Patch Set 5 : If a slur start is observed when a slur is present, completely ignore it (fixes #1256) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+223 lines, -79 lines) Patch
A input/regression/phrasing-slur-multiple.ly View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
A input/regression/slur-grace.ly View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download
A input/regression/slur-multiple.ly View 1 2 3 4 1 chunk +21 lines, -0 lines 0 comments Download
A input/regression/slur-multiple-linebreak.ly View 1 2 3 4 1 chunk +26 lines, -0 lines 0 comments Download
M lily/phrasing-slur-engraver.cc View 1 2 3 6 chunks +68 lines, -37 lines 0 comments Download
M lily/slur-engraver.cc View 1 2 3 4 7 chunks +61 lines, -38 lines 0 comments Download
M lily/spanner.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M ly/grace-init.ly View 1 2 3 2 chunks +7 lines, -4 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M scm/define-grobs.scm View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M scm/define-music-properties.scm View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M scm/define-music-types.scm View 1 2 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
pkx166h
Hello, I get an error when I try to make: --snip-- [/home/james/lilypond-git/out/share/lilypond/current/scm/document-music.scm] [/home/james/lilypond-git/out/share/lilypond/current/scm/document-type-predicates.scm] [/home/james/lilypond-git/out/share/lilypond/current/scm/document-identifiers.scm] [/home/james/lilypond-git/out/share/lilypond/current/scm/document-backend.scm ...
12 years, 9 months ago (2011-07-04 00:18:07 UTC) #1
Reinhold
On 2011/07/04 00:18:07, J_lowe wrote: > Hello, I get an error when I try to ...
12 years, 9 months ago (2011-07-04 12:17:46 UTC) #2
pkx166h
On 2011/07/04 12:17:46, Reinhold wrote: > On 2011/07/04 00:18:07, J_lowe wrote: > > Hello, I ...
12 years, 9 months ago (2011-07-04 22:30:52 UTC) #3
reinhold_kainhofer.com
On Di., 5. Jul. 2011 00:30:52 CEST, pkx166h@gmail.com wrote: > On 2011/07/04 12:17:46, Reinhold wrote: ...
12 years, 9 months ago (2011-07-04 23:05:04 UTC) #4
Carl
LGTM, with one comment. I do wonder if David is right, and we should be ...
12 years, 9 months ago (2011-07-05 05:40:49 UTC) #5
kieren_macmillan_sympatico.ca
> I do wonder if David is right, and we should be using spanner_id instead ...
12 years, 9 months ago (2011-07-05 13:12:11 UTC) #6
Reinhold
On 2011/07/05 13:12:11, kieren_macmillan_sympatico.ca wrote: > > I do wonder if David is right, and ...
12 years, 8 months ago (2011-07-06 14:56:51 UTC) #7
Neil Puttock
http://codereview.appspot.com/4643067/diff/15002/input/regression/phrasing-slur-multiple.ly File input/regression/phrasing-slur-multiple.ly (right): http://codereview.appspot.com/4643067/diff/15002/input/regression/phrasing-slur-multiple.ly#newcode12 input/regression/phrasing-slur-multiple.ly:12: altPhSlur = #(ly:export (make-music 'PhrasingSlurEvent 'span-direction START 'spanner-id "alt")) ...
12 years, 8 months ago (2011-07-07 19:07:13 UTC) #8
Reinhold
12 years, 8 months ago (2011-07-09 09:01:39 UTC) #9
Thanks, Neil for your excellent code review (again and again, I'm really
fascinated by your abilities!).

I've uploaded a new patch that includes all those suggestions.

Cheers,
Reinhold

http://codereview.appspot.com/4643067/diff/15002/input/regression/phrasing-sl...
File input/regression/phrasing-slur-multiple.ly (right):

http://codereview.appspot.com/4643067/diff/15002/input/regression/phrasing-sl...
input/regression/phrasing-slur-multiple.ly:12: altPhSlur = #(ly:export
(make-music 'PhrasingSlurEvent 'span-direction START 'spanner-id "alt"))
On 2011/07/07 19:07:13, Neil Puttock wrote:
> remove ly:export
> 
> (identifiers don't need it; it's only useful for exporting scheme code
directly
> inside a music block)

thanks for pointing that out. Initially, I had it without the ly:export, but I
got some error messages (can't remember which), so the first thing I tried was
adding those ly:export calls (and probably something else to fix those errors)
and I didn't get the errors any more ;-)

Yeah, I'm code a lot by trial-and-error ;-)

http://codereview.appspot.com/4643067/diff/15002/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):

http://codereview.appspot.com/4643067/diff/15002/lily/phrasing-slur-engraver....
lily/phrasing-slur-engraver.cc:52: vector<Stream_event *> start_events_;
On 2011/07/07 19:07:13, Neil Puttock wrote:
> why not use
> Drul_array<vector<Stream_event * > > events_;

Because (1) I don't like such nested data structures and (2) it doesn't simplify
the code.

http://codereview.appspot.com/4643067/diff/15002/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/4643067/diff/15002/lily/slur-engraver.cc#newcod...
lily/slur-engraver.cc:176: for (int j = slurs_.size () - 1 ; j >= 0; j--)
On 2011/07/07 19:07:13, Neil Puttock wrote:
> for (vsize j = slurs_.size (); j--;)

Ah, nice idea to use the check to decrement j to the proper value for each loop!

http://codereview.appspot.com/4643067/diff/15002/ly/grace-init.ly
File ly/grace-init.ly (right):

http://codereview.appspot.com/4643067/diff/15002/ly/grace-init.ly#newcode15
ly/grace-init.ly:15: s1*0\graceSlur
On 2011/07/07 19:07:13, Neil Puttock wrote:
> \startGraceSlur ?

I was thinking of \melisma + \melismaEnd ... But you are right, most other
spannes use \start... + \stop...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b