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

Adds Preset Discovery #265

Merged
merged 20 commits into from
Jan 2, 2023
Merged

Adds Preset Discovery #265

merged 20 commits into from
Jan 2, 2023

Conversation

abique
Copy link
Contributor

@abique abique commented Dec 29, 2022

No description provided.

@baconpaul
Copy link
Collaborator

Curious why the preset stuff isn’t in an extension and is in the top clap directory?

@abique
Copy link
Contributor Author

abique commented Dec 29, 2022

Curious why the preset stuff isn’t in an extension and is in the top clap directory?

Because it is not part of the plugin instance. You don't need a plugin instance to index the presets.
The preset-load extension is the plugin extension which is used to load the preset from a file which have been indexed using preset discovery.

On the other hand, yes it could go in ext/draft, but somehow as it doesn't extend directly the plugin/host it feels to me that it shouldn't go there.

@defiantnerd
Copy link
Contributor

Because it is not part of the plugin instance. You don't need a plugin instance to index the presets.

I beg to differ :) Presets are different in connected hardwares. At least, the plugin instance should probably provide additional per instance presets.

@abique
Copy link
Contributor Author

abique commented Dec 29, 2022

Because it is not part of the plugin instance. You don't need a plugin instance to index the presets.

I beg to differ :) Presets are different in connected hardwares. At least, the plugin instance should probably provide additional per instance presets.

Then we'd need a preset bank extension right?

But in a way it isn't trivial to index because it means:

  • it needs a connected hardware to be indexed
  • it needs a connected hardware to be loaded

So somehow we'd need to model "connectable" or "dynamically available" content into our browser. I think it opens many questions which aren't trivial.
Also if the content is specific to one plugin instance and not to the hardware itself, then what's the point to index it?

@defiantnerd
Copy link
Contributor

Then we'd need a preset bank extension right?

This would be a similar, but different extension for the plugin, I guess. And it is an edge case, indeed.

// Create a preset provider by its id.
// The returned pointer must be freed by calling preset_provider->destroy(preset_provider);
// The preset provider is not allowed to use the indexer callbacks in the create method.
// Returns null in case of error.

Choose a reason for hiding this comment

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

What are the 'indexer callbacks'? clap_preset_indexer_t doesn't have any functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now it doesn't have, maybe it could have some I'm not sure yet.

@baconpaul
Copy link
Collaborator

Curious why the preset stuff isn’t in an extension and is in the top clap directory?

Because it is not part of the plugin instance. You don't need a plugin instance to index the presets. The preset-load extension is the plugin extension which is used to load the preset from a file which have been indexed using preset discovery.

On the other hand, yes it could go in ext/draft, but somehow as it doesn't extend directly the plugin/host it feels to me that it shouldn't go there.

So one of my synths has entirely in-memory presets. How would you index those without an instance? I guess I would have a clap which makes a factor which makes and discards an instance?

Also the JUCE Preset API is entirely instance based. I am not sure if I will be able to adapt it to this (other than doing the create-and-discard trick). And there's no requirement JUCE presets are on file system.

So I guess: Are we implicitly requiring the idea of "one physical on disk file per preset" here?

@baconpaul
Copy link
Collaborator

Or another way to ask it: is there any requirement at all that the contents of the path is a file you can stat?

@baconpaul
Copy link
Collaborator

(what I'm thinking is: if path becomes clap_preset_unique_name then the in-memory thing is easy. I just advertise 50 names and load them from memory when you hit me up. If it is actually something the daw will do fs::is_file on, then that won't work obviously).

@abique
Copy link
Contributor Author

abique commented Dec 29, 2022

Curious why the preset stuff isn’t in an extension and is in the top clap directory?

Because it is not part of the plugin instance. You don't need a plugin instance to index the presets. The preset-load extension is the plugin extension which is used to load the preset from a file which have been indexed using preset discovery.
On the other hand, yes it could go in ext/draft, but somehow as it doesn't extend directly the plugin/host it feels to me that it shouldn't go there.

So one of my synths has entirely in-memory presets. How would you index those without an instance? I guess I would have a clap which makes a factor which makes and discards an instance?

Also the JUCE Preset API is entirely instance based. I am not sure if I will be able to adapt it to this (other than doing the create-and-discard trick). And there's no requirement JUCE presets are on file system.

So I guess: Are we implicitly requiring the idea of "one physical on disk file per preset" here?

If the preset are in-memory and per-instance we don't index them, what would be the point?
Yes we're talking about preset which are on disk.

@abique
Copy link
Contributor Author

abique commented Dec 29, 2022

Or another way to ask it: is there any requirement at all that the contents of the path is a file you can stat?

Yes, because we list the filesystem and then do find the preset files by extensions.
You can always create a dummy file as a "gateway" to something.

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.

I mentioned this yesterday and @baconpaul also mentioned it in this PR before the discussion kind of diverged into JUCE implementation details, but I do think having some form of shorthand to expose presets embedded into the plugin itself (which is extremely common) would be immensely useful. The suggestion was that you'd just use your plugin's .clap file as the preset location, but that adds complexity and potential for making mistakes on the plugin's side. Since this is such a common use case, I propose adding adding a string constant for the location that indicates 'this CLAP file', like $PLUGIN or %PLUGIN%. Or URIs as previously mentioned, where file:///path/to/files refers to the /path/to/files directory, plugin:/// refers to the .clap file, and plugin:///foo could refer to a foo bank inside of the plugin's file (if that's useful to support). Neither approach requires the interface to be changed, and they're both unambiguous when dealing with regular file paths versus presets stored within this .clap file.

The second matter was informing the host that a preset has been added, removed, or modified. Creating a temporary file for the host to watch and touching it when these things happen works. I wonder if there's a nicer API for this though, couldn't we just have a clap_host_preset_discovery_context or something like that with a callback and a method on the factory to set that context object? That would make rescanning based on triggers from the pluin a lot more robust and easier to implement in a cross platform way.

include/clap/clap.h Outdated Show resolved Hide resolved
include/clap/preset-discovery.h Show resolved Hide resolved
include/clap/preset-discovery.h Show resolved Hide resolved
include/clap/preset-discovery.h Outdated Show resolved Hide resolved
@baconpaul
Copy link
Collaborator

I propose adding adding a string constant for the location that indicates 'this CLAP file', like $PLUGIN or %PLUGIN%. Or URIs as previously mentioned, where file:///path/to/files refers to the /path/to/files directory, plugin:/// refers to the .clap file, and plugin:///foo could refer to a foo bank inside of the plugin's file (if that's useful to support).

I think the file:// and plugin:// URI solution or some way to refer to internal presets is a very good idea also.

@abique
Copy link
Contributor Author

abique commented Dec 30, 2022

Right now if you write a juce iplug2 etc plugin and support getNumPrograms / getProgramName / loadProgram then your presets show up in logic and reaper and so on in the plugin preset list if you make an AU or a VST3. That's kinda all you have to do and it just kind of works. That is how the plugins defacto expose their factory presets.

So first question: Do we want a plugin which coded to that mechanism to also expose those presets to CLAP using this mechanism. I had assumed "yes" but if the answer is "no" then everything is fine.

Programs are out of the scope for this extension.
In Bitwig we also show vst2 and vst3 programs.

CLAP doesn't have a program/bank management extension yet.

@baconpaul
Copy link
Collaborator

OK then I mis-understood the extension. And no JUCE, iPlug, DISTRHO etc plugin will generate a scannable preset list using their in-framework API for programs. Gotcha.

@abique
Copy link
Contributor Author

abique commented Dec 30, 2022

The second matter was informing the host that a preset has been added, removed, or modified. Creating a temporary file for the host to watch and touching it when these things happen works. I wonder if there's a nicer API for this though, couldn't we just have a clap_host_preset_discovery_context or something like that with a callback and a method on the factory to set that context object? That would make rescanning based on triggers from the pluin a lot more robust and easier to implement in a cross platform way.

I clarify something:

  1. host must watch for each location recursively, so when you add a preset (using the file manager), the host should pick it up automatically using file system notification
  2. invalidation_watch_file is for invalidating the list of locations, file extensions, etc...

I don't think that the plugin can solve this for the indexer.
Monitoring filesystem changes is not difficult and it is a requirement for any DAW, because the user may drop files at anytime using the file explorer (even samples).

Also there is no "plugin daemon / service" which is running all the time. And plugins aren't a single instance thing either.

@defiantnerd
Copy link
Contributor

IMHO the strict focus on files annoys me a bit. Files come with all kind of implications that make such a library complicated (paths, user privileges, UTF8 characters). I am also not a big fan of "tags" like "Woodwind".. this is the kind of library stuff that didn't work for other hosts, too.

@abique
Copy link
Contributor Author

abique commented Dec 30, 2022

IMHO the strict focus on files annoys me a bit. Files come with all kind of implications that make such a library complicated (paths, user privileges, UTF8 characters). I am also not a big fan of "tags" like "Woodwind".. this is the kind of library stuff that didn't work for other hosts, too.

What do you suggest then?

Regarding the indexing, it is not a problem to have a lot of information, the daw can always decide to show "less" or to organize the presets in a more minimal way.

@abique abique mentioned this pull request Dec 30, 2022
@baconpaul
Copy link
Collaborator

I am also not a big fan of "tags" like "Woodwind".. this is the kind of library stuff that didn't work for other hosts, too.

What do you suggest then?

The easiest solution is to be silent on the keywords.

The second easiest is to be incomplete.

The hardest is to be relatively comprehensive

But the first and third are often better than the second. I wonder why we need to define any categories at all?

@baconpaul
Copy link
Collaborator

But if we are going to define categories, I would at least ask @mkruselj to think about some of his experience with genres, tags, and categories and give the list a once over :)

@mkruselj
Copy link

My suggestion would be to stay silent on the keywords, to be honest. Better don't define any if it's going to be sparse like this.

@abique
Copy link
Contributor Author

abique commented Dec 31, 2022

This list isn't definitive, it can be extended.
I don't see a point in not having it, the plugin can ignore it that's fine.
The extension doesn't force the plugin to describe its content in a certain way, but it just gives the option to use a language which maybe will be understood by most.

@abique
Copy link
Contributor Author

abique commented Jan 2, 2023

My suggestion would be to stay silent on the keywords, to be honest. Better don't define any if it's going to be sparse like this.

I've had a second thought at it and OK, I'll kill it.

@abique abique marked this pull request as ready for review January 2, 2023 08:45
@abique abique merged commit 0513d1b into free-audio:next Jan 2, 2023
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.

8 participants