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

added project load as explicit option #375

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Conversation

defiantnerd
Copy link
Contributor

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 abique requested a review from robbert-vdh December 28, 2023 10:21
@abique
Copy link
Contributor

abique commented Dec 28, 2023

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?

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.

My opinion on this is still the same as before. CLAP_STATE_CONTEXT_FOR_PROJECT is equivalent to the regular usage of the state extension, so adding it effectively either deprecates the state extension, or duplicates the functionality. I think it's a good idea to avoid duplicating functionality unless absolutely required. In this case not having CLAP_STATE_CONTEXT_FOR_PROJECT means that both extensions can supplement each other. Most plugins won't need the state context, so they won't have to add support for it either. But if the functionality is duplicated, then now both plugins and hosts need to handle the situation where only one of the extensions is implemented. And worse, the behavior can be different between the old state extension and the CLAP_STATE_CONTEXT_FOR_PROJECT state context.

@defiantnerd
Copy link
Contributor Author

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

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

defiantnerd commented Dec 30, 2023

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

abique commented Jan 2, 2024

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

defiantnerd commented Jan 2, 2024

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

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

abique commented Jan 3, 2024

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

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

abique commented Jan 3, 2024

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

abique commented Jan 6, 2024

Do we have a consensus?

@Bremmers
Copy link

Bremmers commented Jan 6, 2024

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.

@abique abique merged commit b9a6470 into free-audio:next Jan 8, 2024
4 checks passed
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.

5 participants