PitchShiftEffect: add description comments #10858
Conversation
This commit adds the comments, primarily for the usage of the RubberBand API, for the Pitch shift effect processing.
| @@ -134,19 +136,30 @@ void PitchShiftEffect::processChannel( | |||
| m_currentFormant != formantPreserving) { | |||
| m_currentFormant = formantPreserving; | |||
|
|
|||
| // Set the RubberBand option for the formant preserving. | |||
There was a problem hiding this comment.
This comment just repeats the code.
| pState->m_pRubberBand->setFormantOption(m_currentFormant | ||
| ? RubberBand::RubberBandStretcher:: | ||
| OptionFormantPreserved | ||
| : RubberBand::RubberBandStretcher:: | ||
| OptionFormantShifted); | ||
| } | ||
|
|
||
| // Get the parameter value of the Pitch knob | ||
| // based on the current setting of the Range knob. |
There was a problem hiding this comment.
This comment does also not add any value.
You may explain that one of from 0...X and the other from -x to +y ... and describe the implications.
| if (m_pSemitonesModeParameter->toBool()) { | ||
| pitchParameter = roundToFraction(pitchParameter, kSemitonesPerOctave); | ||
| } | ||
|
|
||
| // Calculate the result pitch setting for the RubberBand. |
There was a problem hiding this comment.
Here also no value. You may tell us why std::pow(2.0 is required.
There was a problem hiding this comment.
I have left some comments. Sorry if they look rude, that was not meant like that.
I am happy about such Document PRs, Thank you.
It is just, that I share the opinion that comments are only a last resort to explain difficult code parts in cases it was not possible to put the information into the code itself.
That helps that the reader does not waste time reading it and limit the risk that the code comment is rotting after refactoring.
In this case, think about the pitfalls you have managed and write about them.
|
Absolutely no problem, thank you very much for the advice and proposals. I will do it this way. |
This commit removes the unnecessary comment of the setting the formant preserving for the RubberBand processing based on that it just repeats the code.
The commit changes the comment of the pitch value setting with a more detailed explanation of why the pow is needed, and the formula of the calculation is added.
The commit adds a closer explanation of how the resulting pitch value is changing based on the range value with a couple of real examples.
The commit adds the description of rounding to fraction to keep the resulting pitch value in the semitone chromatic scale.
The commit removes unuseful comments about retrieving data from RubberBand.
|
Thank you for merging. 👍 |
This PR adds the comments, primarily for the usage
of the RubberBand API, for the Pitch shift effect processing.