logicomacorp / WaveSabre

Official WaveSabre repository
MIT License
245 stars 33 forks source link

Synth sound regression #57

Closed yupferris closed 3 years ago

yupferris commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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