Bitinvader - Fix saving with automation and division by 0

[zonkmachine]

Fixes #5799 and division by 0 detected with fpe debug flags.

Save and Load complete wavetable or you will face issues when the project is automating the Length knob. Hard coded wavetable length is defined in bit_invader.cpp. Maybe it should be in bit_invader.h but this looks fine and other plugins did it similarly.

The normalize function may divide with zero so we solve this the same way as the normalize function in Graph.cpp by starting off with a small value for max.

Testing

Use the steps to reproduce the issue from #5799 and especially try saving the project with Ctrl+s while the Length knob is at it's smallest setting when it's being controlled with an LFO.

• Add a Bit Invader and an LFO Controller.
• Connect the Length knob to the controller.
• Save and Reload multiple times and watch the graph of the Bit Invader get more and more destroyed in steps.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.28%2Bg3d8b882-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10734-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.28%2Bg3d8b882-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10734?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10733-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10733?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10730-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10730?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/y3bvlie9t6ubassj/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36520391"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dk40vjcamekv4142/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36520391"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10731-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.28%2Bg3d8b88284-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10731?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "ef2b6a110e8332cfb48a71ed63427eb9273ec695"}

[zonkmachine]

Alternatively start loadSettings( with clearing out the wavetable to do away with the multiple calls to m_graph.setLength() which maybe isn't very clear.

	// Clear wavetable before loading a new
	for (int i = 0; i < WAVETABLE_SIZE; i++)
	{
		m_graph.setSampleAt(i, 0.f);
	}

Edit: used this method instead, fixed!

[Spekular]

LGTM aside from one potential simplification.

[thmueller64]

I tested this PR and it fixes the problem!
For the code, I added some minor remarks.

[zonkmachine]

Reviewed and tested. I think we're done here. ;)

[zonkmachine]

Merge?

[DomClark]

There's something I'd like to check first if that's alright, but I'm a little busy right now. Will try to get round to it before the end of the day.

[zonkmachine]

I'm a bit unsure in what order things happen when you save. for instance, I think changing m_sampleLength in the middle of a save can not lead to any unvanted behaviours but I'm not completely sure.

[DomClark]

While this now saves the whole wavetable, it still doesn't load the whole wavetable. graphModel::setSamples only sets samples up to the current length of the wavetable, which is not necessarily the maximum length at the time of loading.

lmms/plugins/bit_invader/bit_invader.cpp

Lines 212 to 213 in 8cc9d8e

	m_graph.setLength( sampleLength );
	m_graph.setSamples( (float*) dst );

sampleLength is the current value of the length model, which may have been smaller when saved, or may be connected to a controller and forced to a lower value. I think the size parameter from base64::decode should be used to set the length before loading. Something like this perhaps?

int size = 0;
char* dst = 0;
base64::decode(_this.attribute( "sampleShape"), &dst, &size);

m_graph.setLength(size / sizeof(float));
m_graph.setSamples(reinterpret_cast<float*>(dst));
m_graph.setLength(sampleLength);

[zonkmachine]

Why not just set it to wavetableSize then? I think one of my first iterations of this fix looked something like this.

m_graph.setLength( wavetableSize );
m_graph.setSamples( (float*) dst );
m_graph.setLength( sampleLength );

[DomClark]

Why not just set it to wavetableSize then?

The base64 data may be smaller than the wavetable size, if loaded from an older project or a tampered project. This would read dst out of bounds.

[zonkmachine]

The base64 data may be smaller than the wavetable size

Yes, this is cool! I tried it with bumping the wavetable size to 256 and it worked fine loading older projects. I may open an issue about that. Wavetable synths on the market have sample sizes that are multiples of 2 and with a sample length of 256 we could load for instance Doepfer A112 samples into the Bit Invader.

[zonkmachine]

Now merge?