AcademySoftwareFoundation / openfx

OpenFX effects API
Other
410 stars 120 forks source link

Add `OfxParamTypeStrChoice`, a string-backed enum of choices #129

Closed gregcotten closed 6 months ago

gregcotten commented 12 months ago

Summary

Currently there is no way to update a list of choices except by appending them to the end of the list of choices. This is because OfxParamTypeChoice simply remembers what integer index was selected by the user.

For UX reasons, plugin developers sometimes need to be able to provide alphabetically sorted lists (or just a curated order) of choices for a user to pick through. Unfortunately OfxParamTypeChoice doesn't provide a solution to this particular problem.

The new OfxParamTypeStrChoice should have 2 N-string arrays:

kOfxParamPropChoiceEnum - a list of strings that could be some sort of non-user-facing internal identifier kOfxParamPropChoiceOption - a corresponding list of strings (same size as kOfxParamPropChoiceEnum) of "pretty" user-facing display name strings

This allows the plugin developer to have constant implementation-detail enum identifier strings while having a pretty display name that they can change between releases for UI/UX reasons.

DaVinci Resolve already defines an extension:

/** @brief String to identify a param as a Single string-valued, 'one-of-many' parameter */
#define kOfxParamTypeStrChoice "OfxParamTypeStrChoice"

/** @brief Set a enumeration string in a choice parameter.

    - Type - UTF8 C string X N
    - Property Set - plugin parameter descriptor (read/write) and instance (read/write),
    - Default - the property is empty with no options set.

This property contains the set of enumeration strings corresponding to the options that will be presented to a user from a choice parameter. See @ref ParametersChoice for more details..
*/
#define kOfxParamPropChoiceEnum "OfxParamPropChoiceEnum"

/** @brief Indicates if the host supports animation of string choice params.

    - Type - int X 1
    - Property Set - host descriptor (read only)
    - Valid Values - 0 or 1
*/
#define kOfxParamHostPropSupportsStrChoiceAnimation "OfxParamHostPropSupportsStrChoiceAnimation"

Motivation

Fills a UI gap for plugin developers.

Problem

Plugin developers need to update their plugins with the latest and greatest features which can include adding new entries to a choice param. Currently they are stuck adding the entry to the end of the choice param (not to mention removing an old entry).

Impact

Purely additive.

Documentation Impact

A new param type, probably OfxParamTypeStrChoice since it's already used by Resolve.

Stakeholders

Everyone, I hope!

fxtech commented 12 months ago

Is there a property to query if the host supports this?

On Fri, Oct 6, 2023, 6:29 PM Greg Cotten @.***> wrote:

Summary

Currently there is no way to update a list of choices except by appending them to the end of the list. This is because ChoiceParam simply remembers what integer index was selected by the user.

For UX reasons, plugin developers sometimes need to be able to provide alphabetically sorted lists (or just a curated order) of choices for a user to pick through. Unfortunately ChoiceParam doesn't fulfill this particular problem. DaVinci Resolve defines an extension regrettably officially namespaced OfxParamTypeStrChoice with the following extensions:

/ @brief String to identify a param as a Single string-valued, 'one-of-many' parameter */#define kOfxParamTypeStrChoice "OfxParamTypeStrChoice" /* @brief Set a enumeration string in a choice parameter. - Type - UTF8 C string X N - Property Set - plugin parameter descriptor (read/write) and instance (read/write), - Default - the property is empty with no options set.This property contains the set of enumeration strings corresponding to the options that will be presented to a user from a choice parameter. See @ref ParametersChoice for more details../#define kOfxParamPropChoiceEnum "OfxParamPropChoiceEnum" / @brief Indicates if the host supports animation of string choice params. - Type - int X 1 - Property Set - host descriptor (read only) - Valid Values - 0 or 1*/#define kOfxParamHostPropSupportsStrChoiceAnimation "OfxParamHostPropSupportsStrChoiceAnimation"

Motivation

Fills a UI gap for plugin developers. Problem

Plugin developers need to update their plugins with the latest and greatest features which can include adding new entries to a choice param. Currently they are stuck adding the entry to the end of the choice param (not to mention removing an old entry). Impact

Purely additive. Documentation Impact

A new param type, probably OfxParamTypeStrChoice since it's already used by Resolve. Stakeholders

Everyone, I hope!

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openfx/issues/129, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM64AQZKW3WBTSEZKA4HTTX6CH5RAVCNFSM6AAAAAA5WOY6QKVHI2DSMVQWIX3LMV43ASLTON2WKOZRHEZTCMBUGIYTGNY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

gregcotten commented 12 months ago

Not in the Resolve extension, but obviously could add!

fxtech commented 12 months ago

I guess the idea is you try to add it and then fall back to them other choice list if the host returns an error? So I suppose the query isn't really necessary.

On Sat, Oct 7, 2023, 9:16 AM Greg Cotten @.***> wrote:

Not in the Resolve extension, but obviously could add!

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openfx/issues/129#issuecomment-1751723928, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM64AX6GWZHTSJRL4Z7MSTX6FP5RAVCNFSM6AAAAAA5WOY6QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJRG4ZDGOJSHA . You are receiving this because you commented.Message ID: @.***>

gregcotten commented 12 months ago

Just a simple example of the issue plugin developers currently face.

Imagine a nice alphabetized list of choices in a drop down menu:

A
B
C
D

In version 1.1.0 of the plugin, the user selected "B", and the host remembers that by saving the index selected as 1.

Now it's been a few months and we've decided to add a great new feature "Aa" to the list for version 1.2.0. Currently, in order to preserve the user's plugin state, we would need to add this item to the bottom of the list:

A
B
C
D
Aa

That's fine, but not great for our users - they're used to traversing this particular drop-down menu in alphabetical order, and now we've sullied it! It would be much better for the list to look like:

A
Aa
B
C
D
gregcotten commented 11 months ago

A key thing to note that I didn't effectively communicate initially:

StrChoiceParam should have 2 N-string arrays:

kOfxParamPropChoiceEnum - a list of strings that could be some sort of non-user-facing internal identifier kOfxParamPropChoiceOption - a corresponding list of strings (same size as kOfxParamPropChoiceEnum) of "pretty" user-facing display name strings to select from

This allows the plugin developer to have constant implementation-detail enum identifier strings while having a pretty display name that they can change between releases for UI/UX reasons.

Guido-assim commented 11 months ago

Would it not be better to have integer identifiers instead of string identifiers? That way things can be kept backwards compatible with the current Choice parameter type. There can be an optional property to specify the identifiers to use instead of the indices. It would also save some string compares to check values. The current option list control in our software does not support saving a string value, we would have to create a new control for that.

gregcotten commented 11 months ago

Would it no be better to have integer identifiers instead of string identifiers? That way things can be kept backwards compatible with the current Choice parameter type. There can be an optional property to specify the identifiers to use instead of the indices. It would also save some string compares to check values. The current option list control in our software does not support saving a string value, we would have to create a new control for that.

While I like this idea, one of the inspirations for adding this feature as specified is to automagically add host support for a large set of plugins that are currently already using this feature in Resolve.

revisionfx commented 11 months ago

Need to define behavior for host (what to display in menu) when an old entry does not exist

gregcotten commented 11 months ago

Resolve's behavior is to display an empty string (!!!)

gregcotten commented 11 months ago

Revised proposal: Extend ParamTypeChoice with an integer key

Guido-assim commented 11 months ago

To elaborate on my previous comment: the idea is to add an optional int X N property to the existing "OfxParamTypeChoice" parameter, named something like "OfxParamPropChoiceIdentifiers" or "OfxParamPropChoiceValues". This property should have the same number of values and ordering as the "OfxParamPropChoiceOption" property. If this property is present then instead of the index in "OfxParamPropChoiceOption" the value of "OfxParamPropChoiceIdentifiers" at that index is used as the value of the option parameter. This way the options can be extended and reordered while remaining compatible with older values.

fxtech commented 11 months ago

I thought about this some more and why can't it just be an optional "order" list, the same length as the choice list, that has the order #s for the choices. Example: choices = A B D E Default order = 0 1 2 3 The host, as always, stores the index of the currently selected string choice, ie B = 1 Now, we add C to the end of the list: Choices = A B D E C And define the order list as such: 0 1 4 2 3 So the host can sort the strings by the order list and display them as A B C D E However, selecting "C" would still store its original index (4) with the script/whatever. This is perfectly backward compatible and purely optional, and quite easy for hosts to implement.

barretpj commented 11 months ago

Well it could, but I'd say that would make both the plugin and host code pretty messy. The original proposal is to use what Resolve already supports; is there any major reason not to follow that?

fxtech commented 11 months ago

I think the main issue is some hosts don't store string values, only integer indeces for choice/option parameters. (FWIW Silhouette stores choice values as strings, and uses an option ID to decouple the value from the label, so your proposal would be easy enough to add). But conceptually, the optional order list solves two problems: 1. you can add new options/reorder the list, and 2. change the labels, and it's fully backward compatible without having to potentially add a new control type, and then the extra infrastructure to query if the host even supports it.

garyo commented 10 months ago

I think Guido's proposal of the plugin providing the int value to be stored is simpler than adding a new param type. It also means hosts can still use integers as the stored value of choice params so it's probably really easy to implement. I think it's stronger than using an index-order array too.

Here's @fxtech 's example reworked using Guido's scheme: Original choices using kOfxParamPropChoiceOption: A, B, D, E OfxParamPropChoiceValue (implicitly or explicitly): 0, 1, 2, 3

Then to add C in the middle in plugin v2: kOfxParamPropChoiceOption: A, B, C, D, E OfxParamPropChoiceValue: 0, 1, 4, 2, 3

The host displays the choices in the order specified by the option property array, and the value saved/loaded in the project is the OfxParamPropChoiceValue. In this case, if the user saved a project in v1 with option D, the value will be 2 (just as it is today), and when that project is loaded with plugin v2, value 2 will be mapped to D which is now the 4th element. Then the user can modify the param value to C which will be saved as value 4.

The default value of the choice param should be interpreted as a member of the OfxParamPropChoiceValue array (i.e. look it up there and use that element).

For compatibility, hosts can assume that a plugin that doesn't specify a OfxParamPropChoiceValue array uses a default of 0, 1, 2, 3 ... . This is nice because a plugin that works fine today can add a new value in the middle tomorrow.

The only remaining question is what to do about values that are removed in a plugin version. I suggest that a host which encounters a saved value that no longer exists (either because it's off the end of an implicit value array or missing from an explicit one) should use the default value of the param. And if the default value is nonexistent, use 0. That's my idea anyway.

fxtech commented 10 months ago

For compatibility, hosts can assume that a plugin that doesn't specify a OfxParamPropChoiceValue array uses a default of 0, 1, 2, 3 ... . This is nice because a plugin that works fine today can add a new value in the middle tomorrow.

Except for hosts that don't support the OfxParamPropChoiceValue property. Once a new item is inserted in the list, the wrong options could be used.

You could also sunset obsolete values with an order list in a backward-compatible way as well - just mark ones that should not appear with an order of -1. Hosts that don't support the order list would continue to display all of the options as before, but this is surely safer than having old options just go away entirely, to be replaced with a new potentially arbitrary default.

garyo commented 10 months ago

Except for hosts that don't support the OfxParamPropChoiceValue property. Once a new item is inserted in the list, the wrong options could be used.

Yes, that's the situation today. Wrong items get used if an item is inserted in the middle. With this proposal, a plugin can detect whether OfxParamPropChoiceValue is supported by the host by trying to set it on the param. propSetInt should return kOfxStatErrUnknown if not. Is that too late?

You make an excellent point about the need to "sunset" obsolete values. Let me see if I understand your idea, @fxtech . The ChoiceOption array is as usual, and plugins should only ever append to it, never remove or reorder. But changing the strings is OK (e.g. to fix a typo or clarify meaning). The value stored by the host is the index into the ChoiceOption array. So far this is all just the same as today. You propose to add OfxParamPropChoiceOrder which is used as sort keys for the options when displaying. So the option with the lowest Order gets shown first, etc. And if Order[i] is < 0, that option is not displayed by the host. In your proposal the Order is only used transiently for ordering the display, not for storage. (So to be clear, a plugin could change order from [0, 1, 2] to [0, 100, 1000] in a new version and it would work just the same.)

And just to flesh it out, the default OfxParamPropChoiceOrder if not specified is [0, 1, 2, ...]. And it does not interact with the default value in any way.

But all that leads to a question: when a host loads a plugin with a sunsetted option as the saved value, what should it display? Reset to default? Here's a suggestion: take the negative of order[i] and use that as the 1-based index into Option to replace with. So to remove B from [A B C] and replace it with C on load, the plugin specifies order of [0 -2 1] (or [100 -2 200] or similar). There are probably other ideas here too, like another additional param.

A nice property of your version, @fxtech, is that when a plugin adds a new option C, it adds it at the end as usual, [A B D E C] and specifies Order, and a host that does support Order shows the options in the desired order A B C D E, but an old host that doesn't support Order shows it at the end, and in both cases the stored value is 4, so the plugin doesn't have to do anything special re: checking for host support. And sunsetting just does nothing on older hosts (the plugin can handle that as best it can, as it does today).

We should specify that behavior is undetermined if OfxParamPropChoiceOrder contains duplicate non-negative values (i.e. plugins shouldn't do that).

What do folks think? OfxParamPropChoiceOrder or OfxParamPropChoiceValue?

fxtech commented 10 months ago

Yes that's a good summary @garyo. What I like about this proposal is it simplifies things on both sides - it's completely backward compatible for host vendors and a plugin vendor doesn't need to check for the support at all. The use of the absolute value of negative orders is interesting - though it presumes there is a 1:1 replacement option that perfectly emulates the old behavior. I was thinking the plugin would just support the old, removed options, and the host would hide them if one wasn't being used. It could opt to show the sunsetted option in the list if it was actually used.

garyo commented 10 months ago

it presumes there is a 1:1 replacement option that perfectly emulates the old behavior

Presumably the plugin has to have some idea what it wants to do here, right? Some new option that it wants users to be upgraded to, even if not 100% compatible?

It could opt to show the sunsetted option in the list if it was actually used.

That's a reasonable option as well -- then the plugin can decide what to do at runtime. Host folks, does that seem doable?

Guido-assim commented 10 months ago

I find OfxParamPropChoiceValue less confusing than OfxParamPropChoiceOrder. @garyo Would you not need an OfxParamPropChoiceOrder of [0 -3 1] to remove B from [A B C] and replace it with C, since the negative index is 1-based (while the order is 0-based)? Maybe we can have a OfxParamPropChoiceReplace property with the old value followed by the new value to replace sunsetted values. The only advantage I see for OfxParamPropChoiceOrder is when the host does not support it because the values are still index based. I guess a host has to support OfxParamPropChoiceValueor OfxParamPropChoiceOrderwhen they want to support the new API version?

fxtech commented 10 months ago

I'm on board with @Guido-assim 's proposal for OfxParamPropChoiceValue. I'll implement this in the next Silhouette release.