Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hairpins_overlap_fix #23948

Merged
merged 2 commits into from
Aug 12, 2024
Merged

hairpins_overlap_fix #23948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cea54e
fix #23940: fixed hairpins overlap
RomanPudashkin Aug 7, 2024
e65f2fc
ignore hairpins on linked staves
RomanPudashkin Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/engraving/playback/playbackcontext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ void PlaybackContext::handleSpanners(const ID partId, const Score* score, const
return;
}

auto intervals = spannerMap.findOverlapping(segmentStartTick, segmentEndTick - 1);
auto intervals = spannerMap.findOverlapping(segmentStartTick + 1, segmentEndTick - 1);
for (const auto& interval : intervals) {
const Spanner* spanner = interval.value;

Expand All @@ -376,6 +376,11 @@ void PlaybackContext::handleSpanners(const ID partId, const Score* score, const
continue;
}

const Staff* staff = spanner->staff();
if (staff && !staff->isPrimaryStaff()) {
continue; // ignore linked staves
}

int spannerFrom = spanner->tick().ticks();
int spannerTo = spannerFrom + std::abs(spanner->ticks().ticks());

Expand Down
83 changes: 77 additions & 6 deletions src/engraving/tests/playback/playbackcontext_data/dynamics/hairpins_and_repeats.mscx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<museScore version="4.20">
<programVersion>4.2.0</programVersion>
<museScore version="4.40">
<programVersion>4.4.0</programVersion>
<programRevision></programRevision>
<LastEID>2149</LastEID>
<LastEID>3025</LastEID>
<Score>
<Division>480</Division>
<showInvisible>1</showInvisible>
Expand Down Expand Up @@ -94,22 +94,92 @@
</Part>
<Staff id="1">
<Measure>
<startRepeat/>
<endRepeat>2</endRepeat>
<voice>
<TimeSig>
<eid>47244640280</eid>
<eid>9616431775769</eid>
<sigN>4</sigN>
<sigD>4</sigD>
</TimeSig>
<Dynamic>
<subtype>p</subtype>
<velocity>49</velocity>
<eid>11879879540772</eid>
</Dynamic>
<Spanner type="HairPin">
<HairPin>
<subtype>0</subtype>
<eid>13052405612638</eid>
<Segment>
<subtype>0</subtype>
<offset x="0" y="1.75"/>
<off2 x="1.16325" y="0"/>
<eid>13056700579905</eid>
</Segment>
</HairPin>
<next>
<location>
<measures>1</measures>
</location>
</next>
</Spanner>
<Chord>
<eid>9715216023667</eid>
<durationType>quarter</durationType>
<Note>
<eid>9710921056276</eid>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
</Chord>
<Chord>
<eid>9745280794739</eid>
<durationType>quarter</durationType>
<Note>
<eid>9740985827348</eid>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
</Chord>
<Chord>
<eid>9783935500403</eid>
<durationType>quarter</durationType>
<Note>
<eid>9779640533012</eid>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
</Chord>
<Chord>
<eid>9814000271475</eid>
<durationType>quarter</durationType>
<Note>
<eid>9809705304084</eid>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
</Chord>
</voice>
</Measure>
<Measure>
<startRepeat/>
<endRepeat>2</endRepeat>
<voice>
<Dynamic>
<subtype>f</subtype>
<velocity>96</velocity>
<eid>8766028251171</eid>
</Dynamic>
<Spanner type="HairPin">
<prev>
<location>
<measures>-1</measures>
</location>
</prev>
</Spanner>
<Spanner type="HairPin">
<HairPin>
<subtype>0</subtype>
<direction>down</direction>
<eid>5549097746526</eid>
<Segment>
<subtype>0</subtype>
Expand Down Expand Up @@ -184,6 +254,7 @@
<Spanner type="HairPin">
<HairPin>
<subtype>0</subtype>
<direction>down</direction>
<eid>408021893214</eid>
<Segment>
<subtype>0</subtype>
Expand Down
43 changes: 32 additions & 11 deletions src/engraving/tests/playback/playbackcontext_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class Engraving_PlaybackContextTests : public ::testing::Test
}
};

//! Checks that hairpins outside/inside repeats don't overlap. See:
//! https://github.com/musescore/MuseScore/issues/16167
//! https://github.com/musescore/MuseScore/issues/23940
TEST_F(Engraving_PlaybackContextTests, Hairpins_Repeats)
{
// [GIVEN] Score with hairpins and repeats
Expand All @@ -87,29 +90,47 @@ TEST_F(Engraving_PlaybackContextTests, Hairpins_Repeats)
// [GIVEN]
DynamicLevelMap expectedDynamics;

// [GIVEN] 1st hairpin (inside the repeat): f -> fff
// [GIVEN] 1st hairpin (before the repeat): p -> f
constexpr mpe::dynamic_level_t p = dynamicLevelFromType(mpe::DynamicType::p);
constexpr mpe::dynamic_level_t f = dynamicLevelFromType(mpe::DynamicType::f);

const std::map<int, int> p_to_f_curve = TConv::easingValueCurve(1920, HAIRPIN_STEPS, static_cast<int>(f - p),
ChangeMethod::NORMAL);

for (const auto& pair : p_to_f_curve) {
mpe::timestamp_t time = timestampFromTicks(score, pair.first);
ASSERT_FALSE(muse::contains(expectedDynamics, time));
expectedDynamics.emplace(time, p + static_cast<dynamic_level_t>(pair.second));
}

// [GIVEN] 2nd hairpin (inside the repeat): f -> fff
constexpr mpe::dynamic_level_t fff = dynamicLevelFromType(mpe::DynamicType::fff);
constexpr int f_to_fff_startTick = 0;
constexpr int f_to_fff_startTick = 1920;

std::map<int, int> f_to_fff_curve = TConv::easingValueCurve(1440, HAIRPIN_STEPS, static_cast<int>(fff - f), ChangeMethod::NORMAL);
const std::map<int, int> f_to_fff_curve = TConv::easingValueCurve(1440, HAIRPIN_STEPS, static_cast<int>(fff - f),
ChangeMethod::NORMAL);

for (const mu::engraving::RepeatSegment* repeatSegment : repeats) {
int tickPositionOffset = repeatSegment->utick - repeatSegment->tick;

for (const auto& pair : f_to_fff_curve) {
mpe::timestamp_t time = timestampFromTicks(score, f_to_fff_startTick + pair.first + tickPositionOffset);
ASSERT_FALSE(muse::contains(expectedDynamics, time));
int tick = f_to_fff_startTick + pair.first + tickPositionOffset;
mpe::timestamp_t time = timestampFromTicks(score, tick);

if (tick != f_to_fff_startTick) { // f already added by the previous hairpin
ASSERT_FALSE(muse::contains(expectedDynamics, time));
}

expectedDynamics.emplace(time, f + static_cast<dynamic_level_t>(pair.second));
}
}

// [GIVEN] 2nd hairpin (right after the repeat): ppp -> p
// [GIVEN] 3rd hairpin (right after the repeat): ppp -> p
constexpr mpe::dynamic_level_t ppp = dynamicLevelFromType(mpe::DynamicType::ppp);
constexpr mpe::dynamic_level_t p = dynamicLevelFromType(mpe::DynamicType::p);
constexpr int ppp_to_p_startTick = 1920 + 1920; // real tick + repeat tick
constexpr int ppp_to_p_startTick = 3840 + 1920; // real tick + repeat tick

std::map<int, int> ppp_to_p_curve = TConv::easingValueCurve(1440, HAIRPIN_STEPS, static_cast<int>(p - ppp), ChangeMethod::NORMAL);
const std::map<int, int> ppp_to_p_curve = TConv::easingValueCurve(1440, HAIRPIN_STEPS, static_cast<int>(p - ppp),
ChangeMethod::NORMAL);

for (const auto& pair : ppp_to_p_curve) {
mpe::timestamp_t time = timestampFromTicks(score, ppp_to_p_startTick + pair.first);
Expand Down Expand Up @@ -292,8 +313,8 @@ TEST_F(Engraving_PlaybackContextTests, Dynamics_Overlap)
constexpr mpe::dynamic_level_t ff = dynamicLevelFromType(mpe::DynamicType::ff);
constexpr mpe::dynamic_level_t pp = dynamicLevelFromType(mpe::DynamicType::pp);

std::map<int, int> ff_to_pp_curve = TConv::easingValueCurve(1920 - Fraction::eps().ticks(),
HAIRPIN_STEPS, static_cast<int>(pp - ff), ChangeMethod::NORMAL);
const std::map<int, int> ff_to_pp_curve = TConv::easingValueCurve(1920 - Fraction::eps().ticks(),
HAIRPIN_STEPS, static_cast<int>(pp - ff), ChangeMethod::NORMAL);
for (const auto& pair : ff_to_pp_curve) {
mpe::timestamp_t time = timestampFromTicks(score, pair.first);
expectedDynamics.emplace(time, ff + static_cast<dynamic_level_t>(pair.second));
Expand Down