free-audio / clap

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

added project load as explicit option #375

Closed defiantnerd closed 6 months ago

defiantnerd commented 6 months ago

the context "loading a project/song" is the most prominent example of using a context information for plugin state persistence. it is also implemented in other audio standards.

I also changed the order of the constants using 1 for the "if in doubt" option.

abique commented 6 months ago

So, CLAP_STATE_CONTEXT_FOR_PROJECT is equivalent to clap_plugin_state->load()?

If I remember well, we discussed and adding it implies deprecating clap_plugin_state, is that correct?

defiantnerd commented 6 months ago

The extension is indeed replacing the original state extension. But I don't see another way for this, since it does not fit the purpose. But we can not deprecate the original state extension for now.

CLAP_STATE_CONTEXT_FOR_PROJECT is not the same as the simple state extension, since `FOR_PROJECT' is potentially more destructive. In my case the plugin instances are bound to specific unique hardwares (identified by serial number) and audio is also being output to external outs, delay compensated.

When loading a song, the order of plugin restoration can not be determined. Therefore the absolute assignment to a device is crucial when loading a project. But it is not giving a pleasent user experience, if the connection would be part of the state it can be ignored if the state is used to restore "in duplicate" context or "a preset loaded" context. The distinction of the context is important here and the functionality can not be provided by the original state extension.

I hope this clears things up a bit for you.

robbert-vdh commented 6 months ago

CLAP_STATE_CONTEXT_FOR_PROJECT is how the state extension, and the default state handling in any other plugin API for that matter, is generally used by hosts. Your use case also fits the CLAP_STATE_CONTEXT_FOR_PROJECT usage of the regular state extension.

defiantnerd commented 6 months ago

Again, in theory, you're right - but to make it work well in a good user experience, it must like I've proposed - I already handle it this way in VST3 and hosts we recommend support that feature ("adding the Project context to the loadable stream).

And yes, if the state-context extension is present and supported, it should be used exclusively.

abique commented 6 months ago

So from what I read, it appears to me that it is unclear how state->save() maps to the state context. And maybe this doesn't need to be specified and can remain ambiguous.

If someone wants to clear this ambiguity, he then uses the state context extension.

defiantnerd commented 6 months ago

My original proposal does not need a context for the save() function. We can happily remove it. The actual idea is to know what to load and what to ignore from a state depending on the context. I am happy to change that back again.

Indeed different save() makes it complicated and also creates probably incompatible states - which is nothing we want.

baconpaul commented 6 months ago

So I’m super late and not that invested here but…. If this is just adding a context to the state extension, why not take a state extension instance as an argument.

That is, yu have something like “set context for (clap state *, context)”

then you require the implementation of state by api not just by documentation.

The host then does

set context for s S->load Context complete

and the load function manages the state internally with a guard.

Gives you the awkward state maintenance but avoids duplicated load and save api points

this may be a terrible idea and ignore it if it is

abique commented 6 months ago

@baconpaul the decorator api was suggested in the past but I didn't like it. Because, you may not see it at all and it requires the plugin to cache the context and you never know what mistake can happen. While on the other hand, passing the context to the load/save is a coding style I prefer.

Compare:

if (can_use_state_context)
   save_ctx(ctx)
else
   save()

to:

if (can_use_state_context)
   set_save_ctx(ctx)
save()

The first approach feels more 'atomic' to me because there's only one call.

baconpaul commented 6 months ago

Yeah that makes sense My thinking was if the save ctx to a pointer to just the regular state extension it would force the constraint that you must implement both and avoid redundancy but at the cost of a non atomic api

abique commented 6 months ago

I think that the duplication of the save() method isn't the end of the world, I agree it isn't great but it isn't hard either to deal with.

What save() should do in regard to the context, is ambiguous Tim thought PRESET was best, I thought PROJECT was better and at the end of the day, the plugin decides what it wants to save and what works best by default for its particular needs. I think it is totally fine to not specify to which context save() maps to.

If we agree on that direction, then we need to adjust a bit the document for example:

In case of doubt, fallback to clap_plugin_state->{load,save}() to let the plugin choose the ideal fallback context.

abique commented 6 months ago

Do we have a consensus?

Bremmers commented 6 months ago

I would suggest you accept this PR and update the comments. AFAICS only Robbert isn't too happy with this. I used to agree with him, but it has become clear to me that using the context extension for two states and the normal state extension for one state is confusing and will lead to problems.