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

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

Closed
gregcotten opened this issue Oct 6, 2023 · 21 comments · Fixed by #133
Closed

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

gregcotten opened this issue Oct 6, 2023 · 21 comments · Fixed by #133

Comments

@gregcotten
Copy link

gregcotten commented Oct 6, 2023

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

fxtech commented Oct 7, 2023 via email

@gregcotten
Copy link
Author

Not in the Resolve extension, but obviously could add!

@fxtech
Copy link
Contributor

fxtech commented Oct 7, 2023 via email

@gregcotten
Copy link
Author

gregcotten commented Oct 8, 2023

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

gregcotten commented Oct 10, 2023

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.

@gregcotten gregcotten changed the title Add Ordered-Dictionary Style Choice Param Add OfxParamTypeStrChoice, a string-backed enum of choices Oct 10, 2023
@Guido-assim
Copy link

Guido-assim commented Oct 11, 2023

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

gregcotten commented Oct 11, 2023

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

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

@gregcotten
Copy link
Author

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

@gregcotten
Copy link
Author

Revised proposal: Extend ParamTypeChoice with an integer key

@Guido-assim
Copy link

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

fxtech commented Nov 8, 2023

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

barretpj commented Nov 8, 2023

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

fxtech commented Nov 8, 2023

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

garyo commented Nov 9, 2023

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

fxtech commented Nov 9, 2023

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

garyo commented Nov 9, 2023

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

fxtech commented Nov 9, 2023

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

garyo commented Nov 9, 2023

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

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

fxtech commented Dec 5, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants