surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.08k stars 394 forks source link

VK maps velocity to 1..100(ish) instead of 1..127 #6409

Closed j5v closed 2 years ago

j5v commented 2 years ago

Bug Description: The highest velocity I can produce from the virtual keyboard (at the bottom of the Surge window) seems to be around 100. It should be 127. Likely a scaling error.

Surge XT Version Version: Surge XT 1.1.nightly.4869069 Build: 2022-07-19 @ 20:20:25 on 'fv-az30-93/pipeline' with 'MSVC-19.29.30146.0' using JUCE 6.1.6 System: Windows 64-bit Standalone on Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz Sample Rate: 44.1 kHz

Reproduction Steps: Steps to reproduce the behavior:

  1. Click at the bottom edge of the VK, and estimate the input velocity from a patch that can expose the velocity. Surge VK

You might want to use a debug output on a local build. I've used (a) estimate +/-5% using loudness and timbre variations from a velocity-sensitive patch. (b) For a good estimate, map velocity to pitch and calculate the input velocity from the resulting pitch, and

Expected Behavior: Velocity range 1..127 should be possible from the VK.

Screenshots: The red horizontal line shows where the max velocity of Surge matches the 1..127 range between the grey lines, as output by a MIDI controller (Sticky Fingers). velocity map (external app)

Computer Information (please complete the following!):

Additional Information: You might want to leave a little margin at the bottom, so a 127 can be produced reliably, rather than risking clicking off the bottom edge of the Surge window (a design feature borrowed from the Sticky Fingers controller surface).

j5v commented 2 years ago

FYI, I've just removed the Surge logo from my app (on a feature flag), as it's not formally part of the Surge family.

baconpaul commented 2 years ago

Lemme look this week - Maybe we are mis scaling somewhere

baconpaul commented 2 years ago

OK here's why

It used to be we didn't use keyboard velocity.

The way it works is: The positional velocity multiplies the overarching keyboard velocity

So we have a conflict. The key binds which move the velocity up and down will also change the max of the pressed velocity.

Moreover we initialize the keyboard velocity to

float midiKeyboardVelocity{96.f / 127.f};

Which is, indeed, about 100.

So I think this (1) never worked and (2) was never consistent.

If I change that line to 127/127 then indeed I get the full range. But then if I use the keyboard gestures to increase or decrease keyboard velocity then the mouse velocity is maxed out.

I think the right thing is set the mkv to 127/127 and then the mouse range will work but the mouse + keyboard conflict. I think this makes the most sense for 1.1.0

but that will still have the inconsistency

I'll merge the 127/127 change but curious @mkruselj on your thoughts too

baconpaul commented 2 years ago

In fact I'm not going to merge the 127/127 until I head from some other folks.

mkruselj commented 2 years ago

It sucks that the two things are linked like that. There should be a separate fixed velocity setting for keyboard and mouse press should just scale the click position between 1 and 127. If this is not possible I would keep it as is and document it.

baconpaul commented 2 years ago

It’s a one line juce patch to remove the multiply why don’t we put this in 11n and we can do the patch and set the default to 127/127 for 110

mkruselj commented 2 years ago

Sure!

baconpaul commented 2 years ago

Just so I don't have to go find it again

juce_MidiKeyboardComponent.cpp around line 145

void MidiKeyboardComponent::updateNoteUnderMouse (Point<float> pos, bool isDown, int fingerNum)
{
    // few lines redacted
    const auto eventVelocity = useMousePositionForVelocity ? noteInfo.velocity * velocity : velocity;

that multiply is what does it. Don't want to do another juce patch this late in cycle but we can patch it out easily for 1.1.1..

baconpaul commented 2 years ago

And PR in to set to 127

@j5v thanks for flagging this!