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

Remove iplug2 dependency #15

Closed
wants to merge 3 commits into from
Closed

Conversation

olilarkin
Copy link
Contributor

Possible duplicate of #13, which i only just spotted

This doesn't make changes w.r.t to Eigen or json.hpp, which should be dependencies of this repo IMO

also renames format.sh and uses pragma once for all include guards

@mikeoliphant
Copy link
Contributor

Yes - PR #13 removes the IPlug2 dependency, and separates the NAM model code from the other dsp code.

I didn't worry about removing the IPlug dependency from the other dsp code (noise gate, IR, EQ) - since they have other "IPluggy" structure to them. To be honest, I think that code could live in the plugin repo rather than here.

I went with "double" instead of "float", since that was what the plugin is currently using. I think maybe using a template class so as to be able to accept either double or float I/O might make sense, but I wanted to get the separation done first.

@olilarkin
Copy link
Contributor Author

Agree, ideally this repo would just be the model parsing and NN inference code. @sdatkinson iPlug already has EQ code: https://github.com/iPlug2/iPlug2/blob/master/IPlug/Extras/SVF.h which might be nicer. Also there are nice audio file-loading and convolution libraries out there!

@sdatkinson
Copy link
Owner

Getting to this now. Resolving conflicts and jotting down notes:

  • This PR keeps NAM DSP code in dsp/dsp.[cpp,h]. main moves to NAM/. I think NAM is fine.
  • Use float instead of double in place of iplug::sample. double is preferred for now, can let the models override for their own preference. I'd presume that DSP in single-precision is uncommon since "classical" numerical methods are probably the default, so I'm fine to let the models explicitly indicate how they're breaking from the norm at least for now.
  • There was an #include "IPlugConstants.h" in dsp/dsp.h that I'm not sure what it was doing...

Unfortunately this series of PRs is going to break NeuralAmpModelerPlugin so it's going to be a moment before I can figure out whether things are all good or if there were bugs introduced in this (I know, I know). Just hang tight. I'll try to catch it up and iron out things if needed.

But I've got the merge conflicts resolved; I'm going to open a separate PR that appends the resolution onto your commit, @olilarkin and merge that.

Thanks all for your help--I really appreciate it!

@mikeoliphant
Copy link
Contributor

  • There was an #include "IPlugConstants.h" in dsp/dsp.h that I'm not sure what it was doing...

It was just required to use "iplug:sample" - so no longer required if those have been removed.

@sdatkinson
Copy link
Owner

Shipped via #22

@sdatkinson sdatkinson closed this Apr 2, 2023
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.

3 participants