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

Made it so that AddSynth isn't lowpassed by default #22

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

bratpeki
Copy link
Contributor

I've made a slight change of setting the global AddSynth frequency from 94 to 127, so that the sound isn't lowpassed by default. It's a one line change I think would benefit users.

@tresf
Copy link
Member

tresf commented Jul 12, 2024

I've made a slight change of setting the global AddSynth frequency from 94 to 127, so that the sound isn't lowpassed by default. It's a one line change I think would benefit users.

Hi, I haven't tested this patch, so I'm sorry if this would otherwise be obvious, but do you mind explaining in slightly more detail what detriment that the default value has? Also, if this is already fixed upstream, that would help justify its inclusion. We're OK accepting minor patches since we do not know how soon we'll be able to update to Zyn Fusion.

@bratpeki
Copy link
Contributor Author

I don't mind explaining at all!

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.

image

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.

@bratpeki
Copy link
Contributor Author

Also, this repo and the LMMS submodule are pointing to the same commit, so this would be a minor QOL change that could be made effective immediately, since there were no other changes inbetween.

@tresf
Copy link
Member

tresf commented Jul 13, 2024

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.

Thanks for this information, this warrants its inclusion downstream until we're able to update.

"Turn the filter off before working on stuff, it's on by default."

Roger that. Merging.

Thanks kindly for this attention to detail. Note, the Zyn submodule at LMMS will need to be bumped after this is merged for it to appear in nightly builds.

@tresf tresf merged commit eca2dcf into LMMS:master Jul 13, 2024
@JohannesLorenz
Copy link
Contributor

It happens seldom that a PR is merged in ~ 24 hours. I had 2 questions in mind:

  1. Why not changing SUB and PAD?
  2. This has been adjusted on zyn's master, too, right? We should use the same values as there, to avoid merge conflicts or user confusion?

@JohannesLorenz
Copy link
Contributor

Repo-related question @tresf : Can we require approval prior to merge in this repo? Or who can do that?

@tresf
Copy link
Member

tresf commented Jul 13, 2024

Repo-related question @tresf : Can we require approval prior to merge in this repo? Or who can do that?

Done.

@tresf
Copy link
Member

tresf commented Jul 13, 2024

avoid merge conflicts

This is unavoidable per OP's analysis:

[...] upstream [...] uses a completely new mechanism

...

It happens seldom that a PR is merged in ~ 24 hours. I had 2 questions in mind:

This only affects people making their own instruments, so I see it as a small, sensible change. If you think I merged this too quickly, I can hold off on these types of thing, but PRs -- especially one-liners -- can often take weeks, months or years to get merged if we require TOO much approval. I believe @bratpeki offered up exactly what's needed to justify this change and I believe this change to be minor enough as to not negatively impact users, even after we switch to Zyn Fusion.

@JohannesLorenz
Copy link
Contributor

No big problems with merging it so fast, just with me being too slow 😆

[...] upstream [...] uses a completely new mechanism

Fair enough, though I think we could still use the same values as on upstream to make it more "common" to the users.

@bratpeki
Copy link
Contributor Author

I discussed this some more on the dev channels with Lost and GeekyGami. Both Sub and Pad should also be changed! I'll get to looking at what the default values are and update this PR, if possible! Thank you!

This pull request was closed.
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