zynaddsubfx / zyn-fusion-issues

Issue Only Repo
31 stars 0 forks source link

Portamento #304

Open zynmuse opened 2 years ago

zynmuse commented 2 years ago
  1. With portamento ON, Polyphonic selected, Time set high, Up/Down about half way.

Using the zyn piano keybourd I hold a note for a while, release, hold a different note. Frequency slides up slowly from one to the other. Seems OK! So long as I give the note time to be reached portamento works fine.

If I don't wait for the frequency to reach the second note, then portamento doesn't work on any subsequent notes.

In order to get portamento back again I have to wait without pressing any keys a few seconds, possibly the length of original slide time, before portamento starts to work again.

It looks like a timer needs to be reset when a portamento slide is interrupted.

There are lots more problems with different modes and settings but it may be that they stem from this one!


  1. In monophonic mode portamento only works by overlapping two notes, so I can't make any portamento from the keyboard. With monophonic mode by overlapping notes I cannot change the slide time, none of the knobs do anything.

  2. An explanation of how portamento is supposed to work would be a good starting point, then we could easily comment on what doesn't work, or suggest a better way of it working.
fundamental commented 2 years ago

There's some difference in how both of us understand mono mode. I'm able to get portamento to work just fine going between notes in mono mode. Per poly mode I honestly don't know the intended behavior.

zynmuse commented 2 years ago

1.Polyphonic mode portamento works well except for the timer problem explained. It's fine if the keep the time shorter than the notes.

  1. Monophonic mode works fine overlapping notes but none of the knobs do anything.
fundamental commented 2 years ago

The knobs appear to work just fine in monophonic mode. Note that some require proportional mode, so that's a separate UX issue. mono mode does seem to have the issue that if a portamento sweep is cut short it's broken until all keys are released, so that's something which ought to be sorted out.

polluxsynth commented 2 years ago

When it comes to portemento, I'd really like to have to types of behavior, regardless of poly/mono/legato mode setting:

  1. Whenever a new note is played, the pitch always slides from the previously played pitch.
  2. Reminiscent of "Auto glide" mode on many synths: Portamento is only exhibited if there is a previous note held down. This would work also in polyphonic mode and would be a nice way to play a monophonic line with only some notes exhibiting portemento, without having the return-to-previous-note paradigm that the mono and legato modes exhibit.

I get the impression that the "Trigger type" button is supposed to toggle between two modes that are somewhat, but it doesn't match the above behavior. There are probably a lot of corner cases that need to be ironed out, and perhaps others have other ideas, but I thought it would be a good starting point for further discussion on how we want this to work.

fundamental commented 2 years ago

I get the impression that the "Trigger type" button is supposed to toggle between two modes that are somewhat

Nope. There's some drips of information hiding in the old docs about the trigger mode, but it boils down to something like:

if(sign(dist(prev_note-this_note)) == (trigger_type? 1 : -1))
  do_portamento()

With portamento and a bunch of other features the ideal would be to have some nice design docs with all sorts of figures in them, though it sure does chew through a lot of time trying to get that sort of documentation organized and maintained.

polluxsynth commented 2 years ago

So basically the trigger type governs (is supposed to govern) whether one gets portamento when playing upwards or downwards on the keyboard? That would rule out having portamento for both directions?

fundamental commented 2 years ago

Close, dist() in this case would be defined 0..infinity, so it's whether portamento applies to only close notes or only far away notes.

polluxsynth commented 2 years ago

Ok, then I think understand what you mean, though the meaning of the pseudocode escapes me: if dist is 0..infinity, then sign(dist(something)) would be either 0 or 1 I would have thought. Ah, the threshold is missing. Perhaps like this:

if(sign(dist(prev_note-this_note) - thresh) == (trigger_type? 1 : -1))
  do_portamento()
fundamental commented 2 years ago

ah, yep. That's my mistake, you've got the right interpretation now.

polluxsynth commented 2 years ago

It struck me that the parameter is not 'trigger type', it's 'threshold type' (the tooltip text says 'type of threshold'), although in the GUI it should really be something like 'trigger when distance larger than' (although I have no idea how to shorten that to something that will fit in a button, '> thresh' perhaps), because "threshold type" sounds more like something that should be an enum and not something that can be enabled or disabled.

polluxsynth commented 2 years ago

Going back to the original problem reported by @zynmuse (sorry for hijacking the thread somewhat above), it seems that in Poly and Mono modes there is some design philosophy that says "don't start a new portamento until the previous one has finished". And this goes awry if a third note is pressed during the portamento time, as that means that since the second note played did not reach its target pitch within the portamento time, the third one won't either.

I can think of some logic to this behavior, because one would naturally expect portamento to start at the pitch that the (last playing in poly mode) voice was actually playing. But to avoid having to determine that, as it involves digging out the current pitch of another voice, one possible philosophy could be "if we don't know what pitch the last played voice is actually playing, and we don't if it's still gliding, just skip the portamento to make things easier".

An alternative behavior would the one that I noted in https://github.com/zynaddsubfx/zyn-fusion-issues/issues/312, in which, among other problems, when a note in legato mode is triggered (either by pressing a new key, or by the voice allocator when an existing key is released and there are one or more keys still being depressed), the portamento doesn't start at the pitch that the single playing voice happens to be at, as would be expected, but rather at the pitch of the previously playing note. This creates a bit of an odd effect if the second note is only pressed briefly, as the portamento will start at a pitch which is not actually currently playing. It doesn't sound to bad though, and does give a consistent portamento behavior regardless of whether the previous note reached its pitch or not.

So my suggestion would be just to skip this portemento inhibiting feature, and let all notes do portamento from whatever pitch the previous note was going to reach after finishing its glide phase. It may not be consistent with how a true analog synth would behave (where the glide would naturally start at whatever pitch the voice happened to be playing), but would at least be predictable.

Perhaps this could be improved by coming up with a function to extract the current pitch of the last playing note, to determine where to start the glide for the next note.

Alternatively, as @zynmuse suggested, one could cut the (non) portemento time short for notes for whom portemento is not triggered. But I think this would result in somewhat surprising behavior, as, say three notes are pressed in quick succession (within the portemento time). The first note exhibits portemento, the second won't, because it was pressed within the portamento time of the second, but the third note will exhibit portemento, as the timeout for the second note was cut short since portamento was disabled for that note as mentioned.

friedolino78 commented 2 years ago

I had a similar problem when designing the LFO fadein and fadeout feature. If the key is released while fading in the fadeout has to start at the current intensity to prevent a discontinuity in what ever param is LFOed.

For portamento I would like to prevent discontinuities, too. At least we should have an option for this more analog like behaviour. actually I don't see the problem in "digging out the current pitch"

Another option to make the portamento behaviour more "natural" could be a slew rate limiter.

polluxsynth commented 2 years ago

I've created a PR for the original problem https://github.com/zynaddsubfx/zynaddsubfx/pull/147, but this only fixes the original problem of portamento being delayed for one complete (but retriggered for every note played) portamento cycle.

One root problem here seems to be that the portamento function is implemented in the Controller class, so there can only be a single note exhibiting portamento in poly mode. So it's not the case that after one note is played which exhibits portamento that subsequent notes are consciously inhibited from having portamento, on the contrary it's by deisgn only possible for a single note to exhibit portamento at any one time. One solution to that might be to move the portemento function to the SynthNote class so that each note played has its own portamento "envelope". I haven't looked deeper into it though.

Another problem is that as when portamento is initiated, it starts from the target pitch of previously played note, rather than the actual pitch at the time of the portamento. As @friedolino78 noted, this should be fixable.

EDIT: After thinking about it, SynthNote doesn't seem to be the right place, being the base class for the various synth engines means that each synth would get its own portamento engine which is rather wasteful. I'm rather thinking the NotePool would be a better place, because that's where the information common to all synths is collected.

polluxsynth commented 2 years ago

I've been looking into this a bit (current work in my dev branch: https://github.com/polluxsynth/zynaddsubfx/tree/dev), and my plan is to for each note played have a PortamentoRealtime struct which is manipulated by the Controller::initportamento() and Controller::updateportamento() methods, and read by the synth engines in the same way that they access a subset of the portamento varables in the Controller class today, but having the realtime struct on a per-note basis rather than per part.

I am trying to grasp exactly where to put the realtime portamento data. One idea is to allocate the struct when needed, i.e. when a new note is created, and I see there is a special Allocator class that is used for instance when new synth notes are created in Part::NoteOnInternal(), which I'm assuming must be used here in the realtime thread, rather than a plain new. The NotePool would have a copy of the pointer so it could be deleted (or rather, dealloc:ed) when the note descriptor is marked as OFF and the realtime portamento data is no longer needed.

Putting it directly in a NoteDescriptor wold not work as the note descriptors are moved around when the note pool is cleaned, so any pointers sent to the synth engines will point to the wrong descriptor after a while.

However, another idea would be to statically allocate an array of PortamentoRealtime structs, as many as the note descriptors (i.e. POLYPHONY), which are then referred to by the respective note descriptors, the point being that the elements in the portamenot array will not be moved about. A simple round-robin "try-the next-free-portamento-descriptor-and-if-not-free-try-the-next-etc" would be sufficient for as an allocator. Being statically allocated it's a bit wasteful of space, though. The realtime portamento data contains four floats or so, and for comparison a note descriptor seems to be four uint8_t:s, an uint32_t and a legatoMirror flag (which as noted in the code sticks out as a sore thumb a bit as it probably causes a further 64 bits to be used on modern machines).. On the other hand, the resulting 4 * 60 = 960 bytes of space used is probably not much to write home about even in a memory cramped environment these days.

So, what is a reasonable strategy in this environment? Waste 1 kbyte of static space with the guarantee that there is memory for realtime portamento data for all notes, or have the overhead of running an allocator to allocate a mere 4 or so floats for the realtime portamento struct for each new (non-legato) note played? I'd lean towards the former considering how little memory is involved compared to the computed waveforms for instance, but there's perhaps something I haven't considered in this regard, considering that there is concern voiced in the comments in the code regarding to the amount of space used by the note and synth descriptors for instance.

fundamental commented 2 years ago

Thinking about this for a while, it feels like it might be more natural to leave it in the synthnote level (even if it does waste a few bytes here and there). If it's at that level, then each note has a destination frequency along with a frequency slew-rate/convergence speed (already stored in the controller). It's possible to put it in the notepool, but it does involve some extra code complexity as you've described. Then notepool is responsible for making sure that the destination frequency is updated and synthnote does the rest.

polluxsynth commented 2 years ago

At least initially I was going to leave the actual portamento initialization and realtime update in the Controller class, though your insights make me think it should eventually be moved once I get it working. Also, after some experimentation I agree it should be kept out of the notepool as far as possible.

My question still stands though: if anything needs to be allocated at the same time as the ADnote/SUBnote/PADnote, it should be done by memory.alloc, rather than a simple new?

fundamental commented 2 years ago

if anything needs to be allocated at the same time as the ADnote/SUBnote/PADnote, it should be done by memory.alloc, rather than a simple new?

Yep. Allocation on the realtime thread is done from a memory pool rather than the heap. This is done to avoid triggering any kernel syscalls which would cause the realtime thread to lose its priority and get rescheduled until later.

polluxsynth commented 2 years ago

Work-in-progress: https://github.com/polluxsynth/zynaddsubfx/tree/dev .

Right now, polyphonic portamento on a per-note basis works, same with monophonic portamento, although the portamento for new notes starts on the target pitch for the previous note, rather than as I want it the actual pitch that the previous note is at when the new note is started. Trivial to fix, just haven't done it yet.

However, legato mode is currently broken. There is no portamento at all in legato mode, and the note allocation is weird. Shouldn't be to hard to fix hopefully.

polluxsynth commented 2 years ago

After a couple of debugging sessions, I have something on my dev branch (https://github.com/polluxsynth/zynaddsubfx/tree/dev) that works insofar that it all notes that are supposed to exhibit portamento actually do experience portamento, even in polyphonic mode. This is achieved by letting the part handle a portamento object, where one is created for every new note played, each synthnote being active referencing the portamento object much in the way they do currently, it's just in a different place. My reason for letting the part manage this rather than synthnote, is that when a new note is triggered during an ongoing portamento, I want the new note to start where at the pitch which the old one is currently playing, which might be at an intermediate pitch if the old note is exhibiting portamento (currently I've not implemented that, that's the next step, but it should be trivial given the design). This means that the part needs to have access to the portamento data, and I felt putting that data in synthnote created an inverse dependency: the part should be in control of the synthnote, not asking it things.

It also makes sense because portamento is a part thing rather than a synth thing, although it's debatable whether the user view needs to correspond 100% to the internal design.

Once created, the portamento object is administered by the note pool, in a similar way to the way the individual synthnotes are administered, i.e. deallocated by the note pool when a note is killed. This creates a well defined point in time when the portamento object is deallocated, when no one is referring to it anymore. I do have to take special consideration of the part however, which keeps track of the last used portamento object in order to determine the pitch at which to start new portamento operations (like I said, not yet implemented, but the design is geared towards this); this is done by having a deallocation callback so the part can do its internal management of saved portamento object pointers without the notepool being actively involved.

When legato mode is in effect, the portamento object is reused, thus mimicing the way the synthnotes are reused, after the initial upgradeToLegato.

This design seems to hold up but comments are of course welcome.

One thing, when I actually do get around to submitting a PR, should all the patches be collapsed into one, or should there be a few major commits, or left as it is now? I've been trying to keep refactorization and implementation separate, but as is often the case, refactorization is not always apparent until the new implementation is underway, and it's often cumbersome to rebase the commits in a logical order.

zynmuse commented 2 years ago

I'm glad someone is addressing this. From my point of view I would like to see a clear explanation detailing exactly how portamento will appear to the user in all modes - monophonic, legato, polyphonic and with all parameters explained. This can eventually form part of the manual, but at this point will allow us to comment and agree on what we would like to see. Without this it is possible that what is implemented will not be acceptable to everybody. Obviously I'm happy for you to implement it!

On Sat, Oct 16, 2021 at 7:08 AM polluxsynth @.***> wrote:

After a couple of debugging sessions, I have something on my dev branch ( https://github.com/polluxsynth/zynaddsubfx/tree/dev) that works insofar that it all notes that are supposed to exhibit portamento actually do experience portamento, even in polyphonic mode. This is achieved by letting the part handle a portamento object, where one is created for every new note played, each synthnote being active referencing the portamento object much in the way they do currently, it's just in a different place. My reason for letting the part manage this rather than synthnote, is that when a new note is triggered during an ongoing portamento, I want the new note to start where at the pitch which the old one is currently playing, which might be at an intermediate pitch if the old note is exhibiting portamento (currently I've not implemented that, that's the next step, but it should be trivial given the design). This means that the part needs to have access to the portamento data, and I felt putting that data in synthnote created an inverse dependency: the part should be in control of the synthnote, not asking it things.

It also makes sense because portamento is a part thing rather than a synth thing, although it's debatable whether the user view needs to correspond 100% to the internal design.

Once created, the portamento object is administered by the note pool, in a similar way to the way the individual synthnotes are administered, i.e. deallocated by the note pool when a note is killed. This creates a well defined point in time when the portamento object is deallocated, when no one is referring to it anymore. I do have to take special consideration of the part however, which keeps track of the last used portamento object in order to determine the pitch at which to start new portamento operations (like I said, not yet implemented, but the design is geared towards this); this is done by having a deallocation callback so the part can do its internal management of saved portamento object pointers without the notepool being actively involved.

When legato mode is in effect, the portamento object is reused, thus mimicing the way the synthnotes are reused, after the initial upgradeToLegato.

This design seems to hold up but comments are of course welcome.

One thing, when I actually do get around to submitting a PR, should all the patches be collapsed into one, or should there be a few major commits, or left as it is now? I've been trying to keep refactorization and implementation separate, but as is often the case, refactorization is not always apparent until the new implementation is underway, and it's often cumbersome to rebase the commits in a logical order.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zynaddsubfx/zyn-fusion-issues/issues/304#issuecomment-944864644, or unsubscribe https://github.com/notifications/unsubscribe-auth/APGUENTZWCXJKNX45W7YTTLUHEJGPANCNFSM5D4NTVLQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

polluxsynth commented 2 years ago

From my point of view I would like to see a clear explanation detailing exactly how portamento will appear to the user in all modes - monophonic, legato, polyphonic and with all parameters explained. This can eventually form part of the manual, but at this point will allow us to comment and agree on what we would like to see. Without this it is possible that what is implemented will not be acceptable to everybody.

That's a very good point. This is turning from a simple bugfix to something that is basically a rework of the portamento function.

What I'd like to see is something that from a user perspective is a simple as possible, and also be musically useful of course. Basically, I think portamento should essentially behave the same way regardless of the poly/mono/legato mode set. Poly/mono/legato should only govern the assignment of notes. I also believe that it's a useful and desirable feature to have portamento in poly mode.

Something like this:

(START EDIT) The basis for the portamento function is the corresponding function in a monophonic analog synthesizer. In an analog synthesizer, portamento is implemented as a lag circuit on the CV from the keyboard to the oscillators, providing a smooth glide of the pitch rather than jumping to the new pitch when a new note is played. Thus, a key feature of portamento is that whenever a new note is played, regardless of whether the target pitch of the previous note had been reached or not, the journey to the new pitch starts at whatever pitch prevailed at the time.

In a polyphonic analog synthesizer, a trivial implementation of portamento is using separate lag circuits at the keyboard CV inputs of each voice. This does create a bit of a chaotic portamento behaviour however, because the glide for a played voice will start at whatever pitch the individual voice had previously, which is not necessarily the same as the previously played note, as it would be in the case of monophonic portamento. Nevertheless, this is the most common implementation of portamento on a polyphonic analog synthesize, and the mild chaos that ensues when portamento is enabled is still musically useful.

In a digital synthesizer, it is possible to emulate the behaviour of an analog polyphonic synthesizer, especially if the implementation models the behaviour in terms of a fixed set of voices which are allocated much in the same way as the analog voices would be. However, in synthesizers such as ZynAddSubFx, voices are allocated on an as-needed basis, and deallocated afterwards, so when triggering a new voice, it has no "previous pitch". Unless we purposely want to emulate the behaviour of a voice allocated analog synthesizer, we can explore other portamento models.

Indeed, in a digital synthesizer, it is possible to use the monophonic portamento behaviour as the basis also for polyphonic portamento. This means that when triggering a new note, the pitch of the note should start at the pitch of the previously played note, which may not actually be the target pitch of that note if there was an ongoing portamento at the time the new note was triggered.

For example, assume C1 is played, and then C2. C2 starts its pitch at C1 and glides upwards. Half way, say when the to-be-C2 voice has reached G1 during its glide, C3 is played. The portamento for C3 should then start at G1, i.e. the pitch that the second played note had reached at that time, rather than for instance at the target pitch of C2, which would give the impression of a C2 suddenly being played from nowhere, before gliding up to C3. This mimics the smooth new-notes-start-at-the-current-pitch behaviour of a monophonic portamento implementation, which is both logical and consistent.

Thus we have the following rules for portamento behaviour in ZynAddSubFx: (END OF EDIT)

  1. Whenever a note is played which should have portamento, the portamento should start at the pitch that the previously played note was at when the new note was played. This slightly long-winded explanation is because I think that if the previous note was in the process of gliding due to portamento, the new note should start precisely at that pitch. This would be the same for polyphonic, monophonic and legato modes.
  2. In poly mode, each note should have its own portamento, i.e. each note will glide individually from the initial pitch (see 1. above for how it the initial pitch is determined) to the target pitch (= the note played)).
  3. Whether a note should have portamento or not is initially controlled by the portamento enable button, and is further restricted by the up/down knob, and the trigger setting (either having only portamento when the pitch distance from the previously played note is larger or smaller than the set value, respectively).
  4. Currently, when portamento is enabled in poly mode, all notes have portamento, whereas in mono/legato mode, the player must play legato (hold one note while playing another) in order for the note to have portamento. I would like to have a new parameter for this, similar to they way 'Auto Glide' works on several analog synths. I.e., when Auto Glide is enabled, notes must be played legato to get portamento, when Auto Glide is disabled, all notes will have portamento regardless of the playing style. Thus, in all modes, either all notes have portamento (auto glide disabled), or only when playing legato (auto glide enabled). To put it another way, currently, poly mode always has 'auto glide disabled', and mono/legato always has 'auto glide enabled'. I'm not sure if Auto Glide can be implemented in a backwards compatible way though, as the default value for Auto Glide, for backwards compatibility, needs to be different depending on poly vs. mono or legato modes.
polluxsynth commented 2 years ago

If anyone wants to try a proof-of-concept of individual note glide, with each note starting its glide at the current pitch of the previously played note, including any ongoing portamento, I've now pushed it to my dev branch: https://github.com/polluxsynth/zynaddsubfx/tree/dev . Still needs some work in the details but it should work flawlessly from a functional point of view.

polluxsynth commented 2 years ago

(Fixed a missing initialization (causing random segfaults) and rebased branch to current master: https://github.com/polluxsynth/zynaddsubfx/tree/dev)

polluxsynth commented 2 years ago

Nearing completion, now with portamento tests: https://github.com/polluxsynth/zynaddsubfx/tree/dev .

Have purposely not implemented 'auto glide' (point 4 above), as I want to get the portamento function into a consistent state (and reviewed if possible) before going on with any enhancements.

polluxsynth commented 2 years ago

I updated my spec above (https://github.com/zynaddsubfx/zyn-fusion-issues/issues/304#issuecomment-944883483) with a description on how portamento works on an analog synth, and using that as a basis and rationale for the proposed behaviour in Zyn.

fundamental commented 2 years ago

Thanks for bearing with the delays. Overall progress sounds like it's going well :)

One thing, when I actually do get around to submitting a PR, should all the patches be collapsed into one, or should there be a few major commits, or left as it is now? I've been trying to keep refactorization and implementation separate, but as is often the case, refactorization is not always apparent until the new implementation is underway, and it's often cumbersome to rebase the commits in a logical order.

In my experience the most critical thing is to make sure that the code is in a working state with tests passing at each commit. Once that's done then most of the work is in place. Usually I end up making a bunch of changes, turning things into a set of actual changes as well as fixes up to the functionality changes. In those cases I tend to use git rebase -i to combine the functionality changes along with their subsequent tidying up commits together, so it's just a sequence of functionality enhancements along the way.

When there's major changes that I've planned, sometimes there's a refactoring commit before anything else, but a lot of this is developmental styling. Overall it's good to see energy being put into how commit sequences are structured and seeing energy put into the commit message styling as well :)

I would like to have a new parameter for this

+1

Have purposely not implemented 'auto glide' (point 4 above), as I want to get the portamento function into a consistent state (and reviewed if possible) before going on with any enhancements.

That totally works for me.

I updated my spec above (#304 (comment)) with a description on how portamento works on an analog synth, and using that as a basis and rationale for the proposed behaviour in Zyn.

I like the description and I think that when autoglide is in, this one comment should be preserved in one of the doc/ directories as it provides a pretty clear description of what the intent is.

To confirm, https://github.com/zynaddsubfx/zynaddsubfx/pull/147 will be made obsolete by the new pull request from your dev branch, correct?

polluxsynth commented 2 years ago

I've created a PR now (https://github.com/zynaddsubfx/zynaddsubfx/pull/150). I opted to squash most of the commits into one, because looking at the single squashed commit I think it's fairly clear what is going on, and having all the individual commits makes for a confusing read, as there are things that I moved around between various locations before settling down on a final one.

I agree that the tests should pass for all commits, and indeed they do for all commits in my original dev branch. (There were a few commits where the Portamento object leaks memory though, but they were noted as such.)

I'm also a fan of rebase -i to reorder things, or add fixes to previous commits. Sometimes though, I feel the amount of work needed to fix subsequent merge failures as rebase attempts to merge the remaining stack of commits outweighs the benefit.

Yes, https://github.com/zynaddsubfx/zynaddsubfx/pull/147 is obsoleted by the new PR (or rather, it's included as a separate commit).

The only thing I'm not completely happy with is the way the Threshold feature can cut off a chain of notes exhibiting portamento. E.g. with the default threshold of 3, playing C1, C2, C3, D3, C3 will cause C2 and C3 to exhibit portamento (and C1 too, depending on if it was the first note played, or what the previous note was, according to the threshold rules), then D3 will have no portamento on account of threshold, and neither will C3 for the same reason. Thus we have (at least) two glides upwards to C2 and C3, yet the second C3 hits its pitch directly with no portamento. It's completely logical and follows all the rules, yet gives a slightly arbitrary impression when it comes to which notes exhibit portamento which don't. Especially if one slips on the keys and manages to squeeze in a note by mistake which is close in pitch to another, intended, note. But hey, the threshold feature is their precisely to cause certain intervals exhibit portamento and certain intervals not. And this behaviour is not new, but perhaps more noticeable now when there is no 'grace period' during which no notes exhibit portamento while the first note with portamento reaches its target pitch.

zynmuse commented 2 years ago

I like the way the portamento now works! Thank you for all your efforts with addressing this.!

polluxsynth commented 2 years ago

Thanks @zynmuse! Good to hear that!

There are two remaining issues with portamento which I want to address:

First of all, now that the portamento function works, I want to implement an 'auto' switch. As noted above, when portamento is enabled, in Poly mode, all notes have portamento, whereas in Mono/Legato, only notes played legato have portamento. What I'd like is to have a separate switch for this, so that the dependency on legato playing can be enabled or disabled, for all modes. The drawback is that previous configurations which don't address this switch will not behave in the same way as before (assuming the default setting for 'auto' is off, then Mono/Legato will exhibit portamento regardless of legato playing), but as I understand it @fundamental was ok with this. It shouldn't be hard to implement now.

The other issue is related to the way legato mode works, and is not directly a portamento issue. In this mode, two voices are assigned per note played (well, that should really be two voices are assigned, as only one note can be played in this mode), but only one is heard. Whenever a new note is played, the other voice is quickly crossfaded in and the old one crossfaded out. I think the idea is that when playing a new note in legato mode, the waveform parameters should be computed for the new pitch, so that it sounds the same as if it were played without legato. However, a quick test with a couple of patches shows that this is not the case, at least not consistently. For instance, in legato mode, play a low note, hold it, and then a high one. Then release the notes, and replay the high one on its own. The tone quality will be vastly different between the two instances of the high note, which indicates to me that this functionality has been broken, probably for quite some time. Or else I just don't understand how it is supposed to work.

I haven't tested all three synth engines, so perhaps it works for one or two but not the other(s).

My point is that if everyone is happy with the current behavior, legato mode could be simplified by omitting this dual voice paradigm (there's a lot of code in order to support it). Or, the functionality should be fixed to work properly. However, with portamento enabled, switching waveform parameters at the start of the portamento glide means that there will be a marked tonal change at that point in time, which is likely not very desirable. And most legato playing I would think happens between notes that are fairly close so retaining the waveform parameters would not be much of an issue.

On the other hand, simplification of the code aside if the dual voice paradigm were removed (which would be a Good Thing in itself of course), while slightly wasteful on resources (two voices instead of one), since legato mode is monophonic, the part in question will not be consuming an inordinate amount of resources, compared to a poly voice with long release time for instance. So it would not be a huge improvement in behavior.

polluxsynth commented 2 years ago

@fundamental, while working on implementing the auto portamento mode, I've come across an inconsistency, it seems that some boolean parameters, such as portamento.portamento_receive, are stored as booleans in XML (and consequently created with xml.addparbool()), whereas others, such as portamento.portamento, are stored as continuous parameters (and consequently created with xml.addpar()). I could understand if the choice for continuous parameters was based on them being MIDI CC values, which are always 0 to 127, even if the underlying parameter only has two states (for instance, CC 64 i.e. sustain pedal). But there are parameters such as portamento.proportional which is boolean, and only controllable via OSC, yet is stored as a continous parameters. Is there some deeper thought here or is it more of a legacy reason?

I suppose what I'm really asking is if the to-be-introduced portamento.automode should be a boolean or continuous?

fundamental commented 2 years ago

So, the old booleans and floats as 0..127 values is mostly a historical note at this stage. With the MIDI learn subsystem or the automation macro system, the metadata about a given parameter (as stored in the rtosc::ports objects) are used to map MIDI CC quantized information into the real domain of a parameter. Given that, if you have a parameter which is actually a boolean, use a boolean.

polluxsynth commented 2 years ago

Good. I've created a PR for zynaddsubfx https://github.com/zynaddsubfx/zynaddsubfx/pull/175 and one for zest https://github.com/mruby-zest/mruby-zest-build/pull/71 now.

pgervais commented 1 year ago

@polluxsynth @zynmuse Now that both PRs mentioned in the last message above have been merged, it looks like this issue could be closed. Opinions?

polluxsynth commented 1 year ago

It's fine by me. @zynmuse will have to answer for himself of course but he did say on Jan 3, 2022, that he was happy with how it worked now.