Issue 2584 (redo 1967): please make partcombine merge slurs

The partcombiner does not really bother about keeping the number of
generated start and end slur events matched, so this attempts to cope
by implementing the following behavior:

a) multiple slur starts on the same moment are not an error but the
   same as one.
b) multiple slur ends on the same moment are not an error but the same
   as one.
a2) there will be a slur with direction UP if there is at least one
   such start event, and there will be a slur with direction DOWN if
   there is at least one such end event.  This might imply a double
   slur, but for ending it, a single slur end is sufficient.

Consequently, a^(_( c)) (the second closing paren not required, just
added for prettiness) will add a double slur.

diff --git a/input/regression/phrasing-slur-multiple.ly b/input/regression/phrasing-slur-multiple.ly
index f9ccca61b257ba55a54807c46ee8bc026b71ad9b..dc1edfc44ae27043593b822d8838679ffd973e5d 100644 (file)
--- a/input/regression/phrasing-slur-multiple.ly
+++ b/input/regression/phrasing-slur-multiple.ly
@@ -1,8 +1,9 @@
-\version "2.15.4"
+\version "2.15.40"
 
 #(ly:set-option 'warning-as-error #f)
 #(ly:expect-warning (_ "already have phrasing slur"))
 #(ly:expect-warning (_ "cannot end phrasing slur"))
+#(ly:expect-warning (_ "unterminated phrasing slur"))
 
 \header {
   texidoc = "LilyPond does not support multiple concurrent phrasing slurs with the 
@@ -11,13 +12,14 @@ slur will not be generated.  However, one can can create a second slur with
 a different spanner-id."
 }
 
-altPhSlur = #(make-music 'PhrasingSlurEvent 'span-direction START 'spanner-id "alt")
-altPhSlurEnd = #(make-music 'PhrasingSlurEvent 'span-direction STOP 'spanner-id "alt")
+sp=#(define-event-function (parser location n e) (index? ly:event?)
+     (set! (ly:music-property e 'spanner-id) (format "sp~a" n))
+     e)
 
 \relative c'' { 
   % This will give warnings ("Already have phrasing slur" and "Cannot end phrasing slur")
-  c4\(\( d4\)\( e4\) f\) |
-  % This will give two overlapping slurs:
-  d\(  d\altPhSlur e\) f\altPhSlurEnd |
+  c4\(\(\sp1\( d4\)\(\sp1\( e4\) f\) |
+  % This will give two overlapping slurs and "unterminated phrasing slur" from above
+  d\(  d\sp2\( e\) f\sp2\) |
   
 }

diff --git a/input/regression/slur-multiple.ly b/input/regression/slur-multiple.ly
index f73d93a3166d29ce4c2afb5da56b300c57b542f3..42c859afcd4bb98016961a80f39fe4f2e075f243 100644 (file)
--- a/input/regression/slur-multiple.ly
+++ b/input/regression/slur-multiple.ly
@@ -1,8 +1,9 @@
-\version "2.15.5"
+\version "2.15.40"
 
 #(ly:set-option 'warning-as-error #f)
 #(ly:expect-warning (_ "already have slur"))
 #(ly:expect-warning (_ "cannot end slur"))
+#(ly:expect-warning (_ "unterminated slur"))
 
 \header {
   texidoc = "LilyPond does not support multiple concurrent slurs with the 
@@ -11,13 +12,14 @@ slur will not be generated.  However, one can can create a second slur with
 a different spanner-id."
 }
 
-altSlur = #(make-music 'SlurEvent 'span-direction START 'spanner-id "alt")
-altSlurEnd = #(make-music 'SlurEvent 'span-direction STOP 'spanner-id "alt")
+sp=#(define-event-function (parser location n e) (index? ly:event?)
+     (set! (ly:music-property e 'spanner-id) (format "sp~a" n))
+     e)
 
 \relative c'' { 
   % This will give warnings ("Already have slur" and "Cannot end slur")
-  c4(( d4)( e4) f) |
-  % This will give two overlapping slurs:
-  d(  d\altSlur e) f\altSlurEnd |
+  c4((\sp1( d4)(\sp1( e4) f) |
+  % This will give two overlapping slurs and "unterminated slur" from above
+  d(  d\sp2( e) f\sp2) |
   
 }

diff --git a/lily/phrasing-slur-engraver.cc b/lily/phrasing-slur-engraver.cc
index a3b10d027a158472ef1309b3bfb66aa7c00c5d0b..ea2439534c2b210e42250269471c7c26c684d3c6 100644 (file)
--- a/lily/phrasing-slur-engraver.cc
+++ b/lily/phrasing-slur-engraver.cc
@@ -175,7 +175,7 @@ Phrasing_slur_engraver::process_music ()
       Stream_event *ev = stop_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");
 
-      // Find the slur that is ended with this event (by checking the spanner-id)
+      // Find the slurs that are ended with this event (by checking the spanner-id)
       bool ended = false;
       for (vsize j = slurs_.size (); j--;)
         {
@@ -186,26 +186,80 @@ Phrasing_slur_engraver::process_music ()
               slurs_.erase (slurs_.begin () + j);
             }
         }
-      if (!ended)
+      if (ended)
+        {
+          // Ignore redundant stop events for this id
+          for (vsize j = stop_events_.size (); --j > i;)
+            {
+              if (id == robust_scm2string (stop_events_[j]->get_property ("spanner-id"), ""))
+                stop_events_.erase (stop_events_.begin() + j);
+            }
+        }
+      else
         ev->origin ()->warning (_ ("cannot end phrasing slur"));
     }
 
-  for (vsize i = 0; i < start_events_.size (); i++)
+  vsize old_slurs = slurs_.size ();
+  for (vsize i = start_events_.size (); i--;)
     {
       Stream_event *ev = start_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");
-      bool have_slur = false;
-      // Check if we already have a slur with the same spanner-id.
-      // In that case, don't create a new slur, but print a warning
-      for (vsize i = 0; i < slurs_.size (); i++)
-        have_slur = have_slur || (id == robust_scm2string (slurs_[i]->get_property ("spanner-id"), ""));
-
-      if (have_slur)
-        ev->origin ()->warning (_ ("already have phrasing slur"));
-      else
+      Direction updown = to_dir (ev->get_property ("direction"));
+
+      bool completed;
+      for (vsize j = 0; !(completed = (j == slurs_.size ())); j++)
+        {
+          // Check if we already have a slur with the same spanner-id.
+          if (id == robust_scm2string (slurs_[j]->get_property ("spanner-id"), ""))
+            {
+              if (j < old_slurs)
+                {
+                  // We already have an old slur, so give a warning
+                  // and completely ignore the new slur.
+                  ev->origin ()->warning (_ ("already have phrasing slur"));
+                  start_events_.erase (start_events_.begin () + i);
+                  break;
+                }
+
+              // If this slur event has no direction, it will not
+              // contribute anything new to the existing slur(s), so
+              // we can ignore it.  This is not entirely accurate:
+              // tweaks or context properties like those set with
+              // \slurUp can still override a neutral direction, so
+              // when encountering a slur event with "opposite"
+              // direction first, then one with neutral direction, we
+              // only let the "opposite" direction remain, while if
+              // the order is the other way round, a double slur
+              // results since the direction of the first slur is no
+              // longer attributable to a "neutral" slur event.  A
+              // mixture of neutral and directed events is nothing
+              // that the partcombiner should crank out, and it would
+              // be decidedly strange for manual input.
+
+              if (!updown)
+                break;
+
+              // If the existing slur does not have a direction yet,
+              // give it ours
+
+              Direction slur_dir = to_dir (slurs_[j]->get_property ("direction"));
+
+              if (!slur_dir)
+                {
+                  set_grob_direction (slurs_[j], updown);
+                  break;
+                }
+
+              // If the existing slur has the same direction as ours, drop ours
+
+              if (slur_dir == updown)
+                break;
+            }
+        }
+      // If the loop completed, our slur is new
+      if (completed)
         {
           Grob *slur = make_spanner ("PhrasingSlur", ev->self_scm ());
-          Direction updown = to_dir (ev->get_property ("direction"));
           slur->set_property ("spanner-id", ly_string2scm (id));
           if (updown)
             set_grob_direction (slur, updown);

diff --git a/lily/slur-engraver.cc b/lily/slur-engraver.cc
index 40b3b6bd6343201ae0db673753b09cd826b298d8..e4962852888aa809b9875aea09109313f82105cd 100644 (file)
--- a/lily/slur-engraver.cc
+++ b/lily/slur-engraver.cc
@@ -176,7 +176,7 @@ Slur_engraver::process_music ()
       Stream_event *ev = stop_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");
 
-      // Find the slur that is ended with this event (by checking the spanner-id)
+      // Find the slurs that are ended with this event (by checking the spanner-id)
       bool ended = false;
       for (vsize j = slurs_.size (); j--;)
         {
@@ -187,31 +187,80 @@ Slur_engraver::process_music ()
               slurs_.erase (slurs_.begin () + j);
             }
         }
-      if (!ended)
+      if (ended)
+        {
+          // Ignore redundant stop events for this id
+          for (vsize j = stop_events_.size (); --j > i;)
+            {
+              if (id == robust_scm2string (stop_events_[j]->get_property ("spanner-id"), ""))
+                stop_events_.erase (stop_events_.begin() + j);
+            }
+        }
+      else
         ev->origin ()->warning (_ ("cannot end slur"));
     }
 
+  vsize old_slurs = slurs_.size ();
   for (vsize i = start_events_.size (); i--;)
     {
       Stream_event *ev = start_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");
-      bool have_slur = false;
-      // Check if we already have a slur with the same spanner-id.
-      // In that case, don't create a new slur, but print a warning
-      for (vsize j = 0; j < slurs_.size (); j++)
-        have_slur = have_slur || (id == robust_scm2string (slurs_[j]->get_property ("spanner-id"), ""));
+      Direction updown = to_dir (ev->get_property ("direction"));
 
-      if (have_slur)
+      bool completed;
+      for (vsize j = 0; !(completed = (j == slurs_.size ())); j++)
         {
-          // We already have a slur, so give a warning and completely ignore
-          // the new slur.
-          ev->origin ()->warning (_ ("already have slur"));
-          start_events_.erase (start_events_.begin () + i);
+          // Check if we already have a slur with the same spanner-id.
+          if (id == robust_scm2string (slurs_[j]->get_property ("spanner-id"), ""))
+            {
+              if (j < old_slurs)
+                {
+                  // We already have an old slur, so give a warning
+                  // and completely ignore the new slur.
+                  ev->origin ()->warning (_ ("already have slur"));
+                  start_events_.erase (start_events_.begin () + i);
+                  break;
+                }
+
+              // If this slur event has no direction, it will not
+              // contribute anything new to the existing slur(s), so
+              // we can ignore it.  This is not entirely accurate:
+              // tweaks or context properties like those set with
+              // \slurUp can still override a neutral direction, so
+              // when encountering a slur event with "opposite"
+              // direction first, then one with neutral direction, we
+              // only let the "opposite" direction remain, while if
+              // the order is the other way round, a double slur
+              // results since the direction of the first slur is no
+              // longer attributable to a "neutral" slur event.  A
+              // mixture of neutral and directed events is nothing
+              // that the partcombiner should crank out, and it would
+              // be decidedly strange for manual input.
+
+              if (!updown)
+                break;
+
+              // If the existing slur does not have a direction yet,
+              // give it ours
+
+              Direction slur_dir = to_dir (slurs_[j]->get_property ("direction"));
+
+              if (!slur_dir)
+                {
+                  set_grob_direction (slurs_[j], updown);
+                  break;
+                }
+
+              // If the existing slur has the same direction as ours, drop ours
+
+              if (slur_dir == updown)
+                break;
+            }
         }
-      else
+      // If the loop completed, our slur is new
+      if (completed)
         {
           Grob *slur = make_spanner ("Slur", ev->self_scm ());
-          Direction updown = to_dir (ev->get_property ("direction"));
           slur->set_property ("spanner-id", ly_string2scm (id));
           if (updown)
             set_grob_direction (slur, updown);