lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Warn about LADSPA problems

Open michaelgregorius opened this issue 1 year ago • 3 comments

Introduce a method which checks if a file that's loaded has the LADSPA controls saved in an old format that was written with a version before commit e99efd541a9.

The method is run at the end so that problems in all file versions are detected. If a real upgrade was to be implemented it would have to run between DataFile::upgrade_0_4_0_rc2 and DataFile::upgrade_1_0_99.

See #5738 for more details.

michaelgregorius avatar May 18 '24 17:05 michaelgregorius

Nice! Maybe the pop-up message should mention that you can try and save the project on a previous version of lmms like 1.1 or 1.2 ? At least it seem to have worked for Greshz-CoolSnip.

zonkmachine avatar May 18 '24 18:05 zonkmachine

@michaelgregorius thanks for this warning. Some initial thoughts:

  • Like other plugin-specific warnings, we've tried to move towards context-aware. If a hint as to the plugin name can be provided in the error, this would be much appreciated.
  • // This is not an upgrade. Can it be a downgrade then? Put simpler, can we disable/mute this plugin as a courtesy to the user?

From what I understand, this PR can be close (or if merged first, reverted) if we find a way to patch this behavior, which last I remember was held up in #5738 awaiting some feedback from @JohannesLorenz.

tresf avatar May 20 '24 15:05 tresf

Hi @tresf,

* Like other plugin-specific warnings, we've tried to move towards context-aware.  If a hint as to the plugin name can be provided in the error, this would be much appreciated.

In this case the routine would have to collect a list of the names of all affected plugins and show them in the message. This should be feasible.

* `// This is not an upgrade`.  Can it be a downgrade then?  Put simpler, can we disable/mute this plugin as a courtesy to the user?

The comment was rather meant as a hint in that it is not a classical upgrade routine which moves data around but rather something that detects problems and warns the users about them.

Muting the affected plugins could also be an option as it should be similar to the collection of the names as in that we'd have to find the parent that can be muted instead of finding the parent with the name. In that case a combination might be nice where the error message states names of the affected plugins that have been muted.

From what I understand, this PR can be close (or if merged first, reverted) if we find a way to patch this behavior, which last I remember was held up in #5738 awaiting some feedback from @JohannesLorenz.

Exactly! Fixing this problem for good would be the better option than showing warning messages to the users. The latter might undermine the users trust in the application.

michaelgregorius avatar May 20 '24 16:05 michaelgregorius

I found a very large savefile from 2013, i.e. before toby's commit, which has stuff like this in the MMP:

            <ladspacontrols port10="4.41" port00="4.41" port11="0.756525" port01="0.756525" link="1" port00link="1" ports="4" port01link="1">
              <connection>
                <port00 id="5"/>
              </connection>
            </ladspacontrols>

Both the date and your code (because my savefile has a ladspacontrols tag which has attributes starting on "port" and are not "ports") imply that the warning should come? But I do not see it.

JohannesLorenz avatar Jun 30 '24 23:06 JohannesLorenz

@JohannesLorenz, can you please provide the test file?

Or alternatively add code to dump the current state of DataFile at the start of DataFile::findProblematicLadspaPlugins and then load the file? The latter should be possible using the free function QDebug operator<<(QDebug dbg, const QDomNode& node) that can be found in DataFile.cpp.

It sounds like the file is fixed in between before it reaches DataFile::findProblematicLadspaPlugins.

michaelgregorius avatar Jul 01 '24 06:07 michaelgregorius

@JohannesLorenz, can you please provide the test file?

Sorry, @michaelgregorius , I forgot! Will do soon .

JohannesLorenz avatar Jul 20 '24 15:07 JohannesLorenz

@michaelgregorius Sorry again. The warning indeed comes at the correct moments. I tested this with 10 files, in all cases, it was right. I just confused some filenames when I wrote that post.

JohannesLorenz avatar Jul 20 '24 20:07 JohannesLorenz

Houston, we have a problem! :smile:

As @cyberrumor reported, if you click an XPF in the presets where the instrument has a LADSPA effect in the FX tab, the check is also active. This is weird, because

  1. You get a dialog in the middle of a Drag-Drop-Operation
  2. LMMS should not ship corrupted XPF files

I assume in this case, the best would be to fix the affected files - either by removing the port values (which is sad, because we corrupt the original files), or by fixing them (which might be more work and almost close to a complete fix for #5738 ).

Any opinions? (Looking mostly at @michaelgregorius)

JohannesLorenz avatar Nov 02 '24 19:11 JohannesLorenz

Here is the complete list of affected files:

$ find data/presets/ -type f -iname "*.xpf" -exec grep -H 'ladspacontrols.*port' '{}' \; | cut -d: -f1 | sort -u
data/presets/AudioFileProcessor/Erazor.xpf
data/presets/AudioFileProcessor/orion.xpf
data/presets/AudioFileProcessor/SString.xpf
data/presets/BitInvader/alien_strings.xpf
data/presets/BitInvader/drama.xpf
data/presets/BitInvader/invaders_must_die.xpf
data/presets/BitInvader/pluck.xpf
data/presets/Kicker/Clap dry.xpf
data/presets/Kicker/Clap.xpf
data/presets/Kicker/SnareMarch.xpf
data/presets/LB302/AcidLead.xpf
data/presets/LB302/AngryLead.xpf
data/presets/LB302/STrash.xpf
data/presets/Monstro/Growl.xpf
data/presets/Monstro/HorrorLead.xpf
data/presets/Monstro/Phat.xpf
data/presets/Monstro/ScaryBell.xpf
data/presets/Nescaline/Detune_lead.xpf
data/presets/Nescaline/Fireball_flick.xpf
data/presets/OpulenZ/Bagpipe.xpf
data/presets/OpulenZ/Combo_organ.xpf
data/presets/OpulenZ/Funky.xpf
data/presets/OpulenZ/Harp.xpf
data/presets/OpulenZ/Organ_leslie.xpf
data/presets/OpulenZ/Square.xpf
data/presets/Organic/Pwnage.xpf
data/presets/Organic/Rubberband.xpf
data/presets/SID/CheesyGuitar.xpf
data/presets/SID/MadMind.xpf
data/presets/SID/Overdrive.xpf
data/presets/TripleOscillator/Bell.xpf
data/presets/TripleOscillator/BrokenToy.xpf
data/presets/TripleOscillator/CryingPads.xpf
data/presets/TripleOscillator/DetunedGhost.xpf
data/presets/TripleOscillator/DirtyReece.xpf
data/presets/TripleOscillator/Drums_HardKick.xpf
data/presets/TripleOscillator/ElectricOboe.xpf
data/presets/TripleOscillator/E-Organ2.xpf
data/presets/TripleOscillator/Erazzor.xpf
data/presets/TripleOscillator/FuzzyAnalogBass.xpf
data/presets/TripleOscillator/Garfunkel.xpf
data/presets/TripleOscillator/GhostBoy.xpf
data/presets/TripleOscillator/OldComputerGames.xpf
data/presets/TripleOscillator/PercussiveBass.xpf
data/presets/TripleOscillator/Play-some-rock.xpf
data/presets/TripleOscillator/PMbass.xpf
data/presets/TripleOscillator/PMFMFTWbass.xpf
data/presets/TripleOscillator/PM-FMstring.xpf
data/presets/TripleOscillator/PowerStrings.xpf
data/presets/TripleOscillator/SEGuitar.xpf
data/presets/TripleOscillator/SquarePing.xpf
data/presets/TripleOscillator/Supernova.xpf
data/presets/TripleOscillator/SuperSawLead.xpf
data/presets/TripleOscillator/TINTNpad.xpf
data/presets/TripleOscillator/WarmStack.xpf
data/presets/Watsyn/Epic_lead.xpf
data/presets/Watsyn/Pulse.xpf
data/presets/Xpressive/Dream.xpf

These are 58 of 175 files, but the number of occurrences is higher: 122. I.e. every 3rd file is affected, 2 issues per affected file.

JohannesLorenz avatar Nov 02 '24 19:11 JohannesLorenz

Hi @JohannesLorenz,

it seems to depend on how old the presets are that have been copied into the users directory on the first start. If I for example check the presets for TripleOscillator using the ones found in the current master then only the preset TripleOscillator/E-Organ2.xpf shows the warning for me. However, I guess if users have started on an older version then they might see the warning for more presets.

Here's the list of preset that show the warning in master:

  • LB302/STrash
  • OpulenZ/Organ_leslie
  • TripleOscillator/E-Organ2

I have not checked all of the gazillion ZynAddSubFX presets but they do not appear in your list at all and for the first few dozens that I have tested I did not get the warning.

I guess the best solution would be to fix the problem for good so that users with older presets are not affected either.

michaelgregorius avatar Nov 02 '24 22:11 michaelgregorius

Good point @michaelgregorius , my regex was wrong: port[^s] instead of port (and the zyn presets cannot be affected - XIZ and XMZ are zyn specific file endings, they cannot contain LADSPA references). Then this confirms your list of only 3 plugins. In this case, a manual fix would also be sufficient. Though I would like to take the challenge to write a scripted fix for those 3 - unless you want to fix them?

JohannesLorenz avatar Nov 02 '24 22:11 JohannesLorenz

[...] In this case, a manual fix would also be sufficient. Though I would like to take the challenge to write a scripted fix for those 3 - unless you want to fix them?

I have no ambitions doing that, so please go ahead! :sweat_smile:

michaelgregorius avatar Nov 02 '24 23:11 michaelgregorius