Skip to content

piano metronome fix#4344

Merged
walterbender merged 8 commits intosugarlabs:masterfrom
Commanderk3:metro
Feb 21, 2025
Merged

piano metronome fix#4344
walterbender merged 8 commits intosugarlabs:masterfrom
Commanderk3:metro

Conversation

@Commanderk3
Copy link
Member

metro.mp4

@pikurasa I found that while using the metronome, it records the last rest note also. So when you switch it ON again next time, it adds the previous rest duration. In this PR, a rest note is not added when you start playing the piano after switching the metronome ON. Please watch the video.
But imagine a case where a user wants to add rest before starting. The user can turn ON the metronome, he will wait for the right time, and then start playing the piano. Currently, there is no implementation for this. Do you think this would make sense?

@Commanderk3 Commanderk3 marked this pull request as draft February 4, 2025 11:56
@Commanderk3
Copy link
Member Author

Now I am working on the countdown.

@pikurasa
Copy link
Collaborator

pikurasa commented Feb 5, 2025

I think this is an improvement.

However, I'm still experiencing a mysterious issue where rests are inserted between notes. Do you experience that, too, @Commanderk3 ?

You can see the issue in this test:
test3.webm

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 5, 2025

@pikurasa Yes, I have tested it.
I will fix it.

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 5, 2025

But apart from that one thing, I want to ask, why can't notes overlap? Each note value corresponds to only one note. Look at this image :
image

Notice how E6 and D6 have the same starting time but has different end time. We can't do this in music keyboard. Is it because we can't perform such actions in blocks?
Also you cant record playing chords, I will fix that too, maybe in a different PR.

@walterbender
Copy link
Member

You can do the E6/D6 thing if you put the D6 note inside the E6 note.

@Commanderk3
Copy link
Member Author

You can do the E6/D6 thing if you put the D6 note inside the E6 note.

Yup, it's working. Thank you.

@Commanderk3
Copy link
Member Author

The latest commits resolve the issue. The following features were added :->

  1. Added count-down box before music keyboard starts recording.

  2. When the metronome is ON it will highlight its background giving feedback that it is ON.

  3. Enhanced the existing implementation of recording rest duration. Added a threshold value "EPSILON" which can be changed according to need. Now it won't record any unnecessary rest notes.

Have a look at the demo video :->

metrofix.mp4

@walterbender @pikurasa Please review this PR. Any kind of feedback is welcome!

@Commanderk3 Commanderk3 marked this pull request as ready for review February 6, 2025 22:08
@walterbender
Copy link
Member

@pikurasa

@pikurasa
Copy link
Collaborator

I need some more time to test different tempi and scenarios (e.g.stop, then start), but that said: This is very much on the right track!

@Commanderk3
Copy link
Member Author

@pikurasa One thing I wanted to ask you is that, the metronome is using a "loop" function to play the beat (in BPM). But, it starts its first beat after a gap. When the metronome activates we may want to have the first beat at that moment itself.

A similar observation can be seen in the Tempo widget.

@pikurasa
Copy link
Collaborator

@pikurasa One thing I wanted to ask you is that, the metronome is using a "loop" function to play the beat (in BPM). But, it starts its first beat after a gap. When the metronome activates we may want to have the first beat at that moment itself.

A similar observation can be seen in the Tempo widget.

If I understand you correctly, I don't think we need that.

It's ok that the user initiates the metronome or the tempo widget, and that they don't hear anything until the next beat.

@pikurasa
Copy link
Collaborator

I tested some more of the corner cases.

  1. Simultaneously played notes are not being recorded as simultaneous. However, I tested this against the master, and it's not a regression. Rather, it's something we need to fix, which we can do in a separate PR.
  2. The way this branch works now, a user can keep metronome running and press play within the widget to initiate playback. Both the playback and the metronome will continue and the user can continue recording. I didn't expect this. I expected that metronome and recording would stop once playback is initiated. That said, it works well, and I can't quite decide if it's a "feature" or a "bug". For example, a user can use this to mockup a canon pretty well.

So, I think we could merge this soonish, after a code review by @walterbender , if we want to. It's certainly a big improvement.

@Commanderk3
Copy link
Member Author

@pikurasa I am aware about simultaneous notes not being recorded. I will fix it in a separate PR.

Regarding the playback and metronome played together, I actually never intended it to happen.

@walterbender walterbender merged commit 8f44f67 into sugarlabs:master Feb 21, 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