-
Notifications
You must be signed in to change notification settings - Fork 345
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
MythMusic: Improve Spectrum and Squares #771
Conversation
By borrowing lessons learned from Spectrogram: process the whole signal, use the same 16k FFT and MelScale, upgrade to 128 display bars, separate left and right channels. On the menu, Spectrum is renamed to SpectrumBars and SpectrumDetail becomes then new Spectrum to sit next to the related Spectrogram.
8bed561
to
3000550
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor comments.
m_analyzerBarWidth = m_size.width() / 64; | ||
m_analyzerBarWidth = m_size.width() / 128; | ||
|
||
if (m_analyzerBarWidth < 6) | ||
m_analyzerBarWidth = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly replace this with something like:
m_analyzerBarWidth =
std::max(m_size.width() / kNumBarsWanted, kMinBarSize);
and define the two constants in the Spectrum class with:
static constexpr int kNumBarsWanted {128};
static constexpr int kMinBarSize {6};
I know there are close to 15,000 other magic numbers in the code, so I'll leave it to you whether to change this. I do think this type of change makes the code more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, for a number used in 1 place, the number is easier to read than words. Because if it is words I have to go lookup where the words resolve to a number. So reading 128 and 6 right here is easier for me.
But certainly numbers used in more than 1 place should be words instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You understand the code, so its not surprising that you find the numbers easier. For someone reading the code for the first time, the descriptive constant names would be easier.
Wow, that's bad! But totally unrelated to this PR. I think I can fix it but let's work on that over on #790. |
This applies the lessons learned in Spectrogram to the original Spectrum and Squares, greatly improving them: