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

feat: implement tuner in the sampler widget #4549

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

haroon10725
Copy link
Contributor

@haroon10725 haroon10725 commented Mar 17, 2025

@walterbender @pikurasa Before moving on, can you please review the code?
I have implemented the frontend yet.

The colors are constant at the moment, they will be changing w.r.t pitch.

@haroon10725 haroon10725 marked this pull request as draft March 17, 2025 11:52
Copy link

✅ All Jest tests passed! This PR is ready to merge.

@omsuneri
Copy link
Member

@haroon10725 please ignore the lint workflow failure i m working on that you need not to be worry about that

Copy link

✅ All Jest tests passed! This PR is ready to merge.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

@haroon10725
Copy link
Contributor Author

image

@Commanderk3
Copy link
Member

@haroon10725
I have set up pitch detection and it is working fine. Perhaps we can be together on this one? With this frontend the tuner will be completed.

@Commanderk3
Copy link
Member

My setup shows the offset in cents and we can show that in this meter that you made.

@haroon10725
Copy link
Contributor Author

@Commanderk3 Yes sure, we can discuss the frontend and pitch detection approach in tomorrow's meet.

@pikurasa
Copy link
Collaborator

image

That looks pretty decent.

I'm not sure that we really need the extra step of having the user click "start tuning" before the feature starts running.

@haroon10725
Copy link
Contributor Author

That looks pretty decent.

Thanks

I'm not sure that we really need the extra step of having the user click "start tuning" before the feature starts running.

Yes, we can remove that button, allowing users to start by just clicking the tuner icon in the toolbar.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

@haroon10725
Copy link
Contributor Author

haroon10725 commented Mar 19, 2025

@walterbender @pikurasa Now the user can just start and stop the tuner by just clicking the button in the toolbar.
tuner.webm

@haroon10725
Copy link
Contributor Author

haroon10725 commented Mar 19, 2025

Now I am starting to implement the backend of it. @Commanderk3 you showed some demo for it. Can you please share the code here. May be we can take a look at it and implement it using Tone.js.

I think this was the code, let me go through it https://github.com/cwilso/PitchDetect/blob/main/js/pitchdetect.js

@Commanderk3
Copy link
Member

@haroon10725 Yes something like that. YIN uses correlation function and after that a series of other functions to refine the frequency.
I will make a branch and will share it here. Is it okay?

@Commanderk3
Copy link
Member

https://github.com/Commanderk3/musicblocks/tree/tuner

@Commanderk3
Copy link
Member

Commanderk3 commented Mar 19, 2025

I made a draft PR @haroon10725
You may test the branch and let me know your thoughts.

@haroon10725
Copy link
Contributor Author

I made a draft PR @haroon10725

I think there is no need for it, I got your code. We can continue discussing here only. Don't create separate PRs for the same thing.

@Commanderk3
Copy link
Member

Okay, I will close it then.

@haroon10725
Copy link
Contributor Author

haroon10725 commented Mar 19, 2025

Okay, I will close it then.

Yes it would be easy for the maintainers, I am going through the code which you shared. You can then implement the backend code you shared.

We can use Tone.js to open mic and close. Some of the analysis can also be done through it. Let me see.

@Commanderk3 What do you think about it?

@Commanderk3
Copy link
Member

Commanderk3 commented Mar 19, 2025

I am confused about how you wanna use Tone here. Did you find anything?
@haroon10725 Did you test my branch?

@haroon10725
Copy link
Contributor Author

haroon10725 commented Mar 20, 2025

In Tone we have
await Tone.start();
mic = new Tone.UserMedia();
await mic.open();
which opens the mic, futher more we have an analyser function also
analyser = new Tone.Analyser("fft", 2048);
mic.connect(analyser);

Then we can use the YIN algorithm on it.

We can remove the Web Audio API, and use Tone.js. Tone.js also uses Web Audio API under the hood, by doing this our code will be consistent
@walterbender @pikurasa @Commanderk3 What do you think about it.

@farhan-momin
Copy link
Contributor

farhan-momin commented Mar 20, 2025

Now I am starting to implement the backend of it. @Commanderk3 you showed some demo for it. Can you please share the code here. May be we can take a look at it and implement it using Tone.js.

I think this was the code, let me go through it https://github.com/cwilso/PitchDetect/blob/main/js/pitchdetect.js

@haroon10725 Yeah i referred this repo to implement the feature which i demonstrated in meet. Here is the code for the feature 638b82a

Its basically uses an auto-correlate function for for pitch detection.

For the tuner feature we can use any method (YIN or auto-correlate) that works well.

Im currently trying to use tone.js for listening to audio. will update 638b82a when its completed for your reference

@Commanderk3
Copy link
Member

Alright, I get it now @haroon10725
Thanks for explaining

@farhan-momin
Copy link
Contributor

Ive made changes to use tone.js for Tuner in https://github.com/farhan-momin/musicblocks/tree/tuner.

I was getting incorrect outputs initially. The argument "fft" in analyser = new Tone.Analyser("fft", 2048); was the reason for it.
The pitch detection algorithm i used is based on Time domain. so i used "waveform" instead.

@walterbender
Copy link
Member

What is the status of this PR?

@haroon10725
Copy link
Contributor Author

What is the status of this PR?

@walterbender Frontend of the tuner is completed, implementing backend and then changing colors w.r.t to pitch has to be implemented.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

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.

6 participants