hydrogen-music / hydrogen

The advanced drum machine for Linux, macOS, and Windows
http://www.hydrogen-music.org
GNU General Public License v2.0
1.03k stars 172 forks source link

Sampler: fix note length calculation #1868

Closed theGreatWhiteShark closed 11 months ago

theGreatWhiteShark commented 11 months ago

there was a bug in the calculation of user defined note length in the Sampler. The delay introduced due to humanization, like lead lag, is given in frames but was mixed with variables given in ticks. This resulted in absurdly large delays and negative note lengths.

Fixes #1852

theGreatWhiteShark commented 11 months ago

While the crash of #1852 does not occur on my machine anymore, there is still something off.

  1. I still get occasional negative nReleasesFrames passed to ADSR::applyADSR because pSelectedLayerInfo->SamplePosition is already ahead of the note length before the ADSR is applied. This doesn't sound right
  2. There is still a conceptional flaw. When changing tempo or passing a tempo marker the length of a note with user-defined length does change. This is because the latter is defined (in the PatternEditor) in ticks and rendering is done based on frames. Since each tempo corresponds to a different tick size (tick - frame conversion), it does also correspond to a different length in frames.

But right now only the length of the note itself is scaled, not the sample position (the point till which the sample was already rendered). This means when we have a note which is 55% rendered and tempo is increased dramatically effectively cutting the note length in half, the next time the Sampler tries to render this note it will false assume it is fully rendered. After all the untouched sample position is beyond the freshly updated end of the note.

I will have a look at these in another PR. Depending on how much and which code needs to be touched I might also put the fix in 1.3. only.

cme commented 11 months ago

Ooft yes, as soon as the note's played it should no longer care about the tempo I guess. Since it's still possible in the meantime, I've added #1870 to make the ADSR tolerant to the possibility by issuing a warning and moving directly to Release phase. Better than trampling past the buffer!

theGreatWhiteShark commented 11 months ago

Ooft yes, as soon as the note's played it should no longer care about the tempo I guess.

This could lead to audible glitches and odd behavior when changing tempo or toggling the timeline. E.g. playing two notes with the same user-defined length in a row and manually increasing tempo by a margin might result in the second note to finish before the first one.

But this is just required for tempo changes done using the BPM widget, OSC/MIDI actions, or adding of Tempo Marker. All changes applied through the (fixed) timeline are known in advanced and are covered within computeFrameFromTick.