Thanks for the updated patch. I can't try this code out right now because I'm at work so I'm just running it in my head -- I just wanted to get you comments as soon as possible. * WWaveformViewer::mousePressEvent -- I don't see why you remove the m_pScratchPosition->slotSet(0.0)? Does it matter? * In your patches lately you're changing a lot of constructor initializer lists. I haven't mentioned it sofar because I didn't want to slow you down but the way you are changing it from is the preferred way we do it in Mixxx because 1) Many classes are written this way and 2) The Google C++ Style Guide specifies it this way (and that's the style guide we aim to follow). http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_Initializer_Lists In the future could you keep the initializer lists in Google C++ style? * there are some tabs in the patch * Instead of hard-coding the PID values, please use the existing CO's that contain them m_pScratchControllerP, m_pScratchControllerI, m_pScratchControllerD (or just delete the COs, they are useful for testing though so I would like to keep them). * I don't think you should make any changes to VelocityController. The changes you made no longer make it a PID controller. Also, since we are only using P I think that you shouldn't change the I and D codepaths since there's no need. I could be reading this wrong but I think the change you made to the P code path (dividing the position and target_position by dt) is completely equivalent to dividing the P parameter by the latency which is what I suggest in #8 and IMO cleaner / simpler. * Oops, I just noticed when trying to figure out why you were multiplying the P component for high latency by the buffer size that this is to cancel out dividing by the buffer size in the delta-position/set-point (which on my first read I read as you dividing by dt) Instead of the buffer size we should always use time because buffer size varies with both latency and sample rate whereas time only varies with latency. Using buffer size changing your sample rate between 44.1/44/96 will change the behavior of scratching at the same latencies. * Looking back on the m_dScratchTime variable, I have no clue what that was for :). I think it's incorrect that it accumulates dt. The observation time argument should just be 'dt' as you have changed it to be. * Why have you made the change to stop accumulating the error in VelocityController if the error is less than 0.1? Did you see issues with denormals accumulating or other numerical precision issues? * I prefer to keep the set-point of the PID built into VelocityController via the reset()/setTarget() method since then the controller is in charge of calculating its own error. Plus it keeps your patch simpler -- it's already hard enough to tell what's going on since this is a complicated part of Mixxx. * As we talked about in Bug #1117762, if you click the waveform it's intentional that you have "grabbed" it and the track stops. I'll have to try out your patch to see how this works in practice but I think that when first enabling the scratch controller the rate should be controlled by the velocity controller. Plus "paused ? 0 : 1" discards the current rate -- for example if the track was going in reverse, pitch-bended or inertia-thrown before scratch starts then the rate will temporarily jump to 1 before scratching begins which could be discontinuous. I realize this is probably also a problem for the current implementation too. I'll try it out and comment again. One way around this could be to make the response to isEnabled() be false for the first callback right after we enable scratching. That way the RateController keeps the current rate -- whatever it was -- before we start controlling the rate. In general in the future I think RateControl should be completely rewritten as one monolithic controller that is aware of everything rather than having the PID controller in scratch-controller be selectively enabled for when scratch mode is turned on. This way we could model things like inertia and momentum and things without having to worry about "handoff state" between the different controllers like we do now.