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

CLAP 1.1.10 #359

Merged
merged 29 commits into from
Nov 6, 2023
Merged

CLAP 1.1.10 #359

merged 29 commits into from
Nov 6, 2023

Conversation

abique
Copy link
Contributor

@abique abique commented Nov 3, 2023

No description provided.

messmerd and others added 24 commits November 2, 2023 01:25
Adds a CLAP_PARAM_IS_ENUM flag to clap_param_info_flags to denote an enum parameter. This flag tells the host when a parameter represents a set of named options mapped to integers, which can help the host decide how to present the parameter to users in its UI.
Clarify the meaning of boolean return values
Copy link
Contributor

@defiantnerd defiantnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum is great, documentation changes are cool.

// this to format parameter values before displaying it to the user. [main-thread]
// given 'value' argument. eg: "2.3 kHz". The host should always use this to format parameter
// values before displaying it to the user.
// Returns true on success.
Copy link
Contributor

@messmerd messmerd Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the value_to_text() function, I think it would be useful if the host could know when conversion from the double value to the string in out_buffer is trivial (i.e. 1.0 --> "1.0").

I could compare the string std::to_string(value) with the string returned from value_to_text(), but they might still be different strings due to precision or locale differences in floating point to string conversions. (for example, "1.0000000" and "1.0" compare as false). This makes it difficult and inefficient for hosts to know whether the parameter value to string conversion was trivial.

In LMMS (and presumably any other DAWs where users work with plain integers or floats when editing parameter automation), this often leads to me having to display two nearly identical strings to users alongside each other - "1.0" (the parameter's internal value which users use to automate the parameter) and "1.0000" (the supposedly fancy parameter text from value_to_text()). In these cases, I would like to be able to only display one.

To fix this issue, maybe the return value of value_to_text() could be false when an error occurred OR the conversion to string would be trivial? This would be a recommendation for plugins to return false to let the host do a trivial double-to-string conversion instead of doing that same conversion themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do your users edit plain floats? To edit a parameter they should type in a string which the host hands to string to value and let the parameter handle it no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LMMS uses plain float or integer values in the automation editor and parameter editing dialog boxes, not custom strings like those returned from value_to_text(). The parameter/automation system could potentially be reworked in the future, but would require a lot of work just to better support CLAP.

Until that happens, LMMS and any other hosts that do not (yet) support CLAP's recommended practice of hiding internal parameter values from users and only exposing strings from value_to_text() will have an unnecessarily rough time which I believe could be alleviated if CLAP provided better guidance to plugin devs on how to implement value_to_text().

That is, use a return value of false to indicate that a particular parameter has no special way of presenting itself and can just be trivially converted to a string. This is similar to how leaving value_to_text() unimplemented indicates that none of the parameters have a special way of presenting themselves.

I understand if this is not a change you'd like to make, but to me it is better to specify the meaning of the false return value AND make it useful for certain hosts than it is to leave it unspecified and allow diverging implementations in plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i view 'false' as in 'i am unable to convert this value to a string'. for instance if you send an out of range value or haven't implemented the clap api. I don't think we should overload it to mean "I did nothing"; but rather it means "don't trust the value of the text buffer as being correctly populated". We could definitely add this to do the doc if people agree, though.

I understand and am sympathetic to the work in LMMS to rework to use strings through the editor. But I'm not that thrilled about changing the base API for that (although others may disagree). Having the param value <> string be bidirectional and both as a requirement of the plugin developer and a guide for the host author seems good.

Perhaps one solution would be to offer an LMMS specific extension where you could advertise this fact?

But also: taking a quick look at surge and the conduit plugins, I think none of our few hundred params would return true - maybe the 8 macros would but I think even those we convert to % * 100 at the edge. I'm not sure how other plugin authors would score.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also I'm very excited for LMMS to get CLAP support; many of our users ask for quality electronic open source DAWs which can support Surge and since we don't ship a VST2 etc... if I can help separately from this issue please always do tag me in)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, this isn't a roadblock to implementing CLAP support in LMMS - it's just (in my opinion) a "nice to have" enhancement. If the value_to_text() function remains the same as it is, that's fine with me. I just figured it would be worth a shot suggesting a way to change it.

I also appreciate your offer to help regarding Surge support in LMMS. In the next few weeks, I'll be trying to get Surge to an operational state in LMMS's CLAP host, so I'll let you know if any issues come up that I don't know how to handle.

Copy link
Contributor

@Trinitou Trinitou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small format suggestions

include/clap/ext/draft/midi-mappings.h Outdated Show resolved Hide resolved
include/clap/ext/draft/preset-load.h Outdated Show resolved Hide resolved
include/clap/ext/gui.h Outdated Show resolved Hide resolved
include/clap/plugin.h Outdated Show resolved Hide resolved
Copy link
Member

@robbert-vdh robbert-vdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@abique abique merged commit e292447 into main Nov 6, 2023
6 checks passed
@abique
Copy link
Contributor Author

abique commented Nov 6, 2023

Thank you everyone!

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.

7 participants