Notes with Chords: Support multiple chords & notes, and volume control#323
Notes with Chords: Support multiple chords & notes, and volume control#323ShacharHarshuv wants to merge 1 commit intoShacharHarshuv:masterfrom
Conversation
…ematic mapping errors where chord 4 entries were mapped to wrong chords (e.g., Mi4 was mapped to viidim instead of IV) - All chord entries now correctly follow the pattern: 1->I, 2->ii, 3->iii, 4->IV, 5->V, 6->vi, 7->viidim - This fixes the issue where IV chord was generating incorrect notes like B1 B2 F3 B3 D4 instead of F major chord
|
Someone is attempting to deploy a commit to the shacharharshuv's projects Team on Vercel. A member of the Team first needs to authorize it. |
ShacharHarshuv
left a comment
ShacharHarshuv
left a comment
There was a problem hiding this comment.
Thanks so much for your hard work contributing for OpenEar! I added a couple of suggestions to simplify code and improve the algorithm. I recommend reading all comments before you start addressing them.
| Do2: { | ||
| chord: viidim, | ||
| chord: ii, | ||
| solfegeNote: 'Do', | ||
| }, | ||
| Do3: { | ||
| chord: vi, | ||
| chord: iii, | ||
| solfegeNote: 'Do', | ||
| }, | ||
| Do4: { | ||
| chord: V, | ||
| chord: IV, | ||
| solfegeNote: 'Do', | ||
| }, | ||
| Do5: { | ||
| chord: IV, | ||
| chord: V, | ||
| solfegeNote: 'Do', | ||
| }, | ||
| Do6: { | ||
| chord: iii, | ||
| chord: vi, | ||
| solfegeNote: 'Do', | ||
| }, | ||
| Do7: { | ||
| chord: ii, | ||
| chord: viidim, | ||
| solfegeNote: 'Do', |
There was a problem hiding this comment.
We can't merge that change (and subsequent changes in this object) because it completely changes the meaning of the exercise for existing users.
Instead of changing this array, we should map the labels themselves. Please revert these changes and thinkin of how to change the labeling in a future pull request.
| type ChordAndNotesSettings = { | ||
| numberOfChordChanges: number; | ||
| numberOfNotesPerChord: number; | ||
| chordVolume: number; | ||
| melodyVolume: number; | ||
| }; |
There was a problem hiding this comment.
This duplicates NoteWithChordsSettings
Instead of adding a new type alias, you can add these properties there (after line 350 currently)
| }; | ||
|
|
||
| return { | ||
| settingsDescriptors: [chordChangesDescriptor, notesPerChordDescriptor, chordVolumeDescriptor, melodyVolumeDescriptor], |
There was a problem hiding this comment.
I don't think you actually need all of the consts. It would be simpler and easier to read if you just dump those in the array.
| }; | ||
| }; | ||
|
|
||
| const chordAndNotesSettings = useChordAndNotesSettings(); |
There was a problem hiding this comment.
I feel like this function and const are redundant. Since we are not going to reuse them. I always prefer to keep things simple if possible (i.e. avoid reusable symbols if not necessary)
| console.log('🎵 Available chord numbers:', Array.from(availableChordNumbers)); | ||
| console.log('🎵 Current Exercise Settings:'); | ||
| console.log(` 🎼 Key: ${settings.key || 'Not set'}`); | ||
| console.log(` 🎼 Voice Mode: ${settings.voiceMode}`); | ||
| console.log(` 🎼 Harmony Mode: ${settings.harmonyMode}`); | ||
| console.log(' ──────────────────────────────'); |
There was a problem hiding this comment.
Please remove those logs and all other logs you added. I want to keep master free of logs so that you can add only the logs you need for debugging while debugging.
|
|
||
| const chordChangesDescriptor: SettingsControlDescriptor<ChordAndNotesSettings> = { | ||
| key: 'numberOfChordChanges', | ||
| info: 'How many different chords to practice in one session (max 4 for 4/4 feel)', |
There was a problem hiding this comment.
| info: 'How many different chords to practice in one session (max 4 for 4/4 feel)', | |
| info: 'Number of chord changes in one question', |
- The term for this in open ear is "question". Session refers to consecutive sitting on an exercise that can include more than one question.
- I don't think there is a strong enough indication of meter here.
|
|
||
| const notesPerChordDescriptor: SettingsControlDescriptor<ChordAndNotesSettings> = { | ||
| key: 'numberOfNotesPerChord', | ||
| info: 'How many notes to practice over each chord (max 4 for 4/4 feel)', |
There was a problem hiding this comment.
| info: 'How many notes to practice over each chord (max 4 for 4/4 feel)', | |
| info: 'How many notes to practice over each chord', |
|
|
||
| const chordVolumeDescriptor: SettingsControlDescriptor<ChordAndNotesSettings> = { | ||
| key: 'chordVolume', | ||
| info: 'Volume of the chord accompaniment (0.1 = very quiet, 1.0 = full volume)', |
There was a problem hiding this comment.
| info: 'Volume of the chord accompaniment (0.1 = very quiet, 1.0 = full volume)', | |
| info: 'Volume of the chord', |
|
|
||
| const melodyVolumeDescriptor: SettingsControlDescriptor<ChordAndNotesSettings> = { | ||
| key: 'melodyVolume', | ||
| info: 'Volume of the melody note to identify (0.1 = very quiet, 1.0 = full volume)', |
There was a problem hiding this comment.
| info: 'Volume of the melody note to identify (0.1 = very quiet, 1.0 = full volume)', | |
| info: 'Volume of the melody note', |
| // Generate chord progression by selecting from available chord numbers | ||
| const selectedChordNumbers: number[] = []; | ||
| for (let chordIndex = 0; chordIndex < settings.numberOfChordChanges; chordIndex++) { | ||
| const randomChordNumber = randomFromList(Array.from(availableChordNumbers)); |
There was a problem hiding this comment.
I'm worried about the way this algorithm changes for the single-chord and single-note use case. Instead of every answer getting an equal chance, every word has an equal chance.
Example: the user selected Do1, Do3, Do5, and Fa3. (In the original notation).
In the previous logic you have these probabilities:
- Do1: 25%
- Do3: 25%
- Do5: 25%
- Fa3: 25%
In your new logic you have these probabilities:
- Do1: 16.7%
- Do3: 16.7%
- Do5: 16.7%,
- Fa3: 50%
this could create a strong sense of unbalanced bias towards Fa3.
Here's how you could fix it:
function pushAnswer(answer: NoteWithChord) {
// ... (the current code, only instead of returning it pushes a new segment)
}
let previousChord: Record<HarmonyMode, RomanNumeralChordSymbol> | null = null;
for (let i = 0; i < settings.numberOfChords; i++) {
const chosenAnswer = randomFromList(
previousChord
? settings.includedAnswers.filter(
(answer) =>
noteWithChordDescriptorMap[answer].chord !== previousChord,
)
: settings.includedAnswers,
);
pushAnswer(chosenAnswer);
const chosenChord = noteWithChordDescriptorMap[chosenAnswer].chord;
for (let j = 0; j < settings.numberOfNotes - 1; j++) {
const chosenAnswer = randomFromList(
settings.includedAnswers.filter(
(answer) =>
noteWithChordDescriptorMap[answer].chord === chosenChord,
),
)
pushAnswer(chosenAnswer);
}
previousChord = chosenChord;
}
ShacharHarshuv
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1 participant
No description provided.