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

Plugins: Choose Index.ar calc function based on phase arg #3436

Merged
merged 2 commits into from
Jan 26, 2018

Conversation

jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Jan 16, 2018

  • Index_next_k() accesses index ZIN0(1) once per control cycle -- i.e., assumes a kr index.
  • Index_next_a() accesses index ZIN0(1) inNumSamples times per control cycle -- ar index.

Both calculation functions do GET_TABLE only once per control cycle -- so the bufnum input does not support ar under any circumstances.

But, the Ctor chooses next_k for a control-rate bufnum, regardless of the rate of the input that matters (index). So this PR changes INRATE(0) to INRATE(1).

Five other UGens follow the same pattern: IndexL, FoldIndex, WrapIndex, IndexInBetween and DetectIndex.

Test: without the fix, the following displays an obvious sample-and-hold. With the fix, Index.ar is correctly equivalent to BufRd.ar without interpolation.

(
s.waitForBoot {
	b = Buffer.alloc(s, 512, 1, completionMessage: { |buf|
		buf.sine3Msg([4], [1], [0.25pi], asWavetable: false)
	});
	s.sync;
	{
		var phase = Phasor.ar(0, 1, 0, 1000);
		[Index.ar(b, phase), BufRd.ar(1, b, phase, interpolation: 1)]
	}.plot(duration: b.duration); 
};
)

I'll suggest 3.9.1 for this as it's a small bugfix.

Index_next_a() is clearly meant for an audio-rate *index* input,
not an audio-rate bufnum input.

Same for IndexL, FoldIndex, WrapIndex, IndexInBetween
and DetectIndex.
@jamshark70 jamshark70 mentioned this pull request Jan 16, 2018
@patrickdupuis patrickdupuis modified the milestones: 3.9.x, 3.9.1 Jan 16, 2018
@patrickdupuis
Copy link
Contributor

I am working on a UnitTest for this. It isn't fun :( Please give me a couple days to add the test before merging this.

Copy link
Contributor

@patrickdupuis patrickdupuis left a comment

Choose a reason for hiding this comment

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

This needs a UnitTest. I'm writing a test at the moment.

@patrickdupuis
Copy link
Contributor

I have a draft UnitTest over here: https://gist.github.com/patrickdupuis/077577a8c2800f24c2c3849a0592176d

Currently, the second test fails because SynthDef not found. I don't know why this is happening. I would appreciate some expert help :)

@patrickdupuis
Copy link
Contributor

I found the issue was being cause by server.remove in tearDown {}.

@patrickdupuis
Copy link
Contributor

I have a UnitTest ready for this. Will push to this PR branch later tonight.

@patrickdupuis
Copy link
Contributor

Oops... forgot to change the name before pushing ;)

@jamshark70
Copy link
Contributor Author

Patrick's unit test is in now -- should be ready to merge.

@joshpar joshpar self-requested a review January 24, 2018 04:30
@joshpar
Copy link
Member

joshpar commented Jan 24, 2018

looks good from here.

Classlib: Testsuite: Meaningful name for Index rate test

On branch patrickdupuis-indexUnitTest

filename should match class name

delete old test file
@jamshark70 jamshark70 force-pushed the topic/FixIndexUGenRates branch from 3651c1a to fe3bbf0 Compare January 24, 2018 08:05
@jamshark70
Copy link
Contributor Author

Squashed the unit test down to one commit. Should be ready to merge.

@patrickdupuis patrickdupuis merged commit 13f28e6 into supercollider:3.9 Jan 26, 2018
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.

5 participants