osc: Allow enabling parameter value feedback for all plugins #901
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, this happend in _sel_plugin when the selected plugin
changed (which was called at the end of _strip_select2 too). By moving
this code:
1. The list of plugins is only updated when the strip changes, and not
when just the selected plugin changes (in which the case the
selected strip and list of plugins remains the same, so this saves
unneeded work).
2. The list of plugins is updated earlier, which allows it to be used
by OSCSelectObserver (in the next commit).
3. The list of plugins is cleared when no strip is selected.
There are two ways to identify a plugin:
1. The user-visible number, usually named "piid". This is a 1-based
index into the list of visible, non-channelstrip plugins only.
2. The plugin id. This is a 1-based index into the list of all plugins as passed to nth_plugin
The sur->plugins vector is used to translate the former into the latter.
Previously, the latter, already translated, index was passed to
OSCSelectObserver to indicate the currently selected plugin. To prepare
for sending the piid of the selected plugin in a feedback message (in
the next commit), this commit moves the translation of piid to plugin id
into OSCSelectObserver.
To emphasize the new meaning of the plug_id variable, it is renamed to
selected_piid. Also, since the piid is 1-based, change it to a uint32_t
and use 0 to mean "no plugin selected" instead of -1.
This commit is expected to subtly change behaviour: When selecting
a different strip, OSCSelectObserver would (via OSC::_strip_select2 and
refresh_strip) send feedback for the plugin with the same plugin id as
the previously selected plugin. Then OSC::_strip_select2 would do the
piid to plugin id translation for the new strip and call set_plugin,
causing a second round of feedback for the plugin with the same piid as
the previously selected plugin.
If the new strip has hidden plugins (i.e. piid and plugin id do not
match), this could mean that you get feedback for two different plugins.
With this commit applied, the translation happens in both cases and the
feedback returned should be for the same plugin twice.
Whenever the active plugin changes (because the plugin changes or the selected strip changes), the name, some other properties and parameter values for the newly selected plugin is sent as feedback. However, there was no clear indication of which plugin was selected exactly. Applications or surfaces that need to know the active plugin could track the currently selected plugin by remembering the most recently selected index, but unexpected plugin changes (e.g. when switching to a strip with fewer plugins) and the asynchronous nature of OSC messages make this more complicated than it should be. By sending the active plugin id as feedback, clients do not have to guess. When no plugin is selected (e.g. on startup or when no strip is selected) or the currently selected index is not a plugin (but a some other kind of processor), a value of 0 is sent. This uses the `/select/plugin` message, which is also used to change the selected plugin. There is a slight discrepancy: the control messages accepts a delta on the selected plugin, while the feedback sends an absolute value.
Before this commit, OSCSelectObserver would read the feedback value when it was created, but then never update it anymore. In practice, the OSCSelectObserver is created on startup, and when the surface connects and configures feeback, this value is not applied. For example, when sending `/set_surface` with a feedback value of 4 (Send SSID as path extension), `/strip/*` would get their ssid put into the path, but `/select/plugin/*` messages would not have their parameter id in the path. When setting the corresponding checkbox in the default feedback preferences, it is applied as expected. This commit passes the new feedback value to the OSCSelectObserver instance whenever it changes, which ensures the value is applied as expected.
When handling the `/set_surface` command, the code would set plug_page_size to the new value first, and call `sel_plug_pagesize()` later. The latter then sees the page size is already the same, so it leaves it unchanged and also does not send the page size to the OSCSelectObserver object. In practice, this means that only the default plugin page size from the preferences or set with `/set_surface/plugin_page_size` take effect and values set with `/set_surface` are ignored. Exactly the same thing happens for the send page size. This code has been like this since it was first introduced in comit 9c0f6ea (OSC: Allow set_surface to set send and plugin page sizes., 2017-06-13) This commit fixes this by omitting the first assignment.
This is only set to 0 and never read. In plugin_init(), a value is calculated for this variable, but because it declares a local variable with the same name, the value is never assigned. And since the value is only ever used inside plugin_init(), there is no need to keep the instance variable. This prepares the code for sending feedback for multiple plugins instead of just the selected one.
It is filled and used only in init_plugin(), so it can just be a local variable. This prepares the code for sending feedback for multiple plugins instead of just the selected one.
This makes two changes, in the case where parameter paging is disabled
(plug_page_size == 0):
1. When switching to a plugin with fewer parameters, generate zero
values for the no-longer-valid parameters.
2. Any current page offset is ignored.
When parameter paging is enabled, behavior is unchanged.
In addition to the above improvements, this also prepares the code for
sending feedback for multiple plugins instead of just the selected one.
Previously, a list of input parameters was made by _sel_plugin for the currently selected plugin. Now, it happens whenever the current strip is changed for all plugins in the current strip. This prepares for supporting plugin parameter feedback for all plugins instead of just a single one in an upcoming commit. This might result in a little less or a little more work, depending on the usage pattern (changing strips is more work, changing plugins and back is less work).
This makes it a bit more consistent and easier to distingish paid (parameter id) and piid (plugin id).
This is a refactor that does not change behavior, but prepares the code for sending feedback for multiple plugins instead of just the selected one.
Currently, feedback is only sent about parameter for a single, selected plugin. By setting this new "all_plugins" bit in the `/set_surface` message (or preferences), behaviour is changed to provide feedback for all plugins in the currently selected strip. If enabled, this bit changes the format of the `/select/plugin/*` messages by adding an extra (first) argument containing the plugin id (piid). This matches the format that is accepted by the `/select/plugin/parameter` message already (which allows the piid to be specified or omitted). The code that tracks the selected plugin is still active even when the all_plugins bit is set, but the current plugin is not used (i.e. the normal messages without the piid are not sent anymore to prevent confusion about the meaning of the messages). Plugin (parameter) paging is still used and applies to all plugins at the same time. If the "send ssid as path" bit is set, this extra piid argument is also put into the path. Since the existing `*_message_with_id` helper functions only support a single id argument and we needed two, an extra local helper function (`plugin_parameter_message`) is introduced that conveniently handles all variants of these messages needed (ids as argument or path, including a piid or not, including a paid or not, with string, float or int value). This results in a long function calls and duplicates a bit of code from the `*_message_with_id` helper functions, but the alternative of adding `*_message_with_two_ids` (or something) helper functions would probably end up with more duplicate code and more complex calls, so this is probably the cleanest approach. This function also includes a usleep call which was copied from the `*_message_with_id` helper functions verbatim. Without it, some messages seem to be dropped. This has a bit of code smell (race condition?), but that is out of scope for this commit. To clear parameter values that are not longer present (i.e. when switching to a plugin with fewer parameters), the existing `plug_size` variable (that tracked the number of parameters sent on the previous update) is generalized and renamed to a `params_sent` vector, that is index by piid and contains the number of parameters sent for each plugin. Element 0 is used for the selected plugin when the all_plugins feedback bit is not set, or unused when the bit is set.
matthijskooijman
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, feedback is only sent about parameter for a single, selected plugin. By setting this new "all_plugins" bit in the
/set_surfacemessage (or preferences), behaviour is changed to provide feedback for all plugins in the currently selected strip.If enabled, this bit changes the format of the
/select/plugin/*messages by adding an extra (first) argument containing the plugin id (piid). This matches the format that is accepted by the/select/plugin/parametermessage already (which allows the piid to be specified or omitted).The main change is in the last commit, the commits leading up to that are small refactorings to simplify reviewing the big change. The first five commits are the same is #897 and #900, so should probably be ignored here and reviewed in those PRs (I'll rebase once those are merged).
I'll also update the documentation, once Ardour/manual#266 is merged.