corrected wet/dry knob range #3422
Conversation
|
This also breaks projects that rely on this behaviour. |
|
@grejppi Yes, this will make breaking changes, but it has to be done. Right now, this knob is adding unintentional non-linear phase and gain staging to the mixer. Chances are the people using this behavior don't know why it is behaving that way (and they shouldn't, because it's a bug). I cannot stress how bad this is. |
|
I would like to say that I often use this feature. It is easier than finding the right W/D mix and then adding an Amplifier to just turn the knob under 1. Also, I am one of the more experienced LMMS producers and I have no clue what "unintentional non-linear phase and gain staging" means. Please explain in laymans' terms. |
|
I don't think it has to be done. This thing (be it a feature or a bug) has been there for 10 years, and people use it because can sound good with certain effects. I cannot see how this is bad. |
|
@Jousboxx of course! It's not as scary as it sounds. Basically, to handle wet/dry signal mixing, LMMS gives plugin developers two separate values: a wet value and a dry value. In an ideal world, these values would be between 0-1, and when summed together they equal 1. To get the output of your signal with the right wet/dry mix, you'd use the following equation: signal_output = wet_signal * wet_amount + dry_signal * dry_amount When dry_amount = wetdry_knob_value; That is to say, when wet/dry knob is all the way up, it's a dry signal. All the way down, it's an all wet signal. You can flip this the other way around, but ultimately is the same behavior. But this all depends on the knob range being in the range 0 to 1. But it's not. The main issue isn't really that the wet/dry knob is in the wrong range. Plugin developers could take that value directly and scale it to fit their needs. However, the fact that LMMS is providing separate wet/dry variables for developers creates an issue. Why? Because the wet/dry values are uneven. The Dry value has the range 0 to 2, and the wet value has a range -1 to 1. This means that when the knob is all the way dry, you actually double the signal because you multiply it by two, and when the knob is set to be all wet, this multiplies the signal by -1. What this does is flip the phase. You may recall from a physics class that when you sum two sine waves whose phases are flipped, you cancel out that sine wave. If you are unknowingly flipping the phase of your signal, you are going to have a bad time mixing your track. Typically, this sort of thing happens when you have antinodes and nodes in the room you are mixing, but in LMMS, we apparently emulate this feature ;) The issue becomes more complex because depending on where the knob is, you are changing the amount of gain and phase modification you are adding. It doesn't behave the way you expect it to because of the different wet/dry values. I would define this behavior non-linear behavior. |
See my comment above. I would agree that this knob has some funkiness to it, but it does not exhibit behavior of a wet/dry knob. The solution is to either make it behave like a wet/dry knob in my one line fix, or change the name of this knob.
Some quirky bugs are worth holding onto. This is not one of those bugs. Just because it hasn't been noticed in ten years doesn't make it any less wrong.
Why is it bad? Because this knob doesn't even behave like a wet/dry knob in any way right now. Even when you don't touch it, it's screwing up the mix of your track. LMMS should have a fighting chance to sound as professional as it looks. |
|
If this becomes a huge backwards compatiblity problem, we split the baby and deprecate it. This can be done by using a visual warning mechanism when using range Note, I used to use a bug in Kicker to make drops and when Vesa fixed it, I kept my mouth shut, knowing it was a bug. The problem with this bug is it wasn't clearly obvious, so let's deprecate it to be progressive while also being respectful to the musicians. |
It's actually the opposite. The knob value is the wet value, and the dry value is calculated like that.
No, you don't. When the knob is all dry you get
Isn't that the point of effects, to screw with the sound in different ways? |
Put some printfs in the effect loop, you'll clearly see the ranges are as I stated above.
Sane behavior? Are you kidding me? Why make a function call to
This isn't an effect, though. This just adds more complexity to what really should be simple arithmetic. |
|
To be even clearer:
Or use
|
Oh... I've been calling the middle position dry all this time, as it's the place where wet goes to 0.
The operation can be as simple as one CPU instruction. so I wouldn't worry that much about complexity. |
|
Okay. It behaves like a normal wet/dry knob... if you use half of the knob. I was wrong in that it isn't changing the gain staging when it's all dry, so things are fine when you don't touch it. Yet another very strange bug-turned-quirk in the LMMS audio engine. sigh. |
|
Wet/dry formula is correct. We just abuse the formula to achieve some effects; I do not remember whether this was intentional.
|
|
Just a comment about the design of this choice: |
|
I had the liberty of discussing this will @PaulBatchelor last night. For historical reasons, the design should include the backwards-compat option with a deprecation warning. The place for this should be on each effect to ensure new effects are created without this toggled on by default. The toggle would then be completely removed for the next major release (e.g. 1.3 or 2.0 whatever we call it). We will do our best to detect controllers and project setting which would toggle this on by examining the range used. This should be possible through an upgrade routine. After the next major release, those in strong opposition of the toggle removal can build from source with the option turned back on. It's very difficult to quietly revert a 10-year mistake but we must progress forward. I think this offers a compromise between backwards compatibility and future efforts. Here's a mockup of how we may present this change to the users.
|
|
This looks like a good plan to move forward. |
Why do you call it a mistake? There are users that were well aware of this behavior. You can perfectly change the knob to [0, 1] without dropping the feature. |
I call it a mistake because Paul was updating the display names with this update, specifically adding translation support as well as moving a few coordinates. |
|
What's the advantage of a legacy range toggle over a 0-1 range with an "invert effect" toggle? I feel like the latter would be better. |
I'll quote @PaulBatchelor:
So it's not really an "invert effect" setting. It's more like a "send me junk" setting. 😆 |
It is true that the ChangeLog does not mention this, but the mistake certainly turned into feature. Let us call it a "mistake".
As @tresf reminds us, these are different effects. Current behavior is sort of a chaotic sound effect. The "mistake" should be available in the context menu and enhanced by increasing ranges. |
|
I think this is something that we want adressed for 1.3, so I'm keeping this PR related to |
|
Still not merged after 6 months... |
|
@PaulBatchelor I just read your explanation of the problem. I'm not sure why I wasn't notified when you tagged me, or maybe I just missed the email. I understand what you're saying, but I have not observed that behavior. I use the wet/dry function often for mixing (including setting the knob to negative values) and I am pretty certain I would have noticed if it increased the gain and/or flipped the phase of my sound. Just to be sure, I set up a simple MMPZ that has a sine wave going through an amplifier effect that does nothing (it's set at 100% volume). If what you were saying was a problem, I would expect the following behavior: However, it sounds the same regardless of where I put the wet/dry knob. This is what I expect as it is working properly and the effect produces no change to the signal. You probably encountered a bug that occurs based on some other thing going on in LMMS, not just in general in all situations. Here is a link to the MMPZ so you can see what I did and test it for yourself: https://drive.google.com/file/d/0B8dZ3IVOKKA6TzlZMFNfSDg2Nk0/view?usp=sharing |
|
After thinking about it some more, I realize that the hypothetical behavior I listed is not entirely correct. But the actual behavior seems to be bug-free, and not anything like what you explained in your comment. |
Right. As I read it, this PR is about how the code should be, not how it's necessarily malfunctioning.
@gi0e5b06 can you please be more insightful with this comment? I think backwards compatibility concerns here are valid and the PR should be regression tested. Unless someone's volunteering to do that, the PR is stuck. This is unfortunate but not uncommon. We do need more help with these types of things and the OP is on semi-permanent leave leaving the responsibility of the merge to those that didn't write it. If you're comfortable with owning this, let's discuss on Discord about it in |
|
I'm the original author of this PR. Does anybody actually want this still?
If nobody wants it, I'd rather this get closed than have it remain in PR
purgatory.
…On Thu, Sep 7, 2017 at 10:44 AM, Jousboxx ***@***.***> wrote:
After thinking about it some more, I realize that the hypothetical
behavior I listed is not entirely correct. But the actual behavior seems to
be bug-free, and not anything like what you explained in your comment.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3422 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHwyfa0Vqy71B6SLQ4rEz4RwDFAmSr7Uks5sgCSJgaJpZM4Mbnlb>
.
|
|
@PaulBatchelor Yes. @tresf There is no backward compatibility issues here. Merge it into master but not into stable-1.2. |
|
LMMS is full of quirks, and this is just one of them. In the interest of preserving even some backwards compatibility am very much against changing this behavior in a minor release. That said, I would find a 0..1 range more ideal as well, but that is better left for a major breaking release like 2.0 sometime far in the future. |
|
@grejppi OK. So in 2042, this bug may be fixed. Great... I was suggesting to merge it into master (1.3), which is planned for 2021 at the earliest (that itself is already a joke) but waiting a few decades seems a safer way to go... |
|
@gi0e5b06, insulting the project and its maintainers won't be tolerated. Consider this your warning. |
|
There was no insult in what I wrote. The history of this project shows that there are on average three years between two minor releases. Back to the topic: every one seems to agree this bug should be fixed. Simple question: when will that happen? |
|
Only two people call it a bug though 😌 |
This is pushing and off topic. Contributors can't spend their time poking at a clearly back-logged project about how long a bug is going to be open. Crowdsourced projects don't survive on finger pointing or insulting the shortcomings of the admins. This is a direct insult to the admins that wake up and work on the project and the lack of sympathy in the responses following the warning are a sign that the agenda is not to test, code or debug. The next comment that's regressive will be removed. Further regressive comments will be reported and dealt the with through the hosting service. |
|
To get this merged, "it can go to master" is simply the wrong answer. We should weigh the impact of what changing a model's default value has on backwards compatibility and what our options are to mitigate more issues caused by it. This is mentioned in the comments above. |
|
To move this PR forward, we need to rename the attribute or add a new e.g. - m_wetDryModel.loadSettings( _this, "wet" );
+ m_wetDryModel.loadSettings( _this, "wetdry" );But we need to be able to fall-back onto the old m_useDeprecatedWetDry = false; // true = use old "wet" range (-1, +1)m_wetDryModel.loadSettings( _this, "wetdry" ); // since 1.3.0
// "wetdry" was previously "wet", but had invalid values for many, many years
// Let's preserve them for projects which may still require them
list = elementsByTagName( "effect" );
for( int i = 0; !list.item( i ).isNull(); ++i )
{
QDomElement el = list.item( i ).toElement();
// TODO: Can we just ask the FloatModel if it was null/non-null
// That would avoid redundant DOM parsing
if( el.attribute( "wet" ) != null ) {
delete m_wetDryModel;
m_wetDryModel = new FloatModel( 1.0f, -1.0f, 1.0f, 0.01f, this, tr( "Wet/Dry mix" ) );
m_wetDryModel.loadSettings( _this, "wet" );
m_useDeprecatedWetDry = true;
}
}And then on save... if (useDeprecatedWetDry) {
m_wetDryModel.saveSettings( _doc, _this, "wet" );
} else {
m_wetDryModel.saveSettings( _doc, _this, "wetdry" );
}It's a bit gross, but it would respect the old behavior while deprecating it and forcing the new; living happily together. For those wanting the old knob range, you'd have to edit new projects by hand. |
|
FYI! I think this is probably what they tried to implement although the response is wrong as described here. Polarizing mixer (attenuverter). |
Fixes #3261.
After examining git commit history, I found a bug introduced nearly 10 years ago! While making some sweeping changes in the LMMS codebase, someone accidentally changed the range of the knob, which has given us the strange behavior we see today. In the git diff I linked to above, you can see the correct ranges being overridden with the incorrect ranges with no explanation.