zynaddsubfx
zynaddsubfx copied to clipboard
LMMS
Update instruments submodule
We need to verify that each instrument's creator version is smaller than the LMMS zyn version. Did you verify that?
This seems rather benign of a change. Of course @JohannesLorenz will know the technicalities of this, but @zonkmachine asked for a description and although the PR is ~~not~~ now labeled properly, @JohannesLorenz's question raises some eyebrows about compatibility.
@Snowiiii Is this mainly to help with some bugged presets in LMMS?
@JohannesLorenz with regards to "createrVersion", is this strictly necessary? For example, if a preset's settings were tweaked with a newer Zyn version does it completely break in older Zyn or is this statement one made out of paranoia?
In my opinion, the best way to test this PR is to simply build LMMS with these presets and make sure that they sound proper (e.g. before.wav vs. after.wav, but due to the number of presets that ship with Zyn and the lack of a framework for testing audible similarities, we can't accommodate this, at least not easily or with the tools we have readily available.
It is not paranoia 😁 It is an update over 10 years, in that time, for sure, we added new stuff, fixed compatibility etc. If we merge this and not exclude the new instruments, many users might report bugs with broken instruments.
It is not paranoia 😁 It is an update over 10 years, in that time, for sure, we added new stuff, fixed compatibility etc. If we merge this and not exclude the new instruments, many users might report bugs with broken instruments.
Right, but as a project we need to know the scope of the breaks versus the fixes that this PR aims to achieve. I agree, we want it to sound correct, but in your experience, how many breaking sound changes to presets have occured in those 10 years? Perhaps a better question... how common was it to update the presets to the new version to fix a particular bug? Or maybe a better better question... Can we just see a diff of the files in question and try to read the changes?
I tested the latest instruments in LMMS and compared them with the old ones, They sound all the same and nothing is buggy or broken. They added alot of cool new Instruments
I tested the latest instruments in LMMS and compared them with the old ones, They sound all the same and nothing is buggy or broken. They added alot of cool new Instruments
This is sort of my instinct as well. Since @JohannesLorenz contributes to the Zyn project upstream, I don't want to make any false assumptions though.
how many breaking sound changes to presets have occured in those 10 years? Perhaps a better question... how common was it to update the presets to the new version to fix a particular bug? Or maybe a better better question... Can we just see a diff of the files in question and try to read the changes?
422 files have changed, too much to just view them.
I talked to fundamental, and also looked at the git log. It seems unlikely that any of those commits are related to code changes, they are just related to intended sound changes (not a programmer thing, but an artist thing).
Nonetheless, if any of those 2024-instruments (changed or added) uses features that are not available in 2014-zyn, this can cause crashes, or even speaker/hearing damage (unlikely, but possible). I am not sure if this is acceptable for us in LMMS (I think rather no). But the alternatives are:
- Rejecting this PR here
- Testing 422 instruments manually
- Waiting until we update the zyn core - I do not remember why we waited with this, @tresf do you? Might be because Lv2 and CLAP will come anyways, but IIRC we wanted to keep zyn even with Lv2/CLAP? Or did we?
Waiting until we update the zyn core - I do not remember why we waited with this, @tresf do you? Might be because Lv2 and CLAP will come anyways, but IIRC we wanted to keep zyn even with Lv2/CLAP? Or did we?
In short, waiting on Lv2, etc. The long: https://github.com/LMMS/lmms/issues/1860
In short, waiting on Lv2, etc. The long: LMMS/lmms#1860
So, it is indeed planned to remove the zyn submodule from LMMS?
In short, waiting on Lv2, etc. The long: LMMS/lmms#1860
So, it is indeed planned to remove the zyn submodule from LMMS?
I think the end-game is to remove our own fork, yes, but not necessarily remove it as a bundled plugin, however we decide to do that.
422 files have changed [...]
to break down this 422 number:
- 160 are newly added
Cris Owl Alvarezbanks, ⚠️ one of which crashes Zyn on playback.banks/Cris Owl Alvarez/0131-FX heavy rain.xizcauses the remote process to crash on playback.
- 128 are newly added
net-wisdombanks, none of which crash Zyn on playback. - 49 are newly added
olivers-otherbanks, none of which crash Zyn on playback. - The remaining 85 changed presets are in the
olivers-100andCompanionbanks, none of which crash Zyn on playback.- to @JohannesLorenz's credit, there are some differences... For example,
olivers-100/0007-FM Saxophone 2.xizsounds considerably different, as doesolivers-100/0010-FM Brass 2.xiz. Someone with Zyn Fusion would have to confirm if these are artistic differences or regressions caused by missing features.
- to @JohannesLorenz's credit, there are some differences... For example,
... so this reduces the scope of "change" drastically.
More thoughts...
- What may be a bigger concern is that if we do merge presets with future unmapped features, it has the potential to cause future regressions (progressions?) when the unmapped features become available in a future Zyn release. Comically, we could handle this as an upgrade routine if people complain by stripping out any features that didn't exist when loading old projects, mimicking the broken sound.
- Although I tested playback, I didn't attempt to load any of the new presets using the Zyn UI (I'm on Mac and the Zyn UI appears to be broken on master). Someone else sppot-checking the UI is a good idea.
[...] too much to just view them.
I'm interested in knowing how to "view" them
With all that said, I think the PR needs justification if we're to dive further. For example, @Snowiiii are there certain presets in particular that you feel would benefit LMMS?
to break down this 422 number:
First of all, thanks for breaking it down and inspecting. If I got you right, you loaded each and every of those and even checked if there are no bad sounds (e.g. no over-distorted sounds, no popping, clipping etc). If this is the case, then that is already a good justification for the PR.
there are some differences...
For me, this is OK. LMMS users will rarely use these few patches, and if they will, their songs will sound "better". For those who will want to keep the original sounds - they could still fetch the old patch from the old instruments commit.
What may be a bigger concern is that if we do merge presets with future unmapped features, it has the potential to cause future regressions (progressions?) when the unmapped features become available in a future Zyn release. Comically, we could handle this as an upgrade routine if people complain by stripping out any features that didn't exist when loading old projects, mimicking the broken sound.
Writing such routines would work, but would be that complicated that it IMO does not justify this PR here. Alternatively, we just tell the users: "Patch your savefiles manually" - again, I guess these are all edge cases. How many users will really use those specific patches and will be unhappy about it being broken?
Although I tested playback, I didn't attempt to load any of the new presets using the Zyn UI (I'm on Mac and the Zyn UI appears to be broken on master). Someone else sppot-checking the UI is a good idea.
For both UIs, they work relatively separate. Zyn first loads the patch into the core. In our case, after loading, the patch inside the core would be 2.4 compatible. Then the 2.4 UI reads from the 2.4 core. So, I do not expect such an issue.
All in all, I consider this PR doable. There might be few edge cases, but users should be able to handle them?
First of all, thanks for breaking it down and inspecting. If I got you right, you loaded each and every of those and even checked if there are no bad sounds (e.g. no over-distorted sounds, no popping, clipping etc). If this is the case, then that is already a good justification for the PR.
Correct, and I had headphones in. banks/Cris Owl Alvarez/0131-FX heavy rain.xiz is still a problem child tho as it temporarily deadlocks LMMS while (I assume) it tries to recover from the crashed process. 😆
banks/Cris Owl Alvarez/0131-FX heavy rain.xizis still a problem child
We could either create another fork (of submodule instruments) which does not have that instrument, or create a ~blacklist~ denylist for non-working instruments.
banks/Cris Owl Alvarez/0131-FX heavy rain.xizis still a problem childWe could either create another fork (of submodule
instruments) which does not have that instrument, or create a ~blacklist~ denylist for non-working instruments.
I'd vote for the denylist? Unless it's a quick patch in Zyn 2.4. I think forking instruments is a bit too much entropy.
I'd vote for the denylist? Unless it's a quick patch in Zyn 2.4. I think forking instruments is a bit too much entropy.
I would agree. However, that instrument loads for me - it just takes a few seconds. Does this crash for you in standalone-zyn (with the newest zyn from master)?
I'd vote for the denylist? Unless it's a quick patch in Zyn 2.4. I think forking instruments is a bit too much entropy.
I would agree. However, that instrument loads for me - it just takes a few seconds. Does this crash for you in standalone-zyn (with the newest zyn from master)?
In that case, let's merge. I had only assumed a crash based on the very slow load time.