Skip to content
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

Add "Continuous" FloatModels (stepsize = 0) #7623

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

regulus79
Copy link
Contributor

@regulus79 regulus79 commented Dec 17, 2024

Description

Originally, this PR gave a finer stepsize to TripleOscillator's volume knobs. However, this pr has changed and expanded to instead provide functionality for continuous knobs/models (no step size).

To make a mondel continuous, simply set it's stepsize to 0.0f. Normally this would result in division by zero, but this PR adds some new if statements to check if the stepsize is 0, and if so, not to round.

Changes

  • A new function was added to AutomatableModel.h, AutomatableModel::addValue(float amount). This is an alternative to AutomatableModel::incValue(int steps), which does not work when the stepsize is 0.
  • A check for the stepsize being 0 was added to FloatModel::getRoundedValue(), FloatModel::getDigitCount(), FloatModelEditorBase::wheelEvent(), FloatModelEditorBase::setPosition(). For getDigitCount(), I have it always return 10 digits, but this is arbitrary.
  • TripleOscillator's oscillator volume models stepsizes were set to 0.0f. So far, this is the only model which has been changed to be continuous.
  • (slightly unrelated) the numberOfStepsForFullSweep when scrolling on a knob and holding alt was changed from 2000 to 10000 to provide finer control.

Might address #5626

Old PR description This changes the step size for the volume models in TripleOscillator to be 0.001f instead of 1.0f. This makes FM more usable, as previously only setting the lower osc to 1%, 2%, and maybe 3% volume would give useful tonal results, but now many more values which can be chosen.

@sakertooth
Copy link
Contributor

I think this would be a benefit to a lot of our knobs and sliders too? We could probably generalize the benefit to more than just TripleOscillator.

@regulus79
Copy link
Contributor Author

Good idea. I have come up with a solution where if the step size of a model is 0.0f, then it will act as a continuous knob. Currently only TripleOscillator's volume knob implements this feature, but I can add more. (If it is likely that a significant number of the FloatModels in LMMS will have to be updated to be continuous, it may be more suited for a separate pr. Or not, I'm not sure.)

@regulus79 regulus79 changed the title Allow fine control of TripleOscillator oscillator volumes Add "Continuous" FloatModels (stepsize = 0) Dec 18, 2024
@sakertooth
Copy link
Contributor

Wait, it seems like we already have fine control for our knobs? Just hold shift and drag the knob and the changes are more precise. The only problem is that this doesn't apply for our sliders, which could be useful in places like the mixer channel maybe.

@regulus79
Copy link
Contributor Author

Wait, it seems like we already have fine control for our knobs? Just hold shift and drag the knob and the changes are more precise.

Even when holding shift, the adjustments are still locked to the stepsize. I cannot get more than 0.1% precision when turning a track volume knob, no matter if I hold shift or not.

The only problem is that this doesn't apply for our sliders, which could be useful in places like the mixer channel maybe.

That's a good point, maybe I can try to look into that (although it's not directly related to this pr).

@sakertooth
Copy link
Contributor

sakertooth commented Dec 18, 2024

Even when holding shift, the adjustments are still locked to the stepsize. I cannot get more than 0.1% precision when turning a track volume knob, no matter if I hold shift or not.

There were efforts in #6770 that tried to address this. I just want to make sure we aren't trampling over already finished work.

@regulus79
Copy link
Contributor Author

regulus79 commented Dec 18, 2024

The changes in #6770 do make it so that mouse modifier keys do provide finer control when scrolling on knobs, but they do not change the model by less than one step. It appears that this was an intentional decision, as the code checks if the scroll amount is less than a step, and if so, rounds up to 1 step.

// Compute the number of steps but make sure that we always do at least one step
const float stepMult = std::max(range / numberOfStepsForFullSweep / step, 1.f);
const int inc = direction * stepMult;
model()->incValue(inc);

@sakertooth
Copy link
Contributor

The changes in #6770 do make it so that mouse modifier keys do provide finer control when scrolling on knobs, but they do not change the model by less than one step. It appears that this was an intentional decision, as the code checks if the scroll amount is less than a step, and if so, rounds up to 1 step.

I'm somewhat confused. Maybe @michaelgregorius could pitch in here, but wouldn't the simple solution be then to stop rounding up to 1 step, and use AutomatableModel::setValue instead of AutomatableModel::incValue, or simply make AutomatableModel::incValue take a float number of steps? However, if @michaelgregorius rounded up to 1 step, it was probably for a specific reason, so hopefully they could share why they did that. Because the way I see it currently, that rounding up to 1 step somewhat ruins the point as it changes how "fine" the control is really meant to be.

@michaelgregorius
Copy link
Contributor

I only preserved the previous behavior in #6770. If you look at the diff then you will find that the code previously looked as follows:

const float stepMult = model()->range() / 2000 / model()->step<float>();
const int inc = ((we->angleDelta().y() > 0 ) ? 1 : -1) * ((stepMult < 1 ) ? 1 : stepMult);
model()->incValue( inc );

As you can see the check for stepMult < 1 previously ensured that at least a step of 1 is applied.

@regulus79
Copy link
Contributor Author

regulus79 commented Jan 20, 2025

As far as I know, this PR is ready to merge (some further testing would be good though). I can make another PR to change all the 0.0001f stepsize models in LMMS to be perfectly continuous after this is merged.

Edit: actually wait no, I still want to get more comments on the code. Right now some of the stuff in there is a bit arbitrary, like returning 10 digits for continuous models in FloatModel::getDigitCount().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants