Skip to content

Conversation

@messmerd
Copy link
Member

@messmerd messmerd commented Jul 18, 2025

This PR removes the gate knob from effects, effectively hard coding it to 0.

I also removed the gate's XML attribute from the preset files and mmp project files. This isn't strictly necessary but I just wanted to be thorough.

Out of all the presets, only Monstro's Growl.xpf and HorrorLead.xpf used gate values that were non-zero, though I could not hear any difference with them set to 0. And when autoquit is disabled (as it has been by default since #4378 over 5 years ago), there is no difference anyway.

I also made a couple performance improvements:

  • Previously the RMS calculation was performed even when auto-quit was disabled, which meant that expensive calculation was completely wasted. Now the calculation is only performed when auto-quit is enabled.
  • I replaced the RMS calculation (which involves squaring every sample in every frame for almost all effect instances) with a simpler method. Now it just checks for any samples that exceed the threshold, and early returns as soon as it sees the buffer is not quiet. There is no squaring and most of the time it will not be looping over the entire buffer. If there is any impact from this change on the sound of a project, I imagine it would be negligible.

Supersedes #7486

Next step

After this, I'd like to convert the Decay knob for effects into a context menu entry. Because it's such a niche feature, it should not be taking up valuable space in the effect views. Once in the context menu, it would no longer be automatable in new projects, though I don't think that's a problem. And it would still be backwards compatible with any old project out there that automated it for whatever reason.

After that is done, #7438 will no longer need to conditionally display the Decay knob.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird compile error I am getting trying to test the PR. I believed this was already reported by someone else though:

/home/saker/projects/lmms/plugins/ZynAddSubFx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp:199:24: error: no member named 'F_OPEN_UTF8' in namespace 'lmms'
  199 |     FILE *file = lmms::F_OPEN_UTF8(std::string(filename), "w");
      |                  ~~~~~~^
/home/saker/projects/lmms/plugins/ZynAddSubFx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp:321:59: error: no member named 'F_OPEN_UTF8' in namespace 'lmms'
  321 |     gzFile gzfile  = gzdopen(lmms::fileToDescriptor(lmms::F_OPEN_UTF8(filename, "rb")), "rb");

@sakertooth
Copy link
Contributor

sakertooth commented Jul 20, 2025

Weird compile error I am getting trying to test the PR. I believed this was already reported by someone else though:

/home/saker/projects/lmms/plugins/ZynAddSubFx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp:199:24: error: no member named 'F_OPEN_UTF8' in namespace 'lmms'
  199 |     FILE *file = lmms::F_OPEN_UTF8(std::string(filename), "w");
      |                  ~~~~~~^
/home/saker/projects/lmms/plugins/ZynAddSubFx/zynaddsubfx/src/Misc/QtXmlWrapper.cpp:321:59: error: no member named 'F_OPEN_UTF8' in namespace 'lmms'
  321 |     gzFile gzfile  = gzdopen(lmms::fileToDescriptor(lmms::F_OPEN_UTF8(filename, "rb")), "rb");

Seems like my Zyn submodule wasn't updated.
Edit: PR tested now, runs fine.

Copy link
Contributor

@allejok96 allejok96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment on the threshold calculation is that it looks Sufficiently Small ™

The other code changes look sane.

I tested playing a song with the setting on and off. The Gate knob is gone and the graying out of Decay works. My ears couldn't notice any difference, but my CPU graph could.

@messmerd messmerd merged commit c86fe78 into LMMS:master Jul 22, 2025
10 of 11 checks passed
@messmerd messmerd deleted the remove-gate branch August 6, 2025 18:05
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

Successfully merging this pull request may close these issues.

3 participants