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

Add voice data to formula modulator #7727

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

nuoun
Copy link
Contributor

@nuoun nuoun commented Jul 25, 2024

  • state.voice_limit = poly limit
  • state.voice_n = active voice index
  • state.voice_order = voiceOrderAtCreate

Just a draft until we decide if this is what we want and if so if this is the best way

I think the first two would be very useful to have at voice level for anything from unison detune to arpeggiator modulators. VoiceOrder was already passed to the voice constructor so easy to add but not sure how useful it will be compared to an actual active voice counter (which I still need to figure out how to do).

@mkruselj
Copy link
Collaborator

First two are indeed very good. voice_n in particular will be great to provide i.e. fixed panning offsets for each voice (think vintage Oberheim). I am not sure if voice_order is useful at all... active voice counter seems a lot more useful.

@baconpaul
Copy link
Collaborator

There’s already a voice counter kicking around somewhere. It’s how we do the last voice calculation on release. Not sure we need to add another one. But also since you can’t write cross voice modulators I’m not quite sure how it would be used in formula.

Is the voice index stable under pother note releases? Let’s make sure to define these semantics carefully

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

First two are indeed very good. voice_n in particular will be great to provide i.e. fixed panning offsets for each voice (think vintage Oberheim). I am not sure if voice_order is useful at all... active voice counter seems a lot more useful.

So I got rid of voice_order. Oberheim style voice panning would be amazing to have as well, is there currently a way to do panning at voice level rather than scene?

There’s already a voice counter kicking around somewhere. It’s how we do the last voice calculation on release. Not sure we need to add another one. But also since you can’t write cross voice modulators I’m not quite sure how it would be used in formula.

Yes there is the atomic int polydisp in SurgeSynthesizer.cpp, which I'm currently caching and passing directly to SurgeVoice() to set the voice index. Then just updating that value in FormulaModulationHelper.cpp like "cycle" / "intphase" is being done makes it a counter rather than an index.

Is the voice index stable under pother note releases? Let’s make sure to define these semantics carefully

Will poke around and test a bit more over the weekend as there's probably some edge cases that I missed but so far it seems to work fine for poly play mode in a single scene at least.

@mkruselj
Copy link
Collaborator

You can of course use the Random modulator to modulate the scene panning. But this is random and not fixed.

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

You can of course use the Random modulator to modulate the scene panning. But this is random and not fixed.

Closest I could think of so far is the stereo filter configuration but that's far from ideal. I suppose it would be nice to have actual panning sliders per mixer channel at some point but for individual voices it might need to be done at the oscillator level instead?

@Andreya-Autumn
Copy link
Collaborator

Oberheim style voice panning would be amazing to have as well, is there currently a way to do panning at voice level rather than scene?

The pan control is in fact per-voice, so the answer is yes, apply any voice-level modulator to pan. Be advised in wide mode, pan is borked, and you should probably avoid going past ≈73 percent. But you can pan voices independently even in the mono modes!

I'm not familiar with the Oberheim but I'm guessing voice index determines pan such that the voices spread evenly across the stereo field? In that case, the additions discussed here would let you would let you do that indeed.

Will poke around and test a bit more over the weekend as there's probably some edge cases that I missed but so far it seems to work fine for poly play mode in a single scene at least.

Make sure you test with two scenes. Also latch one scene and not the other. Basically throw nonsense at it and see what comes out. The Surge voice manager is... complicated. So let's make sure we actually get what we want.

@Andreya-Autumn
Copy link
Collaborator

but for individual voices it might need to be done at the oscillator level instead?

We have that planned for XT2. It's in the issues somewhere. Alongside the "Pan is borked" one I mentioned. :)

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

The pan control is in fact per-voice, so the answer is yes, apply any voice-level modulator to pan. Be advised in wide mode, pan is borked, and you should probably avoid going past ≈73 percent. But you can pan voices independently even in the mono modes!

That's good to know, thank you! :)

I'm not familiar with the Oberheim but I'm guessing voice index determines pan such that the voices spread evenly across the stereo field? In that case, the additions discussed here would let you would let you do that indeed.

Not sure how it's exactly done over different Oberheim models but if I'm not mistaken the essence is that it adds both a fixed panning control for each voice plus a global control to spread voices out over the stereo field, the main difference with Surge would be that it also affects unison voices. Discodsp's OB-Xd VST (which has an older free version iirc) does a pretty good job at implementing this I think and it makes it sound rather lush and wide with the added stereo depth.

Make sure you test with two scenes. Also latch one scene and not the other. Basically throw nonsense at it and see what comes out. The Surge voice manager is... complicated. So let's make sure we actually get what we want.

Yeah the different scenes might be an issue in the current implementation, luckily the scenes are separated well in SurgeSynthesizer.cpp at least and the complexity there does make me hope we can get away with the smallest possible change here.

@Andreya-Autumn
Copy link
Collaborator

Sounds like pan per voice + a width control. We have those, but unison voices position (in wide mode) are fixed. So we can't get quite all the way there in XT1. For XT2 we've discussed improving control over unison panning strategy. But yeah, that's for XT2.

@mkruselj
Copy link
Collaborator

The crucial thing is that every voice is a static offset on an Oberheim. So you have say 8 voice cards and as you play polyphonically they either rotate in round robin (incrementing the voice index) or they advance the voice index only when a new key is played (meaning playing the same key multiple times only takes one voice - we have that as an additional option on Play Mode right click, it's the "piano mode"). That's the voice index thing we'd need to properly emulate an Obie.

@baconpaul
Copy link
Collaborator

We do not have that voice index (nor do we need it) easily in the synth

@nuoun nuoun closed this Jul 26, 2024
@nuoun nuoun deleted the formula-voicestate branch July 26, 2024 17:55
@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

Yes actual round robin would need some data structure to keep track of active voices, voiceOrderAtCreate which we do have is the next best thing which I think could do the rotating panning but not something like an arpeggiator, closing this now as it seems there's no further enthusiasm to add this.

@baconpaul
Copy link
Collaborator

Something like this would be super useful just we need to make sure we know what it is.

Like a stable voice index is hard because of releases. So do we want which voice of how many sounding we are sorted by creation order? Maybe?

So I’m enthusiastic for the class of idea just want to make sure to be careful since once we add something it’s hard to remove

@mkruselj
Copy link
Collaborator

Why close the PR? The first two additions are great!

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

Why close the PR? The first two additions are great!

It was just an idea but thinking this over more it's just not something that can be easily added properly I don't think, keeping track of voice releases is not trivial as Paul said and just using voiceOrderAtCreate % maxpoly is rather limited.

@Andreya-Autumn
Copy link
Collaborator

Spontaneously I think what Paul said sounds good.

So do we want which voice of how many sounding we are sorted by creation order? Maybe?

If I understand that right. It means we have two things. Let's call them state.currentVoiceCount and state.VoiceNum
(better names needed).

Play and hold a single note, they're both 1.
Play and hold another, currentVoiceCount is 2, voiceNum is still 1 for the first, and 2 for the other.
Play and hold yet another, you get the pattern. The last voice will have voiceNum 3.
Release the first one, count goes down to 2, the remaining ones become voiceNum 1 and 2.

That'd make sense to me!

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

If we have certain modulation (lets say panning) depending on the voice index and have the voice index of still active voices change dynamically after previous voices end, it would make existing voices start doing different things, is that really what we want though? - (we could save the initial voice index inside the state table but that would mean that each voice does not have awareness of it's position relative to the other voices)

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

Imo, a round robin approach for voice index without the voice stealing makes most sense, so in your example after releasing the first note index 1 becomes available again and would be used again after we roll over when going over the max poly number and start counting from 1 again, and 2 and 3 stay reserved as those voices are still playing.

That way each voice will keep it's own unique voice index, but this requires some data structure which needs to keep track of active voices so each index stays unique, without having a good way to delete active voices after release. My initial thought is a vector that gets renewed by each active voice and deletes the ones that have not gotten a recent update.

@Andreya-Autumn
Copy link
Collaborator

Well I think that might at least be one thing that we want!

For pan for example, it does sound super fun to me to have voices move around dynamically to make space for one new ones, or take up space left by ended ones. Can always slew limit them if the shifts are too sudden. And the round-robin thing wouldn't really work here because "how much space is there for each voice" is answered by current voice count not max poly.

But yeah I see the merits of both.

@nuoun
Copy link
Contributor Author

nuoun commented Jul 26, 2024

There's also an issue when a voice ends and a new voice starts during the same sample block, can't really reorder that properly without a way to keep track of the leased out index values I would think (but luckily we do have voiceOrderAtCreate for that which will always keep a unique number)

@nuoun nuoun reopened this Jul 30, 2024
@nuoun
Copy link
Contributor Author

nuoun commented Jul 30, 2024

Still a draft but after sleeping on it and consulting the missus this is the WIP version of what I'd like to suggest here, the parameters that get added to the voice formula editor are:

state.voice_poly = updating poly voice counter as used by GUI
state.voice_limit = polylimit
state.voice_order = voiceOrderAtCreate (incremental counter for each new voice gives each a unique ID which will help on the Lua side to keep voices apart)
state.voice_index = basic round robin voice index constructed from voiceOrderAtCreate % polylimit (optional as this and more can also be done on the Lua side)

This is more or less what I added initially but still needs more work, specifically the voice counter is still an issue as the atomic int polydisplay seems to not be passable as a pointer like all the other values to the constructor SurgeVoice() from SurgeSynthesizer.cpp so I'm currently saving and grabbing it using SurgeStorage but would much rather also pass it directly. (May need a this pointer from SurgeSynthesizer.cpp passed to SurgeVoice() as polydisplay is public?)

The main change here is to add a "global" table that is shared with every Lua instance (just like the "surge" table for the prelude currently is) and the entries of this new table get cleared every new function environment so they can be garbage collected. Having this table available on the initial Lua state allows all the individual formula modulators (and currently also the WTSE) over all voices to share parameters in the same table so more complex stuff as figuring out the relative position of modulators/voices becomes possible.

Can split this up in 2 smaller PRs, one for the global table and one for the new parameters if that helps with reviewing, the main advantage seems to me that this really does not require any major changes. The voice parameters are all readily available and even the global table method already works using the "surge" table.

@nuoun
Copy link
Contributor Author

nuoun commented Aug 3, 2024

So this does the following

  • Adds voice_id (=voiceOrderAtCreate), voice_max, voice_count, is_display parameters to formula modulator
  • Adds voiceCount int to SurgeStorage
  • Fakes voice_count of "1" and sets defaults for other params on display calls
  • Adds "shared" table to formula modulator
  • Sets entries of "shared" table to nil on new setSurgeFunctionEnvironment()
  • Adds "shared" table to formula debugger
  • Adds lua_isfunction to formula debugger
  • Does a bit of cleanup

I still need to do some testing on other platforms but if we can reach consensus whether this is what we want, it passes review and finally when we agree on the final var names this can be merged I hope!

@baconpaul
Copy link
Collaborator

Sorry I’m unclear is this ready for review now? Or still a draft? Fine either way of course but if it is ready and you are waiting and I think it’s a draft that’s a bummer :)

@mkruselj
Copy link
Collaborator

mkruselj commented Aug 6, 2024

It seems we're not in Draft anymore, Alice.

@baconpaul
Copy link
Collaborator

oh but the git looks all wrong. There's all that workflow change stuff. Let me see if I can figure out why.

nuoun added 3 commits August 6, 2024 08:40
Clang Format

add "global" table to Lua state plus redo voice data for formula editor

Default sandbox functions

Add back "global" table clear in setSurgeFunctionEnvironment()

Add stack test

squash

Fix segfault in testrunner

clang format

Move Nimbus from libsamplerate to our Lanczos downsampler (surge-synthesizer#7726)

1. Makes it ready to move to sst-effects
2. Makes it so we can almost remove libsamplerate (but some
   twist smidgeof work to do that)
3. Uses less CPU

Addresses surge-synthesizer#7359

Remove libsamplerate dependency (surge-synthesizer#7728)

After porting Nimbus ot lanczos in 4b3ffe6 we can now remove the
libsamplerate dependency altogether as long as we fix up the twist
fm (which we did with a simple application of another lanczos ds)
and remove src code from the unit tests (where it was explicitly
tested)

Move Nimbus to sst-effects (surge-synthesizer#7729)

* Move Nimbus to sst-effects

Addresses surge-synthesizer#7359

With an almost-complete port (param name dynamism is in surge
but not in the submodule yet)

* f

Update LFO and Step Seq presets to have the LFO EG enabled (surge-synthesizer#7732)

Code Checks github action, as part one of move to actions

Move to actions here will be a bit trickier, but we will
get there. Step one is have any action at all, so add a code
check action.

Pull Request moves to Github Actions (surge-synthesizer#7733)

The PullRequest support of our azure pipeline is now on GitHub actions, building in various configurations and running the tests. The azure pipeline still exists to trigger the release stage, which is what I'll port tomorrow, or maybe thursday, and then be done with the azure->actions project.

- Adds voice_id, voice_max, voice_count, is_display parameters to formula modulator
- Fakes voice_count of "1" and sets defaults for other params on display calls
- Adds "shared" table to formula modulator
- Sets entries of "shared" table to nil on new Lua function env
- Makes formula debugger also use "shared" table
- Adds lua_isfunction to formula debugger
- Some cleanup
@baconpaul baconpaul force-pushed the formula-voicestate branch from 72db658 to 1ee52c6 Compare August 6, 2024 12:40
@baconpaul
Copy link
Collaborator

Yeah OK I rebased it properly and pushed. Now it's 8 files changed rather than 50! Phew.

Let me let CI run again but I'll look at the code now

src/common/dsp/SurgeVoice.cpp Outdated Show resolved Hide resolved
src/common/SurgeStorage.h Show resolved Hide resolved
src/common/dsp/modulators/FormulaModulationHelper.cpp Outdated Show resolved Hide resolved
src/common/dsp/modulators/FormulaModulationHelper.cpp Outdated Show resolved Hide resolved
@baconpaul
Copy link
Collaborator

OK all looks good but did have a few comments I'd appreciate you taking a peek at. Thanks!

@nuoun
Copy link
Contributor Author

nuoun commented Aug 6, 2024

Thanks for the review (and cleaning up the mess!), I'll try to go over your comments one by one.

nuoun added 2 commits August 7, 2024 15:11
- Rename voiceMax -> polylimit
- Rename voiceid -> voiceOrderAtCreate
- Add constexpr const char definitions for "surge" and "shared" table names
- Replace "return rows;" with "continue;" in createDebugDataOfModState() error condition
@nuoun
Copy link
Contributor Author

nuoun commented Aug 7, 2024

Ok if this passes and there are no other changes needed then I think we're good to go, only thing that remains is to decide on the final parameter names inside the Formula state table, currently they're named:

  • voice_count
  • voice_polylimit
  • voice_id
  • is_display
  • shared for the new global table name

Then when this is merged I will open an issue to remove voiceCount from SurgeStorage, ponder about a new tutorial patch to showcase the new features and have a look at what can be added to the manual.

nuoun added 2 commits August 7, 2024 17:17
voice_polylimit -> poly_limit
is_display -> is_rendering_to_ui
@baconpaul baconpaul merged commit c914d3b into surge-synthesizer:main Aug 7, 2024
12 checks passed
@nuoun nuoun mentioned this pull request Dec 20, 2024
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.

4 participants