fix: Tap Ugen samplerate compensation with BufRateScale.kr()

[morfant]

Purpose and Motivation

To prevent unintentional changes in pitch due to the difference between the system samplerate and the buffer samplerate.

Types of changes

• Bug fix

To-do list

• Code is tested
• All tests are passing
• Updated documentation
• This PR is ready for review

[jamshark70]

I'm curious... does this fix a specific bug case?

To my knowledge, the only way to have a buffer whose sample rate differs from the server's rate is to read a file from disk.

Tap is not really meant to be used with a disk file -- it's intended to be used with an allocated buffer. Allocated buffers will always have the server's sample rate, hence BufRateScale would be 1.

So I'm not certain this is strictly necessary. I could imagine somebody might want to use Tap as a delayed buffer player, but it strikes me as an odd usage, in which one would give up control over rate, startPos etc. for no significant gain. My instinct would be to discourage that usage -- but if others really want it, I wouldn't object too strongly.

[dyfer]

I'm in agreement with @jamshark70 and I also don't really see a reason to add BufRateScale here. @morfant would you mind answering James's question what is the practical issue this PR solves?

[morfant]

@jamshark70 @dyfer Sorry for the very late reply.
I didn't know the process how the pull request be treated.
I have to reply back as soon as possible, but didn't. Sorry again.

Yes, I found out this issue using Tap with buffer from disk file.
It's also true that I wasn't quite sure how to use the Tap UGen properly.

Then instead of approval this request how about append some comment on documentation?

bufnum | The index of the buffer to use(This UGen assumes using server-allocated buffer)

Even though Tap's example uses a server-allocated buffer, but to people like me who got to know Tap in SC, it doesn't seem natural.

In my personal opinion, a buffer read from disk can be used for Tap. Actually doing that with MultiTap made some interesting sounds for me. I think it might be a weird/new way to use Tap/MultiTap.

I agree with that it would be a bit unreasonable to accept this request.
But I think it might be a little bit of kindness for people who are just learning SC.

Have a good weekend.

[telephon]

I agree that unintended use should be as meaningful as possible. So I think it is a good idea to add the BufRateScale.

[telephon]

So as a first step, you could follow the suggestions that @jamshark70 gave

[jamshark70]

Ah, but I just noticed the target of this PR is 3.12.

We always merge new changes into supercollider:develop -- not into a version branch.

So we can't merge this one as it is.

Let me see if there's a way to change the target. If not, we would have to close this one and you would have to re-create it against the develop branch.

[jamshark70]

OK, done! I think it's OK to merge now.

[morfant]

Ok Thanks! I am thankful for you people’s open mind. Do I have to close this thread?

[jamshark70]

Do I have to close this thread?

No -- at the top, it now reads "morfant wants to merge 2 commits into supercollider:develop" which is the right branch to merge into.

[telephon]

there is one test that failed that is completely unrelated to the patch.

[dyfer]

Thanks everyone!

[dyfer]

Just for clarification:

We always merge new changes into supercollider:develop -- not into a version branch.

That's not entirely correct - it's fine to merge bugfixes or minor changes to a version branch, if we indeed plan to release a subsequent bugfix release (e.g. 3.12.2). However, at this point there's no clear plan for that, so indeed merging to develop is the right approach. See our wiki entry for reference.

[morfant]

Thanks everyone! Thanks for kind explanation!

[telephon]

welcome!