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

Draft clap.location/1 extension #404

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

Conversation

roelofvkr
Copy link

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
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

@abique
Copy link
Contributor

abique commented Apr 22, 2024

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

Copy link
Contributor

@abique abique left a comment

Choose a reason for hiding this comment

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

Extending the struct will break the ABI.

@roelofvkr
Copy link
Author

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
Copy link
Author

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.

@roelofvkr roelofvkr requested a review from abique May 14, 2024 12:15
@abique
Copy link
Contributor

abique commented May 19, 2024

This PR opens a few questions:

  • should we have a generic key/value object in clap that could be re-used by other extension, so you'd implement this only once
  • it is missing invalidation of the data
  • index is simple, but what if you had the plugin path within the project instead?

I think there are two overlapping things here:

  • a short term solution for being able to order multiple plugin instances
  • a long term solution for understanding the project structure (tracks, tracks group, parallel containers, device chain, ...)
    If that is correct maybe we just go with a simple solution for the track index without a dictionary and then someone may need to start thinking about a more complex solution for the long term one. Maybe it could be receiving a stripped down version of DAW project (without plugin states, ...)

What do you think?

@baconpaul
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Contributor

abique commented Sep 2, 2024

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:

  • key (string) / value (string/bool/int64/double/void?)
  • need to be able to list available keys??
  • need to be able to inspect a key (value type and is read-only)
  • need to be able to subscribe to a notification when a value change, when a key is added or removed

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. 🤔

@abique
Copy link
Contributor

abique commented Sep 2, 2024

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Contributor

abique commented Sep 5, 2024

Would we need a color here?

@roelofvkr
Copy link
Author

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
Copy link
Contributor

abique commented Sep 5, 2024

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
Copy link
Author

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
Copy link
Contributor

abique commented Sep 5, 2024

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
Copy link
Author

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
Copy link
Contributor

abique commented Sep 5, 2024

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
Copy link
Author

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

@roelofvkr
Copy link
Author

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
Copy link
Contributor

abique commented Nov 1, 2024

@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
Copy link
Author

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
Copy link
Contributor

abique commented Nov 1, 2024

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

@abique abique changed the title Add a track index track-info extension Draft clap.location/1 extension Nov 1, 2024
@roelofvkr
Copy link
Author

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

@abique
Copy link
Contributor

abique commented Nov 1, 2024

Thank you 👍

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how to provide this value.

You can already sort using the index in group, why would you need a global one?
Keeping the global one in sync, requires to constantly invalidate it after each insertion/deletion in the document.
And I don't see it necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the global_index.

Copy link
Author

Choose a reason for hiding this comment

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

That's true, but the flipside is that if a plugin wants to, for example, display instances in a flat list in the same order as in the host not having a global index would significantly complicate that sorting process. I understand that for a host it's more work, but it would make host->plugin consistency a lot easier, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

The complexity for the plugin is super easy. You just implement a compare function that compares the path elements one by one.

While for the host, it is really a difficult job. You're basically asking for the host to invalidate and crawl the entire project on every device insertion/deletion. This hooks into the undo etc...

uint32_t global_index;
// Color for this element, should be 0x00000000 if no color is used for this
// element.
clap_color_t color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could have a constant color CLAP_COLOR_TRANSPARENT declared in color.h and reference it here?

Copy link
Author

Choose a reason for hiding this comment

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

That sounds like a good idea, I'll add it.

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.

6 participants