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

fix: division by zero in compatmidirenderer #24839

Closed
wants to merge 1 commit into from
Closed

fix: division by zero in compatmidirenderer #24839

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
20 changes: 9 additions & 11 deletions src/engraving/compat/midi/compatmidirender.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,11 +327,11 @@ void CompatMidiRender::renderTremolo(Chord* chord, std::vector<NoteEventList>& e
}

// render tremolo with multiple events
int t = Constants::DIVISION / 1 << (tremolo.lines() + chord->durationType().hooks());
if (t == 0) {
t = 1;
}
cbjeukendrup marked this conversation as resolved.
Show resolved Hide resolved
if (chord->tremoloChordType() == TremoloChordType::TremoloFirstChord) {
int t = Constants::DIVISION / (1 << (tremolo.lines() + chord->durationType().hooks()));
if (t == 0) { // avoid crash on very short tremolo
t = 1;
}
SegmentType st = SegmentType::ChordRest;
Segment* seg2 = seg->next(st);
track_idx_t track = chord->track();
Expand Down Expand Up @@ -404,15 +404,13 @@ void CompatMidiRender::renderTremolo(Chord* chord, std::vector<NoteEventList>& e
events->clear();
}
} else if (chord->tremoloChordType() == TremoloChordType::TremoloSingle) {
int t = Constants::DIVISION / (1 << (tremolo.lines() + chord->durationType().hooks()));
if (t == 0) { // avoid crash on very short tremolo
t = 1;
}
double tpoch = muse::RealIsNull(tremoloPartOfChord) ? 1.0 : tremoloPartOfChord;
Copy link
Contributor

Choose a reason for hiding this comment

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

I took another look and this check appears to be unnecessary: just at the top of CompatMidiRender::renderTremolo, we already have

    if (muse::RealIsNull(tremoloPartOfChord)) {
        return;
    }


int tremoloEventsSize = chord->ticks().ticks() / t * tremoloPartOfChord;
int tremoloEventsSize = static_cast<int>(static_cast<double>(chord->ticks().ticks())
/ (static_cast<double>(t) * tpoch));
Comment on lines +409 to +410
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the double casts don't make a difference here: when (static_cast<double>(t) * tpoch) > chord->ticks().ticks(), then the result of the division would be between 0.0 and 1.0, so casting that back to int will still result in 0.

But you are changing the meaning of the code. Essentially, this is what's happening:

-(chord->ticks().ticks() / t) * tremoloPartOfChord
+chord->ticks().ticks() / (t * tremoloPartOfChord)

I think that's incorrect: if the whole chord were a tremolo, then (chord->ticks().ticks() / t) would be the correct number of events; and if only part of the chord is a tremolo (i.e. tremoloPartOfChord < 1) then (chord->ticks().ticks() / t) * tremoloPartOfChord would be the correct number.

It looks like the calculation of tremoloEventsSize can stay as it is (even extra casts to double would not have any effect); all that's needed would be to check that tremoloEventsSize is not zero. If it is zero, then just do an early return, since the for loop below would not be run anyway in that case.

Apart from that, the precision of the calculations could of course be improved, by utilising Fraction and/or double everywhere, i.e. in t, tremoloEventsSize, tremoloTime, tremoloEventStep; and cast to int only at the very end. (Note that that ensures so that t != 0, since that would only happen as a result of rounding errors.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok
Initially I had a problem in another branch and this branch just doesn't have this check which you've mentioned.

    if (muse::RealIsNull(tremoloPartOfChord)) {
        return;
    }

And this more than enough.
Thank you for your help and time, @cbjeukendrup

Closing this PR.


constexpr int fullEventTime = 1000;
int tremoloTime = fullEventTime * tremoloPartOfChord;
constexpr double fullEventTime = 1000.0;
int tremoloTime = static_cast<int>(fullEventTime * tremoloPartOfChord);
int tremoloEventStep = tremoloTime / tremoloEventsSize;
ontime += tremoloTime;

Expand Down
Loading