Make some improvements to the pitch tracker widget #274
Conversation
celeste-sinead
commented
May 6, 2024
celeste-sinead
commented
- adds a display of the current pitch and note to the right of the graph, which is nice for controlling pitch in real time. At this point Feature request: display note name (like how a tuner does) #234 seems quite fully satisfied.
- shows minor grid lines in the frequency axis, instead of time, since that's usually what one wants more precision for in this graph
- replaces minimum SNR with minimum amplitude, which is kinda crude and needs tweaking to each environment, but once tweaked generally does a good job of avoiding detecting pitches in background noise (including pitched background sounds).
Because the user is probably more interested in small differences in pitch than small differences in time.
Using SNR (as formulated) didn't work great because, surprise, it turns out that the dumb approximation of mean level for noise level is correlated with signal power. Amplitude is simple and works as desired in practice, although it probably requires tuning to any given environment for best results -.-
There was a problem hiding this comment.
Thanks @celeste-sinead, that looks really nice and works well.
I confirm that the dB/SNR mechanism to find out if the pitch is relevant is not perfect in my tests either.
I've added 2 comments regarding the design of the view/viewModel. Let me know if you'd like to address that in this MR or separately.
| @@ -102,17 +107,37 @@ def __init__(self, parent, engine): | |||
|
|
|||
| self.gridLayout.addWidget(self.quickWidget, 0, 0, 1, 1) | |||
|
|
|||
| self.pitch_view_model = PitchViewModel(self) | |||
There was a problem hiding this comment.
In practice, there is already a viewModel here, it's Scope_Data above. Could Scope_Data be used as a base for a new class that adds the pitch properties, like Spectrum_Data extends Scope_Data ?
| @@ -102,17 +107,37 @@ def __init__(self, parent, engine): | |||
|
|
|||
| self.gridLayout.addWidget(self.quickWidget, 0, 0, 1, 1) | |||
|
|
|||
| self.pitch_view_model = PitchViewModel(self) | |||
| pitch_window = QQuickWindow() | |||
| pitch_component = QQmlComponent(engine, qml_url("PitchView.qml"), self) | |||
There was a problem hiding this comment.
Instead of having 2 views next to each other in the layout, could we have a new single view (that replaces Scope.qml) ?
My idea behind that is that at some point in the future, I would like to migrate the whole layout of Friture from QWidget to QML. So if each audio widget is a single QML component, that makes it easier.
There was a problem hiding this comment.
Yeah I had the same thought as I was putting together these pull requests. Pretty sure it would have saved a lot of hair pulling trying to get the widgets to size correctly if I'd thought of it to begin with 🙃. I think the other comment on the model probably dovetails nicely.
I think I'd rather do that all as a follow-up, rather than amending this PR.
There was a problem hiding this comment.
Alright, let's do it in a follow-up PR then. I'm merging as is.
|
Thanks again @celeste-sinead ! |
