Skip to content

Removed valueHint from effect parameter and turn all values form QVarian...#356

Merged
daschuer merged 4 commits into
mixxxdj:masterfrom
daschuer:rm_value_hint
Nov 10, 2014
Merged

Removed valueHint from effect parameter and turn all values form QVarian...#356
daschuer merged 4 commits into
mixxxdj:masterfrom
daschuer:rm_value_hint

Conversation

@daschuer
Copy link
Copy Markdown
Member

@daschuer daschuer commented Oct 6, 2014

...t into double

While testing the Goaslicer #337
it turns out, that all other value hints, other than VALUE_FLOAT did not work.

This is due to the fact, that a knob gets out of sync if it is turned to other values than full integer in case of an integer value.

If an effect needs integer or boolean parameter, the can expressed as a double as well. The quantization and rounding should be done at the effect, if required.

The feature of the QVariant to be invalid was not working, since it defaults to a double value range of 0.0 ... 1.0 anyway. These are the new defaults if no value is explicit set in the manifest.

This pull request cleans up the code and the effect interface a bit. The performance improvement should be only marginal.

@rryan
Copy link
Copy Markdown
Member

rryan commented Oct 6, 2014

If an effect needs integer or boolean parameter, the can expressed as a double as well. The quantization and rounding should be done at the effect, if required.

I think having actual types to parameters is important and helps avoid bugs since the programmer is encouraged to handle each case separately.

If an effect needs integer or boolean parameter, the can expressed as a double as well. The quantization and rounding should be done at the effect, if required.

This will result in duplicated code in the best case, and inconsistency bugs in the worst case.

The feature of the QVariant to be invalid was not working, since it defaults to a double value range of 0.0 ... 1.0 anyway. These are the new defaults if no value is explicit set in the manifest.

I don't think I follow.. could you clarify?

@rryan
Copy link
Copy Markdown
Member

rryan commented Oct 6, 2014

If an effect needs integer or boolean parameter, the can expressed as a double as well. The quantization and rounding should be done at the effect, if required.

In the event of an integer parameter, you want the widget to reflect the quantization so the user understands the knob has individual gradations. This isn't possible if the quantization happens inside the black box of the effect.

@rryan
Copy link
Copy Markdown
Member

rryan commented Oct 6, 2014

If something isn't currently working with the value hints let's fix that. No reason to throw the baby out with the bathwater. Also there is future work to be done on connecting effect parameters to the GUI. Right now everything is geared towards float values. Button and integer support are not there yet.

@ywwg
Copy link
Copy Markdown
Member

ywwg commented Oct 6, 2014

This looks like something that should have just been a bug report first, rather than jumping ahead and trying to rewrite a lot of stuff. Can we start over with a bug report and then discuss what the best solution might be?

@daschuer
Copy link
Copy Markdown
Member Author

daschuer commented Oct 7, 2014

@daschuer
Copy link
Copy Markdown
Member Author

daschuer commented Oct 7, 2014

Please have a look here:
http://www.mixxx.org/wiki/doku.php/effectparameter_scaling

@daschuer
Copy link
Copy Markdown
Member Author

daschuer commented Oct 7, 2014

The feature of the QVariant to be invalid was not working, since it defaults to a double value range of 0.0 ... 1.0 anyway. These are the new defaults if no value is explicit set in the manifest.

I don't think I follow.. could you clarify?

It is about the remove of "hasMinimum()" the code always has a minimum and it is required to work correct.
Thats why I have removed this and similar part of the code.

@daschuer
Copy link
Copy Markdown
Member Author

daschuer commented Oct 7, 2014

If an effect needs integer or boolean parameter, the can expressed as a double as well. The quantization and rounding should be done at the effect, if required.

In the event of an integer parameter, you want the widget to reflect the quantization so the user understands the knob has individual gradations. This isn't possible if the quantization happens inside the black box of the effect.

This is a valid request. But I cannot see how we can achieve this in an optimum way on a Rotary widget with our limited Skin Space. Because of that, Nicu has introduced the Enum Button including the fly-out menu.

@daschuer
Copy link
Copy Markdown
Member Author

daschuer commented Oct 7, 2014

If something isn't currently working with the value hints let's fix that. No reason to throw the baby out with the bathwater. Also there is future work to be done on connecting effect parameters to the GUI. Right now everything is geared towards float values. Button and integer support are not there yet.

I have some ideas how the support should work. The current solution does not fit the requirements well.
I am very please with Nico solution in his LV2 Branch. But we need some more options to set up the desired scale. Currently the picked scale (Behavior) feels somehow random.

But I cant come up with complete solution for this on my own. So I need your support.
Do you have time to start such a discussion now?

Conflicts:
	src/effects/effectmanifestparameter.h
	src/effects/effectparameter.cpp
	src/effects/effectparameter.h
	src/effects/effectparameterslot.cpp
	src/effects/effectparameterslot.h
	src/effects/native/butterworth8eqeffect.cpp
@daschuer daschuer mentioned this pull request Oct 12, 2014
@daschuer daschuer merged commit a4121f1 into mixxxdj:master Nov 10, 2014
@daschuer daschuer deleted the rm_value_hint branch June 2, 2015 06:09
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