rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Rest handling with patterns

Open jamshark70 opened this issue 6 years ago • 23 comments

  • Title: Easier variable-length rests in patterns
  • Date proposed: 2019-11-01
  • RFC PR: https://github.com/supercollider/rfcs/pull/0004

Summary

Currently it is easy to embed a rest of a fixed value in a sequence: Pseq([0, Rest(1), 2]).

It is not easy to embed a rest with being lazy-calculation value: Pseq([0, Pwhite(1, 3, 1).collect(Rest(_)), 2]).

Motivation

This question keeps coming up repeatedly on the mailing list and forum. It's a legitimate user need.

Users often expect to be able to write Rest(Pwhite(1, 3)) and have the Rest be automatically stream-ified. This might be a reasonable expectation (and not hard to implement). But there is also precedent to reject that approach, e.g., an array of patterns requires a Ptuple() wrapper.

Alternately, we could follow the model of .collect -> Pcollect, .select -> Pselect, .keep -> Pfin and provide a .rest method for patterns and other objects.

Some prior discussion is here.

https://scsynth.org/t/pbind-and-samples-with-different-durations-2/735/13

Specification

  1. A Prest(patternOrStream, n) pattern, to take n values from the source pattern and output Rest() objects with those values -- following the model of Pcollect to implement .collect, etc. a. Default n should be 1. See unresolved questions.

  2. A .rest(n) method that returns Prest(this, n) for patterns, and Rest(this) for other objects. Possibly, for SequenceableCollections, we might do this.collect(Rest(_)).

    • 2.rest --> Rest(2)
    • Pwhite(1, 3, inf) --> Prest(Pwhite(1, 3, inf), 1)
    • (Maybe?) [1, 2].rest --> [Rest(1), Rest(2)] but here, I don't have a concrete use case. Currently the relationship between rests and arrays is undefined.

Drawbacks

A possible objection is "too many ways to write the same thing." In fact, I raised that objection to a .rest method in the past. But we have both .fin and .keep as pattern aliases for Pfin, so there is precedent and I've relaxed my objection.

Unresolved Questions

.rest is not precisely analogous to .collect.

In pattern.collect(...), we expect every value of the pattern to be streamed out and further mapped by the function.

In pattern.rest(...), it's pointless to stream out all the values and turn them into rests, because then you will have a potentially long series of silent events.

Pbind(\degree, Pwhite(0, 7, inf), \dur, Pseq([0.5, 0.5, rest(Pwhite(1, 4, inf) * 0.5)], inf)) -- if we follow the collect model, then you get two notes and then silence, indefinitely.

I believe the best solution is to limit the length of the Prest stream, default = 1. But this is worth discussing.

(xyz).rest vs rest(xyz) vs Rest(xyz)

Note here that I decided to save a dot by using function-style notation: rest(Pwhite(1, 4, inf) * 0.5).

If we support this, then inevitably -- guaranteed -- some user will write Rest(Pwhite(1, 4, inf) * 0.5) and get annoyed that it doesn't work.

I think it wouldn't be hard to fold Prest behavior into Rest:embedInStream. Is this worth doing for user friendliness, or should we be strict and enforce the distinction between a Rest object (simple value) and the rest method applied to patterns?

jamshark70 avatar Nov 01 '19 02:11 jamshark70

@jamshark70 looks like you committed two files by mistake, can you please remove 0000-....md?

mossheim avatar Nov 01 '19 03:11 mossheim

OK... I had renamed using "git mv" but then didn't load the new file in the editor. Dumb mistake.

jamshark70 avatar Nov 01 '19 07:11 jamshark70

I think it wouldn't be hard to fold Prest behavior into Rest:embedInStream. Is this worth doing for user friendliness, or should we be strict and enforce the distinction between a Rest object (simple value) and the rest method applied to patterns?

I tend not to fold it into, unless we find a really clear way to make it understandable. We already do a special case for booleans, but that is a very different behavior and use case.

.rest is not precisely analogous to .collect.

I think it should be analogous. You may want to create an infinite pattern of rests and then interlace it with another non rest pattern.

telephon avatar Nov 01 '19 14:11 telephon

I think it should be analogous. You may want to create an infinite pattern of rests and then interlace it with another non rest pattern.

This, at least, is already handled in the proposal:

Ppatlace([
	Pwhite(0, 7, inf),
	Prest(
		Pwhite(1, 4, inf) * 0.25,
		inf  // <-- HERE, tell Prest to keep going forever
	)
], inf).asStream.nextN(6)

-> [ 0, Rest(0.25), 1, Rest(0.25), 6, Rest(0.75) ]

I'm arguing that this is not the normal case, and based on forum and mailing list questions, users will tend to expect rests to interlace, rather than embed. So we should make the typically expected case into the default.

I tend not to fold it into, unless we find a really clear way to make it understandable.

It certainly needs some thinking-through. I can't do it just at the moment, but I can say for now that "folding" is the most risky and also the least critical element of the proposal. We could drop it easily.

jamshark70 avatar Nov 02 '19 04:11 jamshark70

For quick reading, I'll put the conclusion first: With some testing, I can't find a major technical objection to allowing Rest object to handle patterns/streams internally. All of the cases just work.

The question is: Rests are currently stateless. If we go with what's below, they become potentially stateful. That's a big change and I wouldn't do it casually -- definitely needs some scrutiny. (E.g. now you can do r = Rest(2) and you can put r anywhere in a pattern, and it behaves the same. That wouldn't be true if we change it as below and r = Rest(Pwhite(...)).)

I'm of two minds. On the one hand, "one more way to do things" carries some risk. On the other, I don't like unnecessary roadblocks for users' creative workflow. From that perspective, supporting rest(pattern) but not supporting Rest(pattern) could be seen as needlessly pedantic.

Tests:

To make things more concrete: this is what it would look like if Rest automatically handled embedded patterns:

Rest : Operand {
	var stream, <>repeats = 1;

	*new { |value = 1|
		^super.new.value_(value.dereferenceOperand).unwrapBoolean
	}

	value_ { |val|
		value = val;
		stream = value.asStream;
	}

	dur_ { |dt| this.value = dt }
	dur { ^value.value }

	embedInStream { |inval|
		var out;
		repeats.do {
			out = stream.next(inval);

			// nil values from the stream should go out directly
			// so, the end of the rest stream stops the calling pattern
			// (compatible with nil handling everywhere else)
			if(out.notNil) {
				out = Rest(out);
			};
			inval = out.yield;
		}
		^inval
	}

	// ... remainder of the class as is
}

Simple regression (constant value) -- no change in behavior.

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(0.125)], inf)
).play;
)

p.stop;

Infinite-length pattern laces one value and cedes control:

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pwhite(1, 4, inf).trace * 0.125)], inf)
).play;
)

p.stop;

If the Rest object's stream comes to an end, it stops the calling pattern (as if any other pattern had returned nil).

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pseries(1, 1, 4).trace * 0.125)], inf)
).play;
)

p.stop;

One nice thing is, because Rest and Pattern both handle math operators, it doesn't matter where you put the *: Rest(pattern * something) and Rest(pattern) * something both become Rest(Pbinop('*', pattern, something)) and the behavior is the same. I didn't need to make any logic changes to support that.

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pseries(1, 1, 4).trace) * 0.125], inf)
).play;
)

p.stop;

By contrast, .rest and Prest don't handle this quite as cleanly. Because Prest starts from the beginning every time it's embedded, the Pseries starts from the beginning as well. It's technically ~~correct~~ justifiable and matches other behaviors in the pattern system, but it isn't optimally useful.

// rest is always 1 * 0.125
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), (Pseries(1, 1, 4).trace * 0.125).rest], inf)
).play;
)

p.stop;

// you have to add .asStream
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), (Pseries(1, 1, 4).asStream.trace * 0.125).rest], inf)
).play;
)

p.stop;

jamshark70 avatar Nov 02 '19 06:11 jamshark70

I find it a bit strange that a Rest converts a pattern to a stream internally, making it stateful.

The functionality of partial embedding a pattern seems to be a more generally useful one, independent of Rest. Then we don't need a repeats argument, and Rest can just receive its behavior from the enclosed pattern.

telephon avatar Nov 02 '19 13:11 telephon

The functionality of partial embedding a pattern seems to be a more generally useful one, independent of Rest. Then we don't need a repeats argument, and Rest can just receive its behavior from the enclosed pattern.

We already have Pfin.

Consider this use case:

  • 5 notes of duration 0.25
  • Followed by one rest
  • Then 5 notes
  • Then one rest, whose duration has increased in an arithmetic series from the last rest
  • and repeat these 2 elements indefinitely.

Currently the user is required to write: Pseq([Pn(0.25, 5), Pfin(1, Pseries(0.25, 0.25, inf).asStream).collect(Rest(_))], inf) (edit: initially I forgot the Rest part... which underscores how over-complicated it is -- I know what I'm doing and I still left part of it out). I'm saying that this is clumsy and forum questions suggest very strongly that users expect this requirement to be very simple to write -- they expect it to be easy to insert a single rest into a sequence -- when in fact it is anything but simple to write.

I would go so far as to say that this is the normal, baseline case for variable-valued rests, and it's one that we support relatively badly.

jamshark70 avatar Nov 02 '19 14:11 jamshark70

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

Consider this use case:

All I am saying that whatever is required from rests is also required from any other value, like a number or an event. I am not arguing against your proposal but try to see it as independent of the Rest class itself.

So consider this use case:

  • 5 rests of duration 0.25
  • Followed by one note
  • Then 5 rests
  • Then one note, whose duration has increased in an arithmetic series from the last note
  • and repeat these 2 elements indefinitely.

Note values, unlike rests, can't carry a duration, so this seems what is skewed in the whole picture.

telephon avatar Nov 03 '19 21:11 telephon

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

I'm not quite following what you mean by "parallelism" -- if you are partially embedding a stream and you want the stream to resume the next time it comes around (resume, rather than resetting), then that would seem to be fundamentally incompatible with parallelism (i.e., if parallelism is the point, you probably aren't using Pfin with a stream anyway).

All I am saying that whatever is required from rests is also required from any other value, like a number or an event. I am not arguing against your proposal but try to see it as independent of the Rest class itself.

So consider this use case:

Ok, I see. There are two differences:

  • "5 rests of duration d" can be optimized into one rest of duration 5*d, greatly simplifying the pattern's topology. (Counterargument: There could be another element in the Pbind that needs to advance once for each of the 5 rests, in which case you can't optimize.) Also make those 5 rests variable duration and it's more convincing.

  • A note of some variable value is currently easy to write, but wrapping that variable value in something else is not so easy. A better analogy here might be the way we have to wrap arrays in a second array level for arrayed synth arguments. That expands the picture from "Rest-ifying" to a more general wrapping.

    • But I'd maintain that the normal case for a rest is to embed one while the normal case for other wrapping would be to embed as many values as the source pattern produces. Rests are unique in that a sequence of rests is indistinguishable from a single rest summing their durations.

So it would be worth using this as an opportunity to simplify array arguments as well.

jamshark70 avatar Nov 03 '19 23:11 jamshark70

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

I'm not quite following what you mean by "parallelism" -- if you are partially embedding a stream and you want the stream to resume the next time it comes around (resume, rather than resetting), then that would seem to be fundamentally incompatible with parallelism (i.e., if parallelism is the point, you probably aren't using Pfin with a stream anyway).

No, it can be done (example uses no rest for simplicity):


// patterns take two instead of one
(
var halftoneLadder = Pn(Pseries(0, 1, 24)).asStream;
a = Pbind(
	\note, Pseq([0, 1, 2, 3, Pfin(1, halftoneLadder), 3, 2, 1], inf),
	\dur, 0.1
);

Ppar([a, a <> (octave: 6)]).play

)



// both patterns take only one
// (edited, first version was the wrong way round, sorry!)
(
a = 
Plazy({
	var halftoneLadder = Pn(Pseries(0, 1, 24)).asStream;
	{
		Pbind(
			\note, Pseq([0, 1, 2, 3, Pfin(1, halftoneLadder), 3, 2, 1], inf),
			\dur, 0.1
		)
	}
}.value);

Ppar([a, a <> (octave: 6)]).play

)

Currently, using Rest, we can parallelize patterns. The change as you suggest it breaks this assumption. I'd like to have a container for patterns that works like the example above, but is simple to write, and then use that as a blueprint for the partial embedding of rests as well.

What I don't like is the implicit conversion from a stateless to a stateful internal state, with no indication that this is happening from the semantics of Rest (apart from usefulness).

Alternatively, a message like:


iterest { |n|
    Pfin(n, this.iter).collect { |x| Rest(x) }
}

… may do the job?

Your example would then look like this:

// you have to add .asStream
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Pseries(1, 1, 4).iterest * 0.125], inf)
).play;
)

telephon avatar Nov 04 '19 12:11 telephon

Well, I just lost some text because some of the formulations in this thread (streams in looping pattern structures) are actually serious infloop risks. I'll file a bug report about that. EDIT: https://github.com/supercollider/supercollider/issues/4642

Anyway, to rewrite the main points quickly:

  • Your example does convince me that Rests should remain stateless.

  • I don't see a big gain for iterest -- .iter.rest(n) is only two characters more but considerably more readable (among other reasons, for avoiding the appearance of misspelling "interest").

  • I'm quite concerned about the infloops. We don't see it very often because writing a stream into a pattern is currently a rare use case. If it becomes the way to handle interlaced rests, people will do it more often and that will expose the problem. But that's more general than just Rests.

jamshark70 avatar Nov 17 '19 00:11 jamshark70

* If it becomes _the_ way to handle interlaced rests, people will do it more often and that will expose the problem. But that's more general than just Rests.

Maybe it is possible to interlace in a stateless way. I have a number of old experiments, I'll check which worked.

telephon avatar Nov 17 '19 08:11 telephon

I've found a version that allows you to embed a number of items into a parent stream and continue from where you left off. It might be an issue that it holds on old threads and streams (usually Routines), which might be solvable in some way.




Pblock : Pn {

	var streamDict;

	embedInStream { | inval |
		var outval, stream;
		stream = this.getStream;

		repeats.value(inval).do {
			outval = stream.next(inval);
			if(outval.isNil) {
				^inval
			};
			inval = outval.yield;
		};

		^inval

	}

	getStream {
		var stream;
		streamDict = streamDict ?? { IdentityDictionary.new };

		stream = streamDict.at(thisThread);
		if(stream.isNil) {
			stream = pattern.asStream;
			streamDict.put(thisThread, stream);
		};
		^stream
	}



}

+ Pattern {
	block { | n=1 |
		^Pblock(this, n)
	}
}

// a test


(
a = Pblock(Pseq([1, 2, 3], 2), 2);

b = Pseq([a, 30, 40], inf);
c = Pseq([a, 30, 40], inf);


b.asStream.nextN(20).postln;
c.asStream.nextN(20).postln;
"";
)

// returns
[ 1, 2, 30, 40, 3, 1, 30, 40, 2, 3, 30, 40, 30, 40, 30, 40, 30, 40, 30, 40 ]
[ 1, 2, 30, 40, 3, 1, 30, 40, 2, 3, 30, 40, 30, 40, 30, 40, 30, 40, 30, 40 ]


This does not solve https://github.com/supercollider/supercollider/issues/4642.

telephon avatar Nov 17 '19 14:11 telephon

Hm, that's quite nice.

For garbage collection, perhaps the end condition could remove the entry:

	if(outval.isNil) {
		streamDict.removeAt(thisThread);
		^inval
	};

jamshark70 avatar Nov 17 '19 23:11 jamshark70

Yes, that would be a possibility. But then, the blocked pattern would loop forever. But one could keep a set of those threads whose streams have ended. Of course this would keep a reference to that thread. So instead of the thread itself, the identifyHash could be used, and we'd be free of troubles.

telephon avatar Nov 18 '19 11:11 telephon




Pblock : FilterPattern {

	var <>repeats;
	var streamDict, endedStreamThreads;

	*new { | pattern, repeats = 1 |
		^super.newCopyArgs(pattern, repeats).initDict
	}

	initDict {
		streamDict = IdentityDictionary.new;
		endedStreamThreads = IdentitySet.new;
	}

	embedInStream { | inval |
		var outval, hash, stream;

		hash = thisThread.identityHash;
		stream = this.getStream(hash);

		repeats.value(inval).do {
			outval = stream.next(inval);
			if(outval.isNil) {
				this.removeStream(hash);
				^inval
			};
			inval = outval.yield;
		};

		^inval

	}

	getStream { |hash|
		var stream;

		if(endedStreamThreads.includes(hash)) {
			^nil
		};

		stream = streamDict.at(hash);
		if(stream.isNil) {
			stream = pattern.asStream;
			streamDict.put(hash, stream);
		};
		^stream
	}

	removeStream { |hash|
		streamDict.removeAt(hash);
		endedStreamThreads.add(hash);
	}


}


+ Pattern {

	block { | n = 1 |
		^Pblock(this, n)
	}

}

telephon avatar Nov 18 '19 11:11 telephon

This may also grow indefinitely, but uses less memory.

EDIT: identityHash is not safe here really, because different objects may have the same hash. It would be really handy to have a way to reduce this risk. One way in this case could be instVarHash – but that is slow.

telephon avatar Nov 18 '19 11:11 telephon

@jamshark70 - at this point, what amount of discussion is still needed? Or do we feel like we can decide where all of this should move forward?

joshpar avatar Dec 08 '19 19:12 joshpar

(In other words - can we finalize what should be done at this point to move this to a new feature?)

joshpar avatar Dec 08 '19 19:12 joshpar

Thanks for the reminder. I'm just going into a day and a half of workshops, bit busy.

In short, I think we should go with the simpler alternatives for pretty much all the aspects. I think asRest instead of .rest (to avoid rest vs Rest confusion). If users want a persistent stream, they should write it explicitly. This may annoy some users but a/ Julian is correct that naively auto-streaming breaks parallel usage and b/ while his wrapper class is a clever idea, the persistent references will never be deleted because we don't have a stream destructor, and I don't think we should put something into core that we know will prevent temporary streams from being garbage collected. (Realistically, we're not going to add a stream destructor and users wouldn't use it consistently anyway.)

So it would look like Prest(Pseries(...).asStream) or Pseries(...).asStream.asRest.

I think it wouldn't hurt to have a .value call in Rest's value method, to enable a usage like Rest({ rrand(1, 3) }). I can't check right now whether that's already there or not.

jamshark70 avatar Dec 10 '19 00:12 jamshark70

@jamshark70 - do you feel like this is now at a stage for consideration to work on for 3.12?

joshpar avatar Mar 02 '20 05:03 joshpar

do you feel like this is now at a stage for consideration to work on for 3.12?

I think there is some overall consensus on what the feature should look like. For a handful of reasons, including health reasons, I can't commit to write the PR myself at this time. I'd be delighted if other interested parties took it over. Otherwise, I guess we should close this one.

Thanks for checking in.

jamshark70 avatar Mar 04 '20 06:03 jamshark70

Unfortunately I can't follow up on this currently either.

telephon avatar Mar 04 '20 07:03 telephon