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

Topic/synthdef specs improvements #3814

Open
wants to merge 16 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@scztt
Copy link
Contributor

scztt commented Jun 25, 2018

This PR adds some more flexible ways of modifying ControlSpecs related to SynthDef args.
For settings specs:

SynthDef(\foo, { |freq| freq.spec = ControlSpec(20, 20000) });
SynthDef(\foo, { SinOsc.ar(\freq.kr(spec:ContolSpec(20, 20000)) });

For finding specs:

def.specs[\freq];
Ndef(\foo).specs[\freq];

Where ever possible, reasonable SynthDef-like objects respond to .specs. Also, where it's reasonable existing specs are set using setFrom rather that obliterating them in the specs dictionary itself, to preserve identity of those specs in case they are linked to UI or have dependents.

@scztt scztt added this to the 3.10 milestone Jun 25, 2018

@scztt

This comment has been minimized.

Copy link
Contributor

scztt commented Jun 25, 2018

This had been a local change I've been testing for ages - reasonable for 3.10 unless anyone sees any confounding factors.

@telephon
Copy link
Member

telephon left a comment

overall very good.

@@ -464,6 +464,15 @@ Get all key value pairs from both link::Classes/NodeMap:: (the settings) and def
method::controlKeysValues
Get all key value pairs from default arguments.

method::specs
Get the specs array from SynthDef metadata.Note that, for a NodeProxy with multiple sources, result will be a dictionary containing specs of ALL source SynthDefs.

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

a space missing.

@@ -443,3 +446,35 @@ Pbind(
)
::


subsection:: Inline ControlSpec definitions
Sometimes, it can be clearer to specify a link::Classes/ControlSpec:: for a parameter at the point of it's usage in a SynthDef, rather than elsewhere.

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

of its (not it's)

code::
(
~def = SynthDef(\argumentExample, {
|freq, amp|

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

using a new line for args has its benefits, but is rather unusual. Maybe you can just make it less standing out for the eye if you keep with the usual convention.

spec = spec.asSpec;

defaultValue ?? {
defaultValue = spec.default;

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

you can leave out the trailing semicolon

};

UGen.buildSynthDef.specs[name] !? _.setFrom(spec) ?? {
UGen.buildSynthDef.specs[name] = spec;

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

you can leave out the trailing semicolon

@@ -33,7 +33,7 @@ SynthDef {
}

*new { arg name, ugenGraphFunc, rates, prependArgs, variants, metadata;
^super.newCopyArgs(name.asSymbol).variants_(variants).metadata_(metadata).children_(Array.new(64))
^super.newCopyArgs(name.asSymbol).variants_(variants).metadata_(metadata ?? {()}).children_(Array.new(64))

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

is this a (tiny) API change? Now having no metatdata won't test for isNil but rather isEmpty.

This comment has been minimized.

@scztt

scztt Jun 29, 2018

Contributor

It's a tiny API change, but shouldn't break any code (metadata can already be nil or non-nil, existing code should work in either case). This is a somewhat important change because without it, metadata has to be nil checked and lazily created for ALL internal points of access. This would be reasonable only if there was a significant cost to having a default-initialized metadata dict (there's not), or if there was somehow a special meaning to having a nil metadata (there's not, as far as I could find). Existing accesses to metadata in this class are fairly arcane because of the assumption that it can be nil - those can be significantly cleaned up (I didn't do that as part of this CL because it seems too unrelated).
btw, happy to keep it at nil if it feels better, this just feels like a step towards better factoring.

This comment has been minimized.

@jamshark70

jamshark70 Jun 29, 2018

Contributor

I can't check right now, but please test writing a SynthDef to disk to make sure that empty metadata aren't written to disk.

Currently metadata are written to disk in a second file (the scsyndef binary format is unchanged). If metadata are nil, no extra file is written (and any previously written metadata file will be removed).

A quick glance at the code convinces me that requiring a metadata object in all cases will produce 2 files for every SynthDef.

The relevant bits are in SynthDesc.sc.

This comment has been minimized.

@scztt

scztt Jun 29, 2018

Contributor

Ah, good eye, I didn't know that was happening.

This comment has been minimized.

@scztt

scztt Jun 30, 2018

Contributor

Tested and submitted a fix.

@@ -121,6 +134,11 @@ NamedControl {
}
}

*newSpec { arg name, values, rate, lags, fixedLag = false, spec;

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

I'm just reading now, but you don't return a new instance here, do you?

This comment has been minimized.

@scztt

scztt Jun 29, 2018

Contributor

This is superfluous, removing (methods above should point directly to *new)

ir { | val |
^NamedControl.ir(this, val)
ir { | val, spec |
^NamedControl.ir(this, val, spec:spec)

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

passing parameters by keyword is less efficient. In this case it would be better avoided, I think.

This comment has been minimized.

@scztt

scztt Jun 29, 2018

Contributor

It can only be avoided in the ar case (fixing) - the other cases don't forward a lags argument so we've got to specify spec by key. It might be semantically identical to pass a nil, but that's really not a common pattern in sclang and in any case, this is not performance-critical code.

This comment has been minimized.

@telephon

telephon Jul 1, 2018

Member

ok, that makes sense.

// and here is the bug itself
this.assert( w.map(0.0) == 50,"mapping using the copied spec should result in the original mapping behavior of the original spec.");
}

test_setFrom {

This comment has been minimized.

@telephon

telephon Jun 29, 2018

Member

I'm personally fine with those, though Brian may insist (and perhaps he is right) that we should have only one assert per method. I am undecided.

This comment has been minimized.

@scztt

scztt Jun 29, 2018

Contributor

In a way that would be correct, but also making tests clear and concise is more correct - I'm not adding 29 methods. :)
Adding descriptive messages to each to make failures more clear.

This comment has been minimized.

@brianlheim

brianlheim Jun 30, 2018

Member

@telephon - I'm open to discussing changes to the unit testing standards I proposed! Here I think it's reasonable to have more than one assert, because logically what's being tested is the postcondition of setFrom. I will try to capture that nuance in the guide.

@jamshark70

This comment has been minimized.

Copy link
Contributor

jamshark70 commented on SCClassLibrary/Common/Audio/SynthDesc.sc in 2603893 Jun 30, 2018

Maybe too fussy but you could just write metadata.size == 0 because nil.size == 0.

(Typically we short-circuit logical expressions by metadata.isNil or: { metadata.size == 0 } but here it isn't necessary.)

name = name.asSymbol;

this.initDict;
if (spec.notNil) {

This comment has been minimized.

@telephon

telephon Jul 1, 2018

Member

One more thing I noticed: When this method fails with an error, the spec will have been set.

This comment has been minimized.

@telephon

telephon Jul 2, 2018

Member

My suggestion would be the following:

Just keep this:

if (spec.notNil) {
			spec = spec.asSpec;

			if (values.isNil) {
				values = spec.default
			};

};

But add these methods to NamedControl:

spec {
			^UGen.buildSynthDef.specs[name]
}

spec_ { |spec|
			spec = spec.asSpec;
			this.spec !? _.setFrom(spec) ?? {
				UGen.buildSynthDef.specs[name] = spec
			};
}

and then just set the spec here

this.spec = spec;

This comment has been minimized.

@scztt

scztt Jul 13, 2018

Contributor

Good suggestions, just fixed.

scztt added some commits Jul 13, 2018

sclang: Refactor spec setting
SynthDef already provides access to the specs dict, use that instead. Also, avoid setting specs in cases where NamedControl is not successfully created.
@telephon
Copy link
Member

telephon left a comment

Just one little formatting thing: could you adjust the code convention for arguments to the surrounding code? One day we'll automatically convert all the files, but in the meantime it would be good for readability to keep it consistent.

@snappizz

This comment has been minimized.

Copy link
Member

snappizz commented Jul 15, 2018

unfortunately, i can't let this into 3.10, since we're closed for new features. it can be merged into develop for 3.11 (when it's ready of course). very cool though, i'm looking forward to using this.

@snappizz snappizz removed this from the 3.10 milestone Jul 15, 2018

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Jul 16, 2018

... aahrg, I terribly sorry @scztt, I shouldn't have asked for changes.

@patrickdupuis patrickdupuis added this to the 3.11 milestone Nov 18, 2018

scztt added some commits Dec 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment