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

Simplify DSP & Add Resampling #341

Closed
wants to merge 9 commits into from
Closed

Conversation

olilarkin
Copy link
Contributor

@sdatkinson Just to prepare you... now the UI is cleaned up I would like to try and help you really simplify the DSP processing, hopefully optimising it and adding resampling in the process. I find it a bit hard to work with the existing code and to incrementally change things, when IMO a lot can be improved so in this PR I am starting by stripping things down to basics. I have removed the Gate, EQ and IR processing and removed redundant buffering and methods. I hope to add resampling then build these things back up cleanly. I realise this might be a bit irritating for you but I hope it makes sense!

@sdatkinson
Copy link
Owner

[I see that this is currently marked as a draft, but I think some early feedback here will help this go smoothly]

First impression: Wow, this is a lot of work...there's almost 1000 lines of changes in this PR and another couple 10k in the submodule changes the PR is pulling in (Ok sure, it's mostly nlohmann's json dependency, but I'm still counting moving parts and estimating unknown unknowns...)

Maybe I can see about trying to break this down into pieces?

First thing I see is that this and #370 are both pulling in an iPlug2 update. Would it be a good first step for me to try that out on the existing code and expect it all to work? (Also, I just noticed that the build broke, and it appears to be related to iPlug2, so I'm sort of roughly putting 2 and 2 together and hoping this might fix that? I know we've been off on a non-main branch, and that's been sort of worrying me as well.) If so, then maybe I can get that in its own PR here and you can rebase this on that change? And so forth, and we can work through it?

I may be able to get this down in chunks...but like I said there's a lot going on here and I'm just thinking out loud, trying to figure out a plan to tackle it.

@sdatkinson sdatkinson mentioned this pull request Sep 28, 2023
@olilarkin
Copy link
Contributor Author

Yes, this would look quite different by the time I actually ask you to merge it. The file manager PR should go first, and some of the changes here should go in Iplug2.

@olilarkin olilarkin force-pushed the resampler branch 2 times, most recently from 3fd86ce to 638f39e Compare October 16, 2023 07:38
@sdatkinson
Copy link
Owner

I'm going to have a look at this soon as I'm working on issue-59 and will try to combine things 😄

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