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

Strange behavior from some oscillator types in FM Configurations #1035

Closed
mortfell opened this issue Aug 16, 2019 · 28 comments · Fixed by #1036 or #1070
Closed

Strange behavior from some oscillator types in FM Configurations #1035

mortfell opened this issue Aug 16, 2019 · 28 comments · Fixed by #1036 or #1070
Milestone

Comments

@mortfell
Copy link

mortfell commented Aug 16, 2019

Not sure if this is a bug, or just behavior I don't understand, but want to be sure.
I'm using the nightly from 8/16/2019 that introduced Feedback in the Sine OSC, but it's the same in older versions I tried.

To Reproduce
I'm using a sine oscillator as a carrier and a sine oscillator 1 octave higher as a modulator.
once I introduce feedback the waveform starts to undulate strangely.
fmSquare

This also happens if you do the same thing with a sine operator in OSC 1 and an FM2 operator in as OSC 2. Even with no modulation in FM2 just using it as a sine wave with feedback, starting getting a slowly morphing waveform.
fmSquare2

I went back and checked an older nightly, before Sine OSC had feedback, using feedback on the FM2 oscillator would cause the waveform to do the same morphing.

Expected behavior
In my experience with FM a 1:2 carrier to modulator ratio with some feedback on the modulator should produce something resembling a Square wave, so I feel like there might be something weird happening here..... BUT I also might not be understanding the usage of the sine wave so let me know if I'm wrong.

The undulation sounds cool!!!
Just not what I expected

If I try the same configuration as above but with FM2 Oscillators in OSC slots one in two (simply using OSC1 as a Sine wave and OSC2 as a sine wave with feedback) I get a stable squarish waveform shape... which is what I would expect:
fmSquare3

In summary
Using the sine wave OSC as carrier can cause unexpected (to me) results when FM Routing is enabled. A similar thing happens when using a Wavetable as the carrier... but I don't expect things to be stable when using Wavetables as Carriers anyway, and changing this (if anything weird is happening at all) could break old patches.

This behavior does NOT happen if FM2 or FM3 is used as carrier, and Sine OSC with feedback is used as modulator. On the chance this is undesirable/unintentional behavior seems like this would be worth fixing for the Sine Waves at least.

My experience with the math and programming of FM is very limited, but I do have experience with making FM sounds in tons of synths, and this seemed odd to me, so I thought i would mention.

@baconpaul
Copy link
Collaborator

Yeah so I just confirmed that sin -> sin and fm2 -> sin do the same thing; that sin with fb = 0 is unchanged from the prior; and so forth.

Interestingly sin -> sin and sin -> fm2 should be the same you'd think right? But they aren't. I think they are interpreting the fb depth parameter differently. But that difference could also be part of the problem.

Surge works by having separate oscillator blocks for FM. I will compare the FM2 everything at zero FM2 block with the sin one.

I think the introduction of feedback on the sin oscillator is a bit of a red herring; this all still is odd with fb == 0. But clearly something is different between sine and unmodulated fm2 and they should act the same I think. Or at least we should understand why not!

@mortfell
Copy link
Author

Yes, that's my thinking!
Among the presets I've been working on I want to have a handful of super simple "template" presets for the library. I think it's a cool way of demonstrating some interesting quirks of Surge, and also I find those types of presets to be really fun starting points to design sounds.

I wanted to do a couple such templates with "classic subtractive synthesis shapes" but using FM....
I was confused when I could only get the shapes I was expecting using FM2 and FM3 oscillators.

@baconpaul
Copy link
Collaborator

Oh sure.
The difference is the sine oscillator has FB applied with depth "depth"
The FM2 oscillator has FB applied with depth "32 PI depth^3"

The FM2 oscillator advances phase by omega every step no matter what
The SINE oscillator advances by omega + the input FM signal

If I change those two things there is no difference between SIN as an carrier and FM2 unmodulated as a carrier.

Let me look at the code and clean it up. I think what was there was pretty obviously wrong.

@mortfell
Copy link
Author

Cool! very good to know

baconpaul added a commit to baconpaul/surge that referenced this issue Aug 16, 2019
The Sin osc FM channel was totally different than
the FM2 one in a way which was wierdly inconsistent.
So choose to change the behavior of SIN in FM mode.
This may impact the sound of some patches.

Closes surge-synthesizer#1035
@baconpaul
Copy link
Collaborator

OK so I think that above patch fixes this in that it makes the sin oscillator behave correctly

I wonder if anyone was depending on the incorrect behavior?

This issue will close when I sweep it. Feel free to re-open it if there are still problems with the nightly that results.

@mortfell
Copy link
Author

mortfell commented Aug 16, 2019

Yes.... I was thinking about that as well....
The sine oscillator was available when I started using Surge a lot (month or so ago), but that has been a fairly recent addition right?

Ill check through my presets I've made so far.

@baconpaul
Copy link
Collaborator

The oscillstornhas been there since version 1. My changes to it around waveshaper are new but that fm code is old. I can scan patches to see if anyone uses wine and fm tho

baconpaul added a commit that referenced this issue Aug 16, 2019
The Sin osc FM channel was totally different than
the FM2 one in a way which was wierdly inconsistent.
So choose to change the behavior of SIN in FM mode.
This may impact the sound of some patches.

Addresses #1035
@baconpaul baconpaul reopened this Aug 16, 2019
@mortfell
Copy link
Author

Oh interesting good to know as well

@baconpaul
Copy link
Collaborator

Ok I just swept but also reopened this so I can do that preset scan

@baconpaul baconpaul added this to the 1.6.2 milestone Aug 16, 2019
@mortfell
Copy link
Author

mortfell commented Aug 16, 2019

Noticed something odd looking in a preset
The preset "Blue Pad" by Argitoth uses Sine mode in FM.
But also confusingly the feedback is set %50... which is strange because that preset would have been made before you added feedback to the sine oscillator.
(a number of those presets seem to use the Sine as carrier , and some have feedback set too 0, some dont)

@baconpaul
Copy link
Collaborator

Oh wow thanks that should restore to the default value - 0. I will debug the patch. Appreciated

@mortfell
Copy link
Author

no problem.
Yes, they are is a handful of presets in that bank that use sineOSC as carrier with FM routing.
Some of them have feedback set to 0, some to %50 🤔 strange.

But i guess they will sound different with the changed Sine OSC code?

@baconpaul
Copy link
Collaborator

Yeah maybe. I can do a before after check easily

@baconpaul
Copy link
Collaborator

Oh that 0.5 is a really really (really) aggravating problem that may be really really (really) hard to fix.

The short version is: surge has made the surprising choice to stream uninitialized memory for unused params on osc and fx. Here's the inside of that "Blue Pad.fxp" patch which is streamed at version 9 (we are now on version 11)

                <a_osc1_param0 type="0" value="0"/>
                <a_osc1_param1 type="0" value="1056964608"/>
                <a_osc1_param2 type="0" value="1056964608"/>
                <a_osc1_param3 type="0" value="0"/>

Just random garbage basically. Because the synth doesn't know that it is ct_none

I have to deeply ponder a fix to that. It may be ugly.

@baconpaul
Copy link
Collaborator

I deeply pondered and realized the only way to do this was to give oscillators a chance to override input from old streaming versions. So I've coded that up and will include it in my monster patch change I'm in the middle of.

I should definitely do the same for FX. The vocoder will have a similar problem with my new band tuning extensions. I will open a separate issue for that.

@mortfell
Copy link
Author

ah whole other can of worms :(
Ok, well thanks for all the work. Glad I spotted in when I did I guess

@baconpaul
Copy link
Collaborator

Very very glad. Spotting it now when I’m changing version of streams let’s me fix it. Thanks!!

baconpaul added a commit to baconpaul/surge that referenced this issue Aug 17, 2019
This commit sets up a display of statuses in the synth so we
can bring to the front thigns like MPE and Tuning and bind the
menus more comprehensively. It also brings to bear the ability
(optionally) to store a tuning in a patch and the correct state
machine for what to do when loading a patch with tuning with
tuning active.

It almost completely addresses the remainder of surge-synthesizer#828
Addresses the streaming incompatability in surge-synthesizer#1035

Still outstanding is optionally a "lock" and to apply
the streaming version option to FX (which is surge-synthesizer#1037)

Handle streaming revision parameter set changes in OSCes. Deals with
baconpaul added a commit that referenced this issue Aug 17, 2019
This commit sets up a display of statuses in the synth so we
can bring to the front thigns like MPE and Tuning and bind the
menus more comprehensively. It also brings to bear the ability
(optionally) to store a tuning in a patch and the correct state
machine for what to do when loading a patch with tuning with
tuning active.

It almost completely addresses the remainder of #828
Addresses the streaming incompatability in #1035

Still outstanding is optionally a "lock" and to apply
the streaming version option to FX (which is #1037)

Handle streaming revision parameter set changes in OSCes. Deals with
@mortfell
Copy link
Author

mortfell commented Aug 19, 2019

Hey @baconpaul, was out for the weekend, just got a chance to test the new nightly.
I wanted to run some tests with a couple sounds that use this old sine blending functionality.

The new proper feedback code sound you implemented sounds VERY different than the old. I checked a few patches and also just the way a sine osc being modulated sounds. I know it's probably overall more useful now..... but I would definitely notice something was wrong if I loaded an old song that used this.

I personally don't have anything that uses it as far as I know, but I'm just worried about it because I know this would really bother me if a synth changed the way it's algorithm sounded between versions like that. (it HAS happened with a certain synth and it was very annoying I had to experiment with a handful of legacy installers to find one that had the right sound)

Also noticing how different the old feedback code for the sine sounds and knowing that it's been a part of Surge for a long time. I feel like it might be cool to have just as a flavor, since it's a unique timbre.

I'm wondering if at least for the next stable release (1.6.2) it would be possible to include a legacy version of the Sine, for loading the old patches/intentionally using the old wrong feedback way? Possibly added in one of this ways:

  • a separate oscillator in the menu (Sine (legacy))
  • a slider in the sine osc that selects from the two (legacy feedback)
  • a button somewhere in the wave display that says [old]

I know this might be a bit of an edge case thing, but to me it really feels like it would be best practice considering it audibly changes the sound.

@baconpaul
Copy link
Collaborator

Yeah your number 2 - just add a parameter which is “FM mode” that has a value of “legacy” or “consistent” and then you slide the slider. We have more than enough room. I was thinking about doing exactly that for exactly the reason you share (and set it to legacy as the default). So since we both think that, I’ll do it before we ship 1.6.2.

Since I don’t have visibility on all the presets this seems like the best path.

Presume you agree - I’ll implement this sometime this week.

Note that it means patches you make in this one week period will not work since they will default to legacy when you load them (but that’s OK - you know that and can just set them back)

@mortfell
Copy link
Author

Yeah of course. That sounds great!
updates to the UI looking good btw!

baconpaul added a commit to baconpaul/surge that referenced this issue Aug 19, 2019
The changes to Sin to make the FM feedback and the like work
properly were too big a change for old patches. So add a mode
where FM is legacy or consistent mode. In legacy mode we exactly
match 1.6.1.1 and earlier; old patches load in legacy mode automatically.
New instances load in consistent mode. Streamed patches at version
11 (this version) and higher save properly.

Addresses surge-synthesizer#1035
@baconpaul
Copy link
Collaborator

Turns out that was remarkably easy to implement. I have the PR in place now. Should be in an upcoming nightly.

Also glad the UI changes are working for you! We are closing in on 1.6.2.

baconpaul added a commit that referenced this issue Aug 19, 2019
The changes to Sin to make the FM feedback and the like work
properly were too big a change for old patches. So add a mode
where FM is legacy or consistent mode. In legacy mode we exactly
match 1.6.1.1 and earlier; old patches load in legacy mode automatically.
New instances load in consistent mode. Streamed patches at version
11 (this version) and higher save properly.

Addresses #1035
@baconpaul
Copy link
Collaborator

OK the new nightly is up with a mode for compatibility. Let me know if you think it is all good, and if so I will close this issue!

@mortfell
Copy link
Author

seems perfect to me, just tried a handful of presets that I found featuring the legacy mode, everything is loading correctly as far as i can tell!
i like having that extra option there as well, its interesting to toggle between the two modes.

@baconpaul
Copy link
Collaborator

OK awesome! I'll close this issue as resolved then. Thanks!

@mortfell
Copy link
Author

mortfell commented Aug 20, 2019

Noticed a tiny bug with the new "FM Mode" slider implementation:

weeLilBug

Changing OSC type in any oscillator slot causes any active Sin Oscillators "FM Mode" sliders to switch to "Legacy Mode". (this happens across scenes, not just oscillators in the same scene)

@baconpaul
Copy link
Collaborator

Oh that’s a quality bug report. I have an inkling and will take a look. Thank you!

@baconpaul baconpaul reopened this Aug 20, 2019
baconpaul added a commit to baconpaul/surge that referenced this issue Aug 21, 2019
Fixes the mis-application of change in surge-synthesizer#1035 by making sure the
chance to handle old streaming versions only occurs when, you know,
you are actually streaming!

Closes surge-synthesizer#1035
@baconpaul
Copy link
Collaborator

Anyway great bug report. I found it in 30 seconds and fixed it in 15 minutes. PR coming.

baconpaul added a commit that referenced this issue Aug 21, 2019
Fixes the mis-application of change in #1035 by making sure the
chance to handle old streaming versions only occurs when, you know,
you are actually streaming!

Closes #1035
@mortfell
Copy link
Author

mortfell commented Aug 21, 2019

Sweet no problem.
I'm still working on those presets a little everyday, so I'll make sure to post any bugs I find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants