free-audio / clap

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

Draft `clap.location/1` extension #404

Closed roelofvkr closed 5 days ago

roelofvkr commented 7 months ago

I've added a track index field to the track info extension. This should be entirely compatible with the current version of the extension, as long as plugins don't access the track_index field when CLAP_TRACK_INFO_HAS_TRACK_INDEX isn't set in flags.

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

abique commented 7 months ago

Hi,

You can't extend the struct size like that, it'll break the ABI.

You have to extend this extension by making a complementary one.

Cheers, Alex

roelofvkr commented 7 months ago

Ohh of course, my bad, sorry. What do you mean exactly by making a complementary extension? Would you want to see an entirely separate extension for this, rather than appending it to this extension (and incrementing its version)?

In either case I think it'd be a good idea to make it possible to extend the track-info (or I guess the, would be, extended-track-info) without breaking the ABI like this. Maybe by passing in a set of flags, describing which fields the host can fill or adding a get_string/get_int getter by key (so new values can be added without breaking).

roelofvkr commented 6 months ago

Alright. I've added an extended track-info extension, which would allow plugins to read/write instance metadata in a more generic way. This way of retrieving track info should be easily extensible to include more properties without breaking anything or bumping version numbers.

abique commented 6 months ago

This PR opens a few questions:

I think there are two overlapping things here:

What do you think?

baconpaul commented 6 months ago

I think a key value approach here is smart, but think the host sho have a way to provide the available keys.

Interrsting question on if we want a generic key value extension Alex. Then this could just be the get the track info key value provider. It wouldn’t change the code much but would solve the next version of this problem

pond thought: hierarchical keys?

roelofvkr commented 6 months ago

Interesting thoughts. I think making a shared key/value extension, that can be reused by other extensions makes sense. It would also "solve" the invalidation, as you can say "when any of the track-info values are updated, the host must call clap_plugin_track_info->changed" (this is basically also what my thought was for invalidation in the current state, as it makes sense to me to reuse that same callback).

index is simple, but what if you had the plugin path within the project instead?

I'm not sure if having a path would make sense. I feel like you'd always end up trying to split the path into these components (sorting key/index for the track, sorting key/index for the instance on the track, name of the instance, possibly the name of the track). You wouldn't want to display a path directly I don't think, so unless the path is designed in such a way that it can be used as a sorting key, there wouldn't really be any gains over just using indices.

I don't think having a generic key/val store necessarily overlaps with having a bigger "project layout" extension, but they can complement each other. Let's say the project layout extension would return the project in a simplified DAWProject, you could then use the values provided like the track/instance index to look up information about a specific instance in the project. This way the project layout doesn't need to contain any instance-specific information and could theoretically be shared between instances.

abique commented 2 months ago

I looked again at this PR and I think that either it becomes simpler by using a C struct or we need a full property extension:

Or another approach would be to have a generic C interface for a key/value store and have the track info v2 have an handle for the key value store context, so we could have multiple key/value store objects instead of a global one. :thinking:

abique commented 2 months ago

Maybe there is another approach to this:

enum {
CLAP_PLUGIN_LOCATION_TRACK_GROUP,
CLAP_PLUGIN_LOCATION_TRACK,
CLAP_PLUGIN_LOCATION_PARALLEL_DEVICE,
CLAP_PLUGIN_LOCATION_NESTED_DEVICE_CHAIN,
// ...
};

struct clap_plugin_location_header {
   int kind;
};

struct clap_plugin_location_track {
   struct clap_plugin_location_header hdr;
   const char *name;
   uint32_t index_in_group;
   uint32_t global_index;
};

// More location kinds ...

struct clap_plugin_location
{
   // called by the host whenever the plugin location changes.
   // location described as an array of path element
   void (*set_location)(clap_plugin_t *plugin, struct clap_plugin_location_header *path, int nelts);
};

This approach would allow us to describe correctly the plugin location, because you can have parallel devices (eg: drum machine, chain selector, ...), and a more complex representation than just a track and a plugin.

roelofvkr commented 2 months ago

I think that last approach would be a good way to go about it. Describing a location like a list of path elements makes sense to me. What kind of per-type data do you think is important to give the elements though? There are things I can think of like the solo/mute/armed state of tracks, but those don't seem appropriate to put in the instance location.

To me it seems like most location descriptions can be described using a single struct, basically with what you provided, like:

struct clap_plugin_location_element {
    const char *name;
    int kind;
    // Index within the previous (parent) path element. 
    uint32_t index_in_group;
    // Index of the element in a global context. When sorting elements by this index, 
    // the order should mirror the order as it is displayed in the host. 
    uint32_t global_index;
};

This would also keep the set_location simple, as the path array will be a uniform type.

I also think a separate property extension would be a good idea. This could then contain that information about the context that isn't related to the location of the instance.

baconpaul commented 2 months ago

This is looking, if we add the neighbor plugins, a lot like an extension I’ve wanted where you can interrogate your neighbors to determine parameter sets for downstream modularion. Not sure it belongs here but just wanted to add that comment

abique commented 2 months ago

Would we need a color here?

roelofvkr commented 2 months ago

That's a good point. I can imagine all of these element types having a colour and that being useful (rather than just the track colour as in the track-info extension). Adding it now.

abique commented 2 months ago

The challenge with this extension is the balance between over engineering and minimalism.

We may be tempted to add a lot, which will lead to having a dictionary per-element. Or we try to be minimalist with the current approach which is probably good enough to hold for 10 years.

roelofvkr commented 2 months ago

I think this will be sufficient in a lot of use cases. It allows for quick sorting of instances and contains enough information to display the hierarchical position in a nice and user friendly way. I think it'd be good to keep this scope low and if it's ever necessary to expand on this using something like a "structure" extension, which would allow the plugin to query the host for information about these elements using their global_index.

One more thing that might be good to add though is a value in the kind enum for CLAP_PLUGIN_LOCATION_PROJECT, for hosts which can have multiple open projects at once, which the plugin could then distinguish.

abique commented 2 months ago

You may want to add an optional const char *id or const char *path to the element. In bitwig we use uuids internally for the tracks, and we also have an internal way to describe a path to an element using a string.

Because if we have this kind of handle, then we could later on have a way to query additional information about an element.

roelofvkr commented 2 months ago

Interesting, I was thinking the global_index value would be sufficient for that, as it is also guaranteed to be unique. Would that not work?

abique commented 2 months ago

global index makes sense for a track, but for a device or device chain, I don't think so, and it is certainly not a stable identifier.

roelofvkr commented 2 months ago

Oh of course, makes sense. I'll add it

roelofvkr commented 2 months ago

So I looked at it again and I think this would fit most use cases. The only thing I'm not entirely sure about is the color in each element. I think it would be good to keep, despite the overlap with the track-info extension.

Another thing that I did think of that could not be captured in the current api though is return tracks and the master track. Though of course they could just be sorted in the position where they are displayed in the host, there is no way for the plugin to make that distinction through this api. A logical solution for this would be an optional per-kind flags field, or extending the location kind enum with separate master/return track values.

abique commented 3 weeks ago

@roelofvkr Hi, I'd like to merge this PR for the next clap release, please can you squash and rebase? You'll have to pull -f the next branch.

roelofvkr commented 3 weeks ago

Yes, I'll do so tonight. By squash and rebase I assume you mean squash this entire pull request into a singular commit right?

abique commented 3 weeks ago

Yes you can make it just one commit: draft track-location

roelofvkr commented 3 weeks ago

Alright. I think I've squashed it as asked. It took a second to figure out but is this as expected?

abique commented 3 weeks ago

Thank you :+1: