LMMS
Remove `ValueBuffer`
supersedes #7170 which got closed because of a mess up on @Snowiiii's side. I recovered their commits using git fetch upstream pull/7170/head:valuebuffer-removal.
ValueBuffer is just and wrapper around std::vector, but we dont use any Special methods from it so we should use std::vector instead
Copied description from original PR.
@sakertooth's comment at https://github.com/LMMS/lmms/pull/7170#issuecomment-2035648962 still applies. While passing a pointer to a std::vector<float> doesn't leave us any worse off than passing a pointer to a ValueBuffer, if we actually want to get any advantage from this change we ought to do it properly.
Also, having thought about it some more, I wonder if we can repurpose it in some way to avoid the endless repeating of code like sampleExactBuffer ? sampleExactBuffer[i] : fixedValue.
@sakertooth's comment at https://github.com/LMMS/lmms/pull/7170#issuecomment-2035648962 still applies. While passing a pointer to a std::vector
doesn't leave us any worse off than passing a pointer to a ValueBuffer, if we actually want to get any advantage from this change we ought to do it properly.
I've addressed this one a week ago and am waiting for a follow up review.
Also, having thought about it some more, I wonder if we can repurpose it in some way to avoid the endless repeating of code like sampleExactBuffer ? sampleExactBuffer[i] : fixedValue.
we could but how exactly? I guess we have to write a simple helper function for it.
If what i understand is right, you are asking me to create a helper function to handle the code repetition and modify the code accordingly. But i see one problem in the helper function.
float AutomatableModel::valueAt(size_t index)
{
const auto buffer = valueBuffer();
return buffer ? buffer->data[index] : value();
}
We create a new buffer and check for the buffer immediately. Which might return true in all cases, so the fixed value might not get accessed. Also the new buffer might have a value different from the old one. Atleast that's what i understand. Is there anything else going on which I missed?
We create a new buffer and check for the buffer immediately. Which might return true in all cases, so the fixed value might not get accessed. Also the new buffer might have a value different from the old one. Atleast that's what i understand. Is there anything else going on which I missed?
I don't think there's any creation of a buffer here. Could be wrong, but valueBuffer returns a pointer to the buffer or nullptr depending on various factors, and if its not null we return the value at a certain index in that buffer. Otherwise, we use the fixed value.
This is similar to what the original code was doing. It was calling valueBuffer, then determining if it should use the fixed value or not. From what I can tell, this function does basically the same thing, the only difference is that valueBuffer is called more frequently in situations where its in a loop, which may or may not be an actual problem to worry about.
@sakertooth I need help with the CI. Can't get a clue rn.
Also, I do have a suspicion of new bugs surfacing due to out of bounds access now that we got rid of the clamps.
As of now, most of what's broken was fixed, will look at the rest soon. Also, the new set of errors doesn't seem to be from my changes.
@sakertooth @messmerd i got an idea with regards to fixing the out of bounds access.
float valueAt(size_t index) const
{
const auto buffer = valueBuffer();
size_t clampedIndex = std::clamp(index, 0, buffer->size())
return buffer ? (*buffer)[clampedIndex] : value(0);
}
I typed this as a quick idea, haven't tested this.
Looking at this now, I realized it would be better to redesign the sample-exactness system rather than just remove ValueBuffer, thus replacing complex uses of ValueBuffer directly with something easier to work with. I'm planning to do some work towards this.