fix: Tap Ugen samplerate compensation with BufRateScale.kr() #5606
Conversation
There was a problem hiding this comment.
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.
|
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? |
|
@jamshark70 @dyfer Sorry for the very late reply. Yes, I found out this issue using Tap with buffer from disk file. Then instead of approval this request how about append some comment on documentation?
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. Have a good weekend. |
|
I agree that unintended use should be as meaningful as possible. So I think it is a good idea to add the |
|
So as a first step, you could follow the suggestions that @jamshark70 gave |
|
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
added
comp: class library
|
OK, done! I think it's OK to merge now. |
|
Ok Thanks! I am thankful for you people’s open mind. 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. |
|
there is one test that failed that is completely unrelated to the patch. |
|
Thanks everyone! |
|
Just for clarification:
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 |
|
Thanks everyone! Thanks for kind explanation! |
|
welcome! |
…llider#5606) * fix: Tap Ugen samplerate compensation with BufRateScale.kr()
Purpose and Motivation
To prevent unintentional changes in pitch due to the difference between the system samplerate and the buffer samplerate.
Types of changes
To-do list