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

Fix LMMS plugin issues #6670

Merged
merged 30 commits into from
Aug 22, 2023
Merged

Fix LMMS plugin issues #6670

merged 30 commits into from
Aug 22, 2023

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Mar 14, 2023

This PR fixes two related bugs affecting LMMS instrument plugins, refactors the Vibed instrument, and fixes memory leaks.

Fixes #6669
Fixes #6671
Fixes #6797

1) Automated parameter loading bug (#6669, #6797)

The automation for certain parameters of LMMS plugins was failing to load correctly from the project files.

This bug affected the following parameters when they were automated:

  • Organic's harmonics parameters
  • Kicker's end distortion parameter
  • AudioFileProcessor's interpolation parameter
  • Vibed's volume parameters

In each of these cases, conditional parameter loading logic was used to check for the existence of a parameter with the given name in the XML data. However, only the existence of the XML attribute was checked, which is how non-automated parameters are stored. To do it correctly, they should have also checked for the existence of an XML child element, which is how automated parameters are stored. This PR fixes that oversight, which resolves the bugs.

Note: A parameter may also be stored as an attribute of a child element called "automatablemodel", but that isn't the case for any of the parameters affected by this bug.

I also took the liberty of cleaning up the style/formatting of the loadSettings and saveSettings methods.

2) Vibed data saving/loading bug (#6671)

The Vibed instrument was only saving the parameters and sample data for each of its 9 "Strings" to the project file if that particular string was active at the time the project file was saved. This led to a couple relatively serious issues including data loss.

I fixed the problem by always saving each of the 9 Vibed string parameters and their sample data regardless of whether they are currently active. I incremented the "version" attribute which Vibed XML data uses so that this change is backwards compatible with older project files.

3) Vibed refactor and improvements

The Vibed instrument's pre-C++11 codebase was in great need of an overhaul and since I was already making a number of changes to it, I decided to refactor the whole thing. Besides making the code much more readable, this refactor brings a few additional improvements/fixes such as:

  • Fixes a few memory leaks
  • Replaces many raw pointers with smart pointers
  • Prioritizes stack memory over heap memory for better real-time safety
  • Removes some usages of Qt

4) Memory leaks

Fixes memory leaks affecting nearly all LMMS instrument plugins during heavy tempo automation. (See explanation)

@messmerd messmerd changed the title Fix automated parameter loading Fix LMMS plugin issues Mar 15, 2023
Copy link
Contributor

@Veratil Veratil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General lookover review, nothing in depth.

@superpaik
Copy link
Contributor

Works ok. Tested on Win10-64b

@luzpaz
Copy link
Contributor

luzpaz commented Apr 18, 2023

Any updates on this?

@messmerd
Copy link
Member Author

Any updates on this?

I'd like to complete #6680 first, then fix the merge conflicts it causes with this branch.
But this branch is ready for testing/review - I just need people willing to do it.

@messmerd
Copy link
Member Author

@Testers/reviewers

Here's a project file for testing #6669:
bug6669.zip
It also contains screenshots showing that this PR fixes the bug.

And here are project files for testing #6671:
bug6671.zip
This also contains screenshots showing that the bug has been fixed. Unlike #6669, the bug occurs when saving the project instead of loading it, so I've included two project files - one created and saved in master, and one created and saved in this dev branch.

NOTE: In both the master branch and this dev branch, LMMS occasionally crashes randomly during or after editing automation for the Vibed instrument. I haven't figured out how to reliably cause it yet, but I'll create a new issue for it once I look into it some more. Since it also occurs in master, it seems unrelated to anything contained in this PR and should not affect testing, but I just wanted to give a heads up in case anyone encounters it.

@superpaik
Copy link
Contributor

Minor change propossed on vibed.cpp
Other than that, works ok with project files provided.

Copy link
Contributor

@Veratil Veratil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another small thing, we can probably remove the variable names in the declarations of class functions in the header files.

Comment on lines +393 to +398
m_smoothBtn.move(79, 129);
m_smoothBtn.setActiveGraphic(PLUGIN_NAME::getIconPixmap("smooth_active"));
m_smoothBtn.setInactiveGraphic(PLUGIN_NAME::getIconPixmap("smooth_inactive"));
m_smoothBtn.setChecked(false);
m_smoothBtn.setToolTip(tr("Smooth waveform"));
connect(&m_smoothBtn, SIGNAL(clicked()), this, SLOT(smoothClicked()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note for later enhancements: I feel we could change this to a function and just pass the variable and parameters to it.

@luzpaz
Copy link
Contributor

luzpaz commented Aug 21, 2023

Soft bump

@PhysSong PhysSong merged commit 5cbf2c5 into LMMS:master Aug 22, 2023
@messmerd messmerd deleted the b6669-fix branch August 22, 2023 05:51
@luzpaz
Copy link
Contributor

luzpaz commented Aug 22, 2023

Sweet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants