Remove ValueBuffer #7170
Remove ValueBuffer #7170
Conversation
|
I'm pretty much in favor of removing |
plugins/Amplifier/Amplifier.cpp
Outdated
| const float pan = (panBuf ? panBuf->value(f) : m_ampControls.m_panModel.value()) * 0.01f; | ||
| const float left = (leftBuf ? leftBuf->value(f) : m_ampControls.m_leftModel.value()) * 0.01f; | ||
| const float right = (rightBuf ? rightBuf->value(f) : m_ampControls.m_rightModel.value()) * 0.01f; | ||
| const float volume = (volumeBuf ? volumeBuf->at(f % volumeBuf->size()) : m_ampControls.m_volumeModel.value()) * 0.01f; |
There was a problem hiding this comment.
I'm curious if the modulo was covering for some bug in ValueBuffer before.
There was a problem hiding this comment.
The issue is that this is hiding out of bounds array access. The question is why is this needed.
There was a problem hiding this comment.
I just implemented the Old behaviour from ValueBuffer which was:
float ValueBuffer::value(int offset) const
{
return at(offset % length());
}There was a problem hiding this comment.
Right, and the question is why was modulo being used here.
There was a problem hiding this comment.
Yeah the notes not rendering was fixed. Can you load and play an intensive project like Krem Kaakkuja with no modulo/clamp? If so, then I think we can remove the modulo/clamp.
works without any problems.
There was a problem hiding this comment.
@DomClark I think we can remove the clamp, too, now. What do you think?
There was a problem hiding this comment.
@PhysSong for additional opinion :)
There was a problem hiding this comment.
I think you should use static code analysis to check every occurrence where the value function is used. In all these cases it must be assured that no out of bounds read is possible. If in doubt, I would suggest to add an assert before such calls and then make tests where this assert is being tested.
There was a problem hiding this comment.
I think we should leave it an runtime check
use cmath instead of bits/stdc++.h>
|
Will mention again that I am strongly against passing a We truly need to get support for Edit: I can somehow understand why it may be simpler to return the vector, but for passing a vector into function as an argument I really am not in favor of. |
ValueBuffer is just and wrapper around
std::vector, but we dont use any Special methods from it so we should usestd::vectorinstead