DamRsn / NeuralNote

Audio Plugin for Audio to MIDI transcription using deep learning.
Apache License 2.0
1.3k stars 67 forks source link

Add support for more scales. #62

Closed trirpi closed 1 year ago

trirpi commented 1 year ago

Problem

This PR addresses this issue: https://github.com/DamRsn/NeuralNote/issues/46. Not enough scales are available.

Solution

Added the Dorian scale . If I get an approval that this is the correct way of doing it, I will also add the following:

DamRsn commented 1 year ago

Thanks for contributing!

This seems all good to me, it's definitely the way of doing it.

I haven't tested it yet though. Let's add all the options before merging.

trirpi commented 1 year ago

Added the other scales and seems to work fine on my machine.

Only problem is that most of the code assumes that there's 7 notes in a scale. I just padded the pentatonic and blues scales with 0's but I could also try to make the code work for different scale sizes if that's desired instead.

RustoMCSpit commented 1 year ago

shpuld also allow for custom user scales

trirpi commented 1 year ago

shpuld also allow for custom user scales

Seems outside of the scope of this issue as this would require significant updates to the UI as well. You can create a new issue with this request.

RustoMCSpit commented 1 year ago

does this feature highlight the scales on the piano roll with a different shade or lock on to them or what

trirpi commented 1 year ago

Can this be merged @DamRsn or would are there some changes necessary?

RustoMCSpit commented 1 year ago

does this feature highlight the scales on the piano roll with a different shade or lock on to them or what

@trirpi

RustoMCSpit commented 1 year ago

id like to clarify that scales ≠ modes,

Problem

This PR addresses this issue: #46. Not enough scales are available.

Solution

Added the Dorian scale . If I get an approval that this is the correct way of doing it, I will also add the following:

  • Mixolydian
  • Lydian
  • Phrygian
  • Locrian
  • Minor blues
  • Minor pentatonic
  • Major pentatonic
  • Melodic minor
  • Harmonic minor
  • Harmonic Major

theres no mention of the aeolian (natural minor) here? also can the modes get a different shading to the scales and can they be placed in order? also, there is an ionian mode right??? when we add ionian we should have in brackets (major) and aeolian (minor).

RustoMCSpit commented 1 year ago

i assume theres either a chromatic scale or a way to turn scales off

RustoMCSpit commented 1 year ago

melodic minor modes: https://m.youtube.com/watch?v=E_mto_Dkpo0&pp=ygUTbWVsb2RpYyBtaW5vciBtb2Rlcw%3D%3D

my fear is that this implementation is treating modes and scales as the same thing rather than selecting a scale and then selecting the mode of that scale.

trirpi commented 1 year ago

The scales added here are identical to the scales provided by the Ableton scale feature (minus some unusual Asian and Messiaen scales). The ordering of the scales is also identical to the ordering provided in Ableton.

RustoMCSpit commented 1 year ago

The scales added here are identical to the scales provided by the Ableton scale feature (minus some unusual Asian and Messiaen scales). The ordering of the scales is also identical to the ordering provided in Ableton.

the asian abd messiaen scales (and beyond that) could be put under an 'advanced' / 'obscure' setting.

RustoMCSpit commented 1 year ago

The scales added here are identical to the scales provided by the Ableton scale feature (minus some unusual Asian and Messiaen scales). The ordering of the scales is also identical to the ordering provided in Ableton.

if ableton fails to treat modes differently from scales we shouldnt replicate that, a mode is just a scale at a different starting point.

DamRsn commented 1 year ago

Can this be merged @DamRsn or would are there some changes necessary?

Sorry for the delay, was busy these last few weeks.

Before merging, I'd like to find a better solution than padding 0s for scales with less than 7 notes. The code should just deal with the size of the array, and the hardcoded 7 should be removed.

Could you try to have a look at it?

trirpi commented 1 year ago

Can this be merged @DamRsn or would are there some changes necessary?

Sorry for the delay, was busy these last few weeks.

Before merging, I'd like to find a better solution than padding 0s for scales with less than 7 notes. The code should just deal with the size of the array, and the hardcoded 7 should be removed.

Could you try to have a look at it?

Sure thing, I've changed it. Could you have a look now?

I first wanted to convert the interval arrays (such as MAJOR_SCALE_INTERVALS) to vectors. But vectors can't be constexpr in c++17. So instead I kept them arrays. However, I had to change the function that takes in these arrays (_createKeyVectorForScale) to allow for different array sizes. I did this by templating that function.

DamRsn commented 1 year ago

Looking good, thanks a lot.

I'm modifying the design slightly so that the longer key names can be display properly in the box. Then I'll merge it!

DamRsn commented 1 year ago

Turns out it's easier to merge it first and then I'll add the design modifications!

Thanks a lot for this contribution!