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

Effect modules #6232

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

Paul-Licameli
Copy link
Collaborator

@Paul-Licameli Paul-Licameli commented Apr 5, 2024

Resolves: (direct link to the issue)

Depends on

Take user interface for the various third-party effect protocols, Vamp, and Nyquist out of the executable.

Take out the built-in effects and analyzers (but not the non-modal dialogs in the Analyze menu).

Audacity links fewer third-party libraries.

The graph of modules will be thus, with one new cluster, and addition to the Nyquist cluster:

modules dot

(short description of the changes and the motivation to make the changes)

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

@Paul-Licameli Paul-Licameli added the libraries & modules A pull request that extracts one or more separately compiled & linked libraries from the executable label Apr 5, 2024
@Paul-Licameli Paul-Licameli force-pushed the Effect-modules branch 4 times, most recently from cfd5e9e to 1a1d5eb Compare April 16, 2024 03:17
@saintmatthieu
Copy link
Contributor

I understand that modules allow making some functionality shippable independently on the rest of the application. The only real use cases I know of would be closed-source modules or modules that are just too heavy to be shipped with the normal executable.
The libraries this PR modularizes don't meet any of these two criteria. Could someone please educate me on the use of this PR?

@crsib
Copy link
Contributor

crsib commented Apr 16, 2024

Audacity must depend on libraries for them to work. This makes little sense for features like effects or importers/exporters. They usually register themselves using a well known Registry. They don't need to have any public symbols and Audacity doesn't depend on them.

However, if Audacity doesn't have a symbol that it imports directly from the library - it will not load it.

Modules are loaded at runtime and do not have this limitation. They clearly state, that they extend Audacity functionality, but not the set of public APIs

@Paul-Licameli
Copy link
Collaborator Author

Factoring functionality into modules shrinks the executable and its link dependencies on third party libraries. It enforces non cyclic dependencies as the program and module evolve. It could allow an advanced user to find the modules preference page and disable modules not needed for their work, reducing the program’s memory footprint and startup time.

It was already decided to put each import/export format in its own module. Why not do the same for each effect protocol?

This is work I did in 2020-2021 still waiting for merge.

@saintmatthieu
Copy link
Contributor

I can make sense of that, thanks for clarifying.
I see that this PR also modularizes the built-in effect framework. Isn't that a bit too much, though?

@Paul-Licameli
Copy link
Collaborator Author

I can make sense of that, thanks for clarifying. I see that this PR also modularizes the built-in effect framework. Isn't that a bit too much, though?

It is not true that it modularizes the "framework" of built-in effects.

This PR only moves all the definitions of particular effect subclasses.

I don't think it is too much. What ends up remaining in the executable -- that is isolated as the difficult stuff still needing its rewrite for toolkit neutrality.

Copy link
Contributor

@saintmatthieu saintmatthieu left a comment

Choose a reason for hiding this comment

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

LGTM, but as you might guess I don't have a strong opinion here :D
Thanks @Paul-Licameli for your contribution!

static auto item = std::shared_ptr{
// Delayed evaluation
// Stereo to Mono is an oddball command that is also subject to control
// by the plug-in manager, as if an effect. Decide whether to show or
Copy link
Contributor

Choose a reason for hiding this comment

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

subject to control by the plug-in manager

Is it still the case? It doesn't show up when I open the plugin-manager window

@Paul-Licameli
Copy link
Collaborator Author

I would like @crsib ’s concurring opinion of this PR first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries & modules A pull request that extracts one or more separately compiled & linked libraries from the executable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants