Fix note collisions for 3.0 #3548
Conversation
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
2 times, most recently
from
6de3cc0
to
b8b582b
Compare
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
b8b582b
to
5b7b23b
Compare
|
Let's used the approach in #3560 for now. |
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
5b7b23b
to
fe242ee
Compare
|
• Rebased on current master (as of today) I’ll update this PR as the others will be merged into 2.2.1, to make it easier to keep track of things. |
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
fe242ee
to
96d95c7
Compare
|
@lasconic rebased onto latest master and merged the two PRs into the respective correct commits (first commit to stay, second commit to be |
|
@mirabilos could you please rebase the PR? |
|
@anatoly-os sure, but give me a couple of days or so. With our without the restriking part? |
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
96d95c7
to
891f692
Compare
|
Rebased against latest master, keeping the split into two commits (so the second one can later easily be reverted), check second commit against #3797 and remove one useless/unused variable. |
| }; | ||
|
|
||
| /* track info for each channel (on the heap, 0-initialised) */ | ||
| struct channelInfo *info = (struct channelInfo *)calloc(_highestChannel + 1, sizeof(struct channelInfo)); |
There was a problem hiding this comment.
Can we avoid c-like allocations using C++11 style instead like using new() operator or even std::unique_ptr? The last is known to work without side effects on memory allocation.
|
@mirabilos I left few comments on this PR. Sorry, I absolutely forgot we need to correctly process such kind of notes collisions. Let's finish this PR so that it could be merged. |
|
anatoly-os dixit:
@mirabilos I left few comments on this PR. Sorry, I absolutely forgot
@we need to correctly process such kind of notes collisions. Let's
Indeed.
@finish this PR so that it could be merged.
Can we please first get this merged so we have the (exact) same
functionality from 2.x ported to master, and only then change
the code? It’s work enough keeping track of the 2.x changes,
rebasing them against master, etc. all the time, and I’m glad I
managed to dig out everything involved.
Additionally, these two commits are comprised of code from me
and lasconic. I don’t want to add more complications to that,
and the code clearly was good enough to have been shipped in
actual releases, so it should be good to merge as-is.
While I agree with…
When rebasing, please put `continue` at the next line.
and almost certainly wouldn’t have written that myself (so
this is probably from one of lasconic’s commits), there is
such a thing called “separation of concerns”. This PR very
much concerns itself with bringing master up-to-date with
2.x and really ought to have been dealt with within DAYS,
at most, of the related commits in 2.x.
Can we avoid c-like allocations using C++11 style instead like using
`new()` operator or even `std::unique_ptr`? The last is known to work
without side effects on memory allocation.
This is one of the things I’d prefer to leave to people
with more C++ experience than myself. Another reason to
have fixups later. I’m not capable enough to fulfil this,
if it were formulated as requirement, and so master would
lose bugfixes from 2.x.
Did you track performance impact of this change? Afaics it adds some
computations to on fly generation of the audio.
Yes, we had some discussion on that, with multiple people
involved, but I don’t recall when or on what medium exactly.
Probably IRC. It’s been some months or so since then, after all.
I do recall that the performance impact was less than we’d
have expected, and acceptable to all involved.
Also, again, this PR is *exactly* about making master catch
up with the bugfixes that have been shipping for *several*
releases by now, and as such is more than overdue, especially
in its current form. (The squash was requested by lasconic,
and the split into two commits is needed for upcoming changes.)
|
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
2 times, most recently
from
3801d0f
to
c7d4f17
Compare
|
AppVeyor was not even triggered. Could you push some small changes? |
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
c7d4f17
to
fb4b9a4
Compare
|
anatoly-os dixit:
AppVeyor was not even triggered. Could you push some small changes?
Weird. Sure, did so just now.
|
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
fb4b9a4
to
23ec314
Compare
Also adjust MIDI testcases: the reference output did hardcode the old behaviour of not refcounting.
This brings master to the same state as v2.3.2 wrt. the MIDI issues; *this* commit can later easily be reverted once we switch away from restriking again as @lasconic indicated in: https://musescore.org/en/node/270562#comment-826020
mirabilos
force-pushed
the
fix-note-collisions-3.0
branch
from
23ec314
to
0a0ce26
Compare
|
@anatoly-os is there anything holding off merging this? I had to rebase it again… |
|
My last question is not addressed yet. My last concern is about possible performance drop in playback time due to introducing fixupMIDI() method. |
|
We analysed this before the patch was accepted in 2.x and all involved people agreed that the performance impact was neglegible. |
Same as #3514 for
master.Please test first, I do not currently have a development environment for
master(will do after 2.2.0 is released).