free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.76k stars 97 forks source link

Params extension #260

Closed sadko4u closed 1 year ago

sadko4u commented 1 year ago

Hello! I'm reading the in-place documentation of the params extension and am not satisfied by the description of these methods:

   // Formats the display text for the given parameter value.
   // The host should always format the parameter value to text using this function
   // before displaying it to the user.
   // [main-thread]
   bool(CLAP_ABI *value_to_text)(
      const clap_plugin_t *plugin, clap_id param_id, double value, char *display, uint32_t size);

   // Converts the display text to a parameter value.
   // [main-thread]
   bool(CLAP_ABI *text_to_value)(const clap_plugin_t *plugin,
                                 clap_id              param_id,
                                 const char          *display,
                                 double              *value);

There are several questins that are not clarified in the documentation:

  1. Are the both methods mandatory for the interface?
  2. What does display mean for the value_to_text function? a. The target buffer to store the value? b. What is the guaranteed minimum size of the display buffer should be provided by the host? c. What should happen if there is not enough space for storing the formatted value? How the host will find out that there is no enough space among other errors like unexisting parameter index?
  3. What purpose text_to_value is designed for while we have the get_value method? a. For parsing user input in the generic UI/somewhere else? b. What if the user input is invalid or not possible to parse?
baconpaul commented 1 year ago
  1. All methods in an extension are mandatory
  2. A. Yes; b. It should be at least as big as the size argument; c. Return false
  3. A. Exactly. In surge for instance you can type in a frequency as 440hz or a4 and bitwig lets you do that in its editor. B. Return false and leave the value unchanged is what I would expect but I agree we could document this
baconpaul commented 1 year ago

Oh and practically “256 chars is more than big enough and 64 is probably fine”.

baconpaul commented 1 year ago

Oh and in being mandatory; they are mandatory but “return false” is a working implementation which hosts will work around (most likely by assuming floats and atof semantic but that’s not required)

sadko4u commented 1 year ago

Yes, the main problem is that there is a lack in description of methods. It would be nice if the description contained the detailed answers to my questions.

baconpaul commented 1 year ago

Yup. I think for each new dev who uses clap in earnest we add 40 lines of comments to the headers! I think these comments could be a bit more robust indeed

Trinitou commented 1 year ago

Maybe if the comments had a more fixed style and every argument has to have a short description of what it does. Not sure about whether every argument also should have an [in] / [out] tag as the type/const/non-const actually implies that. But personally I really like that currently the comments are light-weight and only the necessary stuff is expained explicitly. And if something is not clear enough and somebody asks for it, it should be clarified nevertheless.

SamWindell commented 1 year ago

I definitely think we can make some improvements to the docs/names that won't add too much bloat. I made an issue here about it too #250

I'll make a PR with what I think would be good changes.

sadko4u commented 1 year ago

One more question. Here's the description of the clap_param_info:

typedef struct clap_param_info {
   // stable parameter identifier, it must never change.
   clap_id id;

What does it must never change mean? Should it never change while the instance is active, or should it not change even for further versions of plugin, where new parameters can be added? For example, lv2, vst 2.x and ladspa provide symbolic parameter/port identifiers which allow to distinguish the right parameter/port position if the list or the order of parameters has changed. If I change the plugin, is it correct that this identifier can change (if it will indicate parameter's index that has changed) or should I keep it the same to the previous version of plugin?

tim-janik commented 1 year ago

One more question. Here's the description of the clap_param_info:

typedef struct clap_param_info {
   // stable parameter identifier, it must never change.
   clap_id id;

What does it must never change mean? Should it never change while the instance is active, or should it not change even for further versions of plugin, where new parameters can be added? For example, lv2, vst 2.x and ladspa provide symbolic parameter/port identifiers which allow to distinguish the right parameter/port position if the list or the order of parameters has changed. If I change the plugin, is it correct that this identifier can change (if it will indicate parameter's index that has changed) or should I keep it the same to the previous version of plugin?

This id is used by the host to refer to a plugin parameter permanently. E.g. to associate automation and store this association in a host specific project file. The association is supposed to continue working in future versions of the host and plugin. Consequently, the id must not change even across plugin version increments. It becomes part of the plugin's stable interface.

sadko4u commented 1 year ago

Then, why the short symbolic name wasn't used to make the plugin format compatible with others? I mean, it is not trivial task even to generate 32-bit values from 7-char string (restriction of the VST 2.x standard) without possible hash collisions.

sadko4u commented 1 year ago

One more question about value_to_text. Should value_to_text emit units in the formatted parameter value or not? I mean, should it return 440 Hz or just 440 to display corresponding parameter to the user?

defiantnerd commented 1 year ago

It should include the units, if used, so 440 Hz.

sadko4u commented 1 year ago

This is also missing in the specification, so please add.

abique commented 1 year ago

I think this issue has been addressed already (at least it seems so to me in next branch). I close.