Fix automated base notes and their automations (#6548) #6725
Conversation
Fix all base notes that are used in automations and their corresponding automation values. Base notes that are automated are stored as elements in the save file whereas non-automated base notes are stored as attributes. So far the method `upgrade_extendedNoteRange` only upgraded the non-automated base notes that are stored in attributes. This commit fixes the automated ones which are stored in elements. The fix works as follows: * Collect all base note elements. * Store their ids in a set so that we can later identify automations that reference them. * Collect all automation pattern and check if they reference a base note. * Adjust the values and out values of all automations that reference base notes. Note: for many older files the out values will be introduced by the upgrade `method upgrade_automationNodes` and do not appear in the files themselves!
Remove debug code from DataFile::upgrade which was accidentally committed.
|
Will the upgrade function still be run if someone already had their project updated? I'm pretty sure our upgrade functions don't run on projects twice? |
|
If it's in the same function the upgrade won't re-run. Breaking it out into a separate function would make it run, at the cost of DataFile.cpp being permanently uglier. However, I'm not sure how likely it is that someone would upgrade to a 1.3 alpha/nightly, discover that instruments are playing in the wrong octave, and save over the file without fixing it. If they go back to the 1.2 project file, the fixed upgrade routine will run whenever they next try switching to 1.3. If they move to 1.3 anyway and manually fix the octave, the upgrade routine would be redundant. So maybe it's fine if it doesn't re-run? |
I don't think the files' beauty matters when we're trying to make sure things are fixed. 😉
I agree, but there are a number of people who have been working with 1.3 downloads. I wouldn't be surprised if there is more than one person that had this happen to their project.
That's what I'm trying to decide, do we want to just say "if you already opened past this commit then it's up to you to fix" or "we want to maintain proper upgrade routines no matter what". |
|
I think the code may be missing one important step: checking whether the automation track drives a base note of an instrument affected by the original non-standard MIDI range (#1857), or some other instrument. If the automated base note belongs to an affected instrument (see And it gets even more unpleasant: if an automation track drives multiple base notes, it may be necessary to create two versions: one corrected, and one unchanged. The There may also be some extreme edge cases, like if someone uses one automation track to drive several unrelated targets, like a base note and volume, or perhaps more realistically, base note and Master pitch or something. Hopefully, if changes are only ever made to a new copy of the automation track, I suppose it should leave all such "unreasonable scenarios" reasonably intact... Otherwise I don't even want to guess what to do with such cases, other than perhaps make sure the updated base note won't fall of range. |
I'm mostly thinking about the precedent of going out of our way to support backwards compatibility between unstable versions. Of course 1.3 should properly upgrade 1.2 files, but how reasonable is it really to expect 1.3-[long commit hash a] to upgrade a 1.3-[long commit hash b] file that was saved in a broken state?
I don't mean to compare "proper" vs "unproper"; what I wanted to point out was that any projects a user has already fixed after upgrading would be broken if the base note is shifted again by a new upgrade routine. |
I was under the impression that we were moving forward with the idea that master is a "stable" version? People could expect that master will be kept as stable as possible.
As reasonable as we want, honestly. There's old upgrade routines that are literally days between.
This is a good point I didn't think of. So really either way we go we have the possibility of breaking something. |
|
Hi @Veratil, @Spekular, @he29-net, thanks for checking the PR and giving input! So it seems that the situation can be summarized as follows. New version or notIt seems we have two options on how the added code is run:
So it seems that we have to decide which is the lesser of the two evils. I think we should decide based on what the less probable scenario. If we decided to go with a new version please advise on how to do so. Adding the new method at the end of Additional checksThe check for the affected instruments is missing but it should be possible to add it. I think in this case the code would look somewhat as follows:
@he29-net wrote:
Can an automation track drive more that one modulation target? If yes, how can I set this up in LMMS to create a test file? Questions about the current upgrade method@he29-net , why is only the first instrument taken into account? See line 1700 in current master: Can each track only have one instrument beneath it? Or is this a potential bug? In line 1703 a "basenote" attribute is added to the instrumenttrack without checking if the "basenote" is stored as an attribute (non-automated) or as an element (automated). For example the test file provided in #6548 has the following section: Depending on what |
That should be easy to set up; just hold Ctrl and drag multiple controls, one after another, to the same automation clip. Then if you right-click on the clip, there should be an "# Connections →" entry, which contains a list of all connected controls.
I am not 100 % sure, but I personally have no idea how I would put two instruments into one instrument track. I don't think it is possible in the GUI, so my guess is that if you try to create such an instrument track by editing the XML file, only the first instrument will play, or something breaks. Unless multiple instruments in one (non-BB / non-Pattern) track were supported in some ancient LMMS version I'm not aware of, and the core still happens to supports it...
That's a good find; I had no idea that the Given this oversight could lead to some potential weirdness, I would personally lean on the side of fixing and extending the current |
This is handled automatically in Line 1915 in 9f34c5c |
|
@he29-net, thanks to your help I was able to setup a more complex example file. With that file I now also came to the realization that a complete fix would indeed include a potential duplication of automation patterns into ones that contain unaffected elements and ones that contain affected ones. I also found some other potential problems with that example file. Modulated elements, e.g. base notes, do not seem to be reset to their nominal values after playing. This means that if you modulate a base note with a linear automation, play the song and save the file then the base note is very likely saved with a I also wonder why BB tracks are ignored? In my example file I have a BB track that container with a TripleOscillator that has an automated base note. And why are the base notes fixed unconditionally but the notes are fixed conditionally? Another potential problem in the upgrade method is that there are nested for loops that all have I agree that it might be better to fix |
The notes are fixed so that the their labels in piano roll match the tones that will be played (assuming default 12 EDO tuning). Otherwise the update routine could have been be done by only changing base notes -- but the labels would be off. Basically, the base notes are shifted one octave up, making everything sound one octave lower. Then, notes of affected instruments are kept without change in order to preserve the buggy <= 1.2.2 behavior (while reflecting the fact they sound an octave lower in the label, i.e., old LMMS note label 57 = A4 becomes standard MIDI 57 = A3), while all the other notes are shifted one octave higher to counteract the previous unconditional base note change. As for the ignored BB tracks, I think it's because the "container track" (that is being ignored according to the comment) does not contain any instruments. It just contains other, "proper" tracks, that are processed by the subsequent code. |
Move the fix for the automated base notes to the code that fixes the non-automated base notes. Improve the fix for automation tracks and patterns by potentially splitting them into two tracks: * The original track which is adjusted to only contain patterns with targets that are not base notes. * A cloned track that only contains patterns with base note targets. This is done by iterating over all automation tracks and checking which types of automations they contain: * Base note automations * Automations of other targets The result for each automation track are then evaluated as follows. * If an automation track does not contain any base note automations it is kept as it is, i.e. nothing is done. * If an automation track only contains patterns with base note automations its patterns are corrected in place. * If an automation track contains patterns with base note automations and other targets then the track is duplicated so that we can split it as described above. This split and correction is done on a per pattern level. Patterns that would become empty are removed. Cloned tracks will keep the same names and attributes as their original tracks. TODOs ------ * Base notes are read as integers, corrected by an integer value of 12 and then stored back as an integer. In some files base notes have float values, i.e. if the file was stored after the base notes have been automated linearly. * B&B tracks are not corrected although they can contain instruments with (automated) base notes! * Nested for-loops with "i" as their running variables. (Fix in a separate commit)
The GitHub builds seem to need the explicit include which for some reason is not needed on my developer machine.
Fix the problem with the nested for loops that all had variables called "i" by extracting a helper method with a corrected nesting. Due the extraction we do not have to care anymore if the correction is running beneath another for loop with potentially the same variable names.
Fix the base notes and automations of B+B tracks. This fixes for example the problem that when a TripleOscillator had been put into a B+B track, e.g. with version 1.2 that it sounded too high when being loaded with a current master version. The cause seems to be that the notes of the instrument pattern are corrected/transposed too often (likely due to the collection of all tracks starting from the song/document root). The multiple correction of notes is fixed by traversing the song structure in a more consise way. First all track containers of the song are collected and for each of them the tracks are collected. These tracks are then fixed one by one. B+B tracks are getting a special handling while doing so. It is assumed that a B+B track cannot have other B+B tracks inside and therefore all sub tracks of the B+B track are searched for so that they can be fixed recursively. Refactor some more functionality into static helper methods: * Everything beneath a track is now fixed by the helper method `fixTrack`. * The fix of the base notes of the instruments themselves and the extraction of the ids of automated base notes has been moved into `fixInstrumentBaseNoteAndCollectIds`. * The lambda `affected` has been converted into a static method because it is must be accessible by one of the static helper methods. * The code for fixing the automation tracks of a song has been moved into the helper function `fixAutomationTracks`.
|
@he29-net, @Veratil, @Spekular, I have pushed some more commits which fix the B+B tracks and which also implement the splitting of mixed automation tracks. If an automation tracks automates base notes and other targets it is now split into two tracks. The first one contains all the automation for the other targets and the second one the corrected automation for all base note targets. If an automation does not contain any base note automations it is not touched. If it only contains base note automations it is fixed in place. I have used the following file to test the fixes: MultiTargets-1.2.zip. If you play that file in the current master you will find that most tracks sound different than in LMMS 1.2.2. If you load it with this branch it should sound exactly the same. The sound/behavior of presets has not been changed between this branch and master. Can you please test as well? |
michaelgregorius
added
the
needs code review
There was a problem hiding this comment.
I finally finished going through the code, and apart from a few details (3x while(!object.isNull()) missing a space after while, and a typo on line 1826 "because they container a track container"), the only small issue I see are the names of helper functions. affected(), fixTrack(), fixAutomationPattern() etc. would be now visible to all upgrade routines added in the future, since they are not just a local lambda like affected() was before.
Maybe they could be prefixed by midiRange or something. Or maybe it does not matter, because DataFile.cpp is not that complex so it won't cause much trouble. So I'm not insisting on fixing that; just wanted to point it out in case it was missed.
As for the TODO regarding float values on non-automated basenotes; it seems that the base note is loaded into an integer automatable model, so any decimal part would be discarded after a load from XML anyway? I also don't see how could a float make its way into the XML in the first place, if automation isn't involved. So I would say the current code is OK.
Other than that, I like how it is all structured. The upgrade routine got pretty complex, but separating the tasks into multiple functions and commenting on what's being done and why makes the process easy to follow and understand. 👍 I tested it with the supplied file about a week ago and I do not recall any issues, so as far as I'm concerned this PR should be good to go.
Add a space after some while loops. Fix a typo in a comment.
Extract the code that upgrades the extended note range into its own class. This hides the helper functions that are related to the upgrade from the other upgrade methods in DataFile.cpp. The header file is put into the src/core directory as it is not part of the public interface and therefore should not be included in the "global" include directory. The whole thing is just an implementation detail of the upgrade in DataFile.cpp.
|
Hi @he29-net, thanks for the review.! I have fixed the Commit 5335f6d then moves the whole upgrade code for the extended note range into its own class to hide all the helper methods. I have added the header file to the Just as I am typing this I see that the mingw builds are now failing. 😅 |
Add the comment "// namespace lmms" to the closing scope of the lmms namespaces. The "scripted-checks" are quite strict. Perhaps they should be renamed to "strict-checks". ;) Include "cassert" in UpgradeExtendedNoteRange.cpp to hopefully fix the mingw builds.
Towards the end of the development for the fix of LMMS#6548 (via LMMS#6725) the upgrade code was refactored into its own class. While doing so it was forgotten to actually call the `upgrade` method on the `UpgradeExtendedNoteRange` instance. As a result almost all files should currently open in a wrong state with many instruments transposed. This commit fixes this. Also explicitly check the assertion that BB tracks do not contain other BB tracks.
There was a problem hiding this comment.
This file should have gone under include/ not src/core/.
EDIT: I just saw the comment made previously about this subject.. I guess I understand, but I still don't know how I feel about it. Maybe an include/private/ directory would be a good place for private headers such as this. I've seen other repos do it that way.
There was a problem hiding this comment.
Hi @messmerd,
yes, it seems like LMMS is currently missing a concept for these things.
If LMMS for example had a clean separation between the core and the GUI I would assume that only the core classes would be part of a public interface because they would be the most "interesting" and "open" ones to program again. Clients could for example use them to implement their own command line interfaces, reuse DSP algorithms, etc. I guess it would not be too helpful to have the interface of a very specialized GUI dialog in the include directory because what should clients really do with it?
Fix all base notes that are used in automations and their corresponding automation values. Base notes that are automated are stored as elements in the save file whereas non-automated base notes are stored as attributes. So far the method
upgrade_extendedNoteRangeonly upgraded the non-automated base notes that are stored in attributes. This commit implements the upgrades of the automated ones which are stored in elements.The fix works as follows:
Note: for many older files the out values will be introduced by the upgrade
method upgrade_automationNodesand do not appear in the files themselves!