tremolo_tied_notes_fix #24774
tremolo_tied_notes_fix #24774
Conversation
…uration for tied notes
| @@ -32,6 +32,38 @@ using namespace mu::engraving; | |||
| using namespace muse; | |||
| using namespace muse::mpe; | |||
|
|
|||
| static const Note* findFirstTremoloNote(const Note* note) | |||
There was a problem hiding this comment.
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.
| const Note* firstTremoloNote = findFirstTremoloNote(note); | ||
| if (firstTremoloNote) { | ||
| if (firstTremoloNote != note) { | ||
| tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks()); |
There was a problem hiding this comment.
I think you need to do something with ctx.positionTickOffset here
| tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks()); | |
| tnd.timestamp = timestampFromTicks(score, firstTremoloNote->tick().ticks() + ctx.positionTickOffset); |
| const TimestampAndDuration lastNoteTnD = timestampAndDurationFromStartAndDurationTicks(score, | ||
| lastTremoloNote->tick().ticks(), | ||
| lastTremoloNote->chord()->actualTicks().ticks(), | ||
| ctx.positionTickOffset); | ||
|
|
||
| tnd.duration = lastNoteTnD.timestamp + lastNoteTnD.duration - tnd.timestamp; |
There was a problem hiding this comment.
Simpler:
| 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
| const Note* firstTremoloNote = findFirstTremoloNote(note); | ||
| if (firstTremoloNote) { | ||
| if (firstTremoloNote != note) { |
There was a problem hiding this comment.
Either
| const Note* firstTremoloNote = findFirstTremoloNote(note); | |
| if (firstTremoloNote) { | |
| if (firstTremoloNote != note) { | |
| if (const Note* firstTremoloNote = findFirstTremoloNote(note)) { | |
| if (firstTremoloNote != note) { |
or
| const Note* firstTremoloNote = findFirstTremoloNote(note); | |
| if (firstTremoloNote) { | |
| if (firstTremoloNote != note) { | |
| const Note* firstTremoloNote = findFirstTremoloNote(note); | |
| if (firstTremoloNote && firstTremoloNote != note) { |
| } | ||
|
|
||
| const Note* lastTremoloNote = findLastTremoloNote(note); | ||
| if (lastTremoloNote) { |
There was a problem hiding this comment.
Do we need to check lastTremoloNote != note analogously to above?
|
|
||
| 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))); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Resolves: #23168