[MU3] Fix #127341: Implements portamento for fluid #5853
Conversation
Jojo-Schmitz
force-pushed
the
portamento
branch
from
7d079b3 to
ae707d8
Compare
fluid/fluid.cpp
Outdated
| @@ -38,6 +38,11 @@ namespace FluidS { | |||
|
|
|||
| bool Fluid::initialized = false; | |||
|
|
|||
| /* better than a macro to determine inappropriate values for notes*/ | |||
| bool validNote(const int input) { | |||
| return (input < 255 && input > -1); | |||
There was a problem hiding this comment.
shouldn't it be input < 256 or input <= 255?
There was a problem hiding this comment.
Good question. At another place in this file this is claimed to "usually be the TPC" (Tonal Pitch Class), but that in turn has a range of -2 (TPC_INVALID) to 33 (TPC_B_SS), so that doesn't seem to be the case.
It it were (MIDI) pitches though, the limit would rather be 0-127, I believe.
However, as far as I can tell this validNote() is called only from Fluid::setFromKeyPortamento() and that in turn is never called!?!
There was a problem hiding this comment.
This code was copied from fluid synth 2, which isn't quite possible to completely incorporate into MuseScore. Jojo could actually be right, I just wasn't entirely too sure what's necessary for portamento and what's not, or what might be necessary in the future, and I wanted to add as much of fluid synth 2's extra features as possible. It's specifically to do with line 219 of this file:
https://github.com/FluidSynth/fluidsynth/blob/master/src/synth/fluid_chan.h#L219
If the file has changed, look for a macro like #define fluid_channel_is_valid_note(n) (n != INVALID_NOTE) I'm not a fan of macros because I think they can occasionally be unpredictable and otherwise some compilers may or may not optimise them well or add unnecessary symbols.
Long story short, 255 is indeed an invalid value as is any value that's negative.
There was a problem hiding this comment.
-
So
const int INVALID_NOTE = 255;rather than a magic number?
Instead with have this though:enum { INVALID_NOTE = -1};
Confusing? Yes...
- That comment about TPC seems missleading maybe it means PTC, PortamenTo Controler? Or is it really TPC?
Jojo-Schmitz
force-pushed
the
portamento
branch
from
ae707d8 to
8d5cc60
Compare
| ev.setPortamento(true); | ||
| if (onTime == 0) | ||
| onTime++; | ||
| events->insert(std::pair<int, NPlayEvent>(onTime-1, portamentoOn)); |
There was a problem hiding this comment.
Would these events also be present if I exported a midi? That's kind of where I got stuck.
There was a problem hiding this comment.
No idea. How to check?
Anyway, are you going to continue on your PR then? The rebase itself was rather trivial.
There was a problem hiding this comment.
Actually I see you did rebase your PR, so I'm going to close mine
There was a problem hiding this comment.
Well, I didn't ;-)
Jojo-Schmitz
force-pushed
the
portamento
branch
from
8d5cc60 to
6226178
Compare
Jojo-Schmitz
force-pushed
the
portamento
branch
from
6226178 to
ed9e7dc
Compare
Jojo-Schmitz
changed the title
Jojo-Schmitz
force-pushed
the
portamento
branch
from
ed9e7dc to
21f87f7
Compare
Jojo-Schmitz
changed the title
to replace/rebase @MichaelFroelich's #4824
Resolves https://musescore.org/en/node/127341