Skip to content
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

class library: make control set atomic #1687

Merged
merged 6 commits into from
Nov 11, 2015
Merged

Conversation

LFSaw
Copy link
Member

@LFSaw LFSaw commented Oct 18, 2015

this fix by @telephon fixes an annoying behaviour in NodeProxies:

Ndef(\sine).clear


(
Ndef(\sine, {SinOsc.ar(\freq.kr(100)!2).tanh});
Ndef(\sine).set(\freq, 200);
)

-> Ndef('sine')
FAILURE IN SERVER /n_set Node 1010 not found

Previous behaviour:

1. set source: create a bundle that:
1a. sends synth def
1b. waits for completion, then sends synth with args
2. set controls: create a bundle that:
2a. sets group

temporal order will be: 1a. 2a. 1b.

now, it does it different... (maybe ask @telephon for details..)
I use this branch extensively for a week now and think that it works perfectly.

This tests if there are messages that could be canceled. This infers
that messages were added by ‘addCancel’, and thus with the possible
intention to be lazily evaluated.

Thus, there is very little overhead for all other cases.
@telephon
Copy link
Member

The branch breaks:

Ndef(\x, { |freq| SinOsc.ar(freq) }); 
Ndef(\x).send;

crashes the interpreter

This commit includes two fixes concerning the way arguments are passed
to the internal synth control objects.

1) the old synth is released now with the current fade time.

2) pass extra arguments on send as a thunk, and not recursively (this
was an accidental in one earlier commit)
When setting synth controls (the internal objects of a node proxy)
directly, convert the arguments to OSC explicitly so that objects can
be passed also to those.
Protect also NodeProxy:spawn from wrong arguments due to an async
operation. This makes the method less efficient, but safer.
@telephon
Copy link
Member

OK, fixed. So please test it extensively!

@telephon telephon added this to the 3.7.0 milestone Oct 22, 2015
@crucialfelix crucialfelix modified the milestones: 3.7.x, 3.7.0 Nov 7, 2015
@telephon
Copy link
Member

@crucialfelix – because e wrote the OSCBundle stuff together back in the old days: can you have a brief look at the change?

@@ -746,9 +745,9 @@ NodeProxy : BusPlug {

// bundle: apply the node map settings to the entire group
sendAllToBundle { | bundle, extraArgs |
extraArgs = nodeMap.asOSCArgArray ++ extraArgs.value.asOSCArgArray;
var args = Thunk { nodeMap.asOSCArgArray ++ extraArgs.value.asOSCArgArray };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better style IMO would be:

Thunk({ ... })

which makes it clear that you are creating a Thunk object.

@crucialfelix
Copy link
Member

seems okay. its always better not to complicate the core classes, but there is already a bunch of junk in OSCBundle

telephon added a commit that referenced this pull request Nov 11, 2015
@telephon telephon merged commit b7ea286 into master Nov 11, 2015
@telephon telephon deleted the topic/nodeproxy-asyc-fix branch December 4, 2016 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants