-
Notifications
You must be signed in to change notification settings - Fork 757
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
fixes (possibly?) for AudioControl mapping #3063
Conversation
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.
looks good - I'll test later. Could you fix the formatting please? You may have to set the preferences in xcode (if you are using that) to use 4 spaces instead of tabs, and then reformat.
include/plugin_interface/SC_Graph.h
Outdated
@@ -35,6 +35,7 @@ struct Graph | |||
uint32 mNumControls; | |||
float *mControls; | |||
float **mMapControls; | |||
uint32 mAudioBusOffset; |
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.
formatting? maybe convert tabs to space?
So - I have that setting already there, and it appears that my change is using 4 spaces instead of tabs. When I reformat the file, all the other lines are changed (and when I use show invisibles in Xcode, it looks like everything else is tabbed). So - I don’t feel live the white space of stuff I didn’t touch is appropriate to this issue, but I can still update the rest of the file if you think we should. Let me know what you want.
Also - did you run the patched version to make sure it performed as expected?
Best,
Josh
… On Jul 29, 2017, at 1:26 AM, Julian Rohrhuber ***@***.***> wrote:
@telephon commented on this pull request.
looks good - I'll test later. Could you fix the formatting please? You may have to set the preferences in xcode (if you are using that) to use 4 spaces instead of tabs, and then reformat.
In include/plugin_interface/SC_Graph.h <#3063 (comment)>:
> @@ -35,6 +35,7 @@ struct Graph
uint32 mNumControls;
float *mControls;
float **mMapControls;
+ uint32 mAudioBusOffset;
formatting? maybe convert tabs to space?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#3063 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrLqobdidMn2Fdi0RAHCX37VQU4Zhks5sSuyYgaJpZM4OnMQZ>.
|
This definitely fixes the bug in #2839. Thank you very much for this. The formatting seems to be tabs, not spaces, probably it was this way round. I can fix that. |
the rest of the file uses tabs.
code formatting
code formatting
OK, done. |
Thanks! Glad the fix seems to work. If other cases come up, please let me know.
Josh
/*
Josh Parmenter
www.realizedsound.net/josh
*/
… On Jul 30, 2017, at 07:39, Julian Rohrhuber ***@***.***> wrote:
OK, done.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
rereading the example above, I've found that the new version doesn't output any glitches, but also doesn't output any audio. Here is a slightly easier to use version of the first one by @jamshark70:
for me, the last bit outputs silence now (it used to outputs glitches). |
Also, this one crashes the server now :
The OSC messages are identical: With an "a_in" control name: With an "in" control name: |
Thanks for the example. I’ll try to work through this, but do you think you or someone else can make up an example of the version without sound that doesn’t use Pbind? I can’t always tell what is going on there with order of execution (and one thing I saw while debugging the other day had to do with this).
Thanks - I’ll look at the crashing issue as well. That example works well for me to work on this.
… On Jul 30, 2017, at 11:48 AM, Julian Rohrhuber ***@***.***> wrote:
Also, this one crashes the server:
(
SynthDef(\fx, { |out|
ReplaceOut.ar(out, HPF.ar(\in.ar(0 ! 2), 800));
}).add;
)
(
b = Bus.audio(s, 2);
s.makeBundle(nil, {
f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
f.map(\in, b);
});
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrCE_wSSpKz_fgZbZElalhbcY5Nx0ks5sTNAEgaJpZM4OnMQZ>.
|
Sure, here you are:
|
SO - I have it working for your solution below, and the numbers make sense to me in the code.
But - running James’s example (copied below) it doesn’t. I think the Order of Execution is messed up though. The node plot shows this:
```
NODE TREE Group 0
1 group
1070 temp__3
1071 group
1077 temp__0out-353201172_0
1076 group
1084 boop
1073 group
1074 system_link_audio_1
1075 system_link_audio_1
```
Shouldn't node 1077 be after group 1076? I’m thinking this is why the example below now outputs silence with me new changes. Please take a look at the node tree and tell me if I’m mistaken. I’ll push a change to the remote branch after I look at the crash.
```supercollider
(
SynthDef(\boop, { |out, freq = 440, gate = 1, amp = 0.1|
var sig = Mix(VarSaw.ar(freq * [1, 1.004])) * amp,
eg = EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.001, curve: 0), gate, doneAction: 2);
sig = LPF.ar(sig, 3000);
Out.ar(out, (sig * eg).dup);
}).add;
)
(
p = ProxySpace.new.push;
~out = { \in.ar(0!2) }; ~out.play(vol: 0.2);
~player = Pbind(
\instrument, \boop,
\degree, Pwhite(-7, 7, inf),
\dur, 1, \sustain, 0.2
);
~player <>> ~out;
)
p.clear; p.pop;
```
… On Jul 30, 2017, at 2:15 PM, Julian Rohrhuber ***@***.***> wrote:
Sure, here you are:
(
SynthDef(\fxbus, { |out, in|
var sig = In.ar(in, 2);
ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;
SynthDef(\fx, { |out, a_in = #[0, 0]|
ReplaceOut.ar(out, HPF.ar(a_in, 800));
}).add;
)
s.boot;
s.scope;
(
b = Bus.audio(s, 2);
{ Out.ar(b, SinOsc.ar(330) * 0.1 ! 2) }.play;
)
f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]); // OK
f.free;
(
s.makeBundle(nil, {
f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
//s.sendMsg("/n_mapan", f.nodeID, \a_in, b.index, 2);
f.map(\a_in, b);
});
)
p.stop;
f.free;
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrCutwKeKZaic2VmRlUqdbdKwZWhhks5sTPJrgaJpZM4OnMQZ>.
|
…percollider into AudioControlFix # Conflicts: # include/plugin_interface/SC_Graph.h # server/plugins/IOUGens.cpp # server/scsynth/SC_Graph.cpp
new fixes are in for checking. @telephon - was the sample that crashed something that you would expect to hear audio with? |
@joshpar no, I didn't expect audio to come out of that example. It was just there to crash the server. |
I pushed some new things to that branch - and some other files had to be touched. Does the pull request automatically get updated or do I need to do a new one?
Ready for checks I think.
Josh
/*
Josh Parmenter
www.realizedsound.net/josh
*/
… On Jul 30, 2017, at 22:55, Julian Rohrhuber ***@***.***> wrote:
@joshpar no, I didn't expect audio to come out of that example. It was just there to crash the server.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
If you're pushing to the remote branch that you used to make the PR, the PR gets updated automatically. |
Great - that’s what I thought. Should be good for review then!
/*
Josh Parmenter
www.realizedsound.net/josh
*/
… On Jul 31, 2017, at 06:09, Brian Heim ***@***.***> wrote:
Does the pull request automatically get updated or do I need to do a new one?
If you're pushing to the remote branch that you used to make the PR, the PR gets updated automatically.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Excellent. This works for the following test, and doesn't crash any of the above.
|
OK, so one more thing. So far, the audio rate controls worked as analogies to This is the reason why the ProxySpace example still doesn't work:
While this one works:
I'll post a simpler test case. |
Here is a test case:
|
well - that worked because of the bug that we’re squishing. To get that back, we could possibly check if the bus was touched in the current OR last cycle, but that is really dodgy behavior in my opinion. More than happy to try it out, but in the end, we will always end up with a glitch of at least one control period at the end where a bus is no longer touched, but it WAS touched on the last cycle and that data gets repeated.
Josh
… On Jul 31, 2017, at 9:10 AM, Julian Rohrhuber ***@***.***> wrote:
Here is a test case:
(
SynthDef(\fxbus, { |out, in|
var sig = InFeedback.ar(in, 2);
ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;
SynthDef(\fx, { |out|
ReplaceOut.ar(out, HPF.ar(\in.ar(0 ! 2), 800));
}).add;
SynthDef(\source, { |out|
Out.ar(out, SinOsc.ar(330) * 0.1 ! 2 * Line.kr(1, 0, 0.8, doneAction:2))
}).add;
)
s.boot;
s.scope;
(
b = Bus.audio(s, 2);
fork {
loop {
Synth.tail(s.defaultGroup, \source, [\out, b]);
1.5.wait
}
};
)
f = Synth.head(s.defaultGroup, \fxbus, [in: b, out: 0]); // OK
f.free;
// add to head: no sound.
(
s.makeBundle(nil, {
f = Synth.head(s.defaultGroup, \fx, [\out: 0]);
f.map(\in, b);
});
)
// add after: yes sound.
(
s.makeBundle(nil, {
f = Synth.after(s.defaultGroup, \fx, [\out: 0]);
f.map(\in, b);
});
)
p.stop;
f.free;
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrJNfUEE9cnngVxqFRB7ngzSqJXl1ks5sTfx1gaJpZM4OnMQZ>.
|
Modulating the bus argument of an https://github.com/supercollider/supercollider/blob/master/server/plugins/IOUGens.cpp#L666
Why must the |
glad @vivid-synth asked - using the original testing code, this now crashes supernova (before, it just had the same issue). (
SynthDef(\fxbus, { |out, in|
var sig = InFeedback.ar(in, 2);
ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;
SynthDef(\fx, { |out, a_in = #[0, 0]|
ReplaceOut.ar(out, HPF.ar(a_in, 800));
}).add;
SynthDef(\boop, { |out, freq = 440, gate = 1, amp = 0.1|
var sig = Mix(VarSaw.ar(freq * [1, 1.004])) * amp,
eg = EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.001, curve: 0), gate, doneAction: 2);
sig = LPF.ar(sig, 3000);
Out.ar(out, (sig * eg).dup);
}).add;
)
s.boot;
s.scope;
b = Bus.audio(s, 2);
(
p = Pbind(
\instrument, \boop,
\degree, Pwhite(-7, 7, inf),
\dur, 1,
\legato, 0.25,
\out, b
).play;
)
f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]); // OK
f.free;
(
s.makeBundle(nil, { // SUPERNOVA CRASH
f = Synth.tail(s.defaultGroup, \fx);
f.map(\a_in, b);
});
)
p.stop;
f.free; |
i'm getting a bad access here: supercollider/server/plugins/IOUGens.cpp Line 232 in 5f7d811
|
this mirrors changes introduced to scsynth in fdf1fb0
include/plugin_interface/SC_Graph.h
Outdated
@@ -35,6 +35,8 @@ struct Graph | |||
uint32 mNumControls; | |||
float *mControls; | |||
float **mMapControls; | |||
int32 *mAudioBusOffsets; | |||
uint32 mAudioBusOffset; |
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.
this appears to have been accidentally reintroduced - could you please remove it?
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.
or I can do it, I don't want this to get too confusing though :)
done
… On Aug 3, 2017, at 8:54 PM, Brian Heim ***@***.***> wrote:
@brianlheim commented on this pull request.
In include/plugin_interface/SC_Graph.h <#3063 (comment)>:
> @@ -35,6 +35,8 @@ struct Graph
uint32 mNumControls;
float *mControls;
float **mMapControls;
+ int32 *mAudioBusOffsets;
+ uint32 mAudioBusOffset;
or I can do it, I don't want this to get too confusing though :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrAxtAxtyi5F9SesayMC63gXMjVdkks5sUpXpgaJpZM4OnMQZ>.
|
great! |
all right. should any tests / other documentation be added for this? i am otherwise good to merge. |
i think it is ok to merge. we do need some systematic tests for mapping, but this is another issue |
Thanks @joshpar and @brianlheim ! |
@snappizz - pretty sure this is ready to go |
@@ -35,6 +35,7 @@ struct Graph | |||
uint32 mNumControls; | |||
float *mControls; | |||
float **mMapControls; | |||
int32 *mAudioBusOffsets; |
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.
this looks like an ABI change. please increment https://github.com/supercollider/supercollider/blob/master/include/plugin_interface/SC_InterfaceTable.h#L24
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.
@timblechmann changing an API from v1 to v2 gives me some pause -- will this potentially break existing code?
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.
it doesn't break any code, but it does break binaries. you can't use server 3.9 with plugin binaries from 3.8, or vice versa and you have to recompile all your plugins.
This reverts commit 891fcbc.
I know this is closed, but it's causing a rather likely crashes in supernova: #3196 I think the referenced issue needs to be addressed before this gets included in the 3.9 release... |
In sc_synthdef::prepare(void), the memory requirement for a sc_synth structure was underreported because it did not change with supercollider#3063. As a result, there was memory corruption. This commit both fixes the issue and adds clarifying comments to the calculations. Fixes supercollider#3196, supercollider#3266.
fixes #2839