From 4de7a86c22cdfe2c02016b6ea35c5baae6d29cb9 Mon Sep 17 00:00:00 2001 From: Josiah Wolf Oberholtzer Date: Tue, 26 Nov 2019 12:40:32 -0500 Subject: [PATCH 1/7] Add RFC --- rfcs/0000-extend-node-info-response.md | 104 +++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 rfcs/0000-extend-node-info-response.md diff --git a/rfcs/0000-extend-node-info-response.md b/rfcs/0000-extend-node-info-response.md new file mode 100644 index 0000000..105d0eb --- /dev/null +++ b/rfcs/0000-extend-node-info-response.md @@ -0,0 +1,104 @@ +- Title: Extended Node Info Response +- Date proposed: 2019-11-26 +- RFC PR: https://github.com/supercollider/rfcs/pull/0000 + +# Summary + +Node info responses (`/n_info ...`) should be extended to include the SynthDef +name and control names and values when the node queried is a synth. + +# Motivation + +Getting the state of the server node tree is invaluable for writing unit tests. +Comparing the actual server state against the test’s expected state, without +relying on any client side information lets us validate test assumptions +efficiently . + +But there are some gaps in functionality for querying the server node tree +state. `/n_query` doesn’t return the SynthDef name or controls if it identifies +a node as a synth. `/s_get` requires knowing the parameters of the synth’s +SynthDef in advance. + +This leaves `/g_queryTree`, which does identify the SynthDef name and its +control names and values, but can only be called against the Synth’s parent +group. Unfortunately, for large node trees (~100 nodes or more, or with +extensive SynthDef controls) the `/g_queryTree.reply` response is often +malformed; the largest OSC packet cannot hold all of the data and truncates it. +Avoiding this overflow by querying various subtrees and patching the results +together is error-prone and unreliable for the same reasons. Tt’s not possible +to query the state of a synth without potentially querying the state of its +siblings too, which runs the risk of overflowing the OSC packet. + +As a solution, if we extend `/n_query`’s `/n_info` response to include the +SynthDef name and its controls if the node in question is a synth, we can query the +entire node tree by sending a series of `/n_query` requests. This allows us to +perform the depth first traversal of the node tree on the client side and +construct its complete state without risk of error, at the cost of potentially not +capturing modifications that take place while the querying occurs. + +# Specification + +Currently `/n_info` returns the following: + +``` +int: node ID +int: the node’s parent group ID +int: previous node ID, -1 if no previous node. +int: next node ID, -1 if no next node. +int: 1 if the node is a group, 0 if it is a synth +The following two arguments are only sent if the node is a group: +int: the ID of the head node, -1 if there is no head node +int: the ID of the tail node, -1 if there is no tail node +``` + +This should be modified to: + +``` +int: node ID +int: the node’s parent group ID +int: previous node ID, -1 if no previous node. +int: next node ID, -1 if no next node. +int: 1 if the node is a group, 0 if it is a synth +The following two arguments are only sent if the node is a group: +int: the ID of the head node, -1 if there is no head node +int: the ID of the tail node, -1 if there is no tail node +The following arguments are only sent if the node is a synth: +str: the SynthDef name +int: the number of control name/value pairs +N * control number: + str: the control name + int/str: the control value or bus mapping symbol +``` + +The implementation path for scsynth is fairly simple: + +- Refactor `Group_QueryTreeAndControls()` by extracting the logic that + describes a Synth’s SynthDef name and controls into a new + `Node_QueryControls()` function. +- Add a pointer to the node to the `NodeEndMsg` struct. +- Modify `NodeEndMsg::Perform()` to use a big OSC packet rather than the + current small packet for compatibility with `Node_QueryControls()`. +- Modify `NodeEndMsg::Perform()` to write the node’s SynthDef name and controls + into its OSC packet via `Node_QueryControls()` when creating a `/n_info` + response. + +# Drawbacks + +The additional packet size and computation makes `/n_query` potentially slower. +For that reason I’ve only proposed modifying `/n_info` and not the other +automatic node notifications. + +Additionally, logic with a hard expectation of the older OSC message format for +`/n_info` responses could break with this change. A quick search of the +SuperCollider GitHub organization doesn’t show any usage of `/n_query` outside +of the `Node.query` method. Impact on third party libraries would need to be +investigated. + +# Unresolved Questions + +Should this logic be extended to all of the other node notifications (`/n_go`, +`/n_end`, `/n_off`, `/n_on`, `/n_move`)? + +Can `/g_queryTree` be fixed or modified to avoid the overflow problem? + +Is accessing a node in `NodeEndMsg::Perform()` always guaranteed to be safe? From be4c20db25ea1ce9fe8157bba0e2e6995798273f Mon Sep 17 00:00:00 2001 From: Josiah Wolf Oberholtzer Date: Tue, 26 Nov 2019 12:44:01 -0500 Subject: [PATCH 2/7] Renumber RFC --- ...-node-info-response.md => 0005-extend-node-info-response.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename rfcs/{0000-extend-node-info-response.md => 0005-extend-node-info-response.md} (98%) diff --git a/rfcs/0000-extend-node-info-response.md b/rfcs/0005-extend-node-info-response.md similarity index 98% rename from rfcs/0000-extend-node-info-response.md rename to rfcs/0005-extend-node-info-response.md index 105d0eb..118c4f6 100644 --- a/rfcs/0000-extend-node-info-response.md +++ b/rfcs/0005-extend-node-info-response.md @@ -1,6 +1,6 @@ - Title: Extended Node Info Response - Date proposed: 2019-11-26 -- RFC PR: https://github.com/supercollider/rfcs/pull/0000 +- RFC PR: https://github.com/supercollider/rfcs/pull/5 # Summary From bc90d8d6f633401f211ebdaeb7280831efec9120 Mon Sep 17 00:00:00 2001 From: Josiah Wolf Oberholtzer Date: Tue, 26 Nov 2019 13:19:23 -0500 Subject: [PATCH 3/7] Fix typo --- rfcs/0005-extend-node-info-response.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0005-extend-node-info-response.md b/rfcs/0005-extend-node-info-response.md index 118c4f6..723b14e 100644 --- a/rfcs/0005-extend-node-info-response.md +++ b/rfcs/0005-extend-node-info-response.md @@ -25,7 +25,7 @@ group. Unfortunately, for large node trees (~100 nodes or more, or with extensive SynthDef controls) the `/g_queryTree.reply` response is often malformed; the largest OSC packet cannot hold all of the data and truncates it. Avoiding this overflow by querying various subtrees and patching the results -together is error-prone and unreliable for the same reasons. Tt’s not possible +together is error-prone and unreliable for the same reasons. It’s not possible to query the state of a synth without potentially querying the state of its siblings too, which runs the risk of overflowing the OSC packet. From f5d7e981a44ba1483b5eb8458adf8baf99f4bd06 Mon Sep 17 00:00:00 2001 From: Josiah Wolf Oberholtzer Date: Wed, 27 Nov 2019 13:23:06 -0500 Subject: [PATCH 4/7] Address extending non-/n_info notifications --- rfcs/0005-extend-node-info-response.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/rfcs/0005-extend-node-info-response.md b/rfcs/0005-extend-node-info-response.md index 723b14e..664264f 100644 --- a/rfcs/0005-extend-node-info-response.md +++ b/rfcs/0005-extend-node-info-response.md @@ -96,9 +96,15 @@ investigated. # Unresolved Questions -Should this logic be extended to all of the other node notifications (`/n_go`, -`/n_end`, `/n_off`, `/n_on`, `/n_move`)? +- Can `/g_queryTree` be fixed or modified to avoid the overflow problem? -Can `/g_queryTree` be fixed or modified to avoid the overflow problem? +- Is accessing a node in `NodeEndMsg::Perform()` always guaranteed to be safe? + +# Resolved Questions + +- Should this logic be extended to all of the other node notifications (`/n_go`, + `/n_end`, `/n_off`, `/n_on`, `/n_move`)? + + No. These should remain as streamlined as possible. For complex server setups + the additional traffic can be detrimental to performance. -Is accessing a node in `NodeEndMsg::Perform()` always guaranteed to be safe? From 2ad679e25644b4c633a7bae489ea00875cabab9e Mon Sep 17 00:00:00 2001 From: Josiah Wolf Oberholtzer Date: Tue, 3 Dec 2019 12:10:07 -0500 Subject: [PATCH 5/7] Draft 2 --- rfcs/0005-extend-node-info-response.md | 110 ------------------------- rfcs/0005-synth-query-command.md | 72 ++++++++++++++++ 2 files changed, 72 insertions(+), 110 deletions(-) delete mode 100644 rfcs/0005-extend-node-info-response.md create mode 100644 rfcs/0005-synth-query-command.md diff --git a/rfcs/0005-extend-node-info-response.md b/rfcs/0005-extend-node-info-response.md deleted file mode 100644 index 664264f..0000000 --- a/rfcs/0005-extend-node-info-response.md +++ /dev/null @@ -1,110 +0,0 @@ -- Title: Extended Node Info Response -- Date proposed: 2019-11-26 -- RFC PR: https://github.com/supercollider/rfcs/pull/5 - -# Summary - -Node info responses (`/n_info ...`) should be extended to include the SynthDef -name and control names and values when the node queried is a synth. - -# Motivation - -Getting the state of the server node tree is invaluable for writing unit tests. -Comparing the actual server state against the test’s expected state, without -relying on any client side information lets us validate test assumptions -efficiently . - -But there are some gaps in functionality for querying the server node tree -state. `/n_query` doesn’t return the SynthDef name or controls if it identifies -a node as a synth. `/s_get` requires knowing the parameters of the synth’s -SynthDef in advance. - -This leaves `/g_queryTree`, which does identify the SynthDef name and its -control names and values, but can only be called against the Synth’s parent -group. Unfortunately, for large node trees (~100 nodes or more, or with -extensive SynthDef controls) the `/g_queryTree.reply` response is often -malformed; the largest OSC packet cannot hold all of the data and truncates it. -Avoiding this overflow by querying various subtrees and patching the results -together is error-prone and unreliable for the same reasons. It’s not possible -to query the state of a synth without potentially querying the state of its -siblings too, which runs the risk of overflowing the OSC packet. - -As a solution, if we extend `/n_query`’s `/n_info` response to include the -SynthDef name and its controls if the node in question is a synth, we can query the -entire node tree by sending a series of `/n_query` requests. This allows us to -perform the depth first traversal of the node tree on the client side and -construct its complete state without risk of error, at the cost of potentially not -capturing modifications that take place while the querying occurs. - -# Specification - -Currently `/n_info` returns the following: - -``` -int: node ID -int: the node’s parent group ID -int: previous node ID, -1 if no previous node. -int: next node ID, -1 if no next node. -int: 1 if the node is a group, 0 if it is a synth -The following two arguments are only sent if the node is a group: -int: the ID of the head node, -1 if there is no head node -int: the ID of the tail node, -1 if there is no tail node -``` - -This should be modified to: - -``` -int: node ID -int: the node’s parent group ID -int: previous node ID, -1 if no previous node. -int: next node ID, -1 if no next node. -int: 1 if the node is a group, 0 if it is a synth -The following two arguments are only sent if the node is a group: -int: the ID of the head node, -1 if there is no head node -int: the ID of the tail node, -1 if there is no tail node -The following arguments are only sent if the node is a synth: -str: the SynthDef name -int: the number of control name/value pairs -N * control number: - str: the control name - int/str: the control value or bus mapping symbol -``` - -The implementation path for scsynth is fairly simple: - -- Refactor `Group_QueryTreeAndControls()` by extracting the logic that - describes a Synth’s SynthDef name and controls into a new - `Node_QueryControls()` function. -- Add a pointer to the node to the `NodeEndMsg` struct. -- Modify `NodeEndMsg::Perform()` to use a big OSC packet rather than the - current small packet for compatibility with `Node_QueryControls()`. -- Modify `NodeEndMsg::Perform()` to write the node’s SynthDef name and controls - into its OSC packet via `Node_QueryControls()` when creating a `/n_info` - response. - -# Drawbacks - -The additional packet size and computation makes `/n_query` potentially slower. -For that reason I’ve only proposed modifying `/n_info` and not the other -automatic node notifications. - -Additionally, logic with a hard expectation of the older OSC message format for -`/n_info` responses could break with this change. A quick search of the -SuperCollider GitHub organization doesn’t show any usage of `/n_query` outside -of the `Node.query` method. Impact on third party libraries would need to be -investigated. - -# Unresolved Questions - -- Can `/g_queryTree` be fixed or modified to avoid the overflow problem? - -- Is accessing a node in `NodeEndMsg::Perform()` always guaranteed to be safe? - -# Resolved Questions - -- Should this logic be extended to all of the other node notifications (`/n_go`, - `/n_end`, `/n_off`, `/n_on`, `/n_move`)? - - No. These should remain as streamlined as possible. For complex server setups - the additional traffic can be detrimental to performance. - diff --git a/rfcs/0005-synth-query-command.md b/rfcs/0005-synth-query-command.md new file mode 100644 index 0000000..5afadee --- /dev/null +++ b/rfcs/0005-synth-query-command.md @@ -0,0 +1,72 @@ +- Title: Synth Query Command +- Date proposed: 2019-11-26 +- RFC PR: https://github.com/supercollider/rfcs/pull/5 +- Draft 2 + +# Summary + +We should introduct a new command, `/s_query`, to allow retrieving both the +SynthDef name and the names and current values of a Synth. + +# Motivation + +Getting the state of the server node tree is invaluable for writing unit tests. +Comparing the actual server state against the test’s expected state, without +relying on any client side information lets us validate test assumptions +efficiently . + +But there are some gaps in functionality for querying the server node tree +state. `/n_query` doesn’t return the SynthDef name or controls if it identifies +a node as a synth. `/s_get` requires knowing the parameters of the synth’s +SynthDef in advance. This leaves `/g_queryTree`, which does identify the +SynthDef name and its control names and values, but can only be called against +the Synth’s parent group. + +Unfortunately, for large node trees (~100 nodes or more, or with +extensive SynthDef controls) the `/g_queryTree.reply` response is often +malformed; the largest OSC packet cannot hold all of the data and truncates it. +Avoiding this overflow by querying various subtrees and patching the results +together is error-prone and unreliable for the same reasons. It’s not possible +to query the state of a synth without potentially querying the state of its +siblings too, which runs the risk of overflowing the OSC packet. + +As a solution, we can add a new `/s_query` command to return the SynthDef name, +control names, and current values. This allows us to perform the depth-first +traversal of the node tree manually on the client-side. Using a combination of +`/n_query` and `/s_query` commands, we can construct the equivalent of what +`/g_queryTree` provides without risk of error, at the cost of some speed and +potentially not capturing modifications that take place while the querying +occurs. That trade-off is acceptable for the purposes of unit testing. + +# Specification + +The new `/s_query` command should return a `/s_query.reply` response with the +following format. + +``` +int: node ID +str: the SynthDef name +int: the number of control name/value pairs +N * control number: + str: the control name + int/str: the control value or bus mapping symbol +``` + +The response need only be sent to the originating client, unlike `/n_info` +which is broadcast to all clients. + +Some existing server-side `/g_queryTree` logic can be refactored to support the +new command. + +# Drawbacks + +Simply modifying `/n_info` to provide the same additional information as the proposed +`/s_query.reply` would be simpler to implement. However, in the interest of +reducing risk of client breakage, adding a new command is always going to be +safer than modifying a pre-existing one. + +The server command reference will need to be updated to clearly advertise the +version at which `/s_query` became available. + +Adding `/s_query` does nothing to address the inherent problems with calling +`/g_queryTree` against large node trees. From b4b13473684bfc3d634790bc64c43b4363b8e954 Mon Sep 17 00:00:00 2001 From: Josiah Oberholtzer Date: Sat, 28 Nov 2020 17:14:19 -0500 Subject: [PATCH 6/7] Rename /s_query.reply to /s_info --- rfcs/0005-synth-query-command.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rfcs/0005-synth-query-command.md b/rfcs/0005-synth-query-command.md index 5afadee..4eb4b3c 100644 --- a/rfcs/0005-synth-query-command.md +++ b/rfcs/0005-synth-query-command.md @@ -40,7 +40,7 @@ occurs. That trade-off is acceptable for the purposes of unit testing. # Specification -The new `/s_query` command should return a `/s_query.reply` response with the +The new `/s_query` command should return a `/s_info` response with the following format. ``` @@ -52,6 +52,9 @@ N * control number: int/str: the control value or bus mapping symbol ``` +The `/s_query` / `/s_info` naming matches the existing `/n_query` / `n_info` +and `/b_query` / `/b_info` requests and responses. + The response need only be sent to the originating client, unlike `/n_info` which is broadcast to all clients. From 3369e02c63d3da35fba17c35324762b2c08f3756 Mon Sep 17 00:00:00 2001 From: Josiah Oberholtzer Date: Tue, 1 Dec 2020 16:35:20 -0500 Subject: [PATCH 7/7] Fix typo --- rfcs/0005-synth-query-command.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0005-synth-query-command.md b/rfcs/0005-synth-query-command.md index 4eb4b3c..08b1e68 100644 --- a/rfcs/0005-synth-query-command.md +++ b/rfcs/0005-synth-query-command.md @@ -5,7 +5,7 @@ # Summary -We should introduct a new command, `/s_query`, to allow retrieving both the +We should introduce a new command, `/s_query`, to allow retrieving both the SynthDef name and the names and current values of a Synth. # Motivation