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

FX section Delay/Chorus = Delay only #675

Closed
sense-amr opened this issue Feb 27, 2019 · 13 comments
Closed

FX section Delay/Chorus = Delay only #675

sense-amr opened this issue Feb 27, 2019 · 13 comments
Milestone

Comments

@sense-amr
Copy link
Contributor

After looking through the FX section scrutinizing various aspects of it with @itsmedavep and @oddy.o.trax
I discovered something which i had noted before but not thought much of ..

The Delay/Chorus section.. actually NEVER shows Chorus and only ever delay .. regardless of any preset that is selected ..
delaychorusneverchorus

in the above picture i selected the wide chorus preset effect.. as you can see no indication of it being selected is present in the UI...and to the contrary the UI displays that i have selected a DELAY not a CHORUS.. effect..

Whilst understanding clearly that technically a "Chorus" is just a delay with modulation.. we consider that it might be useful to have it explicitly separated for the user ..

We propose as a fix.. that the Chorus related FX be moved to a separate Chorus related dropdown .. which will populate in the dropdown .. as Chorus .. and "CHOR or CHRS" in the box area..

thoughts?

@esaruoho
Copy link
Collaborator

esaruoho commented Feb 27, 2019

this is fixed by #676 - @baconpaul pls review when you have the time.

@sense-amr
Copy link
Contributor Author

and now an adendum to the previous observation..

there appears to be a DELAY/CHORUS .. and a separate CHORUS .. in the FX section..
Incidentally the separate CHORUS selection results in [MOD] in the box ..
hmm now this is the question is the CHORUS fx section different to the DELAY/CHORUS .. if so how and why ? ..

seems a bit redundant to include in the DELAY/CHORUS section considering there is already a CHORUS section ..

@esaruoho
Copy link
Collaborator

closed #676 since not what's wanted

@sense-amr
Copy link
Contributor Author

looking further into this i discovered that Chorus and Delay/Chorus are separate modules in the fx section as these images show.

chorus
delaychorus

So bearing this in mind.. it would definitely be something to look into as to why the Chorus presets are housed within the Delay/Chorus section as @esaruoho has noted .. its probably incorrect placement of those presets.. but also bearing in mind they are separate FX sections with slightly different layouts....

@baconpaul
Copy link
Collaborator

Hey @esaruoho did your sweep fix all the stuff in this issue or is there other stuff here also?

@esaruoho
Copy link
Collaborator

@baconpaul nope, i killed the bits in my PR (where would've moved delay/chorus's chorusFX to chorus-subfolder) - since the chorusFX in Delay/Chorus subfolder made sense to stay in Delay/Chorus subfolder.

@sense-amr
Copy link
Contributor Author

but i still dont know what "Delay/Chorus" is ?

@esaruoho
Copy link
Collaborator

esaruoho commented Mar 5, 2019

but i still dont know what "Delay/Chorus" is ?

well, delay/chorus has delays, some of which also have chorus. there's a delay-like slapback / echo on the chorus-delays, too. they wouldn't really necessarily fit in with Chorus category, because they also use delay, instead of being raw&pure chorus.

@baconpaul
Copy link
Collaborator

Gotta say @esaruoho your answer doesn’t exactly clear it up for me either :)

So are some of the settings in “Delay/Chorus” basically making a chorus using a delay; and all of the settings in chorus making a chorus using a chorus? Is that it?

“Delay/Chorus” is a wierd name though.

@sense-amr
Copy link
Contributor Author

yeah i still don't get it i mean a chorus with a delay is still a chorus .. but a delay with out chorus , is it just a delay ?

@baconpaul
Copy link
Collaborator

I agree. Why not move the chorus-made-with-delay to the chorus folder and then have this just be delays?

@baconpaul baconpaul added this to the 1.6.0 milestone Mar 7, 2019
@baconpaul
Copy link
Collaborator

OK there is a super stupid reason why we can't do the obvious thing of moving the chorus-with-delay to the chorus menu which means we will stick with Delay/Chorus for a little until we do a code change

Right now the structure of the XML which guides the menu is

  <type i="2" name="foobaz">
     <snapshot p0=... name="my thing"/>
  </type>

All the elements contained inside that type menu have the modulator or oscillator type of the container.

So moving the delays to chorus section means they don't use the delay DSP effect any more so don't work.

The fix is to optionally check for the type on the snapshot itself so you can override it. That would happen at CSnapshotMenu around line 70.

May actually not be that hard to code up. Lemme give it a whirl.

baconpaul added a commit to baconpaul/surge that referenced this issue Mar 12, 2019
The reason the chorus-with-delay choruses were in the Delay/Chorus
menu was because the parent menu picked the effect type. So change
that so a snapshot can optionally have an 'i="num"' parameter which
overrides the type of the parent; and then move the Delay/Chorus
chorus settings into the Chorus menu; keep only two of them; rename
them to "Chorus (using Delay)" and "Wide Chorus (using Delay)"
and rename the awful "Delay/Chorus" menu to "Delay".

Closes surge-synthesizer#675
@baconpaul
Copy link
Collaborator

So that C++ code to allow an i= per item was actually super duper easy.

@esaruoho can you beat up on the branch in PR #777 a little and see what you think? Would be nice to get this into beta 7 and I tested it quite a lot.

Thanks

baconpaul added a commit to baconpaul/surge that referenced this issue Mar 12, 2019
The reason the chorus-with-delay choruses were in the Delay/Chorus
menu was because the parent menu picked the effect type. So change
that so a snapshot can optionally have an 'i="num"' parameter which
overrides the type of the parent; and then move the Delay/Chorus
chorus settings into the Chorus menu; keep only two of them; rename
them to "Chorus (using Delay)" and "Wide Chorus (using Delay)"
and rename the awful "Delay/Chorus" menu to "Delay".

Closes surge-synthesizer#675
baconpaul added a commit that referenced this issue Mar 13, 2019
The reason the chorus-with-delay choruses were in the Delay/Chorus
menu was because the parent menu picked the effect type. So change
that so a snapshot can optionally have an 'i="num"' parameter which
overrides the type of the parent; and then move the Delay/Chorus
chorus settings into the Chorus menu; keep only two of them; rename
them to "Chorus (using Delay)" and "Wide Chorus (using Delay)"
and rename the awful "Delay/Chorus" menu to "Delay".

Closes #675
baconpaul added a commit to baconpaul/surge that referenced this issue Jul 10, 2019
The reason the chorus-with-delay choruses were in the Delay/Chorus
menu was because the parent menu picked the effect type. So change
that so a snapshot can optionally have an 'i="num"' parameter which
overrides the type of the parent; and then move the Delay/Chorus
chorus settings into the Chorus menu; keep only two of them; rename
them to "Chorus (using Delay)" and "Wide Chorus (using Delay)"
and rename the awful "Delay/Chorus" menu to "Delay".

Closes surge-synthesizer#675

Former-commit-id: fc4a596d872c008103842b11ddc02b3b64664c5e [formerly 91debf4]
Former-commit-id: 53637135cdddb9be9690b1c1876ff31afeee57cd
Former-commit-id: a25533ddedb1c077eb7613595748f30dddb27342
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 a pull request may close this issue.

3 participants