RC6 plays at least one group of zasfx presets different from previous ver.

[musikBear]

OS : winXP sp3
LMMS RC6

Issue description:
In zasfx we have the Cormi collection
In that collection we have a group named Trillo<>
These FX-presets replayed as randomised beeps -as in a alien cp environment -i guess..
In RC6 this is changed to a continuous sine-wave. The variation over time is gone.

Recreate :
You need need an earlier RC , to hear the correct output -I used RC4. Would be interesting to see if RC5 does it correct or not.

[musikBear]

Here is a spectrum -RC6 first.

It has to do with vibration, or pulsating, and i have found many more.
ISSUE4442_RC6zasfxFail.zip

This is a general zasfx issue

[PhysSong]

Do those presets play well in 1.1.3? I can't reproduce the bug with any 1.1~1.2 releases.

[musikBear]

@PhysSong The presets play correctly in RC4, and i suppose everything previous.
I am very surprised you cant hear any difference between 1.1 and 1.2.
It has been shown on my meager HW, and on a win10 highend HW
Try record and and look at the spectrum, just as the one i have posted, All of the presets in the attached file are different in RC4 and RC6
I will ask a linux user to do tests as well, with my project file.
Here is a recording of the difference in first correct output from RC4 and second wrong output from RC6 https://soundcloud.com/musikbear/trillo4v6

[musikBear]

@PhysSong

I can't reproduce the bug with any 1.1~1.2 releases.

All of the presets in the attached file, has difference. Both on my meager HW and on a win10 highend x64 HW

As i wrote, i used RC4 (correct output) v. RC6 (wrong output)
Here is a recording from now
https://soundcloud.com/musikbear/trillo4v6 RC4 first, then flawed RC6

[Daniele71]

HI guys, first time here !
I can confitm the issue on openSUSE TW.
lmms-1.1.3 ok, 1.2-RC6 (appimage) is not.
Discovered the bug with the patch "Filter Trem clean" from XAdriano Petrosillo.
It seems that in Zyn, Amplitude LFO Frequency is resetted to 0.

Sorry for my bad english ;)

Daniele.

[PhysSong]

It seems like a locale issue. That's why I failed to reproduce the bug(my locale is ko_KR.UTF-8 and it uses periods as decimal point). By forcing some European locales, I was able to confirm that.

This bug seems like a regression in #4244. I suggest this one to handle both . and , for separators:

diff --git a/plugins/zynaddsubfx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp b/plugins/zynaddsubfx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp
index 3ebc2eeec..ea343c7d9 100644
--- a/plugins/zynaddsubfx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp
+++ b/plugins/zynaddsubfx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp
@@ -505,7 +505,14 @@ float QtXmlWrapper::getparreal(const char *name, float defaultpar) const
                return defaultpar;
        }
 
-       return QLocale().toFloat( tmp.attribute( "value" ) );
+       // Handle both period-separated and comma-separated values
+       bool ok;
+       float value = QLocale( QLocale::C ).toFloat( tmp.attribute( "value" ), &ok );
+       if( !ok )
+       {
+               value = QLocale( QLocale::German ).toFloat( tmp.attribute( "value" ) );
+       }
+       return value;
 }
 
 float QtXmlWrapper::getparreal(const char *name,

[musikBear]

@PhysSong im afraid it is more complicated
Take a look here
https://youtu.be/7ewD4cuSkTE
It looks like in RC4 it is related to whether a preset is inside a loaded file, or if it is being loaded to a file.
At least that is reproduceable!
In RC6 i cant get that to happen. Fail no matter loading or loaded (same project)
If you use < RC6 with my project-file, and repeat what i do in the video- What happens then?
Also, do you have more than one lmms-versions installed?
I kind of feel that a thing like this should be tested on a completely clean system, mine isent...

[PhysSong]

@musikBear That's what I expected. Once those presets are loaded into 1.2.0-RC6/1.1.x, some data can be lost if the decimal separator in the preset doesn't match what the system locale uses. It's a permanent loss so old 1.2 RC can't recoveer it.

[PhysSong]

In Qt4, QString::toFloat(as well as toDouble) tries default locale first and then fallback to C locale. The behavior has been changed in Qt5, it doesn't fall back to C locale. So there should be a new function that handles both separators correctly.
In other words, DataFile::LocaleHelper also needs some changes. I'll find related potential issues and fix this.

[PhysSong]

I wrote a header file with some helper functions, but I don't know where to include this file. I can find every toFloat/toDouble calls, but including the file every time doesn't look efficient. AutomatableModel.h might be a good home, but I wonder if there are any better ways...