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

Modern VCO - Reset - Phase Randomness #937

Closed
DerozerDSP opened this issue Sep 18, 2023 · 19 comments
Closed

Modern VCO - Reset - Phase Randomness #937

DerozerDSP opened this issue Sep 18, 2023 · 19 comments
Milestone

Comments

@DerozerDSP
Copy link

Hi,

I was trying the reset input on Modern VCO (basically, Sine Wave for now), linked to a basic gate triggered by Key:

image

But every time I feed a gate (i.e. press a key), the signal restart at different phase. Shouldn't be restart at the same phase position? Otherwise it will introduce lots of randomness.

Can you check and fix please?

Thanks

@baconpaul
Copy link
Contributor

I can check, but you can fix! :)

The default is to keep the phases randomized. If you want the phases to lock to 0 on retrigger choose this menu item:

Screen Shot 2023-09-18 at 2 33 39 PM

If that doesn't work please reopen the issue.

@DerozerDSP
Copy link
Author

DerozerDSP commented Sep 19, 2023

Hi @baconpaul ,

tried, but not always it reset at Phase 0.
Example when it does:

image

Than later I retry/retrigger, and that's the result:

image

Than again, with this result:

image

which are not "0".

Can you check it out? That's the basic path:

image image

Thanks for any helps! Not sure I can "reopen it", can't see any button...

@baconpaul baconpaul reopened this Sep 19, 2023
@baconpaul
Copy link
Contributor

Huh yeah that should work. And your pictures show it isn't!

I reopened and will look.

Thanks

@baconpaul baconpaul added this to the Xt 2.2 milestone Sep 19, 2023
@baconpaul
Copy link
Contributor

So I just looked and I think it is working, but the 'first sample' on reset has an effect from the prior waveform in the half rate filter.

Screen Shot 2023-09-19 at 11 47 39 AM

Here's a network I made. The red-cabled Modern has phase at 0 set and the blue cabled one has it unset. No matter how many times I press trigger, the red curve is exactly phased like that and the blue one shifts around.

ModernPhase.vcv.zip

Here's the vcv file zipped up. Do you see a different behavior?

The waveform when retriggered has a discontinuity but the phase of the new waveform participates starting at zero it seems.

(Note there is also an up-to-8-sample delay since the waveform internally buffers so that may cause a small jitter)

@baconpaul
Copy link
Contributor

PhaseStable

here it is in action

do you see differently?

@DerozerDSP
Copy link
Author

Not sure about your test.
What I've tried is above, and every time I send a trigger to reset, the phase doesn't exactly start at 0.0, as you can see.

Maybe its related to what you said about 8 samples-delay?

Can you trigger and record the wave? You will see the wave not starting always exacting at 0.0 value, such as:

image

@baconpaul
Copy link
Contributor

So I tested the phase is stable under retrigger which it is (and which is the guarantee)

I will need to look closely as to when the trigger happens to figure out the starting point!

@baconpaul
Copy link
Contributor

Does the wave always have the same offset? I could also just have a silent phase shift in sine in modern.

Modern is slightly delayed because of the algo. Does the sin oscillator do the same?

@baconpaul
Copy link
Contributor

Yeah OK I confirmed that with modern when I re-init I see the following as the first four samples.

REINIT -0.0743431
REINIT -1.52876e-14
REINIT 0.0743431
REINIT 0.136391

With the sine oscillator I see this

REINIT 0
REINIT 0.0171226
REINIT 0.03424
REINIT 0.0513476

I know exactly the reason for this. The modern OSC uses a trailing-two-samples derivative but this means the first output point from phase 0 is centered around -dphase, not around 0, so we get the last sample of the prior step.

It's a fix over in surge land but I can fix it for 2.2

Nice. Super detailed fine. Thank you for the report.

@baconpaul
Copy link
Contributor

Dont' have time to do the git shenanigans right now but here's the patc

diff --git a/src/common/dsp/oscillators/ModernOscillator.cpp b/src/common/dsp/oscillators/ModernOscillator.cpp
index e900ff84..56fa21e8 100644
--- a/src/common/dsp/oscillators/ModernOscillator.cpp
+++ b/src/common/dsp/oscillators/ModernOscillator.cpp
@@ -141,7 +141,7 @@ void ModernOscillator::init(float pitch, bool is_display, bool nonzero_init_drif
         unisonOffsets[u] = us.detune(u);
         us.attenuatedPanLaw(u, mixL[u], mixR[u]);
 
-        phase[u] = oscdata->retrigger.val.b || is_display ? 0.f : storage->rand_01();
+        phase[u] = oscdata->retrigger.val.b || is_display ? pitch_to_dphase(pitch) : storage->rand_01();
         sphase[u] = phase[u];
         sTurnFrac[u] = 0;
         sprior[u] = 0;

basically since Modern is lagged by one sample initiate the 0 phase one dphase ahead.

Then my re-init looks like ethis

REINIT -6.20414e-13
REINIT 0.0108317
REINIT 0.0215546
REINIT 0.0321585

baconpaul added a commit to baconpaul/surge that referenced this issue Sep 20, 2023
The modern 2-point DPW algorithm uses points at phase, phase-d and
phase -2d to calculate. This has the defacto one-sample latency
effect (at the oversample sample rate).

This doesn't matter *except* in the retrigger-from-zero case when
that shift makes the first output point at the wrong phase.

So in retrigger, intiialize phase to dphase, not to 0, so you
can hit the zero point for symmetric waveforms exactly.

Addresses surge-synthesizer/surge-rack#937
baconpaul added a commit to surge-synthesizer/surge that referenced this issue Sep 20, 2023
The modern 2-point DPW algorithm uses points at phase, phase-d and
phase -2d to calculate. This has the defacto one-sample latency
effect (at the oversample sample rate).

This doesn't matter *except* in the retrigger-from-zero case when
that shift makes the first output point at the wrong phase.

So in retrigger, intiialize phase to dphase, not to 0, so you
can hit the zero point for symmetric waveforms exactly.

Addresses surge-synthesizer/surge-rack#937
baconpaul added a commit to baconpaul/surge-rack that referenced this issue Sep 20, 2023
@DerozerDSP
Copy link
Author

Thanks! So i'll wait 2.2 for testing it :)

@baconpaul
Copy link
Contributor

You can test it with the nightly in about an hour. I just merged the fix.

https://github.com/surge-synthesizer/surge-rack/releases/tag/Nightly

takes about an hour to update after a merge

@DerozerDSP
Copy link
Author

DerozerDSP commented Sep 20, 2023

@baconpaul question: would be "huge" implement a slider here:

image

to set the desidered phase? So a (min value = random) to [0, 360]°? This way, the user can decide at which phase start the osc without impact the GUI, which would be of course more heavy as implementation, even if...

image

:P

I can open a new Issue as "change request" if that's would be possible and not so impactful :)

@baconpaul
Copy link
Contributor

That is a request we have had open in surge for 6 years and have been unable to do basically. We will probably get to it next year.

Don’t ask why. And it’s not an unreasonable request. Just it is a silly amount of plumbing and runs into a ui problem in the vst and stuff

@DerozerDSP
Copy link
Author

I see. Thanks, can't wait for this :)

@DerozerDSP
Copy link
Author

Finally got it updated on library, manually. Works like a charm, thanks.
I've noticed a "weird" thing on hard sync though: the pitch of master osc is half the freq of slave osc (i.e. it reset within 2 cycle on first hardsync knob values).

Not this way for example: https://www.keithmcmillen.com/wp-content/uploads/2015/06/sync.png
But doubling cycle.

Is this intentionally or a bug? In the latter case, I can open a issue :)

Thanks for a clarification.

@baconpaul
Copy link
Contributor

That is intentional. Surge clsssic does it that way for some reason so we kept it.

@DerozerDSP
Copy link
Author

I see, thanks :) Just curious, may I ask why? In short, which were the reasons about this design? Its uncommon, isn't?

@Andreya-Autumn
Copy link
Collaborator

I don't think anyone could answer that other than Claes (the original Surge author). Perhaps he just liked it that way. 🙃

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

No branches or pull requests

3 participants