Synth Query Command

[josiah-wolf-oberholtzer]

RFC text: https://github.com/josiah-wolf-oberholtzer/rfcs/blob/master/rfcs/0005-synth-query-command.md

[shimpe]

if breaking backward compatibility turns out to be a risk, perhaps a new message can be added like /n_extinfo, which can then safely return the extended data structure

[josiah-wolf-oberholtzer]

@shimpe Agree. If it works universally for groups and synths, I'd be happy.

[muellmusik]

AFAICS risk of compatibility issues is very low. The only case I could imagine is if a receiver checked method length to see if it the message refers to a group, instead of the explicit flag. That would be pretty weird. Or I suppose some side effect of iterating over the whole message, but again seems unlikely.

So unless there are other specific concerns, I would say the small risk of an easily fixable breakage is preferable to the the long term bloat of adding a new OSC message with largely redundant functionality.

[Sciss]

As always, I'm generally strongly against changing the OSC interface. A client will break if it expects a defined number of arguments, and often clients are optimised for performance and don't do unnecessary checks for alternative longer messages.

In the case ScalaCollider, it would in fact tolerate the changed API, but this is not necessary true for other clients in the wild.

Given that a well informed client generally doesn't need to run n_query, but can rely on its memory of n_go, n_move etc. messages, the risk may indeed by relatively low. But:

Should this logic be extended to all of the other node notifications (/n_go, /n_end, /n_off, /n_on, /n_move)?

I would strongly advise against this data pollution. There is absolutely no need to send additional information from these reply messages, it just increases OSC band width. If consensus is to make the change, please constrain it to n_query replies.

[josiah-wolf-oberholtzer]

@Sciss I'll try to take a look at some of the common non-sclang clients and report back.

Agree about not adding the information to the other notifications, but wanted to pose the question so others could confirm. The only compelling reason for providing the additional information in /n_go is when running multi-user servers, for the purpose of maintaining a client-side model of the server state, but I have a hard time imagining that being very important if you can query the state on demand.

[Sciss]

The only compelling reason for providing the additional information in /n_go is when running multi-user servers, for the purpose of maintaining a client-side model of the server state

I think in a multi-client situation, a node N on the server created by client A is opaque to client B, unless the client model says something about this situation, and then it's a requirement of client A to communicate that to client B. The server should always be considered a maximally "dumb" instance, and information should in general flow from client to server, not vice versa. Otherwise you end up in a lot of complexity. In general (I think) a client model for any non-trivial system will always be more complex than the server's view of it.

[josiah-wolf-oberholtzer]

The server should always be considered a maximally "dumb" instance, and information should in general flow from client to server, not vice versa.

And I'm OK with this in the general case as well. The impetus for this RFC is querying to support regression testing, not for use in a performance context. For example, see this unit test where I'm querying the server state. That text dump relies on my modified /n_query (plus some additional transformations on the client side to add labels) because /g_queryTree.reply can't handle that amount of data.

[Sciss]

Sure, unit testing seems a valid case. As I said, I am not much invested in n_query, I would just object to adding more "redundant" information to the active notifications such as n_go, n_move. For instance, in my system SoundProcesses there is a lot of automatic topology sorting when nodes are connected, and that causes a lot of OSC feedback from the server, so keeping those active notifications miminal is good thing IMO.

[mossheim]

thanks for the RFC @josiah-wolf-oberholtzer !

responding to a couple points:

1. i agree that we do not want people relying too much on server state to construct their clients. however, this is information that's already available in some form, so it's not so much a question of what information is available for the client to consume as how easy it is to get it. in addition, having access to this data in a constrained and reliable way will ease application development (as we're seeing here).

2. i see no reason to change the behavior of /n_query when adding a new command could achieve the same end. adding a new command would be unequivocally safer for clients of scsynth. it is also easier to maintain an application that supports some commands and not others than one that supports "the pre-3.11 but not post-3.11 version of xyz command" or vice versa.

if we guess wrong and there is a client out there that's broken by this, the pre-fix client (assuming it is maintained at all) and post-change scsynth are forever incompatible. if we don't take that risk, then there is no chance of breakage whatsoever; the new reply is only sent in response to a query the client doesn't know about.

3. responding to your other questions:

Can /g_queryTree be fixed or modified to avoid the overflow problem?

It can and should be. there ought to be a way to get a snapshot of the whole thing. i think, though, that is tangential to this discussion.

Is accessing a node in NodeEndMsg::Perform() always guaranteed to be safe?

I don't know.

4. as a counterproposal, consider adding /s_query which returns only the extended information you want, at address /s_query.reply, to the sender of the query. if the node does not exist, or is not a synth, /fail is sent to the sender with a relevant error message. this prevents the information from being unnecessarily passed on to all clients, discourages frequent use, and does not duplicate any information.

[jamshark70]

I think in a multi-client situation, a node N on the server created by client A is opaque to client B...

In general, yes, but in fact, Server Command Reference says node notifications are sent to all registered clients (not only to the originator of the s_new or g_new message).

I absolutely agree that it's overkill to include all synth arguments in an n_go reply -- just pointing out that client separation isn't airtight.

Brian's idea of an s_query command is a good one. SynthDef name and argument values are irrelevant for groups so there is no need to query nodes in general for those.

[josiah-wolf-oberholtzer]

OK, I'm amenable to pivoting to a dedicated /s_query command. It does means walking the tree is a little slower. You have to /n_query to determine if a node is a group or a synth, then /s_query if it's a synth to complete its information. But adding a new command guarantees no backwards compatibility problems, and in anecdotal testing over here firing off 100+ /n_query commands is damn fast.

I'm happy to revise this RFC to suggest creating /s_query instead of extending /n_query. @brianlheim @snappizz is that in-scope for the RFC process? Or should I close and create a new one?

[nhthn]

it's in-scope. no issue at all with making major changes to an RFC after opening it.

[jamshark70]

You have to /n_query to determine if a node is a group or a synth, then /s_query if it's a synth to complete its information

You can s_query first, and the /fail reply with a specific descriptive error message would tell you that it's a group. The /error command can suppress the failure message printed by scsynth (I think... if it suppresses both the printed message and /fail reply, that's kinda bad... in which case I'd have to withdraw the suggestion).

EDIT: Or, if you s_query a node ID that exists but is a group, send /fail and don't print anything...? You'll be listening for a reply anyway.

In some ways, scsynth shouldn't necessarily be printing anything for errors (if it is, as said above, maximally dumb) -- send failure OSC back to the client and let the client decide what to do with it. Except NRT mode... maybe I'm just dreaming.

[muellmusik]

I'm amenable to pivoting to a dedicated /s_query command.

This makes sense. Good one @brianlheim

[Sciss]

Another thought. So if there is a new s_query with the purpose of getting more inside information into a synth, also with prospect to unit testing; would it be useful to include (perhaps with a query flag) the information that is otherwise printed through n_trace, namely the momentary input and output values of the ugens:

x = play {
  RLPF.ar(LFPulse.ar(SinOsc.kr(0.2).madd(10, 21), 0.1), 100, 0.1).clip2(0.4)
};

x.trace;


TRACE 1000  temp_1    #units: 10
  unit 0 SinOsc
    in  0.2 0
    out 0.383626
  unit 1 MulAdd
    in  0.383626 10 21
    out 24.8363
  unit 2 LFPulse
    in  24.8363 0.1 0.5
    out 1
  unit 3 RLPF
    in  1 100 0.1
    out 0.534887
  unit 4 BinaryOpUGen
    in  0.534887 0.4
    out 0.4
  unit 5 Control
    in 
    out 0.02 1 0
  unit 6 BinaryOpUGen
    in  0.02 0
    out 0
  unit 7 EnvGen
    in  1 1 0 0.02 2 0 2 1 -99 1 1 5 -4 0 1 3 0
    out 1
  unit 8 BinaryOpUGen
    in  0.4 1
    out 0.4
  unit 9 Out
    in  0 0.4
    out

(in compressed form of course, so per ugen 32-bit floats for ins and outs; the UGens and their order are known to the client from the SynthDef it sent).

?

[josiah-wolf-oberholtzer]

@Sciss I like the idea a lot, but would want a different command to generate that response. I'm concerned about overflowing the OSC packet when inspecting complex SynthDefs.

There's actually a number of test-oriented inspection / debugging commands I want to propose along these lines - get all UGen input/outputs for a synth (as you proposed now), list all allocated SynthDefs, describe a SynthDef - but I'll make a separate RFC after this one is accepted/rejected.

[Sciss]

Sure, it doesn't have to be the same command; however, keep in mind that number of OSC commands in server is by design extremely small, I would avoid creating an excessive number of new commands. Perhaps the best option is to include in the new command a bit-set of flags that can be used perhaps even at a later point to extend the reply. Also, adding a new OSC command is a "big move" in my opinion, so it's worth discussing the design in length, mapping out all the other cases that should be covered.

[mossheim]

so there seems to be some indication by both @Sciss and @muellmusik that adding new commands -- not functionality, but actual concrete commands -- is a bad thing. i don't understand why that is.

[josiah-wolf-oberholtzer]

My guess is that it's because SC stores command addresses in a hash table for lookup, not because it makes the server API larger. Adding new command addresses to the hash table means potentially slowing down all command lookups by some unknown amount - but you'd have to profile to know how much. If there's another reason, I'm curious.

[josiah-wolf-oberholtzer]

And if it's due to the API size, and not lookup speed, I'm less concerned. There are some upsides to a more verbose API with nested paths instead of the flat OSC address space SC historically uses - API versioning, extensibility - which would make adding and extending commands less painful. I suppose adding something like /v2/node/query right now would probably work with the existing hash table implementation.

[muellmusik]

so there seems to be some indication by both @Sciss and @muellmusik that adding new commands -- not functionality, but actual concrete commands -- is a bad thing. i don't understand why that is.

Sorry @brianlheim, not what I meant. My point was more to do with keeping the API as orthogonal and understandable as possible. Specifically I was objecting to an additional extended version of an existing message, as I think it's strongly preferable to avoid duplicated functionality where possible, and a small risk of easily addressable breakage is preferable to permanent bloat. But your s_query seems a better compromise.

So not opposed to new commands in a general sense, no. @Sciss I'm also curious what you meant by it being a 'big move' in the abstract, and I think it's appropriate to approach proposals for this on their individual merits.

[jamshark70]

Another way to address permanent bloat would be plugin commands: http://doc.sccode.org/Reference/Server-Command-Reference.html#/cmd -- instead of "/n_query_ext", it could be "/cmd", "/n_query_ext".

Users who are interested in extended node query (e.g. for testing) could install the plugin binary defining that command. The automated test suite would install it too. Other users would be completely unaffected.

That depends on the expected use case. If it's primarily automated testing (which I agree is a strong use case), that's more of a specialized usage and would weaken the argument for a permanent, global interface change. If there are other, generally applicable use cases, let's be clear what they are, and evaluate whether they fit with the design philosophy of the server as a dumb DSP pack animal. (We could anticipate users will write their code to forget what synth args they sent because "I can just query it later" -- IMO not something to encourage.)

[Sciss]

I mean the design of scsynth is really minimal in a sense; it made it possible to create supernova with this rigid surface of API. It's a good design in my opinion not to include anything unnecessary in the server, it thus exhibits exactly the opposite long-term behaviour than the language that is plagued by too many tacked on things over time. So for me it's mostly a conceptual cost, but of course you only have to add one entry in the commands table, thus "simple" to extend...

By big move I also mean: Consider that the current server API is like 16 years old and more, of course with additions and adaptations over time, but not that many. So I think it's a good idea to think very carefully about adding a new OSC command and not rush to production just because it makes a particular case more convenient. That is to say, yes, add perhaps the additional information from a synth, but think for a while what other scenarios might be also included, so that you don't have to change it again some years later.

Small is beautiful when it comes to the server. 2c

[Sciss]

Users who are interested in extended node query (e.g. for testing) could install the plugin binary defining that command.

Funny that you say that, I was thinking exactly this not for the baseline proposal (synthdef name), but for my commend on n_trace; I guess indeed it would be better to add such functionality via an additional UGen you "mix into" your synth def, which can respond to u_cmd with special trace reply messages.

[mossheim]

these seem like very decent arguments in general, but i don't see them applying much to the proposal at hand, which is IMO well justified and not in danger of encouraging any of the negative side effects (unintended usage, functionality bloat, slowdown) mentioned above.

a second way to view this proposal is as a solution to a broken "g_queryTree" command; i think it accomplishes that quite well and also stays perfectly in style with the modular and dead-simple design of the rest of the API.

would also like to note that a big point in the user survey was making it easier to introspect the server's state; this makes it easier to accomplish that goal.

[muellmusik]

but it strikes me as a not-especially-safe assumption on that client's part

Well this was the sort of thing I meant about highly unlikely breakage. If clients allocate on the basis of the message itself, and then only use the earlier arguments, it's no problem. If they don't, it's subject to breakage anyway.

More broadly I'm a strong believer that clients that don't bundle scsynth should be specific about version compatibility. They already have this issue anyway if they've ever added a new feature.

But in this case I suspect a s_query is a better design than making n_query more elaborate and clever.

[telephon]

Also, just to add, I think it is a good practice for clients not to rely on the number of parameters that are sent back. In general, this is the easiest way to implement backward compatibility.

[Sciss]

think it is a good practice for clients

Yes, but. Server Command Reference should then prominently say that the API may at any time be extended with additional arguments tailing the return OSC messages.

[josiah-wolf-oberholtzer]

Something odd about /n_query - which would probably be asymmetric with a new /s_query/s_query.reply - is that it relies upon the existing NodeEndMsg::Perform() behavior to send /n_info to all clients. It makes sense that all clients would want to know about node /n_(go|end|on|off|move) in order to update their own models of server state. It makes less sense that all clients would want to know that one client was querying the server state.

[josiah-wolf-oberholtzer]

I've update the RFC to reflect a pivot towards a new /s_query command instead of extending the existing /n_query.

While I'd prefer an extended /n_query, I don't have the bandwidth to verify my POC against a wide range of third-party clients to prove that it's a safe change. And modifying the existing commands opens up a larger discussion about how SC designs for and communicates OSC API changes that probably deserves its own RFC.

[joshpar]

@josiah-wolf-oberholtzer - do you feel like this is now at a stage for consideration to work on for 3.12?

[josiah-wolf-oberholtzer]

@joshpar What's the timeline for 3.12? I've been swamped by moving and my day job, but do intend to get to this together eventually.

[mossheim]

@josiah-wolf-oberholtzer we don't have a set date for 3.12 yet, i'd guess it would probably be in the fall?

[mossheim]

just gave this one last look over and it seems good to me! with your permission, would you like to put this RFC into final comment period?

[muellmusik]

Hi All, @brianlheim suggested that some of the people involved in this rfc might be interested in this PR over on the main sc project: supercollider/supercollider#5244

It adds error warnings and /fail replies to /d_load and /d_loadDir. As it changes server behaviour it could affect third party clients, though it may also be bringing things in line with expected behaviour. Join the discussion there if you're interested.

[josiah-wolf-oberholtzer]

@brianlheim 3.12 sounds good. Things are quieting down a little around here. Should be able to put together a proper implementation over the next month or so.

[josiah-wolf-oberholtzer]

I've made one small change to the spec, renaming /s_query.reply to /s_info. This harmonizes with the existing /n_query//n_info and /b_query//b_info request/response pairs.

Reference implementation for scsynth and supernova is now here: https://github.com/josiah-wolf-oberholtzer/supercollider/tree/s-query

I'll kick off the final comment period shortly.

[mossheim]

thanks @josiah-wolf-oberholtzer ! just took a look at the RFC proposal and your branch to remind myself of the details and it all seems in good shape. i think this is ready for FCP too.

[mossheim]

like i already said (but just saying it again now that it's formally in FCP), i think this proposal is very solid and should be accepted

[telephon]

Yes looks very good.

[josiah-wolf-oberholtzer]

@brianlheim Looks like we're at 14 days since announcing FCP without any objections.

[mossheim]

@josiah-wolf-oberholtzer taking a break from dev work rn

anyone can merge this RFC and announce

[muellmusik]

Just flagging here that the discussion about modifying n_query has arisen again on #13. No decisions yet, but if we do decide to do that, it might make more sense to include s_query's functionality there at the same time and avoid the need for a new command.

[josiah-wolf-oberholtzer]

@muellmusik 100% happy to revisit that. I would have preferred an extended /n_query command rather than an entirely new command.

[muellmusik]

100% happy to revisit that.

Cool. That's nice to hear! I know it can be really frustrating to put in work to these things and then have the approach change. That said I'm often amazed at how really good solutions can emerge from this sort of process! Anyway, let's see where the consensus is and then we can decide how to proceed.