Supercollider: ProxyNodeMap:controlNames faulty logic breaks NodeProxy audio patching

Created on 18 Feb 2019 · 6Comments · Source: supercollider/supercollider

Environment

SuperCollider version: 0685eb153
Operating system: Ubuntu Studio 18.04

Steps to reproduce

https://github.com/supercollider/supercollider/commit/1b88d80c7dd761170fbf970f813466feac9cddcf broke this.

The addition of value = value.asControlInput; into ProxyNodeMap:controlNames means that the <<> and <>> operators are broken for audio-rate NodeProxy inputs. You can use them once. After that, the target proxy's nodeMap will return a control-rate ControlName for an input that was defined at audio rate.

s.boot;
p = ProxySpace.new.push;

~out = { \in.ar(0!2) }; ~out.play(vol: 0.2);

~osc = { |freq = 440| Saw.ar(freq).dup };
~out.controlNames.detect { |cn| cn.name == \in };  // 'audio', [0, 0]
~osc <>> ~out;
~out.controlNames.detect { |cn| cn.name == \in };  // 'control', [ a(n), a(n+1) ]
~osc <>> ~out;  // 2nd time, "no matching input"

Expected vs. actual behavior

ProxyNodeMap tries to determine the input's rate according to the value stored in the node map.

After mapping ~osc <>> ~out, ~out's node map contains a reference to ~osc for its \in input.

~out.nodeMap[\in]
-> NodeProxy.audio(localhost, 2)

At this point, the old logic in ProxyNodeMap:controlNames would ask the source NodeProxy for its rate, returning \audio, and everything was fine.

The new logic attempts to account for "arbitrary objects," so it calls .asControlInput on the node map value.

~out.nodeMap[\in].asControlInput
-> [ a6, a7 ]

... and assumes that .rate, on this, will do something sensible. But in fact, there is no logic in SC to identify the rate of a bus-mapping string.

~out.nodeMap[\in].asControlInput.rate
-> scalar

It looks to me like a case of shortcuts being taken in the object design. If the node map is going to be a source for ControlName objects, then it should store ControlName objects alongside the values -- that is, you really need a dictionary of name -> ProxyInputData(controlName, value). Instead, it's storing only values, and then attempting to recover information from the values (when that information more properly comes from the specification of the input, directly).

Possibly revert 1b88d80c7 until there is a proper fix. The current situation is unusable.

Also, more attention to JITLib's unit testing suite. This kind of thing should be caught in automated testing.

bug JITlib

Source

jamshark70

All 6 comments

I hope this is a reasonable fix for now.

Instead, it's storing only values, and then attempting to recover information from the values (when that information more properly comes from the specification of the input, directly).

The node map should store values, because they may change their rate (like node proxies). The operation <<> shouldn't use the controlNames for rate detection, but the objects themselves, that's the culprit to my eyes.

ctl = this.controlNames.detect { |x| x.name == key };
        rate = ctl.rate ?? {
            if(proxy.isNeutral) {
                if(this.isNeutral) { \audio } { this.rate }
            } {
                proxy.rate
            }
        };

A proper fix would probably be to implement a message that returns the rate for a given control name key, and this would traverse the objects without creating all those ControlName objects.

telephon on 18 Feb 2019

I hope this is a reasonable fix for now.

JITLib has been getting patched very frequently lately; having a solid test suite in place would be great for future maintenance. Clearly it's a complex piece of software and that makes it difficult to think through all of the possible cases whenever a new bug like this arises. We can continue adding 1 or 2 unit tests for each bug fix and get there eventually, but that will be more work overall and more frustration for users as each new update seems to cause a break in a different/new corner case. Just my 2c.

brianlheim on 23 Feb 2019

Given the broad usage, I've not heard too much frustration, but maybe I just haven't heard the existing frustration.

You are right that it would be good to have more tests! Even though I'd love to have one, I am currently not able to write such a test suite.

telephon on 23 Feb 2019

Given the broad usage, I've not heard too much frustration, but maybe I just haven't heard the existing frustration.

The larger point is that we are not using our time efficiently; a bug pops up, we fix it, then a few months later in beta a new bug pops up that resulted from the first fix. Each step requires walking through the logic again, more time in review, etc. I am thinking specifically of this:

3768 and #3795 were fixed by #3797, which caused issue #4060, which was then patched in #4114, which caused #4266, which is now (supposedly) fixed in #4278.

The test cases that were added were all pretty simple -- I think having just a handful of sanity-check test cases would make this code a lot more robust to future changes.

You are right that it would be good to have more tests! Even though I'd love to have one, I am currently not able to write such a test suite.

It doesn't have to be all at once; you open PRs pretty frequently, why not make some of them PRs that add a couple tests around a specific method or functionality? Or, add 1 or 2 more tests to your existing PRs. Either would go a long way toward maintainability.

brianlheim on 23 Feb 2019

3768 and #3795 were fixed by #3797, which caused issue #4060, which was then patched in #4114, which caused #4266, which is now (supposedly) fixed in #4278.

The test cases that were added were all pretty simple -- I think having just a handful of sanity-check test cases would make this code a lot more robust to future changes.

I just saw this, just for future reference: these issues are unrelated to the current one, and are partly unrelated to each other.