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

Enable dynamic parameters #196

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

Federerer
Copy link
Collaborator

@Federerer Federerer commented May 14, 2023

This enables dynamic parameters in LiveSPICE, so there is no need to resolve a circuit after pot change. Current state is WIP, so there is a lot of stuff there that should not be here and needs to be reverted or separated to another PR.

TODO before merging:

  • Rebase on master
  • Switch back to AudioPlugSharp nuget packages
  • Merge a PR with changes to the ComputerAlgebra
  • Switch to master branch of the ComputerAlgebra
  • Set the default value of the Dynamic property to false in all components.
  • Fix the failing tests
  • Add a feature flag
  • Do a code cleanup and add comments in tricky places
  • Check for unwanted changes

@Federerer Federerer marked this pull request as draft May 14, 2023 13:27
@dsharlet
Copy link
Owner

If you merge master into this, we could at least consider merging this with Dynamic defaulted to false (and maybe not Browsable at first), then it might not be such a headache to track master.

@Federerer Federerer force-pushed the feature/parameters branch from 32b48aa to 7ae5006 Compare May 14, 2023 22:06
@Federerer Federerer force-pushed the feature/parameters branch from 85f27ea to 6953911 Compare May 14, 2023 22:31
@Federerer Federerer force-pushed the feature/parameters branch from 5988f09 to 6f75470 Compare May 14, 2023 23:16
@Federerer
Copy link
Collaborator Author

If you merge master into this, we could at least consider merging this with Dynamic defaulted to false (and maybe not Browsable at first)

That would be great. I wanted to set a default to false anyway, as it's a breaking change. Just did a second rebase, plus fixed some bugs related to initial conditions.
I'm adding a checklist to the original description in order to keep track of the progress.

@dsharlet
Copy link
Owner

I think maybe the best way to go is peel off parts of this into separate branches that can be merged one by one. For example, adding the ability for components to Analyze as dynamic or not would be a good change to just make by itself. Just using the value of the pot during simulation but not actually hooking it up to a dynamic control seems like it shouldn't break anything and would actually exercise that code path, so it's not just dead code.

This is both how I'd prefer to work on the feature myself, and also how I'd prefer to review the changes.

@dsharlet
Copy link
Owner

Also, there is quite a bit of stuff in this branch that I don't recognize from last time I previewed this change. IIRC, it looked pretty familiar from the way I had it before giving up on dynamic parameters last time, this time there is quite a bit more other stuff. For example, two different Simulation class implementations. Are those changes necessary?

@Federerer
Copy link
Collaborator Author

For example, two different Simulation class implementations.

Those are mostly related to the asynchronous build pipeline, plus some performance stuff. I just wanted to keep the original algorithm for A/B comparisons in CLI. So that's why there are two simulation classes. But I think this probably can be separated into a separate PR. There are not that much changes new related to dynamic components. From what I can remember, I've changed how the pot curves are applied in order to keep parameters normalized to 0-1 linear range. There are also some variables added into the MNA, so the parameters are extracted into the linear part and didn't end up in the newton iterations. Also, parameters are now applied once per Run call, and what we should do is probably allow for per-sample changes, but that requires more changes in VST, so we might skip it for the v1.

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