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.

Sign up for GitHub

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
Open

note expressions extension #354

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fe8006
note expressions extension
Bremmers Oct 12, 2023
cdfc77a
removed getNumMIDIChannels
Bremmers Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions include/clap/ext/draft/note-expressions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#pragma once

#include "../../plugin.h"

// This extension reports which note expressions are supported by a plugin.
// If this extension isn't present all note expressions are assumed to be supported.

// TODO : is the clap_supported_note_expressions bitmask future-proof? Or should it be a "count / get-info" pattern?
// TODO : does getNumMIDIChannels still make sense in a 'real' extension? It can probably removed if we go for a "count / get-info" pattern?

Bremmers marked this conversation as resolved.
Show resolved Hide resolved
static CLAP_CONSTEXPR const char CLAP_EXT_NOTE_EXPRESSIONS[] = "clap.note-expressions.draft/0";
Copy link
Contributor

@messmerd messmerd Jan 10, 2024 •
edited
Loading

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.


#ifdef __cplusplus
extern "C" {
#endif

enum clap_supported_note_expressions
{
CLAP_NOTE_EXPRESSION_MASK_VOLUME = 1 << 0,
CLAP_NOTE_EXPRESSION_MASK_PAN = 1 << 1,
CLAP_NOTE_EXPRESSION_MASK_TUNING = 1 << 2,
CLAP_NOTE_EXPRESSION_MASK_VIBRATO = 1 << 3,
CLAP_NOTE_EXPRESSION_MASK_EXPRESSION = 1 << 4,
CLAP_NOTE_EXPRESSION_MASK_BRIGHTNESS = 1 << 5,
CLAP_NOTE_EXPRESSION_MASK_PRESSURE = 1 << 6,

CLAP_NOTE_EXPRESSION_MASK_ALL = (1<<7)-1 // just the and of the above
Comment on lines +25 to +26
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 •
edited
Loading

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

};

/*
retrieve additional information for the plugin itself, if note expressions are being supported and if there
is a limit in MIDI channels (to reduce the offered controllers etc. in the host)
*/
typedef struct clap_plugin_note_expressions
{
// [main-thread]
uint32_t(CLAP_ABI* getNumMIDIChannels) (const clap_plugin* plugin, int16_t port_index); // return 1-16

// [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
Comment on lines +35 to +36
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

} clap_plugin_note_expressions_t;


typedef struct clap_host_note_expressions {
// port_index = -1 means 'all ports'
// [main-thread]
void(CLAP_ABI *changed)(const clap_host_t *host,
int16_t port_index);
} clap_host_note_expressions_t;

#ifdef __cplusplus
}
#endif