Add single note dynamics #4541
Add single note dynamics #4541
Conversation
jthistle
force-pushed
the
single-note-dynamics
branch
2 times, most recently
from
445dc75
to
83da807
Compare
| @@ -291,11 +293,15 @@ void InstrumentTemplate::write(XmlWriter& xml) const | |||
| xml.tag("drumset", int(useDrumset)); | |||
| if (drumset) | |||
| drumset->save(xml); | |||
| foreach(const NamedEventList& a, midiActions) | |||
|
|
|||
| if (!singleNoteDynamics) // default is true | |||
There was a problem hiding this comment.
Are there so many instruments that support single-note dynamics change compared to those that don't support it that we should make it a default? Maybe it would be better to always write this value explicitly.
There was a problem hiding this comment.
Yes, there are a fair few more. It's about 190 (non-expression) vs 360 (expression). Although, I guess we could always write this explicitly. In instruments.xml, it's only specified if it's non-expressive. Also, I thought one of our design principles was to only write values when they're not the default?
There was a problem hiding this comment.
Yes, you are correct about not writing default values but this makes sense if there is some sensible default value. So I was asking about the reasons for such a choice. My personal music experience is much more connected to instruments that don't have such kind of expression so my intuition seems to be a bit incorrect here.
There was a problem hiding this comment.
So am I OK to leave it as it is, or do you still think that writing it always explicitly always is the way to go?
There was a problem hiding this comment.
Yes, I believe it is OK to keep the current choice, at least unless there are some other opinions on that.
There was a problem hiding this comment.
I think keep it in. The abstract case should be the most general (i.e. include every possible way of making a sound), and then specific instruments opt-out of features they don't need or can't use. Also, there may be cases where the user wants to override the default setting for artistic reasons even if the real instrument is not capable of producing that sound.
|
@jthistle you can attach sample audio as well to show the significance of the changes. Also, my general suggestion is to keep things simple, but complete for the first implementation. If we start developing everything at once, we won't finish anything. I would focus on the simplest implementation and limited number of options. Then, we will test it thoroughly and merge into master (and 3.0 update). And after that we can improve it step by step. |
|
@anatoly-os I'm not planning on adding any more features at this point. It's complete enough, so the only changes now will be bugfixes, mtests, and any changes needed to import properly once the soundfont's done. A mini feature freeze, in a way ;) Sample audio will be attached to the main post. |
jthistle
force-pushed
the
single-note-dynamics
branch
2 times, most recently
from
4c5ead9
to
ee99b4a
Compare
|
Hi James, |
|
No, I'm afraid you'd have to self-build. |
jthistle
force-pushed
the
single-note-dynamics
branch
3 times, most recently
from
3016ae3
to
48e80cd
Compare
There was a problem hiding this comment.
I’m just looking at readability and coding guidelines. Most of it's good. I did not mark every missed space around operators, etc. (mainly cause I’m on mobile) but give it a careful check to make. I also didn’t mark most of the debug comments cause I read you were not done yet.
jthistle
force-pushed
the
single-note-dynamics
branch
from
1b871d0
to
4b84ebf
Compare
jthistle
force-pushed
the
single-note-dynamics
branch
from
4b84ebf
to
c01863c
Compare
jthistle
force-pushed
the
single-note-dynamics
branch
2 times, most recently
from
c57a39d
to
e6355d9
Compare
|
Leaving debug comments in for now. They'll be removed along with the commit that updates mtests, when that comes. Just so I can debug rendermidi when some massive bug is inevitably found. |
jthistle
force-pushed
the
single-note-dynamics
branch
3 times, most recently
from
06707f2
to
4b26676
Compare
| $MSCORE -t ${f}.cap -o ${f}.cap-ref.mscx &> /dev/null | ||
| echo "${f}.cap.mscx -> ${f}.cap-ref.mscx" | ||
| $MSCORE -t ${f}.cap.mscx -o ${f}.cap-ref.mscx &> /dev/null | ||
| cp ../../../build.debug/mtest/capella/io/${f}.cap.mscx ${f}.cap-ref.mscx |
There was a problem hiding this comment.
This was needed to make the updateReference for Capella work.
jthistle
force-pushed
the
single-note-dynamics
branch
4 times, most recently
from
22e4460
to
24d42cb
Compare
|
Firstly make sure you're using an instrument that can actually play single note dynamics, not a piano or guitar etc. If that doesn't help, have a look in view > synthesizer > dynamics to see what the settings are. |
|
I used the violins. I also do notice something in the playback of Musescore: decrescendo hairpins seem to be respected, crescendo hairpins don't. Synthesizer settings see below:
|
|
I've gotten this working, and it's a big step forward! Thanks for your hard work, @jthistle and others. Here's what I've done to get playback:
|
|
@drowo That's worrying. Could you please send me a link to the score? Ideally, you could make an issue in the tracker and upload the score there, then I'll take a look. Thanks. |
|
@bryhoyt you shouldn't need to be manually checking that single note dynamics box in staff properties. Unless, that is, you selected a piano or were using the default score - then it's working as expected. It gets set automatically based on the staff's instrument, that checkbox is just an override. What instrument was the staff set to? |
|
Now that I've used this a teeny bit, I have loads of ideas for ways it could be extended. But I'll spare you all that now, except for one thing that might be a little easier (?!): How difficult would it be to allow overriding the default CC for any given dynamic line, say with a dropdown in the Inspector? Eg if I want to use CC1 in most cases to control volume, but my .sfz file also supports say CC2 for filter cutoff (a feature Zerberus doesn't currently support, but I very much hope it does in the near future) or some other audio parameter, then I want to be able to control that CC at times too. Ability to select multiple CCs per dynamic would be great too, but I can imagine that being more difficult and less in line with how things currently work. |
|
@jthistle I just verified that use single note dynamics is indeed enabled in staff properties. |
|
@jthistle Oh! Now I understand what that point in the feature description was referring to. Yeah, I used the default score and assigned my own soundfont (I'm in the camp that cares more about the sound, less about the notation, so I wasn't even thinking about what instrument was selected). I can see how what you've done makes sense. |
|
Further, is there a particular reason why the CC options are limited to 1, 2, 4, and 11? Why not simply allow choosing any arbitrary CC from 0-127? |
|
@jthistle Here is the opened issue containing the score: |
|
@bryhoyt well, these are the 4 most common CCs used to control expression with soundfonts. Talking to S Christian, he said he couldn't find a soundfont that used anything else. Either way, I have to manually add support for each CC, so it's not as simple as just entering a number. |
Hi! James, until now, for the output of my composition I've used a SF2 soundfont, "strings_dxs_super_orchestra.sf2" (https://www.polyphone-soundfonts.com/en/files/17-bowed-strings/314-strings-dxs-super-orchestra, by Xiaosu Du), and it works with it ok (using CC11 of course). You can hear a MP3 using that one, in https://clyp.it/ng2earbc?token=f90b874645621d83aac531e28186ff06 Also, with the default, "MuseScore_General.sf3", at least for me and using CC11 also, worked ok.
Of course, I didn't tried all the combinations (different interpolation methods, I sticked with Ease in-out and Exponential, and ended with using "CC events only" in the settings, not because the others methods didn't worked (they do), but because I just ended tweaking via the inspector each hairpin , also for test. ) Fortepianos, etc, also seems to work (you can hear them in Horns in the above composition) Attached screen samples of my settings (latest nightly, "3.1.0 unstable"). Again, much, much thanks for your work, and Christian's too (and all the developers contributing)! |
|
@alago197 I'm surprised MuseScore general works with CC11. I didn't ever check that, so that's good. The next version of MuseScore general will use CC2, and will provide more than just volume control, controlling tone and timbre as well. But, anyway, it's good that it's working for you! |
More goodies then, that's good! :-) . Yes, sure, tested the same score again with the default Musescore_general.sf3, and it's working indeed (with CC11). |
|
Hi! So, I got some time to test the nightly build and have a couple of remarks:
|
|
Right... I can't comment on sfz, I'm not sure how they work really and I haven't done any testing with them. That last point - could you elaborate? That should definitely not be happening. |
|
Check if I configured something wrong. I'm using the soundfont symphonic_sounds.sf2. |
|
I don't understand… using a SF2, no matter what I set on the dynamics tab on the synthesizer, I get exactly the same results for some instruments, even deactivating CC dynamics: dynamics working in single notes (depending on the sf2 instrument patch), but being disturbed (scratch sounds) by notes on other channels (independent of the instrument). Maybe it's a MuseScore issue on treating SF2?! For the MuseScore_General.sf3, it's working through CC11 as described above by @alago197 . Hairpin single note dynamic changes (like f > p ) aren't affected by other instruments' notes. But fp, sfp, sfpp, sfz and so on are being affected by other unrelated instruments. |
|
That's very odd. I'll check it out and see if I can reproduce. Thanks for the report. |
|
If I replace the sfp by f > p on the example I uploaded yesterday using the sf2, it works! |
|
Hmmm, check the sfp you're using isn't set to affect the whole system, it should be a setting in the inspector. |
|
It's set to "part", as all other dynamic changes. Tried all of them (part, system, stave) and got the same result. There is a "velocity change" as well, as a property of the sfp. The standard value is -47. I think this is the final offset value subtracted from the CC at the end of the note, right?! If I set to 0, there is no dynamic change and also no leap. The more I increase the absolute value, the stronger is the dynamic change and also the leap if a note somewhere else is played. I don't think it's directly related, but: even deleting just the note in the other instrument, the leap occurs. Only after deleting the entire bar the leap goes away (see the figure). Pretty weird. |
|
Thank you. I can reproduce, which is good, I guess. I'll try to fix this for you. |
|
Not for me though, for us all! ;-) Thank you a lot!! I'll keep trying to get better results using SFZ and share whatever I figure out. |
|
I have fixed this! See #4805. |
|
Did you have a chance to look at the midi file issue meanwhile? |
|
|
||
| // The lambdas are in the format: | ||
| // func(precalculated values, current tick) | ||
| // The precalculated values are for values that stay constant, involving |
There was a problem hiding this comment.
A bit late note but better late than never :)
Instead of creating a vector of precalculated arguments and passing it as an argument to a lambda function you could save the precalculated values in a lambda closure. It would be cleaner and probably a bit more efficient.
For example, for the "default" option:
preCalculated.push_back(double(exprDiff));
preCalculated.push_back(double(exprTicks));
valueFunction = [](std::vector<double>& pc, int ct) { return int(pc[0] * (double(ct) / pc[1])); };
would become
const double dExprDiff = double(exprDiff);
const double dExprTicks = double(exprTicks);
valueFunction = [dExprDiff, dExprTicks](int ct) {
return int(dExprDiff * (double(ct) / dExprTicks));
};
In C++14 this could probably be done with even nicer syntax, something like
valueFunction = [dExprDiff = double(exprDiff), dExprTicks = double(exprTicks)](int ct) {
return int(dExprDiff * (double(ct) / dExprTicks));
};
but we don't currently use it.
There was a problem hiding this comment.
Feel free to make a PR to implement that if you want :)
Is there a performance improvement because of it?
There was a problem hiding this comment.
I don't think a performance benefit would be significant, unless this code is called really often, it will just make the code a bit cleaner by avoiding an unnecessary vector and using meaningful variables names inside these formulae.
There was a problem hiding this comment.
the performance benefit is obvious: instead of allocating some arrays on the heap (which will prevent some obvious compiler optimizations, by the way) direct passing of the exprDiff and exprTicks will use stack memory and allows compiler to use some optimizations. Since changeCCBetween is used in a loop, which iterates over a lot of elements, this code will be called a hell lot of times, which means, that on each iteration the will be unnecessary allocations and deallocations of the small arrays.
Also using a reference to the local variable in the lambda closure may lead to some undesired behaviour in some cases (in this particular case it is ok, of course).
| @@ -20,6 +20,8 @@ | |||
| #include "mscore.h" | |||
| #include "part.h" | |||
| #include "score.h" | |||
| #include "synthesizer/msynthesizer.h" | |||
There was a problem hiding this comment.
This is rather dirty practice. Now Notation Model class implementation depends on the synth class. This direct dependency prevents usage of the libmscore with other synths. Could you, please, somehow get rid of this direct dependency?
There was a problem hiding this comment.
Yep, reviewing what I wrote I can see how to fix this. Thanks for pointing it out, it slipped past me.
There was a problem hiding this comment.
Hang on, that's not entirely correct, actually. We have synthesizer includes a few times in libmscore, intentionally or not, in rendermidi.cpp:50, undo.h:30, keyfinder.cpp:58. So I may not change this, then. And when you say it prevents usage with other synths, I don't think that's quite correct. The whole synthesizer directory is a kind of API wrapper for any synth - we currently support Fluid, Zerberus, and Aeolus.
There was a problem hiding this comment.
As far as I understand, the main practical problem is caused not by these include statements themselves but by adding a direct dependency on MasterSynthesizer class and other related classes. Almost all of other includes (except the one in undo.h) are made for including synthesizer/event.h which contains only common classes representing MIDI events, and using these classes seems to be OK here.
Concerning switching between expressive/non-expressive patches (which seems to be the cause of introducing this dependency), it should be enough to do it only for channels registered in MIDI mapping inside MasterScore (which is, in fact, already the case as all calls to switchExpressive are made to Channels returned by MasterScore::playbackChannel()). This implies the following:
- There is probably no need to traverse all instruments for this operation, hence, no need for having a member function inside
Instrumentclass. - Switching between patches does not really have to be implemented inside
MasterScoreas this operation seems to be soundfont-specific and is not related to MasterScore concept in general.
The only remaining question is whether it would be cleaner to implement the soundfont-specific logic of searching for an appropriate patch just inside Channel class, like it is done now, or move it to mscore or Synthesizer subsystem. @jthistle, what do you think on this, and don't I miss something in my review of the issue?
There was a problem hiding this comment.
Move it to mscore, I'd say. If our design principles say that we keep Q_OBJECTs out of libmscore, then let's keep it that way. Personally, I'd move the code that checks for the soundfont to the synthesizer, and then from there iterate over all the necessary channels. I'll see if I can get a PR done to do that.
There was a problem hiding this comment.
I have a possible solution to this issue. See mattmcclinch@b2fe733.
There was a problem hiding this comment.
See my comment on the commit
|
|
||
| // The lambdas are in the format: | ||
| // func(precalculated values, current tick) | ||
| // The precalculated values are for values that stay constant, involving |
There was a problem hiding this comment.
the performance benefit is obvious: instead of allocating some arrays on the heap (which will prevent some obvious compiler optimizations, by the way) direct passing of the exprDiff and exprTicks will use stack memory and allows compiler to use some optimizations. Since changeCCBetween is used in a loop, which iterates over a lot of elements, this code will be called a hell lot of times, which means, that on each iteration the will be unnecessary allocations and deallocations of the small arrays.
Also using a reference to the local variable in the lambda closure may lead to some undesired behaviour in some cases (in this particular case it is ok, of course).
Resolves many many feature requests.
This PR is now in theory in a mergeable state (although it should not be merged right now). Code review would be very much appreciated, please. All major changes have been made. The only changes now will be bug fixes, optimization improvements, or renaming and refactoring things.
When reviewing, click on File Filter, and deselect all extensions apart from .cpp and .h. Everything else is mtest/ui files, and doesn't need reviewing (unless of course you really want to).
You can experience this PR using S. Christian's test soundfont, downloadable here. In the mixer, set an instrument's patch to 'CC2 expression'.
Most of the significant work (i.e. not just adding properties, changing inspector files etc.) is in rendermidi.cpp. This file is hidden by default, since the diff is so large. Please make sure, if reviewing, that you do not accidently skip over it.
For reference, here is the proposal post. Note that this PR does not follow the proposal completely, and instead only uses parts of it.
Wording of any new UI elements is open to change, so feel free to suggest better names for things. I'd rather keep internal names the same as they are now, though.
This also adds a new demo, which demonstrates all of this PR's capabilities (except for different interpolation methods).
Key features
Ms::Dynamic::Speed.Optimization
The code is not fully optimized yet, but is still fairly optimized. Please point out ways of making it more so.
So, that's it really. Enjoy!
Sample score:
Audio is downloadable/viewable here.