From 923e6a554e87c9eacdd250088a763419e2ec6d2b Mon Sep 17 00:00:00 2001
From: Robin Gareus <robin@gareus.org>
Date: Wed, 4 Oct 2023 15:10:06 +0200
Subject: [PATCH] Fix MIDI combine (#9466)

Initially thew new region has a length of zero (0:0). When
merging Notes from a truncated region (no explicit note-off)
those notes are lost:
"Stuck note resolution - end time @ 0:0 is before note on: @ 0:0"

Truncate (or split) a region so that a note is cut short:
```
  Region [{<--Note-------->}]
```
becomes
```
  Region [{<--Note--]
                    ^ implicit note-off at region boundary
```

When combining this region with an empty Region after gap,
```
  Region [{<--Note--]  [    ]
```
the result should be
```
  Region [{<--Note->}       ]
```

For this reason, even without a gap between the regions,
the original note length must not be restored.
The result MUST NOT be the same as the original:
```
  Region [{<--Note-------->}]
```
---
 libs/ardour/midi_playlist.cc | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/libs/ardour/midi_playlist.cc b/libs/ardour/midi_playlist.cc
index 83644446a3..1c06bcc406 100644
--- a/libs/ardour/midi_playlist.cc
+++ b/libs/ardour/midi_playlist.cc
@@ -449,29 +449,30 @@ MidiPlaylist::combine (RegionList const & rl, std::shared_ptr<Track> trk)
 
 	std::shared_ptr<Region> first = sorted.front();
 
-	timepos_t earliest (timepos_t::max (Temporal::BeatTime));
+	std::shared_ptr<MidiSource> ms (session().create_midi_source_by_stealing_name (trk));
+	std::shared_ptr<MidiRegion> new_region = std::dynamic_pointer_cast<MidiRegion> (RegionFactory::create (ms, first->derive_properties (false), true, &rwl.thawlist));
+
+	timepos_t earliest (first->position ());
 	timepos_t latest (Temporal::BeatTime);
 
-	for (auto const & r : rl) {
-		assert (std::dynamic_pointer_cast<MidiRegion> (r));
+	new_region->set_position (earliest);
+
+	for (auto const & r : sorted) {
 		if (r->position() < earliest) {
 			earliest = r->position();
 		}
 		if (r->end() > latest) {
 			latest = r->end();
 		}
+		/* We need to explicit set the end, to terminate any MIDI notes
+		 * that extend beyond the end of the region at the region boundary.
+		 */
+		new_region->set_length_unchecked (earliest.distance (r->end()));
+		new_region->merge (std::dynamic_pointer_cast<MidiRegion> (r));
+		remove_region_internal (r, rwl.thawlist);
 	}
 
-	std::shared_ptr<MidiSource> ms (session().create_midi_source_by_stealing_name (trk));
-	std::shared_ptr<MidiRegion> new_region = std::dynamic_pointer_cast<MidiRegion> (RegionFactory::create (ms, first->derive_properties (false), true, &rwl.thawlist));
-
-	timepos_t pos (first->position());
-	new_region->set_position (pos);
-
-	for (auto const & other : sorted) {
-		new_region->merge (std::dynamic_pointer_cast<MidiRegion> (other));
-		remove_region_internal (other, rwl.thawlist);
-	}
+	new_region->set_length_unchecked (earliest.distance (latest));
 
 	/* thin automation.
 	 * Combining MIDI regions plays back automation, the compound
@@ -486,7 +487,7 @@ MidiPlaylist::combine (RegionList const & rl, std::shared_ptr<Track> trk)
 
 	new_region->midi_source (0)->session_saved ();
 
-	add_region_internal (new_region, pos, rwl.thawlist);
+	add_region_internal (new_region, earliest, rwl.thawlist);
 
 	return new_region;
 }