Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soft Takeover: More accurately differentiate between jumps and fader movements #485

Closed
mdmayfield opened this issue Nov 25, 2021 · 9 comments
Labels
enhancement New feature or request next

Comments

@mdmayfield
Copy link

mdmayfield commented Nov 25, 2021

Currently when using Pick Up or Catch Up modes, working with absolute physical controls (faders, knobs) fast movements can fail to register, and parameter jumps can happen too often.

The trouble is:

  • When Jump Max is set to a very low value (say 1%), the fader/knob must be moved very slowly, or else the software target will stop responding due to the distance between two consecutive CC messages exceeding Jump Max; but
  • When Jump Max is set to a value high enough to allow for faster fader/knob movements (say 20%), when the physical control is out of sync from the target value, approaching the target value from either direction will cause an unpleasant (in this case 20%) jump in the opposite direction as it comes close.

After thinking this through at length, I believe I have a good solution. Unfortunately I'm completely unfamiliar with Rust as of this writing; otherwise I would make a real pull request.

My suggested approach to resolve this can probably be best explained in pseudocode (pardon the sloppy half-C, half-JavaScript, half-who-knows-what syntax. It probably has some bugs as I haven't actually implemented this yet). Hopefully this makes sense:

Edit: rewrote this completely, still in a C-like pseudocode, after much additional thought and looking at what the Mixxx project does in a similar situation. This would be a helper function, called as each controller data value is received.

bool isInSync(
		float jumpMax,
		float jumpMin,
		unsigned int prevTimestamp,
		float prevValue,
		unsigned int currTimestamp,
		float currValue,
		float targetValue,
		bool inSync) {

	unsigned int elapsedTime = currTimestamp - prevTimestamp;
	float currDistance = currValue - targetValue;
	float prevDistance = prevValue - targetValue;
	bool sameSide = (currDistance < 0 && prevDistance < 0) || (currDistance > 0 && prevDistance > 0);
	bool approachingTarget = sameSide && fabs(currDistance) < fabs(prevDistance);

	// No matter what, if we're less than JumpMin, don't sync
	if fabs(currDistance) < jumpMin return False;

	// If we were in sync less than TIMEOUT ms ago, stay in sync
	if inSync && elapsedTime <= TIMEOUT return True;

	// New takeover logic. Goals:
	//
	// 1) Avoid awkward "backwards" jumps when approaching the target.
	// 2) Ensure that we pick up fast movements that *pass through* the target, even if
	//    the individual controller values never actually fall inside the Jump Max range.
	// 3) Enable a typical "set and forget" Jump Max of 3% or less without sync loss.
	//
	// Approaching is detected by either a quick movement from outside Jump Max to
	// inside it (both on the same side of the target), OR a slow movement (whose step
	// timings may exceed TIMEOUT due to coarse 0-127 resolution) within Jump Max.
	//
	// Inspired by controllers/softtakeover.cpp in the Mixxx DJ project.

	if abs(currDistance) <= jumpMax {
		if approachingTarget && (elapsedTime < TIMEOUT || fabs(prevDistance) < jumpMax) {
			return False;
		} else {
			return True;
		}
	} else if !sameSide && elapsedTime < TIMEOUT {
			return True;
		}
	}

	return False;
}
@mdmayfield
Copy link
Author

I'm looking into https://github.com/helgoboss/helgoboss-learn/blob/cbb805837b050388990ca36b967155f931e52765/src/mode/mode_struct.rs#L1200 and can see where (a different implementation of) this might fit in, and also improve the behavior of the other takeover modes.

@helgoboss , is this something you would consider merging if I wrote a patch? And if so, do you have any suggestions for how to obtain timestamps for the control messages?

@mdmayfield
Copy link
Author

After a bit more experimentation, it looks like the max time between two received CC messages from an absolute control, to interpret as part of the same movement and override Max Jump settings, would probably be best set around 50-100ms, not 250-1000.

@helgoboss helgoboss added the enhancement New feature or request label Nov 27, 2021
@helgoboss
Copy link
Owner

Hi @mdmayfield. Definitely would consider merging an improvement in this area! Thanks for looking into it. I would suggest adding a new takeover mode if it has some fundamentally different behavior and improving an existing one if it can objectively seen as an improvement of existing behavior. Adding a new takeover mode is a bit more work (but not much) because it will have to be added to the outer "onion" layers as well (model, persistence, API). I could take care of that. If you need help getting things to compile, let me know. There's an issue using the latest stable Rust at the moment (abnormally long compile times), so you will have to use Rust 1.55.0 for now.

As for the timestamp, it would be best to capture the timestamp at the time the MIDI/OSC event comes in (because 1st, messages cross threads, so measuring at a later point in time will lose accuracy and 2nd, an incoming event carries some offset information which we could use to get more refined timestamps). Therefore I think it would make sense to not just pass a ControlValue through ReaLearn but a ControlEvent which contains both a timestamp and the control value. This is new and something that would affect many parts of ReaLearn but I could do it. Makes sense.

@mdmayfield
Copy link
Author

Great, thank you @helgoboss ! I will plan to put together a draft/POC pull request, intended for further review and rework. (So for the first attempt I might use a kludge for the timestamps just to try it out & refine the behavior.)

This is probably worth applying to the existing modes. It's definitely desirable for the Pick Up takeover mode, as it matches commercial implementations on hardware & software and would make a Jump Max of 0% reliably usable.

I've not seen the other modes anywhere else, but really like the idea of them (especially Catch Up). Parallel, Catch Up, and Long Time No See would definitely all benefit from this same change - it would avoid the situation where the software value falls back out of sync with the hardware control if the hardware control catches up and overtakes the target value quickly.

This should be a welcome change to all modes, I believe. Of course, you never know how people will react...

There's an issue using the latest stable Rust at the moment (abnormally long compile times), so you will have to use Rust 1.55.0 for now.

Thank you! So 11 minutes to compile this project on a 12-core i7 is not normal - that's a relief!

@helgoboss
Copy link
Owner

Great, thank you @helgoboss ! I will plan to put together a draft/POC pull request, intended for further review and rework. (So for the first attempt I might use a kludge for the timestamps just to try it out & refine the behavior.)

Looking forward.

This should be a welcome change to all modes, I believe. Of course, you never know how people will react...

😁

There's an issue using the latest stable Rust at the moment (abnormally long compile times), so you will have to use Rust 1.55.0 for now.

Thank you! So 11 minutes to compile this project on a 12-core i7 is not normal - that's a relief!

The first compile can take pretty long, even with 1.55, because it compiles all the dependencies. Subsequent compiles make it acceptable.

@mdmayfield
Copy link
Author

mdmayfield commented Nov 28, 2021

OK @helgoboss , here is a rough sketch of the general functionality. It's not quite right yet - it doesn't behave 100% how I want it to yet - and I'm certain the implementation is suboptimal, kludgy, and/or non-Rust-idomatic... but it's a start.

It's obvious that there are several helper functions that would make this much easier, but I couldn't get the hang of the Rust-isms to allow their use in the time I had. I'll keep working on this as I have time, and will enthusiastically welcome any concrete recommendations for improvements!

mdmayfield/helgoboss-learn@9a52d50 Deleted and rewrote in https://github.com/mdmayfield/helgoboss-learn/tree/feature/soft-takeover

@helgoboss
Copy link
Owner

helgoboss commented Nov 10, 2022

Hey @mdmayfield. I'm finally ;) integrating your code. Thanks again. It's a big improvement and I think I found an elegant way to remove the user-facing jump min/max settings while still staying backward-compatible.

I can't really merge your PR because too many things happened in the meantime and I need to adjust that code a lot, but I will definitely put comments in the code and changelog that this is mainly your contribution. And if you want, feel free to create another small PR, e.g. docs so you appear in the list of contributors for this repo.

I found that there are still ways to get stuck. E.g. using the X-Touch One fader and having jump max at 3%, if I move it all the way down and then wait a bit and crank it very quickly up, the target value gets stuck at 0%. The reason is that the X-Touch skips some intermediate values when you move the fader very fast ... which is okay and probably even reasonable. However, it leads to big control value jumps, often more than 3%. Using your method, it still works - but only if the movement is already ongoing when the big control value jump occurs. If you move it very quickly right at the beginning of the movement, it gets stuck.

So I think I add another piece of logic to the mix: If the last change of the target was caused by this mapping, then I always accept the change and don't look at possible jumps at all. In other words, I always follow the fader, even tolerating larger jumps. I only fall back to the jump prevention logic if something else (not our mapping) has changed the target value in the meantime.

One possibly undesired side effect of this can occur if one uses a control element that allows one to jump to arbitrary values, e.g. touch strips. Then one can create jumps by tapping on arbitrary positions on the touch strip and ReaLearn will not prevent it. Is it desired, or undesired? I think it depends on the situation. Maybe I should make this configurable.

What do you think?

helgoboss added a commit that referenced this issue Nov 11, 2022
the mdmayfield way with some None returns
helgoboss added a commit that referenced this issue Nov 11, 2022
I think this is already covered by the reset in
update_from_target()
@helgoboss
Copy link
Owner

I made it configurable. We have "Pickup" (doesn't include my addition) and "Pickup (tolerant)" (includes my addition, better if not getting stuck is important).

This will come with 2.14.0-pre.10.

Jump Min/Max is not shown anymore but will be respected for old mappings. New mappings will not be able to customize jump min/max anymore. It simply uses 0% - 3%, hard-coded. The effect of "Jump Min" can be reproduced using "Value sequence".

@mdmayfield
Copy link
Author

@helgoboss This sounds great - thank you very much for implementing it! I am looking forward to trying it out!

helgoboss added a commit that referenced this issue Nov 11, 2022
- #224 Added previous/next buttons to the mapping panel
- #747 Added menu and help buttons to the header panel
- #485 Renamed takeover mode Normal to Off
- Fixed bottom section of main panel (was causing artifacts on Windows
  because of overlapping areas)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next
Projects
None yet
Development

No branches or pull requests

2 participants