Skip to content

Separate core NAM code from other plugin dsp code#13

Closed
mikeoliphant wants to merge 2 commits into
sdatkinson:mainfrom
mikeoliphant:separate_nam
Closed

Separate core NAM code from other plugin dsp code#13
mikeoliphant wants to merge 2 commits into
sdatkinson:mainfrom
mikeoliphant:separate_nam

Conversation

@mikeoliphant

@mikeoliphant mikeoliphant commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

This PR separates the core NAM code from other plugin dsp code and removes IPlug dependencies for NAM code.

I tried to keep this as simple and targeted as possible. It looks like bigger changes than it is because of the separation of dsp.h/dsp.cpp files into an IPlug dsp base class for noise gate, IR and EQ and the core dsp code for the network models.

I kept the input to the model processing as "double" for now - since that is what the IPlug plugin is using. I also haven't made any changes to the redundant gain handling - I'll save that for a separate PR.

With the changes in this PR, it is now easy to use the core NAM code directly in a context other than the IPlug plugin.

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

This will also require some changes to the plugin project structures to link to the new source code locations.

Are the various VS project files generated somehow, or is that done manually on a per-project basis?

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

For an example of how this makes it easier to use the NAM code as a submodule in a non-IPlug context, see here:

https://github.com/mikeoliphant/neural-amp-modeler-lv2

It is pointing at my fork of NeuralAmpModelerCore, which has the changes contained in this PR.

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

@sdatkinson - I know you've got a lot on your plate, but maybe now is a good time to think about merging this, since you just released a new version of the plugin?

I've got some performance enhancements I'd like to make, but I don't want to make any more changes until this bit gets resolved.

@sdatkinson sdatkinson left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for your patience waiting for this.

Quick confirmation on the "move" Qs and we'll be good!

Comment thread dsp/dsp.h
Comment thread dsp/dsp.cpp
@sdatkinson sdatkinson self-assigned this Mar 31, 2023
@mikeoliphant

Copy link
Copy Markdown
Contributor Author

Let me know if anything is still unclear. It looks like a lot of stuff is going on, but it is basically just moving the NAM code into a separate folder from the other dsp code - slightly complicated by the fact that code was shared in dsp.h/dsp.cpp so those files had to be split.

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

Since this PR now has conflicts, I could maybe resubmit - perhaps one simpler PR to separate dsp.h/dsp.cpp (perhaps calling the NAM versions NAM.h/Nam.cpp), and a subsequent one for the removal of the IPlug dependency, and then one for moving files to a separate folder?

Just let me know if that would make things easier than handling this PR as is.

@sdatkinson

Copy link
Copy Markdown
Owner

Since this PR now has conflicts, I could maybe resubmit - perhaps one simpler PR to separate dsp.h/dsp.cpp (perhaps calling the NAM versions NAM.h/Nam.cpp), and a subsequent one for the removal of the IPlug dependency, and then one for moving files to a separate folder?

Just let me know if that would make things easier than handling this PR as is.

I'll have a look tonight and let you know. Thanks for your patience :)

@sdatkinson

sdatkinson commented Apr 2, 2023

Copy link
Copy Markdown
Owner

@mikeoliphant issue-13 resolves merge conflicts. Should be able to merge it into your branch and the PR should be good to go!

Scratch that, merged wrong. One sec.

In fact I may just make a separate PR that includes the resolution and merge it so you don't have to fuss about it and we can get through all these PRs 😃

@sdatkinson

Copy link
Copy Markdown
Owner

Got it in #21 :)

@sdatkinson sdatkinson closed this Apr 2, 2023
@mikeoliphant mikeoliphant deleted the separate_nam branch April 2, 2023 20:52
synchu pushed a commit to synchu/NeuralAmpModelerCore that referenced this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants