-
Notifications
You must be signed in to change notification settings - Fork 108
[feature] Chain and Chain Device Implementation #170
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
base: master
Are you sure you want to change the base?
[feature] Chain and Chain Device Implementation #170
Conversation
|
|
||
| return rack_callback | ||
|
|
||
| def create_chain_callback(func, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have chain callbacks (/live/chain/) which refer to the properties of the chain itself and chain device callbacks (/live/chain/device/`) which refer to properties of devices within the chain.
Chain devices are different than top level devices because I think we want to access them inside the chain. This allows us to ask for and modify devices inside a chain.
is it okay that they are all in the same file? or would you like the chain device methods to be separated from the chain methods?
| def chain_get_device_types(chain, params: Tuple[Any] = ()): | ||
| return tuple(device.type for device in chain.devices) | ||
|
|
||
| self.osc_server.add_handler("/live/device/get/num_chains", create_rack_callback(rack_device_get_num_chains)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this /live/device/get/num_chains endpoint in here since it is related to chains, but every other endpoint in here is /live/chain/....
Should I move this to live with the other device endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chains are part of devices so it almost feels like you want to do /live/device/chain/... as your namespace to better represent the hierarchy.
| for chain_device_index, chain_device in enumerate(chain.devices): | ||
| chain_device_data = serialize_device(chain_device) | ||
| chain_device_data["index"] = chain_device_index | ||
| chain_data["devices"].append(chain_device_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User can have a lot of racks+devices+parameters, which would make the endpoint slower and the output file larger.
should we have a parameter that the user can conditionally set if they don't want rack info in song structure?
| chain_methods = [ | ||
| ] | ||
| chain_properties_r = [ | ||
| "has_audio_input", | ||
| "has_audio_output", | ||
| "has_midi_input", | ||
| "has_midi_output", | ||
| "is_auto_colored", | ||
| "muted_via_solo", | ||
| "color_index" | ||
| ] | ||
| chain_properties_rw = [ | ||
| "name", | ||
| "color", | ||
| "mute", | ||
| "solo" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: this could all be in constants because it doesn't change from invocation to invocation. Negligible performance impact. Minor memory impact on startup if you have a lot of device chains.
Description
Please include a summary of the change, and the motivation. If it closes one or more open issues, please tag them as described here.
Checklist
This project is run by a single unpaid maintainer, and long PRs containing a lot of AI-generated content are complex and time-consuming to review. With this in mind, please proof-read all content carefully before submitting, and ensure that surplus files (additional documentation, test procedures, etc) are not checked in before submitting the PR.