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

music keyboard fix #4296

Merged
merged 6 commits into from
Jan 25, 2025
Merged

music keyboard fix #4296

merged 6 commits into from
Jan 25, 2025

Conversation

Commanderk3
Copy link
Contributor

Earlier the music keyboard was not following BPM. Now the widget is following BPM.

@Commanderk3 Commanderk3 changed the title follow BPM music keyboard fix Jan 20, 2025
@Commanderk3 Commanderk3 marked this pull request as ready for review January 22, 2025 17:08
@Commanderk3
Copy link
Contributor Author

Commanderk3 commented Jan 22, 2025

@walterbender @pikurasa This should solve the problem. Following changes I did :->

  1. When notes were played in the keyboard widget, they did not account for the BPM. The PR fixes this issue. One thing that I found: this.bpm = BPM * beatValue * 4. ChatGPT gave me this relation: duration of Notes (s) = (60 * beatValue * noteValue) / BPM. Now I don't know why an extra 4 is multiplied but it was a problem. Therefore, I multiplied an extra 4 here :->
const durationInSeconds = selectedNotes[counter].duration.map(
                        (beatDuration) => (beatDuration * 60 * 4) / this.bpm
);

And this solves the issue.

  1. The extra 125ms is removed because it was causing an unnecessary delay.
  2. "duration (MS)" -> "Note Value".

Here is a demo of musickeyboard.
[Note: With 208 BPM in action block seemed buggy, but it is actually because of my screen recorder]

bpm.mp4

@walterbender
Copy link
Member

When you define BPM, you need to assign a note value to a beat. By default, we assign a quarter note.

@Commanderk3
Copy link
Contributor Author

When you define BPM, you need to assign a note value to a beat. By default, we assign a quarter note.

Pardo me sir, I am assuming you are talking about defining BPM in music blocks project. If so, then am I doing right here ?

image

Duration of notes is same when note values are different or same in action block and music keyboaard. Is it an issue?

@walterbender
Copy link
Member

I think you are doing it correctly. But try experimenting with, say, half notes. 90, 1/2 would be 90 half notes per minute -- twice as fast as 90 1/4 notes.

@Commanderk3
Copy link
Contributor Author

@walterbender Yes sir it works. On changing the beat value to 1/2 it is faster.

noteVal.mp4

@pikurasa
Copy link
Collaborator

I did some testing, and it is an improvement.

Please let me do some more testing. In some of my testing, I'm not getting the expected results. I suspect that it has to do with changing BPM specifically, but I'm not sure yet.

As an aside: We need to improve the UI/UX of this widget (which are probably outside the scope of fixing this issue). For example, it doesn't seem to ever reset its recording when metronome is turned off. This results in long rests, which is making it a bit more time consuming as I test it. Also, when a user changes the value for Note Value for Music Keyboard, it automatically changes the value for Beat Value to be the same number. The reverse, however, is not true.

@Commanderk3 Meanwhile, while I'm testing it, please implement the following: The same BPM specified for Keyboard during recording should be exported within the generated Action Blocks (as its first contained block, above the notes). This will help guarantee that the user gets a similar playback compared to what they played (unless they change the values themselves). (Anticipating a follow-up question:) I don't think we need to export Meter as it doesn't effect the sound.

@Commanderk3
Copy link
Contributor Author

@pikurasa Of course sir, I will start working on it. I also think the same about the Meter block. To be honest I am not sure how it works. It does not show any effect.

@pikurasa
Copy link
Collaborator

the Meter block. To be honest I am not sure how it works. It does not show any effect.

The meter block has implications when you combine it with other features, such as "on every beat do" or "on strong beat do", or when you're counting beats or measures.

@Commanderk3
Copy link
Contributor Author

the Meter block. To be honest I am not sure how it works. It does not show any effect.

The meter block has implications when you combine it with other features, such as "on every beat do" or "on strong beat do", or when you're counting beats or measures.

Understood. Basically it is useful when no note value is defined to a beat.

@walterbender walterbender merged commit 46ccbb5 into sugarlabs:master Jan 25, 2025
4 checks passed
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