8000 Improved device parameter support by steeltrack · Pull Request #114 · ideoforms/AbletonOSC · GitHub
[go: up one dir, main page]

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

Improved device parameter support #114

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

steeltrack
Copy link
Contributor
  • Added individual parameter listeners
  • Added new endpoint for getting a human readable value string for a parameter value (ex: "2500Hz", "Lowpass")
  • Added device selector endpoint (handy for "focusing" onto a device from a controller)
  • Updated README
  • Note: I didn't add an automated test because the guidelines call out using the default Ableton project, but the default doesn't include any devices to test with.

Copy link
Owner
@ideoforms ideoforms left a comment

Choose a reason for hiding this comment

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

Nice additions, thanks @steeltrack! Added a few comments inline.

One reservation is that the parameter value listener results in two responses for every query (one for value and one for value_string), which will double the network traffic for what might already be quite high-volume messages. Have you seen any issues with performance with this?

The alternative would be to have separate start_listen endpoints for value and value_string, which would be more economical, and consistent with the rest of the AbletonOSC API (in which one listener generates one response). I'd prefer this on balance, as users will only want either the value or the value_string, and it's better to minimise traffic where possible as I want to make sure this library can support large sets with a lot of activity.

@steeltrack
Copy link
Contributor Author

@ideoforms Thanks for all of the feedback. I've addressed the inline comments in my latest commit.

To your larger point, str_for_value is a conversion method, not a stored value. Therefore, human readable parameter value strings have no listeners of their own. This is why I grouped both values into one listener even though it increases the amount of network traffic. One alternative would be exposing the conversion method as its own OSC endpoint, but then you have the network traffic of sending the float value and getting the string response.

For what it's worth, I didn't notice performance issues. Also, I somewhat disagree with this point: "users will only want either the value or the value_string."

In order to replicate the Live control below, you must have the parameter float values to transact with the API, which you can then convert to a human-readable string. However, that human-readable string is not actionable (ie: it can't be sent back to the API to change a parameter).

Screenshot 2024-01-16 at 9 00 38 AM

Let me know your thoughts. Thanks again for the thorough review!

@ideoforms
Copy link
Owner

All of that sounds reasonable, and changes look good. Thanks, merging now!

@ideoforms ideoforms merged commit eb1dccd into ideoforms:master Jan 16, 2024
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.

2 participants
0