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

Fix ZynAddSubFX preset regression #7737

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Feb 28, 2025

Attempting to fix #7720 . I think this is wrong yet, because the VK in Zyn has different filter freq than the Piano in LMMS.

Please test or give feedback.

Credits to the idea of this fix go to LostRobot.

@JohannesLorenz
Copy link
Contributor Author

JohannesLorenz commented Mar 1, 2025

I found out why the velocities are different:

The Zyn VK uses a velocity of 100 by default. However, the LMMS piano seems to use 0...63 by default. This seems to be an issue on master.

Can please anyone test this PR?

@JohannesLorenz
Copy link
Contributor Author

JohannesLorenz commented Mar 1, 2025

Testing notes:

There are a variety of things to test in different combinations:

  • Loading an old XIZ (like in the issue description)
  • Loading a newly saved XIZ (you can save one using the "floppy" button in the Instrument Track Window)
  • Loading an old song
  • Loading a newly saved song

All these should be tested both without and with GUI open

So there is really a LOT to test 🤣

Also, this PR lets old preset load with old filter cutoff, after this
had been changed in 9c0fc8f .

Co-authored-by: Lost Robot <[email protected]>
@JohannesLorenz JohannesLorenz marked this pull request as ready for review March 1, 2025 16:42
@messmerd messmerd changed the title Attempt to fix #7720 Fix ZynAddSubFX preset regression Mar 2, 2025
@bratpeki bratpeki self-assigned this Mar 11, 2025
@bratpeki
Copy link
Member

Testing notes

Thank you, thank you and thank you! 😆

The Zyn VK uses a velocity of 100 by default. However, the LMMS piano seems to use 0...63 by default. This seems to be an issue on master.

The LMMS instrument piano uses a fixed value set here:

image

We could set them both to the same value to make it uniform across both keyboard, though.


From a technical standpoint, could you tell me what changes have been made in this repo and in the Zyn repo, if any were made there? If it's enough to just read the discussion of the issue to get the full picture I'll do that!

At first glance, this looks really fine!

@JohannesLorenz
Copy link
Contributor Author

The Zyn VK uses a velocity of 100 by default. However, the LMMS piano seems to use 0...63 by default. This seems to be an issue on master.

This PR is about filter cutoff frequency, these things are usually different. But I guess you mean to fix this as a separate issue?

From a technical standpoint, could you tell me what changes have been made in this repo and in the Zyn repo, if any were made there? If it's enough to just read the discussion of the issue to get the full picture I'll do that!

All changes are made in LMMS:

  1. Upon reloading parameters (which happens when a GUI is opened/closed), the controllers in Zyn and LMMS were different (due to your previous change which raised the cutoff freq) - This is now fixed by letting zyn save the controller as "modified" when saving, so at reload time, it is synced between Zyn and LMMS.

  2. While you changed the cutoff for new instruments, you also did so for old instruments, which made them sound wrong. So this PR lets you load XIZ files with the old filter cutoff.

Copy link
Member

Choose a reason for hiding this comment

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

This fix seems kind of hacky.

Maybe it would be better to add a "version" (or "defaults"?) attribute to the saved settings? You'd save it as version=1, and when loading settings, if no "version" attribute exists it would default to version=0.

Correct me if I'm wrong, but it seems that the root cause of the issue is that default parameter values are not saved (or loaded). So if you had a "version" attribute, it could be used to know which set of defaults are expected and use them when loading. In the future if the defaults changed again, you would simply increment the version and update the version handling code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree that a version attribute is required for a proper fix, however, it seems it was commonly agreed that it should not be done.

To me, a version attribute, i.e. changing our savefiles, is the point where the work we put into it does not justify the advantage of having the cutoff frequency higher. Also, when we switch to Lv2/CLAP zyn, this can be used as an implicit "version" attribute.

So I think the time is better invested in plugin development, and we should rather completely revert the whole cutoff commits from peki and me?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if we have to add versioning for a total fix, let's just revert, TBH.

@bratpeki
Copy link
Member

bratpeki commented Apr 12, 2025

Tresf's Discord comment says we're good to go for a revert!

Making it now.


All the related PRs are listed here.

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.

Zyn presets sound different when the Zyn GUI is opened
3 participants