AcademySoftwareFoundation / openfx

OpenFX effects API
Other
419 stars 122 forks source link

Add kOfxParamPropChoiceOrder #133

Closed garyo closed 8 months ago

garyo commented 11 months ago

Draft, including documentation. @fxtech @gregcotten @Guido-assim @revisionfx : please try this prop-choice-value branch and submit any comments or changes to this PR.

Closes #129.

Guido-assim commented 11 months ago

I implemented this for the next release of Assimilate SCRATCH.

fxtech commented 11 months ago

I implemented this for the next release of Assimilate SCRATCH.

I realized that once this is implemented in a host, if there is a plugin with a value list, the host will load the wrong choice since the value will now map to a different option. This breaks compatibility.

If the value list is interpreted as UI only then this isn't an issue. Which way did you end up implementing it?

Guido-assim commented 11 months ago

I implemented this for the next release of Assimilate SCRATCH.

I realized that once this is implemented in a host, if there is a plugin with a value list, the host will load the wrong choice since the value will now map to a different option. This breaks compatibility.

If the value list is interpreted as UI only then this isn't an issue. Which way did you end up implementing it?

Why would the host load the wrong choice? The mapping of choice labels to values is up to the plugin. Our controls have the option to assign an integer identifier to use as the value instead of an index, for these identifiers I now use the values from OfxParamPropChoiceValue. Te be compatible with previous version a plugin has to use the same values as the previous indexes.

Guido-assim commented 8 months ago

I don't mind the kOfxParamPropChoiceValue, I still have it in our code, but for some the backward compatibility issues are too big. I believe we decided to go with kOfxParamPropChoiceOrder and kOfxParamTypeStrChoice during the TSC meeting.
@garyo @fxtech

garyo commented 8 months ago

Agreed, @Guido-assim. Here's what I plan to do this weekend (directly from the meeting minutes):

We will use kOfxParamHostPropSupportsStrChoice for the host to tell plugins if it supports StrChoice.

garyo commented 8 months ago

The Green Choice Param seems not to behave as expected. The "Green: lots" option has index 1 (and order 2) which results in a scale of g = 0.5; The "Green: some" option has index 2 (and order 1) which results in a scale of g = 1.0;

It's entirely possible my code is messed up. I didn't have a host that supports Order to test with. Feel free to post a patch & I'll apply it.

garyo commented 8 months ago

For kOfxParamPropChoiceOrder option items with a negative order value are excluded from the option list in our code. That is not reflected in this commit, see the example where -100 is used for the order value (ofParameter.rst line 218). We need to make sure what the behavior should be.

What do you do in your host when you load a project with a ChoiceParam option where the saved index is hidden? What do you show in the UI? Do you expose the hidden value or what? What should the spec require or suggest in this case?

Guido-assim commented 8 months ago

For kOfxParamPropChoiceOrder option items with a negative order value are excluded from the option list in our code. That is not reflected in this commit, see the example where -100 is used for the order value (ofParameter.rst line 218). We need to make sure what the behavior should be.

What do you do in your host when you load a project with a ChoiceParam option where the saved index is hidden? What do you show in the UI? Do you expose the hidden value or what? What should the spec require or suggest in this case?

If the saved index is hidden then we use the default value, the UI would show the current (default) value. The behavior is the same as when a saved option is not present anymore with OfxParamTypeStrChoice. My suggestion would be to use the default value but this may be different for other hosts so not necessarily a requirement.

garyo commented 8 months ago

If the saved index is hidden then we use the default value, the UI would show the current (default) value.

I see -- I was expecting you'd load the hidden value and it would be up to the plugin to modify it, but that would present UI challenges, so I see why you're basically resetting the param to default. Let's discuss at the meeting.

Guido-assim commented 8 months ago

If the saved index is hidden then we use the default value, the UI would show the current (default) value.

I see -- I was expecting you'd load the hidden value and it would be up to the plugin to modify it, but that would present UI challenges, so I see why you're basically resetting the param to default. Let's discuss at the meeting.

After discussion about the hide/unhide during the TSC meeting I had a look at our code and hide/unhide would be difficult to implement

garyo commented 8 months ago

Did you close this PR deliberately? I think we should still move forward with it. I added some language per the discussion in the meeting:

+If an Order value is negative, the host _should_ hide the
+corresponding option in the UI. This can be useful for a plugin to
+deprecate certain options. When a host loads a project which includes
+a hidden option, the host _should_ show that option in that effect
+instance. If a host cannot dynamically hide or show options, it may
+instead show hidden options (with negative Order values) as grayed out
+or inactive. A plugin should not set the choice param's default value
+to a hidden option.
garyo commented 8 months ago

I'll reopen this for further discussion.

Guido-assim commented 8 months ago

Did you close this PR deliberately? I think we should still move forward with it. I added some language per the discussion in the meeting:

+If an Order value is negative, the host _should_ hide the
+corresponding option in the UI. This can be useful for a plugin to
+deprecate certain options. When a host loads a project which includes
+a hidden option, the host _should_ show that option in that effect
+instance. If a host cannot dynamically hide or show options, it may
+instead show hidden options (with negative Order values) as grayed out
+or inactive. A plugin should not set the choice param's default value
+to a hidden option.

The closing was by accident. The hide/unhide thing is not possible in our software without some major work on our option control. Grayed out could be possible but then the question is what the order should be for those items, have them at the end of the list? For now I would still like to have to option to consider those items to be removed and fall back to the default value.

garyo commented 8 months ago

OK, probably best to discuss on Slack then. I'll start.

garyo commented 8 months ago

Adjusted doc per Slack discussion to remove requirement for hosts to hide negative orders, and removed negative orders in example plugins. This should be ready to merge now; please review.

barretpj commented 8 months ago

See issue #146