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

Filter and EQ modulation targets #422

Merged
merged 10 commits into from
Sep 21, 2020
Merged

Conversation

paulfd
Copy link
Member

@paulfd paulfd commented Sep 16, 2020

This registers filters and EQ modulation targets, as well as the bindings to midi CC. I also removed the filter pools and EQ pools to put the filters and EQ objects directly in the voices.

Missing some testing although things seem to work OK for now.

@jpcima
Copy link
Collaborator

jpcima commented Sep 16, 2020

Very nice. After a quick look, it looks good to me.
It's a bit difficult to read the diff in places. (good thing to have a lot of red, certainly not a complaint here 👍)

Regarding removed tests, it's a good case for rewriting with RegionCCView in TestHelpers. (cf. RegionT various tests)

@paulfd
Copy link
Member Author

paulfd commented Sep 16, 2020

Hmm I forgot about the helper but I had in mind rewriting it xD Anyway I chose to compare the dot graph in the end because it would have gotten quite heaver and verbose (you need a target and thus a RegionCCView for each filter target/filter number, e.g. you would have alot of

region.parseOpcode({ "cutoff_oncc45", "4.2" });
REQUIRE(RegionCCView(region, ModKey::createNXYZ(ModId::FilCutoff, region.getId(), 0)).at(...).value == ...)

which looked a bit verbose.
If you think it's necessary I'll rewrite them, sure 🙂
The dot graph is a bit more readable in this sense, and it tests the end-to-end processing and creation of connections (from parsing the file to the state of the modulation matrix).
It does not test the connection list within the region, but I think these are unused beyond the mod matrix initialization.

@jpcima
Copy link
Collaborator

jpcima commented Sep 16, 2020

It does not test the connection list within the region, but I think these are unused beyond the mod matrix initialization.

They are not used after the initialization. Their purpose is to create an intermediate state that serves to prepare the CC keys.

As a note, regarding the CC, I wish to go back on a particular design decision: value should be removed from the mod key, and instead changed into sourceDepth. This way if there exist some CCs with identical everything except depth, there is only need to compute one of them.

@paulfd
Copy link
Member Author

paulfd commented Sep 16, 2020

As a note, regarding the CC, I wish to go back on a particular design decision: value should be removed from the mod key, and instead changed into sourceDepth. This way if there exist some CCs with identical everything except depth, there is only need to compute one of them.

Sure !

@paulfd paulfd marked this pull request as ready for review September 16, 2020 21:49
@paulfd
Copy link
Member Author

paulfd commented Sep 16, 2020

Seems to work fine in my tests.

@paulfd paulfd merged commit fd8b00b into sfztools:develop Sep 21, 2020
@paulfd paulfd deleted the filter-eq-targets branch January 21, 2022 10:24
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