avoid confusion between visual waveform samples and frames #12356
Conversation
…naming and use correct range (was double!) for simple, filtered and hsv allshader waveforms
m0dB
changed the title
|
Is this PR ready for merge or work in progress? |
|
ready for merge |
|
I have a review in progress .. |
|
|
||
| visualIndexStart = std::max(visualIndexStart, 0); | ||
| visualIndexStop = std::min(visualIndexStop, dataSize); | ||
| const int visualFrameStart = int(xVisualFrame - maxSamplingRange + 0.5); |
There was a problem hiding this comment.
we prefer static_cast(). Did you consider to use round()?
The same applies to the other waveforms
There was a problem hiding this comment.
i believe this got copied from the legacy waveforms, but I agree. I will replace all this with std::lround
| const int visualFrameStart = int(xVisualFrame - maxSamplingRange + 0.5); | ||
| const int visualFrameStop = int(xVisualFrame + maxSamplingRange + 0.5); | ||
|
|
||
| const int visualIndexStart = math_clamp(visualFrameStart * 2, 0, dataSize - 1); |
There was a problem hiding this comment.
Is -1 correct here, We don't want to swap L and R.
Maybe we should rename dataSize to visualSampleSize or such.
Alternative: Make all frames and ..
for (int i = visualFrameStart; i < visualFrameStop; i += 2) {
There was a problem hiding this comment.
Well spotted. I will change this.
There was a problem hiding this comment.
But note that for (int i = visualFrameStart; i < visualFrameStop; i += 2) { is wrong. I added some comments in waveformrenderersimple.cpp to explain everything.
| @@ -69,10 +67,10 @@ void WaveformRendererHSV::paintGL() { | |||
| const float breadth = static_cast<float>(m_waveformRenderer->getBreadth()) * devicePixelRatio; | |||
| const float halfBreadth = breadth / 2.0f; | |||
|
|
|||
| const float heightFactor = allGain * halfBreadth / 256.f; | |||
| const float heightFactor = allGain * halfBreadth / 255.f; | |||
There was a problem hiding this comment.
I am in doubt this change is correct. At least 255 it is a "magic number" here.
I think the code would be clearer if e change if we normalize maxAll to 1.0f
What is the maxAll value maximum?
The same applies to the other waveforms.
There was a problem hiding this comment.
I changed it because it was 255.f elsewhere. But yes, I do think 255 is correct, it the waveform data filtered members are uint8_t. Shall I change it to const float maxValue = static_cast<float>(numeric_limits<uint8_t>::max()) ?
|
I think the current state is already an improvement. I have just stumbled over some details that can be improved. |
|
Yes, I will do the additional clean up. |
|
I have addressed your comments @daschuer |
|
Still works good. Thank you. |
Avoid confusion between visual waveform samples and frames by better naming and use correct range (was double!)
Continuation of #12205 for simple, filtered and hsv allshader waveform renderers