It seems to me that the AudioProcessorValueTreeState::ComboBoxAttachment requires the user to observe some strange and undocumented restrictions on the itemID.
value range must begin with 0
itemID must be Item index + 1
The setValue method uses the item index to select the item.
I would propose to make the implementation more flexible by always using the item ID (just replace setSelectedItemIndex with setSelectedId). Or if that should not be an option at least include the existing restrictions with the documentation. The proposed change will allow to use arbitrary parameter value ranges with arbitrary step size. the only remaining restrictions would be:
only integer values allowed
no zero allowed
the latter can be even worked around using appropriate Value<>itemID conversion like this:
Thanks for the link. Seems I am not the only one to find that odd.
I have now modified some of the AudioProcessorValueTreeState dependent classes and added my own LabelListener class to solve these problems for me.
In your case you might be able to use the fft window size as itemID to maintain the semantics of the implementation and to sort the combobox entries just as you find useful since the item index does not get used to retrieve the item value.
I have even gone a step further and implemented a populate routine that fills the combobox with the appropriate text values provided by the parameter->getText function. This allows the combobox to be populated as soon as it gets connected with the parameter through a comboBoxAttachment.
I am going to upload the solution to github as soon as I have an example jucer project ready.
I share your observation, that the implementation is inconsistent and error prone. However, I would draw slightly different conclusions:
It is a bug to rely on the ID when updating the parameter in comboBoxChanged but relying on the index when updating the comboBox in setValue. The most flexible for the user is to use the ID, but in any case, it needs to use the same information in both directions.
Using INT_MAX as base for the conversion will not work for numerical reasons. The INT uses the same number of bits like a float (IIRC). Taking conversion errors into account, it cannot map reliably every state from int to float and back. But IMHO it is not needed anyway because of the next point:
The callback setValue uses setUnnormalisedValue, hence it takes the range of the parameter into account. So the user is free to create itās own mapping, allowing more states than the combobox displays. The drawback is, it is undefined, if a number in the host is selected, that doesnāt exist as ID in the combobox (which means, the combobox is not updated).
TL;DR:
agreeing with @martinrobinson-2 and @soundbytes1, ComboboxAttachment has to use consistently either the ID of combobox items or the index.
Argument for ID: allows the developer to change the order of items retaining the selections in existing states
Argument for index: avoids numbers that canāt be mapped on an item in the combobox.
How to fix this is up to @jules or @t0m, who was the last to work on that according to git.
I am using INT_MAX simply as a substitute for the 0 value so that it can be used in the context of the ComboBox. This allows the user to include 0 as a parameter value, which otherwise would not be possible.
I agree with the above. I just lost some time tracking down the ābugā.
can you at least add a couple of documented assertions in ComboBoxAttachement?
How about normalising the float parameter value using the range, then using that to determine the selected index?
void setValue (float newValue) override
{
const ScopedLock selfCallbackLock (selfCallbackMutex);
if (auto* p = state.getParameter (paramID))
{
auto normValue = state.getParameterRange (paramID)
.convertTo0to1 (newValue);
auto index = roundToInt (normValue * (combo.getNumItems() - 1));
if (index != combo.getSelectedItemIndex())
{
ScopedValueSetter<bool> svs (ignoreCallbacks, true);
combo.setSelectedItemIndex (index, sendNotificationSync);
}
}
}
void comboBoxChanged (ComboBox*) override
{
const ScopedLock selfCallbackLock (selfCallbackMutex);
if (! ignoreCallbacks)
{
if (auto* p = state.getParameter (paramID))
{
auto newValue = (float) combo.getSelectedItemIndex() / (combo.getNumItems() - 1);
jassert (isPositiveAndNotGreaterThan (newValue, 1.0f));
if (p->getValue() != newValue)
{
beginParameterChange();
p->setValueNotifyingHost (newValue);
endParameterChange();
}
}
}
}
Iāve not given this much testing yet, but this should remove any dependency on IDs. The only assumption here is that your combo box labels are distributed evenly across your (arbitrary) range.
That sounds good to me.
However, another option would be to allow ranges like NormalisableRange<float> (0, 16, 1), but allowing to connect to a Combobox with less items. That way the developer could reserve items for later use (since you cannot hide items in a ComboBox).
On the other hand, that could be the argument to rely on itemIDs.
The only thing I know is, it should be consistent to use either index or IDā¦
