Fix quantization in the effect engine (metronome effect) #13636
Conversation
…and __restrict__ assumption is violated.
…value is related to the buffer end.
This is required for determining the exact feat position in a buffer.
…a no longer required workaround form the time, where the Mixxx transport was integer precise.
|
Done. |
There was a problem hiding this comment.
the changes here are really hard to follow because they mix offset and timing calculations with buffer indexing (mixing levels of abstraction) and thus I'm neither confident about the reliability nor safety of the code...
Do you think its feasible to reorganize it a little so its easier to understand?
There was a problem hiding this comment.
I have tried to fix that by calculate everything in frames. Maybe I was not too successful or comments are missing.
Can you give some hints what can be improved?
There was a problem hiding this comment.
I will rework it a little and open a PR to your branch ASAP.
There was a problem hiding this comment.
Please take a look daschuer#97
|
I have added the first commit of your branch from daschuer#97 |
Sure, but unfortunately I encountered a couple bugs during testing:
Both of these issues are regressions. |
|
The described issues are fixed now. It was caused by a faulty fix to avoid SINT overflow. |
|
I don't understand the fix... There also remains the issue where the first click is with sync disabled is muffled. |
This is a "feature", the fade in of the effect. But we can safely disable this here. I have removed a redundant check and added comments to the last summit. |
|
Done, please have a look. |
|
fyi, I got daschuer#97 into a state I'm happy with. Please take a look. |
|
I have adopted the full span based approach. But I have not adopted the rest, because of the std::size_t / std::ptrdiff_t / SINT clash with gsl/span. I also must admit that I can not fully understand how your code works. |
|
I think the primary advantage of my span approach was not the fact that it uses span (it just made it easier for myself to do the cleanup I wanted to do). The real improvement was separating the messy sync code from the rest. But I'm okay with the current state as a temporary stopgap. |
ronso0
changed the title
This was originally a fix for #13611 but it turns out that the beat information provided into the effect engine itself was broken. I have fixed this at the root by refactoring GroupFeatureState and the "beat_next" and "beat_prev" COs.
Than I was able to finally fix the metronome, which works now also reliable during scratching.
Not sure if there is a real use case for it, but the metronome was perfectly to test that, and also helps verifying a beat grid by ear. It would be a real fun to play any sample and not only the metronome click. That could become our first "transport" effect. I will file a feature request.
Open is to check/fix other effects. I want to have that reviewed and merged first before building on that.
See commits for details.