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

Synchro, a simple phase modulation synth #5147

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

Conversation

rubiefawn
Copy link
Contributor

@rubiefawn rubiefawn commented Aug 24, 2019

2-oscillator PM synth for creating nasty sounds.

⚠️ Todo

❓ Optional

  • Improve performance. Possible pain points:
    • Heavy use of exp() and pow() during waveform generation
    • The halves of the oversampling double buffer are not guaranteed to be contiguously allocated, which might cause cache issues.
  • Use source .svg for background instead of .png, whenever that becomes possible
  • Pitch envelopes
  • Modulation envelope

❌ Issues

  • Line width for waveform preview is too thin. Ideally it would be 2px wide, but gui::Graph doesn't have any logic for line width
  • There's currently a bug where the modulator will reset its own phase every time the carrier completes a cycle, although the shape of the modulator in each of those windows is correct. For example, if the modulator is 1 octave below the carrier, it should complete a cycle once per every two carrier cycles, but instead it just plays the first half of its cycle twice.

@rubiefawn rubiefawn mentioned this pull request Aug 24, 2019
@BaraMGB
Copy link
Contributor

BaraMGB commented Aug 24, 2019

At first glance you could add an argument in the constructor of the Graph class:
Graph::Graph( QWidget * _parent, graphStyle _style, int _width, int _height, bool readOnly=false )

In the mousePressEvent() you could have an if-statement and test this readOnly-bool.

@BaraMGB
Copy link
Contributor

BaraMGB commented Aug 24, 2019

I love it! It's so simple but you can make very heavy neuro basses with it!

@DomClark
Copy link
Member

Graph is a subclass of QWidget, so you can use QWidget's enabled property. Access it with isEnabled() and set it with setEnabled(bool).

@Reflexe Reflexe added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Aug 25, 2019
@DomClark
Copy link
Member

Since you're no longer modifying the graph code, could you revert the formatting changes there?

plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.cpp Outdated Show resolved Hide resolved
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a review of the code. I've also got a couple of questions about the synth, but they might just be my unfamiliarity with these sorts of things:

  • I'm not quite sure of the advantage of having both "mod" and "mod max" controls. They're functionally identical, their values just being multiplied together. Is this a common thing to have? Can't you get the same result just by not turning "mod" up as high?
  • The "sync" control just seems to be a frequency multiplier. From what I've seen, sync usually means one oscillator resets another oscillator every cycle. Is the current behaviour and wording what is intended?

plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.cpp Outdated Show resolved Hide resolved
plugins/Synchro/SynchroSynth.cpp Outdated Show resolved Hide resolved
{
sampleFrame outputSample = {0, 0};
sampleFrame tempSample = {0, 0};
SynchroOscillatorSettings carrier = { m_carrierDetune.value(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can support sample-accurate automation if you get the model's value from its valueBuffer() when it exists. See the amplifier code for an example.

plugins/Synchro/SynchroSynth.h Outdated Show resolved Hide resolved
src/gui/widgets/Graph.cpp Outdated Show resolved Hide resolved
@rubiefawn
Copy link
Contributor Author

rubiefawn commented Sep 30, 2019

I'm not quite sure of the advantage of having both "mod" and "mod max" controls. They're functionally identical, their values just being multiplied together. Is this a common thing to have? Can't you get the same result just by not turning "mod" up as high?

Yes, but it's harder. Compare it to pitch bend vs pitch bend range. It is slightly redundant, but it increases ease of use, especially in automation.

The "sync" control just seems to be a frequency multiplier. From what I've seen, sync usually means one oscillator resets another oscillator every cycle. Is the current behaviour and wording what is intended?

The current behavior is exactly as intended, but you're right; the wording is a bit off. I'm not sure what to call it exactly, but "sync" seemed closest to the functionality. I am open to suggestions here.

For clarification, the intended functionality is a frequency multiplier, but similar to sync, the phase is still reset every cycle of the base frequency. In addition, the amplitude decreases over time in agreement with the base frequency if the "chop" control is >0, regardless of this multiplier.

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Oct 2, 2019

About sync, I have seen things labeled as "sync" work this way (e.g. Serum). You can think of it as the waveform being "synced" with a waveform with a frequency determined by the Sync knob. In other words, the Sync knob multiplies the frequency of a fake waveform which is causing the real waveform to reset.
(EDIT: This is inaccurate, see my next post below...)

@DomClark
Copy link
Member

DomClark commented Oct 2, 2019

Yes, but it's harder. Compare it to pitch bend vs pitch bend range. It is slightly redundant, but it increases ease of use, especially in automation.

👍

The current behavior is exactly as intended, but you're right; the wording is a bit off. I'm not sure what to call it exactly, but "sync" seemed closest to the functionality. I am open to suggestions here.

OpulenZ and Watsyn call it "mul", with the tooltip "frequency multiplier". (At least I think those knobs are equivalent to this one, correct me if I'm wrong.)

About sync, I have seen things labeled as "sync" work this way (e.g. Serum). You can think of it as the waveform being "synced" with a waveform with a frequency determined by the Sync knob. In other words, the Sync knob multiplies the frequency of a fake waveform which is causing the real waveform to reset.

But it's not playing part of the waveform at normal speed then resetting it, it's playing the whole thing but faster. Plus, I recall you mentioning that some of the other labels on Serum are less than accurate. 😉

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Oct 2, 2019

Sorry for all the messups in the previous posts, I'm super scatterbrained right now. I deleted some posts, now I will speak as if I actually have a brain. =)

The Sync knob is a frequency multiplier, but that waveform is being synced with a hypothetical waveform of the original frequency. So if you're playing a note at 440 Hz with a Sync value of 1.5, then the result is the waveform at 660 Hz being synced at 440 Hz. With integer Sync values, this is identical to pure frequency multiplication, but with non-integer values, it creates a drastically different sound.

I've seen this feature in multiple synthesizers, Serum's just the only one that came to mind. I think I recall some synthesizers from FL Studio also having that feature.

If you use a Sync value of 1.5 (or any other non-integer value), you should hear a drastically different timbre rather than just a frequency multiplication. Note that the copy of Synchro I have is from before this PR was made, let me know if this is no longer true...

@DomClark
Copy link
Member

DomClark commented Oct 2, 2019

It doesn't look to me like the modulator gets synced back to the original frequency any more. Here's the only place where the modulator's phase is modified, and it's just moved forward by the adjusted frequency (mod 2pi), without being reset:

//Find position in modulator waveform
sample_index[1] += sampleRatePi * DetuneOctaves(nph->frequency(),
modulator.Detune);
while (sample_index[1] >= F_2PI) { sample_index[1] -= F_2PI; } //Make sure phase is always between 0 and 2PI

(I may well be wrong - I'm only getting this from the code. My last build of this PR is from before fractional sync was supported, so I can't test it right now.)

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Oct 2, 2019

I think the while (sample_index[1] >= F_2PI) { sample_index[1] -= F_2PI; } part would cause the phase reset. It sets the phase to 0 (ish) whenever the original waveform (ignoring sync) reaches its end, which would interrupt the synced waveform when using fractional frequency multiplications.

@rubiefawn
Copy link
Contributor Author

The frequency multiplication happens in the waveform generation function, so the phase is automatically reset as part of that.

@DomClark
Copy link
Member

DomClark commented Oct 2, 2019

Ah yes, I see. I thought sample_index[1] was the phase after sync had been applied, but it's the phase before. Thanks for clearing that up.

@PhysSong PhysSong changed the base branch from master to stable-1.2 April 22, 2020 02:30
@PhysSong PhysSong changed the base branch from stable-1.2 to master April 22, 2020 02:30
@PhysSong
Copy link
Member

@iansannar I fixed the merge conflict for you. Please pull it before you go further.

@LmmsBot
Copy link

LmmsBot commented Apr 22, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://302-204106384-gh.circle-artifacts.com/0/lmms-1.2.0.717-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/iansannar/lmms/302?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://303-204106384-gh.circle-artifacts.com/0/lmms-1.2.0.717-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/iansannar/lmms/303?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://299-204106384-gh.circle-artifacts.com/0/lmms-1.2.0.717-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/iansannar/lmms/299?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "915c3a535cb69b58b34808c19e45f38592b50939"}

@PhysSong PhysSong requested a review from DomClark April 22, 2020 02:55
@rubiefawn rubiefawn marked this pull request as draft April 22, 2020 05:14
@PhysSong
Copy link
Member

It seems like you removed the SynchroNote destructor accidentally. You should either restore it on the source file or add = default in the header file to fix the undefined reference error.

@rubiefawn rubiefawn marked this pull request as ready for review April 23, 2020 18:25
@PhysSong
Copy link
Member

To prevent compiler warnings, you should make the order of declaration and initialization consistent.

@PhysSong
Copy link
Member

@iansannar Do you want to add something more here? Or, can I start review?

rubiefawn and others added 21 commits January 24, 2023 03:30
Didn't realize commenting out that line would disable the FX rack yikes. Also, realized that my exponential envelope was inverted so fixed that
I keep missing these things
Made these edits on Windows so if things break I won't know till the CI tells me haha
Not sure if this will work but here we go
m_nph over nph, "pluginBrowser" to "PluginBrowser"
i could reboot into linux to compile and test this or i could let the CI do it for me and commit a trillion times. hmm
@rubiefawn
Copy link
Contributor Author

Ah. I have no idea how I did that, but clearly I didn't rebase properly and now the commit history is duplicated 😓

Anyways, after re-examining this, it's clear that it's not even close to finished. I've updated the todo comment at the top.

rubiefawn and others added 3 commits July 9, 2023 04:32
Conflicts:
* cmake/modules/PluginList.cmake
After the merge in commit e94b3c1 the code of the `SynchroSynth`
class did not compile anymore due to several reasons. This commit
includes the fixes that were necessary to make it work again.

It's mostly:
* Removal of the `MM_OPERATORS` macro
* Switch from `sampleFrame` to the "new" `SampleFrame` class
* Adjusting some enums (`Plugin::Type::Instrument`, `Graph::Style::Linear`, `KnobType::Dark28`)
* Use `AudioEngine::outputSampleRate` instead of `AudioEngine::processingSampleRate`

The usage of `SampleFrame` also made slight changes with regards to the
assignment of the result samples necessary.

Fix warnings in three for loops which used `int` as the running variable
over an `unsigned int` type.
@michaelgregorius
Copy link
Contributor

I have resolved the merge conflicts by merging master and have fixed all compilation problems that have amassed over time with commit d67a3ed.

Hope this might revive development. The synth still works. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.