Plugins: Integrator Ctor passes through the first sample only #5352
Conversation
jamshark70
added
bug
mossheim
added
API change
and removed
bug
|
@jamshark70 - checking in again on the documentation update. Can you please add this? We'll be planning an RC for 3.13.0 after our meeting at the end of Aug 2022 if the update can be applied by then. Thanks! |
|
@joshpar – what would you expect the documentation change to be? I can only imagine a warning that past implementations had a one off error. But otherwise, this seems to be just a bugfix. |
|
Ah, I see now that the labeling was changed from bug fix to API change. Well... I don't fully agree with that. The old behavior was simply incorrect. You can call it an API change if the old behavior was justifiable, but was changed to a different justifiable behavior for a good reason. I can't think of many places in the documentation where we say "prior to v3.13, there was a bug that looked like xyz; it is fixed and the new behavior looks like abc." Pretty much, this is the only documentation change that I can imagine here. I think this would look a bit awkward from the user perspective. Completely fair to include this in the changelog. |
|
@jamshark70 |
|
@jamshark70 I just wanted to check in on this PR. It would be great to get this into 3.13! |
I updated the init formula per mtmccrea's suggestions, and verified that the unit test passes. Good to go? |
|
Re: documentation: I've made a note on this change on the draft changelog notes for 3.13 so this will be included in the changelog. Does that satisfy your requirements @joshpar? |
|
Looks good! |
Purpose and Motivation
Integrator double-counts the initial value, because of calling
nextin the Ctor.See the added unit test: Without this fix, the integral of a single 1 followed by endless 0s ends up being 2. With the fix, it's 1 as expected.
(This is related to #2343 but it's only a spot fix of one UGen.)
Types of changes
To-do list
[ ] Updated documentation