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

tremolo_tied_notes_fix #24774

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+341 −24
Open

tremolo_tied_notes_fix #24774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 3 additions & 19 deletions src/engraving/dom/chord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,33 +463,17 @@ bool Chord::containsEqualArticulations(const Chord* other) const
return true;
}

bool Chord::containsEqualArpeggio(const Chord* other) const
{
if (m_arpeggio && other->m_arpeggio) {
if (m_arpeggio->arpeggioType() != other->m_arpeggio->arpeggioType()) {
return false;
}
}

return !m_arpeggio && !other->m_arpeggio;
}

bool Chord::containsEqualTremolo(const Chord* other) const
{
if (tremoloSingleChord() && other->tremoloSingleChord()) {
if (tremoloSingleChord()->tremoloType() != other->tremoloSingleChord()->tremoloType()) {
return false;
}
return tremoloSingleChord()->tremoloType() == other->tremoloSingleChord()->tremoloType();
}

if (tremoloTwoChord() && other->tremoloTwoChord()) {
if (tremoloTwoChord()->tremoloType() != other->tremoloTwoChord()->tremoloType()) {
return false;
}
return tremoloTwoChord()->tremoloType() == other->tremoloTwoChord()->tremoloType();
}

return !tremoloSingleChord() && !other->tremoloSingleChord()
&& !tremoloTwoChord() && !other->tremoloTwoChord();
return false;
}

//---------------------------------------------------------
Expand Down
1 change: 0 additions & 1 deletion src/engraving/dom/chord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ class Chord final : public ChordRest
Chord& operator=(const Chord&) = delete;

bool containsEqualArticulations(const Chord* other) const;
bool containsEqualArpeggio(const Chord* other) const;
bool containsEqualTremolo(const Chord* other) const;

// Score Tree functions
Expand Down
79 changes: 77 additions & 2 deletions src/engraving/playback/renderers/tremolorenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,38 @@ using namespace mu::engraving;
using namespace muse;
using namespace muse::mpe;

static const Note* findFirstTremoloNote(const Note* note)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods Note::first/lastTiedNote contain some additional complications, one seemingly to prevent infinite loops. It might be good to investigate why these complications are necessary there, and if they are necessary here too.

{
while (note && note->tieBack()) {
const Note* prevNote = note->tieBack()->startNote();
const Chord* prevChord = prevNote ? prevNote->chord() : nullptr;

if (!prevChord || !prevChord->containsEqualTremolo(note->chord())) {
break;
}

note = prevNote;
}

return note;
}

static const Note* findLastTremoloNote(const Note* note)
{
while (note && note->tieFor()) {
const Note* nextNote = note->tieFor()->endNote();
const Chord* nextChord = nextNote ? nextNote->chord() : nullptr;

if (!nextChord || !nextChord->containsEqualTremolo(note->chord())) {
break;
}

note = nextNote;
}

return note;
}

const ArticulationTypeSet& TremoloRenderer::supportedTypes()
{
static const mpe::ArticulationTypeSet types = {
Expand Down Expand Up @@ -97,6 +129,8 @@ void TremoloRenderer::doRender(const EngravingItem* item, const mpe::Articulatio

stepDurationTicks = overallDurationTicks / stepsCount;

TremoloTimeCache tremoloTimeCache;

if (tremolo.two) {
const Chord* firstTremoloChord = tremolo.two->chord1();
const Chord* secondTremoloChord = tremolo.two->chord2();
Expand All @@ -113,15 +147,15 @@ void TremoloRenderer::doRender(const EngravingItem* item, const mpe::Articulatio
}

buildAndAppendEvents(currentChord, preferredType, stepDurationTicks, context.nominalPositionStartTick + i * stepDurationTicks,
context, result);
context, tremoloTimeCache, result);
}

return;
}

for (int i = 0; i < stepsCount; ++i) {
buildAndAppendEvents(chord, preferredType, stepDurationTicks, context.nominalPositionStartTick + i * stepDurationTicks,
context, result);
context, tremoloTimeCache, result);
}
}

Expand All @@ -137,6 +171,7 @@ int TremoloRenderer::stepDurationTicks(const Chord* chord, int tremoloLines)
void TremoloRenderer::buildAndAppendEvents(const Chord* chord, const ArticulationType type,
const int stepDurationTicks,
const int startTick, const RenderingContext& context,
TremoloTimeCache& tremoloCache,
mpe::PlaybackEventList& result)
{
const Score* score = chord->score();
Expand All @@ -160,8 +195,48 @@ void TremoloRenderer::buildAndAppendEvents(const Chord* chord, const Articulatio
noteCtx.dynamicLevel = context.playbackCtx->appliableDynamicLevel(note->track(), utick);

NoteArticulationsParser::buildNoteArticulationMap(note, context, noteCtx.chordCtx.commonArticulations);

const TimestampAndDuration& tremoloTnD = tremoloTimeAndDuration(note, context, tremoloCache);
muse::mpe::ArticulationAppliedData& articulationData = noteCtx.chordCtx.commonArticulations.at(type);
articulationData.meta.timestamp = tremoloTnD.timestamp;
articulationData.meta.overallDuration = tremoloTnD.duration;

updateArticulationBoundaries(type, noteCtx.timestamp, noteCtx.duration, noteCtx.chordCtx.commonArticulations);

result.emplace_back(buildNoteEvent(std::move(noteCtx)));
Comment on lines +198 to 206
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it work exactly, is the entire tremolo rendered for the first tied note? And are subsequent tied notes encountered here too, or are they filtered out already? If we encounter them here, won't we render the tremolo multiple times? I don't fully understand why we need to cache something because I suppose we see each note only once, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're rendering all tied notes (not just the 1st one) that have the tremolo articulation. For each tied note we're creating stepCount sub-events (see TremoloRenderer::doRender; there is a for loop there). Each sub-event has ArticulationMeta that stores the global start position and duration of the tremolo, which are then used to trigger a single continuous sound (if the instrument supports it, otherwise we will just play multiple note on/off events). The cache is used to avoid recalculation the start position and duration for sub-events

}
}

const TimestampAndDuration& TremoloRenderer::tremoloTimeAndDuration(const Note* note, const RenderingContext& ctx,
TremoloTimeCache& cache)
{
auto cacheIt = cache.find(note);
if (cacheIt != cache.end()) {
return cacheIt->second;
}

TimestampAndDuration& tnd = cache[note];
tnd.timestamp = ctx.nominalTimestamp;
tnd.duration = ctx.nominalDuration;

const Score* score = note->score();

const Note* firstTremoloNote = findFirstTremoloNote(note);
if (firstTremoloNote) {
if (firstTremoloNote != note) {
Comment on lines +224 to +226
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either

Suggested change
const Note* firstTremoloNote = findFirstTremoloNote(note);
if (firstTremoloNote) {
if (firstTremoloNote != note) {
if (const Note* firstTremoloNote = findFirstTremoloNote(note)) {
if (firstTremoloNote != note) {

or

Suggested change
const Note* firstTremoloNote = findFirstTremoloNote(note);
if (firstTremoloNote) {
if (firstTremoloNote != note) {
const Note* firstTremoloNote = findFirstTremoloNote(note);
if (firstTremoloNote && firstTremoloNote != note) {

tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to do something with ctx.positionTickOffset here

Suggested change
tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks());
tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks() + ctx.positionTickOffset);

}
}

const Note* lastTremoloNote = findLastTremoloNote(note);
if (lastTremoloNote) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check lastTremoloNote != note analogously to above?

const TimestampAndDuration lastNoteTnD = timestampAndDurationFromStartAndDurationTicks(score,
lastTremoloNote->tick().ticks(),
lastTremoloNote->chord()->actualTicks().ticks(),
ctx.positionTickOffset);

tnd.duration = lastNoteTnD.timestamp + lastNoteTnD.duration - tnd.timestamp;
Comment on lines +233 to +238
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler:

Suggested change
const TimestampAndDuration lastNoteTnD = timestampAndDurationFromStartAndDurationTicks(score,
lastTremoloNote->tick().ticks(),
lastTremoloNote->chord()->actualTicks().ticks(),
ctx.positionTickOffset);
tnd.duration = lastNoteTnD.timestamp + lastNoteTnD.duration - tnd.timestamp;
const timestamp_t endTimestamp = timestampFromTicks(
score,
lastTremoloNote->tick().ticks() + lastTremoloNote->chord()->actualTicks().ticks() + ctx.positionTickOffset
);
tnd.duration = endTimestamp - tnd.timestamp;

I.e. instead of doing the addition in lastNoteTnD.timestamp + lastNoteTnD.duration, do the addition still in the ticks world and convert to timestamp world only once

}

return tnd;
}
6 changes: 5 additions & 1 deletion src/engraving/playback/renderers/tremolorenderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,13 @@ class TremoloRenderer : public RenderBase<TremoloRenderer>
muse::mpe::PlaybackEventList& result);

private:
using TremoloTimeCache = std::unordered_map<const Note*, TimestampAndDuration>;

static int stepDurationTicks(const Chord* chord, int tremoloLines);
static void buildAndAppendEvents(const Chord* chord, const muse::mpe::ArticulationType type, const int stepDurationTicks,
const int startTick, const RenderingContext& ctx, muse::mpe::PlaybackEventList& result);
const int startTick, const RenderingContext& ctx, TremoloTimeCache& tremoloCache,
muse::mpe::PlaybackEventList& result);
static const TimestampAndDuration& tremoloTimeAndDuration(const Note* note, const RenderingContext& ctx, TremoloTimeCache& cache);
};
}

Expand Down
9 changes: 8 additions & 1 deletion src/engraving/playback/renderingcontext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,14 @@ inline bool isNotePlayable(const Note* note, const muse::mpe::ArticulationMap& a
const Chord* firstChord = tie->startNote()->chord();
const Chord* lastChord = tie->endNote()->chord();

return !firstChord->containsEqualTremolo(lastChord);
if (firstChord && lastChord) {
if (firstChord->tremoloType() != TremoloType::INVALID_TREMOLO
|| lastChord->tremoloType() != TremoloType::INVALID_TREMOLO) {
return true;
}
}

return false;
}

return true;
Expand Down
179 changes: 179 additions & 0 deletions ...keventsrenderer_data/single_chord_tremolo_tied_notes/single_chord_tremolo_tied_notes.mscx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
<?xml version="1.0" encoding="UTF-8"?>
<museScore version="4.40">
<programVersion>4.4.2</programVersion>
<programRevision></programRevision>
<LastEID>1004</LastEID>
<Score>
<Division>480</Division>
<showInvisible>1</showInvisible>
<showUnprintable>1</showUnprintable>
<showFrames>1</showFrames>
<showMargins>0</showMargins>
<open>1</open>
<metaTag name="arranger"></metaTag>
<metaTag name="audioComUrl"></metaTag>
<metaTag name="composer">Composer / arranger</metaTag>
<metaTag name="copyright"></metaTag>
<metaTag name="creationDate">2024-09-13</metaTag>
<metaTag name="lyricist"></metaTag>
<metaTag name="movementNumber"></metaTag>
<metaTag name="movementTitle"></metaTag>
<metaTag name="platform">Apple Macintosh</metaTag>
<metaTag name="source"></metaTag>
<metaTag name="sourceRevisionId"></metaTag>
<metaTag name="subtitle">Subtitle</metaTag>
<metaTag name="translator"></metaTag>
<metaTag name="workNumber"></metaTag>
<metaTag name="workTitle">Untitled score</metaTag>
<Order id="orchestral">
<name>Orchestral</name>
<instrument id="piano">
<family id="keyboards">Keyboards</family>
</instrument>
<instrument id="violin">
<family id="orchestral-strings">Orchestral Strings</family>
</instrument>
<section id="woodwind" brackets="true" barLineSpan="true" thinBrackets="true">
<family>flutes</family>
<family>oboes</family>
<family>clarinets</family>
<family>saxophones</family>
<family>bassoons</family>
<unsorted group="woodwinds"/>
</section>
<section id="brass" brackets="true" barLineSpan="true" thinBrackets="true">
<family>horns</family>
<family>trumpets</family>
<family>cornets</family>
<family>flugelhorns</family>
<family>trombones</family>
<family>tubas</family>
</section>
<section id="timpani" brackets="true" barLineSpan="true" thinBrackets="true">
<family>timpani</family>
</section>
<section id="percussion" brackets="true" barLineSpan="true" thinBrackets="true">
<family>keyboard-percussion</family>
<family>drums</family>
<family>unpitched-metal-percussion</family>
<family>unpitched-wooden-percussion</family>
<family>other-percussion</family>
</section>
<family>keyboards</family>
<family>harps</family>
<family>organs</family>
<family>synths</family>
<soloists/>
<section id="voices" brackets="true" barLineSpan="false" thinBrackets="true">
<family>voices</family>
<family>voice-groups</family>
</section>
<section id="strings" brackets="true" barLineSpan="true" thinBrackets="true">
<family>orchestral-strings</family>
</section>
<unsorted/>
</Order>
<Part id="1">
<Staff id="1">
<StaffType group="pitched">
<name>stdNormal</name>
</StaffType>
<bracket type="1" span="2" col="2" visible="1"/>
<barLineSpan>1</barLineSpan>
</Staff>
<trackName>Violin</trackName>
<Instrument id="violin">
<longName>Violin</longName>
<shortName>Vln.</shortName>
<trackName>Violin</trackName>
<minPitchP>55</minPitchP>
<maxPitchP>103</maxPitchP>
<minPitchA>55</minPitchA>
<maxPitchA>88</maxPitchA>
<instrumentId>strings.violin</instrumentId>
<Channel name="arco">
<program value="40"/>
<synti>Fluid</synti>
</Channel>
<Channel name="pizzicato">
<program value="45"/>
<synti>Fluid</synti>
</Channel>
<Channel name="tremolo">
<program value="44"/>
<synti>Fluid</synti>
</Channel>
</Instrument>
</Part>
<Staff id="1">
<Measure>
<voice>
<KeySig>
<eid>1554778161175</eid>
<concertKey>0</concertKey>
</KeySig>
<TimeSig>
<eid>1546188226585</eid>
<sigN>4</sigN>
<sigD>4</sigD>
</TimeSig>
<Rest>
<eid>4299262263322</eid>
<durationType>whole</durationType>
</Rest>
</voice>
</Measure>
<Measure>
<voice>
<Chord>
<eid>3083786518643</eid>
<durationType>whole</durationType>
<Note>
<eid>3079491551252</eid>
<Spanner type="Tie">
<Tie>
<eid>4260607557663</eid>
</Tie>
<next>
<location>
<measures>1</measures>
</location>
</next>
</Spanner>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
<TremoloSingleChord>
<subtype>r32</subtype>
<eid>3423088935049</eid>
</TremoloSingleChord>
</Chord>
</voice>
</Measure>
<Measure>
<voice>
<Chord>
<eid>3083786518643</eid>
<durationType>whole</durationType>
<Note>
<eid>3079491551252</eid>
<Spanner type="Tie">
<prev>
<location>
<measures>-1</measures>
</location>
</prev>
</Spanner>
<pitch>65</pitch>
<tpc>13</tpc>
</Note>
<TremoloSingleChord>
<subtype>r32</subtype>
<eid>3423088935049</eid>
</TremoloSingleChord>
</Chord>
</voice>
</Measure>
</Staff>
</Score>
</museScore>
Loading