scsynth: PanAz: fix leaks and imprecisions #4971
Conversation
There was a problem hiding this comment.
Thanks for this!
I haven't reviewed the functionality but I do have some questions from reading through the code.
There are a few points of implicit type conversion and I can't tell by looking at the code what is intentional - if I understand correctly, the error you're fixing was a result of incorrect precision, so I think it's important any precision changes are explicit so we know what behaviour is intended. I've marked all the points I could find where precision is possibly lost by implicit conversion.
I'd probably recommend having everything as double and casting as late as possible, rather than a bunch of casts everywhere.
Also, an optional question about the terminology in the tests - it doesn't block the PR at all, just a thought.
|
@elgiano thanks for the quick response to review, greatly appreciated! @brianlheim, I was wondering if this needed to wait for consensus on changes to UGens? (#4976) |
we already have a consensus as far as i'm concerned, i just haven't closed the issue yet. :) |
PanAz was having a small leak into adjacent-to-focused channels, due to both an imprecision in sc_reciprocal and in ft->mSine. This fixes it without touching either of the two.
elgiano
force-pushed
the
panaz_correction
branch
from
9547a72 to
1ee86fd
Compare
|
@jrsurge ready for another review when you're ready. :) |
|
@jrsurge @brianlheim I don't know why Travis failed, but it doesn't look like it's related to anything in this PR :) |
|
@elgiano yeah. you should be able to restart jobs in travis now if you link your account there to your github account. feel free to restart when you think a failure is spurious. if it looks like it's something that could be avoided, leave a note on the PR. this one seems like brew just froze midway through updating, nothing we can do. |
|
@jrsurge The only pending issue is if we want to do everything with doubles or only cast around the use of |
|
Hi all - we'd like to pull this in for 3.13.0 and are planning an RC for that after our meeting on Aug, 27, 2022. Anything else to add? Or can we close this? |
joshpar
dismissed
jrsurge’s stale review
Looks ok on our side for now. Thanks James!
PanAz was having a small leak into adjacent-to-focused channels, due to
both an imprecision in sc_reciprocal and in ft->mSine. This fixes it
without touching either of the two.
Purpose and Motivation
Fixes #4970
supercollider/server/plugins/PanUGens.cpp
Line 1325 in f806ace
Changing this to
>= 1.fshould fix it.ft->mSine[4096]should be zero (even if it is a very small number instead), so it makes sense to shortcut to 0 IMO.However, due to an imprecision in
float32 sc_reciprocal(),chanposends up not being== 1.fand so this fix doesn't work.supercollider/include/plugin_interface/SC_InlineUnaryOp.h
Line 210 in f806ace
It works however if the float64 version of sc_reciprocal is used.
I've checked other UGens usage of sc_reciprocal, I think this small imprecision causes problems only when it gets into an equality comparison, and PanAz is the only UGen doing that.
Types of changes
Is this a breaking change? e.g I have several past projects where I adjusted panning positions by a small amount to account for this error.
To-do list