Possible zynaddsubfx regression issue in stable-1.2

[JohannesLorenz]

Many of my songs created with 1.0.3 sound OK on 1.1.3, but on stable-1.2 they sound completely wrong (many instruments sound lowpassed or too silent, some others sound like sirens).

Someone else on discord (thanks to nymbus) could confirm the issues, so it's not limited to my PC.

This LMMS project file sounds right with 1.1.3, but the kicks and snares are unhearable with a build of stable 1.2.

We know the following:

• It's not related to effects.
• I think it's not related to that old linear/log regression issue.
• It happens with "zynaddsubfx" tracks.
• It does not happen with "Vibed" tracks.
• Saving the zyn instrument (not the track) in 1.0.3 and loading it again in zyn in 1.2.0, the drumset sounds all right.

This is all currently known. I'll add details when I have ideas about how to search for this bug. Maybe manual binary search is the last option...

[tresf]

Reproduced.

The par_real values have commas instead of decimal places periods. Putting the decimal places periods back fixes it. The software should be more sensitive to european decimal separators in general. Not sure when this got changed -- or -- if it's a Qt default that changed with the introduction of Qt5.

<MICROTONAL>
<string name="name">12tET</string>
<string name="comment">Equal Temperament 12 notes per octave</string>
<par_bool value="no" name="invert_up_down"/>
<par value="60" name="invert_up_down_center"/>
<par_bool value="no" name="enabled"/>
<par value="64" name="global_fine_detune"/>
<par value="69" name="a_note"/>
<par_real value="440,000000" name="a_freq"/>
<!------------ HERE ^ --------------------->
</MICROTONAL>

[JohannesLorenz]

Good find! Any volunteers for fixing this?

[tresf]

https://github.com/LMMS/lmms/blob/stable-1.2/plugins/zynaddsubfx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp#L508

See also markummitchell/engauge-digitizer#51

[tresf]

Patched via 32a7773. Testing appreciated.

Can someone crawl the presets for more commas? I have a feeling this isn't going to stop at just Zyn presets, but that's purely speculative.

Reminder

This change needs to be synced into lmms/zynaddsubfx repo after merged.

[musikBear]

Can someone crawl the presets for more commas?

@tresf Sure, but lets agree on the methodics, before i do something you did not want.
I will

• dl his project
• swap each of lmms defaults instruments with his zasfx
• run through the mmp, and mark all found commas with marker ink
• upload all mmp in a zip

Will that do? Else set up a plan :p

[tresf]

If anyone is aware of other areas in LMMS where commas are saved instead of periods, I'd like to know so that I can consider adding the patch to other areas of code. @musikBear, I'm not sure what you're offering.

[musikBear]

I'm not sure what you're offering. @tresf

'offering'... to test all factory instruments for wrong decimal-usage. by examine the saved mmp's with his project as template

[tresf]

I'm talking about someone doing a regex search through the codebase for decimals separated with a comma, @musikBear. If you want to simulate this by clicking for 2 hours knock yourself out.

[musikBear]

@tresf doing a regex search through the codebase

I looked at this:

<MICROTONAL>
<string name="name">12tET</string>
<string name="comment">Equal Temperament 12 notes per octave</string>
<par_bool value="no" name="invert_up_down"/>
<par value="60" name="invert_up_down_center"/>
<par_bool value="no" name="enabled"/>
<par value="64" name="global_fine_detune"/>
<par value="69" name="a_note"/>
<par_real value="440,000000" name="a_freq"/>
<!------------ HERE ^ --------------------->
</MICROTONAL>

Where you marked an error. Surely not in codebase but in a mmp where microtonal option is used in zasfx

If you want to simulate this by clicking for 2 hours knock yourself out.

FlashDev has tools for search and upmark in XML, but since you want a person to go throug the source-code, i am obviously not qualified 🙈

[JohannesLorenz]

If it helps, a typical approach for linux users would be:

grep '="[0-9]*,[0-9]\+"' -r ~/lmms/projects/ | grep -v 'par_real'

It means: grep every float number (0 or more digits, a comma, then 1 or more digits) inside of quotes (after an equal sign). grep it recursively over all projects. exclude (grep -v) "par_real", since this is from zyn.

For me, this leaded to no results, but I only use zyn, so someone else should execute that search command. If you get too much useless output, you can pipe another grep behind it:

... | grep -v 'whatever'

Any linux users out there with many non-zyn-projects?

[tresf]

@musikBear, see how @JohannesLorenz answered the exact question?

@JohannesLorenz the only problem is that we need to do lmms -d on all mmpz projects first -- I think. Also, did you mean to do projects or presets ? I would like to know if any projects or presets have the commas.

[tresf]

@JohannesLorenz sorry, I was talking generically about built-in projects and presets too.

[JohannesLorenz]

@tresf Good point, I forgot presets and mmpz files.

This script should do all of it. It should output all numerical values with comma in all projects and all presets.

It worked for me. Any more volunteers?

[tresf]

@JohannesLorenz tested and had some issues with the script. We have some projects with spaces that breaks the for loop. The built-in projects and presets generally aren't in ~/projects or ~/presets and since we generally recommend to build in a directory called build/, I changed it to accept the source directory instead.

https://gist.github.com/tresf/bfb05568eacb7a2664bdbe3a12131559
Presets work but projects, I'm not sure about...
It says this:

Grepping mmpz files...
sed: -e expression #1, char 72: unterminated `s' command
QIODevice::read (QFile, "/home/ubuntu/Desktop/lmms/data/projects/shorties/sv-DnB-Startup.mmpz

[JohannesLorenz]

@tresf Thanks for the issue reports.

I updated the script. Any volunteers to test it?

[tresf]

I'm just testing it on the built-in stuff and it's not working...

MacOS 10.13:

Grepping at most     1163 presets...
Grepping        3 mmp files...
Grepping       39 mmpz files (can take a while)...
awk: non-terminated string /Users/own... at source line 1
 context is
	 >>>  <<< 
awk: giving up
 source line number 2
awk: non-terminated string /Users/own... at source line 1
 context is
	 >>>  <<< 
awk: giving up
 source line number 2
mac:~ owner$ 

Ubuntu 14.04:

owner@ubuntu:~/lmms/build$ ./grep_for_comma ./lmms ~/lmms/data/presets/ ~/lmms/data/projects/
Grepping at most 1159 presets...
Grepping 3 mmp files...
Grepping 39 mmpz files (can take a while)...
awk: line 1: runaway string constant "/home/owne ...
awk: line 1: runaway string constant "/home/owne ...

[JohannesLorenz]

@tresf Thanks for retrying. It seems that just the awk is not working. Remove it from the file, it's only there for displaying the filenames. Assuming no comma will be found, it is not needed.

[tresf]

I guess I'm confused why I'm running this in the first place. I really don't know why you wouldn't just run this on your own machine. I'm not placing any more time into this request. If others find more issues, they can open a new bug report.

Here are other instances where we use toFloat or toDouble which could expose the same 1.0 versus 1,0 issue.

https://github.com/LMMS/lmms/search?utf8=%E2%9C%93&q=toFloat&type=
https://github.com/LMMS/lmms/search?utf8=%E2%9C%93&q=toDouble&type=

I'll merge the PR and move on to other things, I guess.

[JohannesLorenz]

I just compiled stable-1.2, and I can see the fix from @tresf is in, but it sounds like the bug is still active. Can please someone confirm or deny?

[PhysSong]

@JohannesLorenz Could you test if #4547 works?

[JohannesLorenz]

@PhysSong Yes, it works (only tested this special issue, i.e. loading an old zyn file)