Instrument base note velocity setting ignores LedCheckbox and isn't copied when cloning instrument

[LostRobotMusic]

In the instrument MIDI tab, the base note velocity setting in the LcdSpinBox applies regardless of whether its LedCheckbox is enabled.
Also, when the instrument is cloned, the value in this LcdSpinBox is reset to its default value and the toggle is disabled.
When the project is saved and then loaded, the base note velocity setting is saved, but the LedCheckbox is disabled.

Tested in a slightly outdated (just a few months old) copy of the master branch.

[michaelgregorius]

The issue is caused by this line in InstrumentTrack.

Cloning is implemented via serialization and deserialization of the Track (see here). However, in the case of cloning Engine::getSong()->isSavingProject() will evaluate to false because it's not the full song that's saved. Therefore the MidiPort's state is not saved and the clone is initialized with the default values.

I wonder why this line is there in the first place and if it can be removed. If the InstrumentTrack is asked to serialize it's state it should not make assumptions of the "outside world" but just fully serialize its state.

[michaelgregorius]

The code in question was added by @Reflexe with commit 723a451 as part of #5021.

[michaelgregorius]

I think the condition that saves the MIDI port has to be changed from

if (Engine::getSong()->isSavingProject() && !Engine::getSong()->getSaveOptions().discardMIDIConnections.value())

to

if (!Engine::getSong()->isSavingProject() ||
    (Engine::getSong()->isSavingProject() && !Engine::getSong()->getSaveOptions().discardMIDIConnections.value()))

Which means that we save the MIDI ports in the following cases:

• We are not in song saving mode, i.e. we are in the state that this issue is about, e.g. cloning.
• We save a song and the MIDI connections are not discarded.

By the way, why should the user decide when saving the file if the info is discarded? Shouldn't LMMS try to reestablish as many connections as possible when the file is loaded? Or alternatively, wouldn't it be better to have an option to discard the information on load?

[Reflexe]

Hi @michaelgregorius , thanks for the great feedback; looks like you are correct and the condition should be corrected.

To be honest, I very vaguely remember writing this code,
From the PRs, I see that it was something that was requested by a user (#4883); it makes sense that in some cases the user would like to export a project in a way that won't depend on external MIDI controllers.

[michaelgregorius]

Thanks for your reply @Reflexe!

If I change the line as described above most of the cloning functionality is reestablished except for the state of the custom base velocity LED. It's not clear to me if the LED should be there in the first place as it is not connected to any boolean model to be evaluated in other places. I wonder if the developer who implemented this just wanted to have a box for the text and the spin box and then used the GroupBox which comes with the LED by default. This might be a reason why the state of the LED is not serialized and therefore not saved/cloned.

The other thing that I have found is that the base velocity is evaluated during playback by some instruments like for example the SF2 player. This can be found by searching the code for the getter baseVelocity(). So if you have an SF2 instrument that plays some notes from the song editor and then change the custom base velocity then the volume of the notes will change while during playback. One of the consequences is that if that information is thrown away during a save a song might sound different after loading it again although technically there is no need for it because the SF2 player is not some MIDI hardware that could change or not being present.

[michaelgregorius]

I have created pull request #7066 to fix some of the problems. There seem to be some conceptual issues that have to be clarified before the rest can be fixed.

[michaelgregorius]

I have merged pull request #7066.

The only remaining problem should be the check box for the custom base velocity which does not seem to have any effect besides disabling the spin box in the GUI. It does not affect any model though.

It seems like the solution is to present the custom base velocity without any checkbox. The following arguments speak for this:

• The MidiPort class doesn't have a boolean model to enable/disable the base velocity. Therefore none of the code can take the state into account and it is always active as was reported originally in this issue.
• The base velocity is evaluated during playback, i.e. it affects the velocity of some instruments as they are being played. Disabling the base velocity therefore does not seem to make any sense because the instruments would then have to use a default value which is not defined.

Put differently: removing the check box and always evaluating the base velocity is equivalent to the current behavior anyway.

[michaelgregorius]

Pull request #7067 removes/hides the LED check box for the "Custom Base Velocity". Here's how it looks in the current pull request:

However, I'd like to propose to also remove the long text from the group box as it only adds visual clutter, especially if users already know the feature. It would look as follows:

Users could learn about the feature via the tool tip of the spin box:

[michaelgregorius]

@LostRobotMusic , closing this one as the problem has been fixed. I have created #7195 to deal with potential further adjustments that are mentioned in the comment above.