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

tonal: 2 small fixes #1092

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

tonal: 2 small fixes #1092

wants to merge 6 commits into from

Conversation

kasparsj
Copy link
Contributor

@kasparsj kasparsj commented May 7, 2024

  • fix scale() to allow both n() and note() at the same time
  • fix scale() to allow scale names without tonic and then default it to C

kasparsj added 2 commits May 7, 2024 23:56
- fix scale() to allow scale names without tonic and then default it to C
@kasparsj kasparsj marked this pull request as draft May 7, 2024 22:13
@kasparsj kasparsj marked this pull request as ready for review May 7, 2024 22:48
@kasparsj
Copy link
Contributor Author

kasparsj commented May 8, 2024

Also I'm wondering should these 2 fail?

  it('scale is passed down to firstOf', () => {
    expect(
        n(0, 2)
            .scale('C:minor')
            .firstOf(3, x => x.note(2, 0))
            .firstCycleValues.map((h) => h.note),
    ).toEqual(['Eb3', 'C3']);
  });
// AssertionError: expected [ 2, +0 ] to deeply equal [ 'Eb3', 'C3' ]

  it('scale works with add inside firstOf', () => {
    expect(
        n(0, 2)
            .scale('C:minor')
            .firstOf(3, x => x.add(2))
            .firstCycleValues.map((h) => h.note),
    ).toEqual(['Eb3', 'G3']);
  });
// AssertionError: expected [ 'C3', 'Eb3' ] to deeply equal [ 'Eb3', 'G3' ]

@yaxu yaxu requested a review from felixroos May 12, 2024 16:43
@yaxu
Copy link
Member

yaxu commented May 12, 2024

Looks good!
I think that's expected behaviour for those tests to fail. The scale function changes the notes, so doesn't act as separate metadata. We could discuss alternatives though.

@felixroos
Copy link
Collaborator

felixroos commented May 13, 2024

fix scale() to allow scale names without tonic and then default it to C

agreed!

fix scale() to allow both n() and note() at the same time

I think note should not be used as an index type (like n).
imo it creates a disjoint between note as a standalone value (roughly between 20 and 100) and as an index (mostly between 0 and 10). Also it gets much weirder if you start to think about what happens if note is a string.. note("f").scale("C:major") // = G10

iirc there was a discussion in discord about how to handle note in scale, and we came up with the idea to let scale quantise any note to fit the scale, e.g. `note("c eb g").scale("C:major") // = note("c e g")

@kasparsj
Copy link
Contributor Author

fix scale() to allow scale names without tonic and then default it to C

agreed!

fix scale() to allow both n() and note() at the same time

I think note should not be used as an index type (like n). imo it creates a disjoint between note as a standalone value (roughly between 20 and 100) and as an index (mostly between 0 and 10). Also it gets much weirder if you start to think about what happens if note is a string.. note("f").scale("C:major") // = G10

iirc there was a discussion in discord about how to handle note in scale, and we came up with the idea to let scale quantise any note to fit the scale, e.g. `note("c eb g").scale("C:major") // = note("c e g")

does that imply that note in strudel is more like midinote in tidal? is there another function that behaves like tidal's note in the sense of a numeric (zero based) scale degre? (probably defaulting to chromatic scale and c3 offset)
... that's how I've thought of tidal's note till now ...

@felixroos
Copy link
Collaborator

fix scale() to allow scale names without tonic and then default it to C

agreed!

fix scale() to allow both n() and note() at the same time

I think note should not be used as an index type (like n). imo it creates a disjoint between note as a standalone value (roughly between 20 and 100) and as an index (mostly between 0 and 10). Also it gets much weirder if you start to think about what happens if note is a string.. note("f").scale("C:major") // = G10
iirc there was a discussion in discord about how to handle note in scale, and we came up with the idea to let scale quantise any note to fit the scale, e.g. `note("c eb g").scale("C:major") // = note("c e g")

does that imply that note in strudel is more like midinote in tidal?

I'd say yes, but i haven't done much with tidal's midinote. Generally, note defaults to c3 / 36.

is there another function that behaves like tidal's note in the sense of a numeric (zero based) scale degre? (probably defaulting to chromatic scale and c3 offset) ... that's how I've thought of tidal's note till now ...

I don't think so, you'd have to take 36 as your default instead of 0. What do you think are the advantages of a 0 based version?
Btw, if you're working with samples, you can still use n if your samples are arranged chromatically.

@kasparsj
Copy link
Contributor Author

kasparsj commented May 21, 2024

I guess this is where the idea comes from:
https://doc.sccode.org/Tutorials/A-Practical-Guide/PG_07_Value_Conversions.html

tidal skips the "degree" part which is good, because it makes the formula much simpler:
midinote = root + octave*12 + scale[note]

The pros:

  • the separation of "octave" and "degree" into 2 pattern-able entities
  • idea that scales are made up of degrees rather then notes
  • not having to think in terms of midi numbers or note names (non-musician/mathematical thinking...)

when using synths or 1 sample sounds, sometimes, I think of n as a shortcut for note, but that might be wrong (perhaps that's the problem with my unit tests - should have used note instead?). if there are 2 or more samples, I think it always should mean just the sample file index.

@felixroos
Copy link
Collaborator

felixroos commented May 23, 2024

I guess this is where the idea comes from: https://doc.sccode.org/Tutorials/A-Practical-Guide/PG_07_Value_Conversions.html

tidal skips the "degree" part which is good, because it makes the formula much simpler: midinote = root + octave*12 + scale[note]

The pros:

  • the separation of "octave" and "degree" into 2 pattern-able entities
  • idea that scales are made up of degrees rather then notes
  • not having to think in terms of midi numbers or note names (non-musician/mathematical thinking...)

when using synths or 1 sample sounds, sometimes, I think of n as a shortcut for note, but that might be wrong (perhaps that's the problem with my unit tests - should have used note instead?). if there are 2 or more samples, I think it always should mean just the sample file index.

I guess the way to emulate that would be to do:

n("0 1 2 3 4").scale("chromatic")

I still find it a bit weird that n can both be a scale degree and a sample index. It could be more logical if there was something like degree that does the same as n but only for scale...

@felixroos
Copy link
Collaborator

@kasparsj should we then only add the defaulting to C in this PR? I'd prefer if note could be used for scale quantization later..

@kasparsj
Copy link
Contributor Author

kasparsj commented Jun 2, 2024

Hi Felix!
But does it break anything?
I still think it's needed to use together: n for selecting a sample and note + scale for selecting a note.
I think we sort of sidestepped a bit and discussed other aspects that were kicked off by this comment, but the actual code in this PR for the 2 fixes is still solid?

@felixroos
Copy link
Collaborator

Hi Felix! But does it break anything? I still think it's needed to use n for selecting a sample and note + scale for selecting a note. I think we sort of sidestepped a bit and discussed other aspects that were kicked off by this comment, but the actual code in this PR for the 2 fixes is still solid?

I still think note should not be used as an index/offset type (see comment) + adding this behaviour would block the possibility to let scale quantize notes to the scale later, which I think is more useful.

The problem you've tried to solve, this one:

n(0, 1, 2) // <- assuming this is the sample number
.note(3, 4, 0) // <- assuming this is the scale index
.scale('C major')

.. can be solved already like this:

n(3, 4, 0)
.scale('C major')
.set.out(n(0, 1, 2)) // <-- set.out takes structure from here

additionally, scale will "swallow" any n, just returning the note, so n will be undefined after scale:

n(3, 4, 0)
.scale('C major')
.s("sawtooth") // <- n in first line won't interfere

@felixroos
Copy link
Collaborator

felixroos commented Jun 2, 2024

I still find it a bit weird that n can both be a scale degree and a sample index. It could be more logical if there was something like degree that does the same as n but only for scale...

I still think it would be useful if there was another control that is specifically for setting a scale step, something like:

n(0, 1, 2) // <- assuming this is the sample number
.degree(3, 4, 0) // <- assuming this is the scale index
.scale('C major')

I just think it should not be note.. I also think the above code is a rare edge case, as it is not very common to let the sample index provide the structure when you already have a pattern for notes.
edit: the more common thing to write would be:

n(3, 4, 0) // <- assuming this is the scale index
.scale('C major')
.n(0, 1, 2) // <- assuming this is the sample number

@felixroos
Copy link
Collaborator

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.

3 participants