Skip to content
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

Synth sound regression #57

Closed
yupferris opened this issue Apr 7, 2021 · 2 comments
Closed

Synth sound regression #57

yupferris opened this issue Apr 7, 2021 · 2 comments

Comments

@yupferris
Copy link
Member

While trying out 64bit stuff, it seems the reverence test project sounds quite different than it used to, in addition to being about 1kb larger, even with latest squishy. It sounds the same in 64/32-bit, so I suspect some kind of regression somewhere. Note that I used vs2019 on Windows. This should probably be bisected, but I haven't looked further into it yet.

@yupferris
Copy link
Member Author

yupferris commented Jun 11, 2021

I compared latest master to the released Reverence, and isolated a very obviously different part here.

Afterwards, I did some bisecting work, and was able to track it down. It turns out it happened when we introduced faster sinusoids, and even more specifically, when we integrated @kusma's improvements over what I originally had (corresponding to this commit in the sinusoid sandbox repo).

A quick comparison between the two impl's reveals that indeed, the MSE is an order of magnitude higher in the newer code:

comparison

I'm honestly a bit surprised I/we didn't notice that it sounded this much different when the change was made (I do recall us being aware of the difference in error, but we thought it didn't matter), as it's quite clear in the sample above. However, neither impl here is particularly wrong, and @kusma's is 2x faster. Additionally, we're technically pre-1.0, so these kinds of breaking changes are acceptable in theory, and it was pre-open-source, so there have been no regressions (that we know of) still for most users since release.

In light of all of that, I'm voting that we keep what we have, instead of trying to "fix" it (which is kinda arbitrary here since we'd just be having less error, and even the original sinusoids had some error, so it's kindof qualitative to begin with) and introduce yet more breakage just to save some older projects, which are still "prestine" in their released form.

@yupferris
Copy link
Member Author

I'll close this, and split out another issue to investigate possible size regressions (this may just be a consequence of using vs2019).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant