Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zyn presets sound different when the Zyn GUI is opened #7720

Open
1 task done
bratpeki opened this issue Feb 19, 2025 · 28 comments · May be fixed by #7737
Open
1 task done

Zyn presets sound different when the Zyn GUI is opened #7720

bratpeki opened this issue Feb 19, 2025 · 28 comments · May be fixed by #7737
Labels

Comments

@bratpeki
Copy link
Member

bratpeki commented Feb 19, 2025

System Information

Windows 10

LMMS Version(s)

A recent nightly

Most Recent Working Version

No response

Bug Summary

2025-02-19.21-59-14.mp4

An issue found by Cynthwave on Discord. Likely a lot of Zyn presets sound more bright until the GUI is opened.

I believe this is directly related to #7381, LMMS/zynaddsubfx#22 and LMMS/zynaddsubfx#23, where I attempted to disable the lowpass filter applied to sounds by default.

The idea behind those PRs was to disable the lowpass, but it slipped my mind that there could be old presets reliant on Zyn's default filter values, or something like that.

@JohannesLorenz @tresf Would it be smart to revert these changes, given the damage they've done to factory presets? Or maybe there could be some fix?

MORE INFO AT #7720 (comment).

Expected Behaviour

The sound doesn't change when the Zyn GUI is opened.

Steps To Reproduce

In the video.

Logs

Screenshots / Minimum Reproducible Project

No response

Please search the issue tracker for existing bug reports before submitting your own.

  • I have searched all existing issues and confirmed that this is not a duplicate.
@bratpeki bratpeki added the bug label Feb 19, 2025
@messmerd
Copy link
Member

You should verify whether default values are saved in Zyn presets or not. If they aren't, that sounds like a problem that should be fixed.

@tresf
Copy link
Member

tresf commented Feb 19, 2025

Pinging @LostRobotMusic as he was a part of the review process that lead to the PR suspectected to have caused this.

@bratpeki
Copy link
Member Author

bratpeki commented Feb 19, 2025

Looking at the recent nightly I've on my system.


I've made two square wave patches and set FREQ to 64 and 127. They worked as expected.

I then made a square wave patch and set this knob to 64:

Image

Both situations have made the square waves sounds as expected when the preset was loaded in and played without opening anything additonal.

@tresf
Copy link
Member

tresf commented Feb 19, 2025

An issue found by Cynthwave on Discord. Likely a lot of Zyn presets sound more bright until the GUI is opened.

Just a friendly reminder to baseline test this against stable / alpha to ensure you're not uncovering an unrelated bug.

@bratpeki
Copy link
Member Author

Upon further investigation, it seems the Zyn presets are stored as XIZ files, which are the ZynAddSubFX patch format. This is different to XPF's, which are LMMS' preset files. It's very possible the external information (FREQ) isn't read as a result of that.

Just a friendly reminder to baseline test this against stable / alpha to ensure you're not uncovering an unrelated bug.

Downloading and checking now!

@bratpeki
Copy link
Member Author

bratpeki commented Feb 19, 2025

Stable and alpha play their presets correctly. It's only nightly that exhibits this behaviour of "bright sound - open Zyn GUI - close Zyn GUI - original sound".

@bratpeki
Copy link
Member Author

My idea is that there are three solutions:

  1. One possible solution might be checking for LMMS version numbers when loading a presets and setting filter accordingly, but that's a lot of work, since you'd have to check that the name matches any and all commits from that LMMS PR upto now.

  2. Alterantively, we move everything from XIZ to XPF. This would unify all of our plugins to work under one format, I'd hope, but is also a lot of work and careful checking.

  3. A revert. Let the user change the filter position every time. Evidently, this is an issue for XIZ files, so if any user finds Zyn banks online, and they aren't XPF, this could be an issue.

@tresf
Copy link
Member

tresf commented Feb 19, 2025

So the XIZ... is LMMS loading it wrong or is Zyn loading it wrong? The patch that keeps the correct behavior is always preferred.

No batching of XIZ -> XPF please, we get those from upstream so this would be a maintenance headache.

@JohannesLorenz
Copy link
Contributor

IIRC no one knows yet what exactly causes the bug? If so, this should be found out before making a decision? I could offer looking at it.

@bratpeki
Copy link
Member Author

That would be great, @JohannesLorenz, thank you!

@musikBear
Copy link

The most important here is to keep backward compatibility, and make sure that no existing projects are messed up. IMO the benefit from changing this zasfx feature is minimal at most. What is the benefit from changing this? User has complete control over output. If the filter is opened completely there are no LP effect on the sound at all. Its also important to remember that for user-made presets LP is not always the default filter because that can be changed as well. In fact i believe even in the factory presets a few of them does not respond to cutoff.
The reason this problem exists at all, is that zasfx was the only vst-instrument that had access to controlling any settings at all. That UI was a blessing back then. I remember i made an enhancement request where more of the vst-dials could be connected to an extended LMMS-UI, because there is an empty tab where we default has envelopes, those are missing in zasfx, all of the options on this special zasfx-UI are only what TobyDox felt was most interesting, and here it could be argued that envelopes would have been more obvious to include than filter-controls. The real issue is that from once being the most/only controllable vst-instrument, zasfx is now the least controllable vst, because all other vsts has the 'wrench'-UI.
TL:DR: Why change a working feature?

@tresf
Copy link
Member

tresf commented Feb 20, 2025

What is the benefit from changing this?

Quoting @bratpeki from LMMS/zynaddsubfx#22 (comment):

Essentially, the additive synthesizer in LMMS, by default, has a low-pass filter applied to its global parameters, which is quite odd and confuses a lot of newcomers, as evidenced by the interactions in the LMMS Discord server.

Since in Zyn, there must always be a filter applied, this PR aims to simply move the frequency of the low-pass frequency to it's maximum value, practically disabling it.

GeekyGami's ZynAddSubFX YouTube video has a section specifically dedicated to disabling the low-pass before you start working on a patch. The section is appropriately titled "Turn the filter off before working on stuff, it's on by default."

This is fixed in upstream Zyn, but it uses a completely new mechanism, as you pointed out in the Discord convo we've had, so I think this would be a nice solution until Zyn is moved to version 3.x.

Related discussions:

@bratpeki
Copy link
Member Author

Essentially, it was made to be a QOL feature, since having a filter on by default is already making a modification to your sound. These PRs were meant to solve that.

I would think they didn't break backwords compat, as project patches are read with filter data ("I've made two square wave patches and set FREQ to 64 and 127. They worked as expected").

The issue is just that they don't read XIZ files with the appropriate filter data until the GUI is opened.

If @JohannesLorenz doesn't figure out what's wrong, we can just revert.

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Feb 20, 2025

The bug is being misdiagnosed here.

The issue being noticed is that XIZ presets sound brighter by default than they did in previous versions, which is true.

However, opening the GUI does not give it the appropriate filter data. If you look back in the LMMS Zyn window, you'll notice the frequency knob is at 127, and yet it sounds like it's much lower. If you move the filter frequency knob in the LMMS window, Zyn will snap back to the correct value, revealing the bug. This means the instrument after opening the GUI is bugged, not before.

So, there are actually two bugs here:

  1. Before LMMS's filter frequency knob is changed for the first time, Zyn's filter frequency is incorrect after the GUI is opened. This bug can be fixed by placing updateFilterFreq(); at the end of ZynAddSubFxInstrument::loadSettings.
  2. XIZ presets are loaded in with LMMS's filter frequency knob set to 127. This can be fixed by special-casing XIZ presets to have a filter frequency knob value of 64 instead of 127.

This all means there's no reason to worry, and backwards compatibility is not in any danger.

@JohannesLorenz
Copy link
Contributor

I would have set m_modifiedControllers[C_filtercutoff] = true in ZynAddSubFxInstrument::saveSettings - that fixes the bug equally, but fixes another bug where the modified controller is not saved.

@JohannesLorenz
Copy link
Contributor

XIZ presets are loaded in with LMMS's filter frequency knob set to 127. This can be fixed by special-casing XIZ presets to have a filter frequency knob value of 64 instead of 127.

When saving a new XIZ, however, you would expect it to be loaded with filter frequency 127, right? So, when loading an XIZ, LMMS needs to decide whether an XIZ is old or new. How can it do that, other than carrying a list of those old XIZ? File date does not seem reliable, and the path might also not work if we put new instruments into the path.

@bratpeki
Copy link
Member Author

Indeed, only XPFs would store such data. I'm +1 for a revert of those commits, simply because the solutions are too much work for a not-so-important feature.

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Feb 22, 2025

When saving a new XIZ, however, you would expect it to be loaded with filter frequency 127, right?

I don't think this is an issue. Even with the behavior prior to these PRs, if the user ever moves that knob at all, the XIZ preset is going to play back differently than how it was saved. If the user wants to save the LMMS knob data, it needs to be saved as an LMMS preset.

@musikBear
Copy link

it needs to be saved as an LMMS preset

Yes, and afair lmms-presets also save microtonal data in 1.3 -right.
Have to admit that i never saved in zasfx own format. All my zasfx presets are LMMS-saved.

@JohannesLorenz
Copy link
Contributor

Should we at least try to fix it in a PR? If yes, who? After all, you came up with the first fix proposal, @LostRobotMusic , so feel free to submit one. Otherwise, I can try, too.

@LostRobotMusic
Copy link
Contributor

You can go ahead and take it if you'd like, I'm unfortunately busy with health-related things over the next couple weeks so I'm not sure if I have the time to set it all up.

@bratpeki
Copy link
Member Author

@JohannesLorenz

When saving a new XIZ, however, you would expect it to be loaded with filter frequency 127, right?

The LMMS GUI had the frequency set to 64 before PR 7381. Since that's what old presets rely on, and a user can change the values for his own XPF presets, I believe it should be 64.

@bratpeki
Copy link
Member Author

Even with the behavior prior to these PRs, if the user ever moves that knob at all, the XIZ preset is going to play back differently than how it was saved.

That's true, yeah, but that's expected behavior. If you don't save the LMMS part of the patch, you don't get the changes made in the LMMS part of the patch.

@bratpeki
Copy link
Member Author

So, two practical fixes:

  1. Revert, which will make FREQ and Zyn filter values 64, ensuring proper playback.
  2. When previewing and loading an XIZ, use FREQ and Zyn filter values 64. For newly made Zyn instrument tracks in LMMS, use FREQ 127 and filter frequencies 100. For XPFs use what's in them.

@bratpeki
Copy link
Member Author

bratpeki commented Feb 28, 2025

2025-02-28.11-17-15.mp4

One of my projects.

Changing the FREQ knobs shows what it would actually sound like with FREQ = 127, whereas without ever having touched it, it plays as FREQ = 64.

I believe this is what Lost was talking about here: #7720 (comment). Also related: #7720 (comment).

@bratpeki
Copy link
Member Author

@JohannesLorenz, opinions on which option we should take? Revert or fix?

@JohannesLorenz
Copy link
Contributor

Sorry, this week was rather busy. I will setup a fix tonight, and we can test. If it will not work, we can still reject it and revert.

@bratpeki
Copy link
Member Author

TYSM!

JohannesLorenz added a commit to JohannesLorenz/lmms that referenced this issue Feb 28, 2025
@JohannesLorenz JohannesLorenz linked a pull request Feb 28, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants