trigger and friends without pattern IDs

[matthewkaney]

Looking into the trigger family of functions, I wanted to see how easily it would be to get rid of the redundancy of having to specify the pattern ID, which I've found confusing before. Since Alex seems open to getting rid of it, I went ahead and wrote up a change that would do just that.

This introduces a new set of controls that would be stored per-pattern and then merged into the rest of the query controls when the patterns are queried. I could see this mechanism eventually including the pattern ID itself, mute/solo state, as well as other information supplied by the editor (for example, code line number(s) and so on). In theory, the pattern history could also be passed in as a per-pattern control, allowing for transitions to be implemented as patterns themselves.

For now, I wanted to see what you all (@yaxu and maybe @Zalastax and anyone else with an opinion) thought about this approach. There are still a few open questions that I have:

• patControls should probably be saved in history and reverted when a pattern is reverted. That's different from (but I think an improvement over) the current behavior.
• I need to also fix reset, but I haven't touched it yet, because I don't really understand it. As far as I can tell, reset is equivalent to ftrigger $ filterWhen (>= 0), but ftrigger always places the 0 point slightly before the pattern is started, so the filter has no effect (except, I guess, if the user calls resetCycles). I can't find much documentation—is this function worth keeping?
• I left in the old behavior where all patterns can look up an arbitrary pattern's start time with cf0 "_t_1", cf0 "_t_2", etc. Is that ever useful? Is it useful to have the ability to trigger a pattern based on when a different pattern was first evaluated?
• Passing in the evaluation time as a per-pattern control theoretically means that trigger could also play nice with once/first, but I don't know enough about how once/first are supposed to work to understand how they'd work with trigger.
• trigger has the same problem that resetCycles used to have where events happening immediately at the start of the trigger get skipped by the next pattern query. That's a problem with the existing behavior as well, and should probably be a separate bug.
• Really minor, but trigger and friends shouldn't be in Control.hs, since they're not specific to control patterns. Where should they be?

This fixes #968, albeit by changing function signatures and thereby introducing a breaking change. I'm open to suggestions as to when would be a good time to merge/release this. Thanks so much!

[yaxu]

Hey @mindofmatthew thanks a lot for looking into this!

patControls should probably be saved in history and reverted when a pattern is reverted. That's different from (but I think an improvement over) the current behavior.

I might well be missing something, but would it be possible to do without patControls? streamReplace has the id and the pattern in hand, could it sneak the id into the pattern state controls at that point, in a similar style to withQueryArc?

I need to also fix reset, but I haven't touched it yet, because I don't really understand it. As far as I can tell, reset is equivalent to ftrigger $ filterWhen (>= 0), but ftrigger always places the 0 point slightly before the pattern is started, so the filter has no effect (except, I guess, if the user calls resetCycles). I can't find much documentation—is this function worth keeping?

I'm also confused about what reset is for. Probably I attempted to implement this feature twice and they both made it! If you agree it's a duplication, then yes please remove it.

I left in the old behavior where all patterns can look up an arbitrary pattern's start time with cf0 "_t_1", cf0 "_t_2", etc. Is that ever useful? Is it useful to have the ability to trigger a pattern based on when a different pattern was first evaluated?

I guess this was so that you could reset pattern b whenever pattern a was evaluated. I've never found this useful so would be happy with this being removed for the sake of simplicity. It's just a feature that fell out of a pattern not knowing its own id really.

Passing in the evaluation time as a per-pattern control theoretically means that trigger could also play nice with once/first, but I don't know enough about how once/first are supposed to work to understand how they'd work with trigger.

Is there a use case for this? I think once/first are mainly useful for playing single events asap.

trigger has the same problem that resetCycles used to have where events happening immediately at the start of the trigger get skipped by the next pattern query. That's a problem with the existing behavior as well, and should probably be a separate bug.

Yes, maybe it's easier to solve with the changes to stream post-link.

Really minor, but trigger and friends shouldn't be in Control.hs, since they're not specific to control patterns. Where should they be?

Good point.. Maybe UI for now?

[matthewkaney]

I might well be missing something, but would it be possible to do without patControls? streamReplace has the id and the pattern in hand, could it sneak the id into the pattern state controls at that point, in a similar style to withQueryArc?

Yes, definitely. The benefits of separately storing patControls are purely speculative right now, so I've gone back and done it by just updating the pattern query in streamReplace. Note that I'm injecting the trigger time, rather than the pattern ID. I think this is more direct and doesn't ever result in the pattern and trigger time getting out of sync (for example, if a pattern gets reverted because of an error).

With that, I also removed the global state values for pattern evaluation times as we discussed. I also removed reset, moved around the order of the trigger functions a bit, and added mtrigger, a mod trigger function that allows you to align to the next cycle boundary modulo n.

Is there a use case for this? I think once/first are mainly useful for playing single events asap.

My use case is something like first $ qtrigger $ s "looper", where I want to send a single "record loop" command to tidal looper, aligned with the cycle clock. I think there are multiple issues with this working—what I ultimately want is a different behavior for disposable/temporary patterns, and I don't fully know how I'd want it to work, so I'll leave this alone for now.

I also left the functions in Controls.hs for now. We should probably do a bigger code reorganization coinciding with 2.0.

Anyway, this is ready to be merged unless you've got other comments. This will change the trigger API, so it can wait until a relevant release.

[Zalastax]

Good work! Really appreciate that you're addressing this annoyance which has been bugging me too!

[Zalastax]

Beware that this adds a new operation which happens at every query. I see a risk for lower performance in the critical path. I would prefer that we do some benchmarking before accepting the PR. I am also curious whether it would be possible to make a larger restructuring in order to achieve your goal some other way. For example, could we change the pattern datastructure type? If Time is now included in all patterns, it should perhaps not be part of the pattern map since it's not optional.

[matthewkaney]

Sure, I haven't spent much time with the benchmark tests, but I can play around and figure out a test for this case.

I would point out that this shouldn't be any more expensive than the # pS "_id_" (pure $ fromID k) that we already do on every query.

[Zalastax]

I see your point - we are already performing a very similar Map.union here. Complexity wise, you are fully correct. From real performance perspective, this might be negligible as well - I don't know before we check some benchmarks. I really don't want to discourage this from being merged as long as performance is not too affected. My proposal could be part of a different change and a larger overhaul.

[yaxu]

It would be good to have a look at benchmarking, there is a benchmarking suite but I think no-one ever really looked at performance issues with it.
Otherwise, shall we go ahead and merge this?

[Zalastax]

Yes, I also think we should go ahead with merging this.

[matthewkaney]

Thanks! I added one last tweak, and am good with this getting merged.

I figured out how to get the benchmark tests to run the other day. I'm still wrapping my head around the strategy for writing useful tests, but happy to help look for performance issues. My current goal is breaking up some of the stream code into smaller, more modular pieces, so that will help with testing (both unit tests and benchmarks).

[Zalastax]

Lovely, @mindofmatthew!

Did you manage to make any comparison in the benchmark between before and after based on the benchmarks that already exist?

[matthewkaney]

Not yet. There are only a few benchmarks for individual pattern functions. I completely agree that this is the process where performance is critical—I want to build a benchmark that tests all of the relevant code so we have a sense of what's adding expense.