Skip to content

Fix: Music Keyboard to Action Conversion Sync #4262

Closed
MostlyKIGuess wants to merge 4 commits intosugarlabs:masterfrom
MostlyKIGuess:keyboardfix
Closed

Fix: Music Keyboard to Action Conversion Sync #4262
MostlyKIGuess wants to merge 4 commits intosugarlabs:masterfrom
MostlyKIGuess:keyboardfix

Conversation

@MostlyKIGuess
Copy link
Member

Summary

This PR addresses the issue where notes in the action block are 2.6666666665 times longer than those played in the MusicKeyboard. It applies a scale factor to each note's duration before generating the fraction, ensuring the durations match their original lengths.

Changes

  • Divides each note's duration by 2.6666666665 before creating the fraction in the action block.

Code Snippet

const scaleFactor = 2.6666666665;
const maxWidth = Math.max.apply(Math, note.duration) / scaleFactor;
const obj = toFraction(maxWidth);

Testing

  • Verified that the exported action blocks now match the playback lengths in the MusicKeyboard.

Video:

  • Uploaded in the matrix channel.

@Commanderk3
Copy link
Member

@MostlyKIGuess It works. But there is one more thing I found. There is a delay (1125 ms) after each note played which is set specifically. Our goal is to make the widget sound the same as the blocks so that users can get a proper idea of how it will fill like when they export it to the action block.
If you make the time to be 1000 ms then we will get our desired result.

time

@Commanderk3
Copy link
Member

If we don't edit the time, for shorter notes like 1/16 it will play very fast and a significant difference can be seen.

@Commanderk3
Copy link
Member

@pikurasa What do you think? Changing the timeout value to 1000 should be fine I guess.

fixifix.mp4

@MostlyKIGuess
Copy link
Member Author

@MostlyKIGuess It works. But there is one more thing I found. There is a delay (1125 ms) after each note played which is set specifically. Our goal is to make the widget sound the same as the blocks so that users can get a proper idea of how it will fill like when they export it to the action block. If you make the time to be 1000 ms then we will get our desired result.

time

Absolutely, I will change it.

@Commanderk3
Copy link
Member

nice work bro 👊

@MostlyKIGuess MostlyKIGuess changed the title Keyboardfix Fix: Music Keyboard to Action Conversion Sync Jan 12, 2025
@MostlyKIGuess
Copy link
Member Author

@pikurasa Do review, this is done as discussed in the matrix channel.

@walterbender
Copy link
Member

Waiting on @pikurasa 's review

@pikurasa
Copy link
Collaborator

I tested it. (Please forgive me; testing these sorts of media things is a bit time consuming.)

  1. The change wrt conversion from widget to action blocks is going in the wrong direction. This PR exports the actions with a different value (e.g. 1/4 becomes 3/32)). The master branch is doing the right thing here in that it is maintaining the same value upon export. (See image below for screenshot from PR.)
  2. For the widget playback (no export) fter doing a few tests with a metronome, I think that both this PR and the master have the same issue: Playback within the widget seems to be at about 120BPM, despite the metronome being set to 90BPM. If this is the case, this may explain the issue. 120/90 is 1.33333, so the playback within the widget would be one-and-a-third times faster than both the widget playback and the action block playback (assuming 90BPM, which is the default). This should be investigated and fixed.
  3. Most problematically, it seems that an extra rest is inserted in-between notes, and this seems to happen somewhat consistently for both widget playback and action block export. And this seems to happen on both PR and master. This needs to be investigated and fixed.
  4. I can't believe I just noticed this now, but the bottom row of the widget should not be labeled "duration (MS)". It should be labeled "note value".

Screenshot from PR. Notice that the values within the widget and action differ. This should not be.
Screenshot from 2025-01-18 19-48-22

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.

4 participants