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

Projects
None yet
5 participants
@jamshark70
Contributor

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.

Plugins: Choose Index.ar calc function based on phase arg
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 referenced this pull request Jan 16, 2018

Closed

Index.ar bug #3418

@patrickdupuis patrickdupuis modified the milestones: 3.9.x, 3.9.1 Jan 16, 2018

@patrickdupuis

This comment has been minimized.

Contributor

patrickdupuis commented Jan 18, 2018

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.

@patrickdupuis

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

@patrickdupuis

This comment has been minimized.

Contributor

patrickdupuis commented Jan 20, 2018

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

This comment has been minimized.

Contributor

patrickdupuis commented Jan 20, 2018

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

@patrickdupuis

This comment has been minimized.

Contributor

patrickdupuis commented Jan 23, 2018

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

@patrickdupuis patrickdupuis referenced this pull request Jan 24, 2018

Merged

add unit test #1

@patrickdupuis

This comment has been minimized.

Contributor

patrickdupuis commented Jan 24, 2018

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

@jamshark70

This comment has been minimized.

Contributor

jamshark70 commented Jan 24, 2018

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

@joshpar joshpar self-requested a review Jan 24, 2018

@joshpar

This comment has been minimized.

Member

joshpar commented Jan 24, 2018

looks good from here.

Testsuite: Add unit test for index rate calc selector
Classlib: Testsuite: Meaningful name for Index rate test

On branch patrickdupuis-indexUnitTest

filename should match class name

delete old test file
@jamshark70

This comment has been minimized.

Contributor

jamshark70 commented Jan 24, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment