fix #12971: note collisions in playback #3513
Conversation
|
I haven't really analyzed the logic of your fixupMIDI() function, but it appears it would take only O(n) time and not O(n^2), which is good. And in any case it shouldn't affect overall score responsiveness, just the delay when hitting the play button. It should be tested with a reasonably large score. to be sure it isn't too bad. But I think your overall approach is sound. |
|
There are a couple MIDI tests failing in Travis CI. Well possible that the references need fixing, not the code |
synthesizer/event.cpp
Outdated
| if (it->second.type() == ME_NOTEON) { | ||
| unsigned short np = nowPlaying[it->second.channel()][it->second.pitch()]; | ||
| if (it->second.velo() == 0) { | ||
| if (np == 0) { |
There was a problem hiding this comment.
Too many unneeded braces for my taste
There was a problem hiding this comment.
OK. Some people at work insist on them, but I agree and will remove them (and see after the tests).
There was a problem hiding this comment.
And maybe using if ((np == 0) || (--np > 0)) here? Not sure though whether this might undefined behavoir.
Edit: as per https://en.wikipedia.org/wiki/Sequence_point this should be clean, the || is a sequence point.
There was a problem hiding this comment.
Hmm, okay. I had a few debugging fprintf(stderr, …); in between, which is why I had the separate logic.
|
@Jojo-Schmitz style concerns addressed, tests fixed, commits squashed |
synthesizer/event.h
Outdated
| int _highestChannel = 15; | ||
| public: | ||
| void fixupMIDI(); | ||
| void haveChannel(int c) { if (c > _highestChannel) _highestChannel = c; } |
There was a problem hiding this comment.
I'm not sure about the method title. has* prefix is usually used to specify functions checking presence of something like
bool hasValue() or something like that. I would never assume that such methods can assign some values inside.
Maybe, 'setHighestChannel()' is better?
There was a problem hiding this comment.
Agreed, I've stumbled across this too, this is not a getter method as the name suggests, but a setter
|
Joachim Schmitz dixit:
+ void haveChannel(int c) { if (c > _highestChannel) _highestChannel = c; }
Agreed, I've stumbled across this too
How about registerChannel() then?
The idea is that this is called for all channels anyway.
In the current implementation we only need the highest
number, since we use a non-sparse array, but like this,
nobody gets the idea to optimise prematurely.
|
MIDI export will still have colliding note if they are in different staves; same stave is fixed though. Also adjust MIDI testcases: the reference output did hardcode the old behaviour of not refcounting.
|
@Jojo-Schmitz @anatoly-os renamed |
|
Let's focus on #3514 since it contains this PR. |
MIDI export will still have colliding note if they are in
different staves; same stave is fixed though