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 kOfxParamPropChoiceOrder #133

Merged
merged 23 commits into from
Mar 11, 2024
Merged

Add kOfxParamPropChoiceOrder #133

merged 23 commits into from
Mar 11, 2024

Conversation

garyo
Copy link
Contributor

@garyo garyo commented Dec 6, 2023

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

I implemented this for the next release of Assimilate SCRATCH.

@fxtech
Copy link
Contributor

fxtech commented Dec 7, 2023

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

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.

Signed-off-by: Gary Oberbrunner <[email protected]>
@garyo garyo force-pushed the prop-choice-value branch from 1033b76 to f831bbc Compare March 2, 2024 16:39
@Guido-assim
Copy link
Contributor

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

garyo commented Mar 2, 2024

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

  • we had agreed on ChoiceValue but that turned out to be hard to implement
  • ChoiceOrder is better; Guido did both that and ChoiceValue, and also StrChoice
    • recommend: ChoiceOrder, but also StrChoice
  • implemented Order (easy) and also StrChoice
  • Gary to add 3 choice params to one of the examples: old, order, strchoice (all the same content)
  • Need query to see if host supports StrChoice?
    • One option is to try to create a param, it'll fail with a particular error if StrChoice is missing.
    • Guido will send to the group the property name to check for presence of StrChoice, then Gary will add to the spec
  • Note: Resolve plugins use this already. So we should require StrChoice in 1.5, and say order is optional.
  • Gary to update spec and create an example

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

garyo added 4 commits March 2, 2024 15:19
Update the doc to describe the operation of the new property.

Signed-off-by: Gary Oberbrunner <[email protected]>
This code was contributed by BlackMagic.

Signed-off-by: Gary Oberbrunner <[email protected]>
@garyo garyo marked this pull request as ready for review March 2, 2024 22:17
@garyo garyo requested review from fxtech and Guido-assim March 2, 2024 22:18
garyo added 10 commits March 3, 2024 10:00
- Add INSTALLDIR CMake cache var
- Use bash getopts to parse args in `scripts/build-cmake.sh`

Signed-off-by: Gary Oberbrunner <[email protected]>
This way the plugin artifact can be unzipped directly into the OFX
plugins folder, because the plugins have the proper bundle structure.

I removed the "*.ofx" files from the include/lib artifact because
they're not really useful as is, without the proper bundle structure.

Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
The CI build issue was an "unsafe" git dir. actions/checkout@v4 is
supposed to fix that; see comments in
actions/runner#2033.

Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Copy link
Contributor

@Guido-assim Guido-assim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Guido-assim Guido-assim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@garyo
Copy link
Contributor Author

garyo commented Mar 4, 2024

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

garyo commented Mar 5, 2024

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

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

garyo commented Mar 5, 2024

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.

@garyo garyo changed the title Add kOfxParamPropChoiceValue Add kOfxParamPropChoiceOrder Mar 5, 2024
@Guido-assim
Copy link
Contributor

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

@Guido-assim Guido-assim closed this Mar 6, 2024
@garyo
Copy link
Contributor Author

garyo commented Mar 6, 2024

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

garyo commented Mar 6, 2024

I'll reopen this for further discussion.

@garyo garyo reopened this Mar 6, 2024
Also add Order support for StrChoice

Still needs work: add to support lib

Signed-off-by: Gary Oberbrunner <[email protected]>
@Guido-assim
Copy link
Contributor

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.

Update StrChoice support example to use this, and also check for
StrChoice support before creating/using the param.

Signed-off-by: Gary Oberbrunner <[email protected]>
@garyo
Copy link
Contributor Author

garyo commented Mar 6, 2024

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

This turned out to be complicated for some hosts, so we will not
mandate it in the spec. The spec now recommends plugins only use
non-negative values for Order, thus preserving the option for hosts to
hide negative values if they can.

Signed-off-by: Gary Oberbrunner <[email protected]>
@garyo
Copy link
Contributor Author

garyo commented Mar 9, 2024

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.

@garyo garyo merged commit 60262ef into main Mar 11, 2024
9 checks passed
@garyo garyo deleted the prop-choice-value branch March 11, 2024 11:41
@barretpj
Copy link
Contributor

See issue #146

garyo added a commit that referenced this pull request Mar 12, 2024
* Set defaults for StrChoice params in examples
* Update doc to say that Choice and StrChoice params should use first
  defined option if the plugin doesn't specify a default
* Fix plugin grouping for Support/Plugins/ChoiceParams

See also PR #133.

Signed-off-by: Gary Oberbrunner <[email protected]>
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.

Add OfxParamTypeStrChoice, a string-backed enum of choices
4 participants