-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] semiparametric EQ effects #1328
Conversation
Inspired by the PLAYdifferently Model 1 hardware mixer
ec524e1
to
34ef199
Compare
Thank you for this PR. Here: https://playdifferently.org/about/ Thinking about this, and looking as the 3 knob type in combination with our quick effect is somehow odd. What is the reason to combine here a filter and a single band EQ like this, while we could do this already with the parametric EQ and the normal Filter effect? I think it is because of convenience. The user should be able to select a different EQ, just by just tweaking the EQ preferences right? So in this case, the forth knob on the controller needs to be remapped to the EQ rack, away from the Quick effect rack ("The filter knob") to become a forth band of the EQ Rack, in this case a filter knob. I think we should step back and rethink the "Filter/HP/LP/BP/Gain" mapping features. In this case, it would be natural to set up the PlayDifferently mode EQ as Sweeping EQ plus a Filter in the Quick effect rack. This would allow to combine two effects freely, without dive into the skinning and mapping system. For example a sweeping bell plus a Moog Filter. My proposal is a simple GUI in the EQ preferences that is default like this Gain -> Deck Gain On top of this we can offer a set of presets:
The Play differently EQ than becomes: Gain -> Deck Gain |
@@ -82,6 +83,14 @@ class EffectManifest final { | |||
m_isMixingEQ = value; | |||
} | |||
|
|||
virtual const bool& isWaveformChangingEQ() const { | |||
return m_isWaveformChangingEQ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably more universal usable if this is a per parameter option. Did you think about that?
If we find a broad consensus to ditch this fake feature. It's also a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably more universal usable if this is a per parameter option. Did you think about that?
I don't understand. In what situation would that be helpful?
If we find a broad consensus to ditch this fake feature. It's also a solution.
Huh? This is a really nice feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so no consensus.
If we have a EQ, that for example had a button that effects the high frequencies, it can have a property on this parameter, that it changes the high gain.
In this case we have only the seeping Bell, which effects the mid range. So we may decide for a knob that visualises it in our waveform EQ fake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a EQ, that for example had a button that effects the high frequencies, it can have a property on this parameter, that it changes the high gain.
But we do not have such an EQ effect that does that without effecting the mids and lows. If we add one in the future it would not be too difficult to add that capability, but there is no need for that here.
static const double kSemiparametricMaxBoostDb = 8; | ||
static const double kSemiparametricMaxCutDb = -20; | ||
} // anonymous namespace | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an external reference about these values? Please add it as a source code comment.
Good point. I think that would make a good full name. For the shortnames they could just be "Hybrid EQ 3" and "Hybrid EQ 4".
Yes, it is for convenience for controllers that only have 3 EQ knobs, or users who do not want to repurpose a gain knob on their controller. Personally my controller only has 3 knobs. The 4 knob version of this effect is a good reason for me to get a new controller. :)
I don't. I'll repeat what @rryan posted to the mailing list about this:
How to make use of a 4 knob EQ is a device-specific issue that I think controller mappings should be responsible for. IMO your proposed solutions make too many assumptions about controllers and would make edge cases really weird or impossible to implement. What if a manufacturer made a controller designed for a 4 band Xone 92 style EQ plus gain knob and bipolar filter knob? Or a user got a controller not specifically made for DJing that had a bunch of knobs and they want all those features? I understand that you want to make using 4 knob EQs as user friendly as possible, but I think your proposed solution would backfire. I think the best way to make this user friendly would be to implement controller mapping preferences so users don't need to manually edit a JS file to change mapping behavior. Or the mapping can manipulate a different Control depending on the state of the EQ effect's parameter4_loaded Control. |
IMHO RJs and your argument applies for the old state of things. Now we have advances since some time. We have not managed over the years to improve the situation. We have also not added a four band EQ. |
How does it become worse? IMO the code duplication is trivial because most of the logic is encapsulated in the shared EngineFilterBiquad classes.
Mapping controllers without coding skills won't work for all situations. There are no magical solutions that will work 100% of the time with every possible controller. In #1329 we already have an example where your proposal would be strange and unintuitive. The mapping has an option to use 4 knobs either for effects metaknobs + dry/wet, deck gain + 3 EQs, or 3 EQs + QuickEffect. With your proposal, if the user wanted to use a 4 knob EQ effect, they'd have to select the 3 EQ + QuickEffect option. How is that more intuitive than making an option for a 4 knob EQ? Device-specific logic belongs in controller mappings.
For the most part, yes, there is a standard 4 or 5 knob layout per deck for DJ controllers. In general we should design Mixxx to match common patterns on DJ controller layouts. But we must not make Mixxx incapable of being used with devices that do not match the common patterns. What if a user wants to map a Novation LaunchControl, Livid Code2, Knobtronix Knobbee 32, or Evolution UC16 with each row for a deck and wants a gain knob, a 4 band Xone 92 type EQ, and a QuickEffect knob? People want to use all kinds of unusual devices with Mixxx and we should not put artificial limits on that. I think Mixxx Controls should only do one thing unambiguously and not perform any magic a developer thinks would be convenient at the time. Do you recall when I proposed the loopauto_toggle button in #1187 because it worked well for my use case but then several other people said it was weird and got in the way of their use case? I think this situation is similar. The QuickEffect superknob should be the QuickEffect superknob, no exceptions. |
Let's move this discussion to a mode finding a solution for to make EQ / Quick effect changing easy. I will try to summarize the current state:
Requirements:
Is this correct so far? |
IMO you are making an issue where there is none. There is no need to make this work without changing controller mappings. Mixxx has implemented many new features that require updating controller mappings to make use of. I don't understand why this should be an exception. We should be careful not to break backwards compatibility for controller mappings, but I don't think there's any requirement to make old mappings forward compatible with new features. IMO the only prerequisite for merging this is merging #1315. A magic C++ hack to switch a knob for the 4th EQ knob is a bad idea for many reasons. If the magic happened automatically when loading a 4 knob EQ, that would exclude the possibility of using gain, 4 knob EQs, and QuickEffects with controllers that have more than 5 knobs per deck. It wouldn't even work for controllers with normal DJ layouts because Mixxx couldn't predict whether the user would want to replace the gain or the QuickEffect knob. Or consider a user with a 4 knob controller who wants to replace the gain knob on their controller, but still wants to change the gain knob with their mouse. It would also be confusing for users without controllers for the QuickEffect or gain knob to disappear. If the magic switching was triggered by a preference option, it would be confusing for users without controllers who would have no use for it. The preference option should only affect controllers and therefore belongs with the controller mapping, which is in a position to handle it appropriately for that specific controller.
Huh? |
The difference here is, that after merging these, a changed EQ setting in preferences requires to change the controller and skin mapping. This would be new to Mixxx.
That is not a requirement to me. It would be a great solution that requires an updated controller and skin mapping before release, but does not require to change skin or controller mapping after changing EQ preferences.
While tool tips are nice in general, I do not understand how this changes the problem here.
Yes, right that would be a bad idea. I think we need a solution where the default mapping is "prepared" for the situation of changing EQs and offers a good default for each EQ config. This should not prevent to change this to an other solution via remapping, If the user did not like the solution.
Yes, right. We can discuss how to issue that. One way could be that this is in the responds of the default mapping. For a four knob Gain / Hi / Mid / Low controller, It is probably the only reasonable solution to replace gain. If the user does not like this, he can either not use a four band EQ or change the mapping as he likes. This is not different from the situation we would have without doing anything. We may also invent some kind of mapping options just like skin options we have already, which allows let the user decide it without remapping.
I think it is easy to explain in a kind of warning. It is confusing in the same way to have only 3 knobs of a four band EQ accessible from the controller. I think such user should simply not pick a four band EQ and should be warned if he does it anyway.
Good point. I think we need a mapping metadata, that informs Mixxx for the type of controller mapping which is used.
Yes, that would be a perfect solution. We only need to find a way how we interface this controller preference option with the EQ preference option. The solution for this should also consider more than one controller connected at a time.
You can look at this PR as a 2 Knob EQ + the quick effect moved to the third EQ knob. I am going to like this idea very much. And I will probably remap my controller like this and check how it feels. This would allow me to make use of any quick effect on the third EQ knob. |
Here is the issue. We would need to implement a whole new system for this mapping metadata, but that would be redundant because mapping scripts can already check the state of 3 EQ knob controller: do nothing |
This is not quite the case. The HPF + LPF on the PLAYdifferently Model1 are designed to sound transparent, not resonant like the Filter effect. The Filter effect can have its resonance reduced, but this parameter is not exposed when loading it as a QuickEffect.
I think this is a very niche use case and should be left to custom controller mapping. |
Yes, that would be a good start. Is looking to parameter4_loaded enough? |
Can we set up a similar rule for a 2 knobs EQ? The 4 knob play differently type does not fit very well to these concepts. Because it requires two knobs of the quick effect rack. |
Do we have a reference how the play differently filters are designed? Our filter has a flat curve by default q = 0.7 with no overshot = resonance. |
My biggest concern with implementing magic in C++ is that it could not work for all situations without some sort of mapping metadata system. If it is left to controller mappings to implement, they can handle it appropriately for that specific controller. There is still the question of the user experience when loading a 4 knob EQ effect. I am not sure if it is better to have mappings handle this automatically or require the user to change a mapping option. If it is automatic, the user may be confused why their gain or filter knob on the controller has become something else. However, I am not sure if this is much of an issue because the user did explicitly choose to load a 4 knob EQ, so reasonably something has to change. If the mapping does not handle it automatically, the user might not think to go to the controller preference page to look for an option. That is only hypothetical right now though because we have no way to expose controller preferences to the GUI. I do not think editing the JS file would be obvious to a user unfamiliar with how Mixxx mappings work. Even if this is documented on the controller's wiki page and the user is aware that they may find relevant information on the wiki, they might not think to check the controller's wiki page in this situation. So I am leaning towards controller mappings handling it automatically. |
I am still not clear what the use case for a 2 knob EQ is. 1 band semiparametric EQ + ??? What else makes sense other than a low resonance bipolar filter knob? |
In the manual on PDF page 8 (labeled page 6), "Contour and Sculpt", there are frequency response graphs. I have not yet tried adjusting the parameters for these effects and watching a spectrum analyzer to approximate those graphs. Do you have any better suggestions for how to go about that? I have briefly tried running Mixxx with JACK, sending Mixxx's output to CalfRack, and using the Calf Analyzer. |
There is a tool to simulate the filter parameter of a fidlib filter: https://uazu.net/fiview/ |
Yes, this seems to be the best solution. The default mapping should provide default solutions for all offered EQ settings. If this does not suite the personal need of a user he can change the mapping as needed. This should be possible by the point and click mapping GUI.
What else might be expected? Not mapping the fourth knob is somehow a broken state. So using gain seems to be the most natural solution. But we should explain it in the EQ gui. |
Ok, how about adding these facilities:
|
What advantage does that have over the existing ability to check the state of parameter4_loaded? All it seems to do is make more work for mapping developers. |
This PR is marked as stale because it has been open 90 days with no activity. |
Superseded by #3582 |
Inspired by the PLAYdifferently Model 1 hardware mixer, these combine a semiparametric EQ with selectable frequency and boost/cut with a HPF & LPF. The Q is fixed at a wide setting. With the 3 knob version, the HPF & LPF are combined on one parameter knob like the metaknob of the Filter effect. With the 4 knob version, the HPF & LPF are controlled independently on different parameter knobs, which could be used with 3 EQ knobs + filter knob on a controller, or alternatively 3 EQ knobs + gain knob.
I still have to play around with the parameter ranges. For now they are meant to imitate the PLAYdifferently Model 1.