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

note expressions extension #354

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

Bremmers
Copy link

No description provided.

@baconpaul
Copy link
Collaborator

@defiantnerd if this was a separate extension we could use it in the wrapper no problem right?


// TODO : is the clap_supported_note_expressions bitmask future-proof? Or should it be a "count / get-info" pattern?

static CLAP_CONSTEXPR const char CLAP_EXT_NOTE_EXPRESSIONS[] = "clap.note-expressions.draft/0";
Copy link
Contributor

@messmerd messmerd Jan 10, 2024

Choose a reason for hiding this comment

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

Suggested change
static CLAP_CONSTEXPR const char CLAP_EXT_NOTE_EXPRESSIONS[] = "clap.note-expressions.draft/0";
static CLAP_CONSTEXPR const char CLAP_EXT_NOTE_EXPRESSIONS[] = "clap.note-expressions/1";

Following the new convention that omits any reference to "draft".
EDIT: Changed it to start at 1 instead of 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the plugin id convention says that it should start at 1.

Comment on lines +25 to +26

CLAP_NOTE_EXPRESSION_MASK_ALL = (1<<7)-1 // just the and of the above
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CLAP_NOTE_EXPRESSION_MASK_ALL = (1<<7)-1 // just the and of the above

At first I thought baking the current number of note expressions into the value of the "ALL" option would introduce an ABI incompatibility if you were to add another note expression to this enum in the future, but thinking about it some more, I think it might be fine - at least from an ABI standpoint.

One potential issue is if you later introduce a special enum value which is incompatible with some of the other values - that could complicate things.

But the bigger issue would occur if a plugin author upgraded to a newer CLAP API version which introduced a new enum value. If they were relying on the old value for "ALL", this would silently introduce a bug when the plugin author recompiles their plugin because now they would be advertising that they support a note expression that they do not actually support.

For that reason, I would remove the "ALL" mask. It's not strictly necessary to have anyway.

Copy link
Collaborator

@baconpaul baconpaul Jan 10, 2024

Choose a reason for hiding this comment

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

So this is a bitmask enum and the intent is to be able to set it with | and query it with &

if you add an enum that doesn’t change but the value of all would add another bit

I guess my view is: the most common case is to say as a plug-in you want all note expressions or none. Those two should be idiomatic and easy. Having to | together the list of every expression is pretty tedious. We can add a const value outside the enum if you want which is just the | of all values (or in the enum) but im not sure what would break if you did that other than people using == vs & improperly would experience a break

Comment on lines +35 to +36
// [main-thread]
uint32_t(CLAP_ABI* supportedNoteExpressions) (const clap_plugin* plugin, int16_t port_index, int16_t channel); // returns a bitmap of clap_supported_note_expressions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// [main-thread]
uint32_t(CLAP_ABI* supportedNoteExpressions) (const clap_plugin* plugin, int16_t port_index, int16_t channel); // returns a bitmap of clap_supported_note_expressions
// Returns a bitmap of clap_supported_note_expressions.
// [main-thread]
uint32_t(CLAP_ABI *get_supported)(const clap_plugin *plugin,
int16_t port_index,
int16_t channel);

Modified the formatting and conventions to be consistent with the rest of the CLAP API

@abique abique force-pushed the next branch 5 times, most recently from bfe862b to 961be0b Compare September 20, 2024 08:20
@abique abique force-pushed the next branch 2 times, most recently from 076ddda to dbf7513 Compare November 1, 2024 20:20
@abique abique force-pushed the next branch 5 times, most recently from 533e849 to 50f004f Compare November 19, 2024 08:51
@abique abique force-pushed the next branch 3 times, most recently from de3907e to c279d5a Compare December 20, 2024 08:47
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.

4 participants