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

Wrapping PlayControl #3

Open
mdahlgrengadd opened this issue May 11, 2017 · 23 comments
Open

Wrapping PlayControl #3

mdahlgrengadd opened this issue May 11, 2017 · 23 comments

Comments

@mdahlgrengadd
Copy link

Hi! I'm doing sort of a sequencer and wondering if I can add a playControl to a transport that itself is controlled by a "master" play control?:

masterTransport = new wavesAudio.Transport();
masterPlayControl = new wavesAudio.PlayControl(masterTransport);

segmentEngine = new wavesAudio.SegmentEngine();
wrapPlayer = new wavesAudio.PlayControl(segmentEngine);
masterTransport.add(wrapPlayer);

The reason for doing it is so that during playback the wrapPlayer engine can be replaced and playback in the new engine continues from current position (like in the PlayControl example).

@mdahlgrengadd
Copy link
Author

Here is an example of what Im trying to do (Only works in Chrome on Desktop). The waveforms have been split up by chords and played through segment engine.

So it sort of works... the problem is if for instance if looping is set on the wrap Player, then the tempo of the wrapped Player is wrong during first run, after it loops it gets it right.

Another issue is that if I try too loop just part of the audio buffer, it will play that part but then continue in silence for as long as the audio buffer is, ignoring the loop boundaries.

So I'm not sure if the way I wrap play controls this way is supported?

@mdahlgrengadd
Copy link
Author

Here's a codepen showing the speed problem.

http://codepen.io/mdahlgrengadd/pen/Gmxrjr

Will try to do a codepen also showing the second issue.

@NorbertSchnell
Copy link

First of all, apparently there are a couple of bugs (e.g. #2) that we have to fix very rapidly. Thanks – also in advance – for all simplified code examples that help to identify the bugs.
Yes, the PlayControl is itself a TimeEngine that can be added to a Transport. But in fact, I don't think that in the current version a PlayControl inside would loop. However, that could (and probably should) work. We'll look into this.

@NorbertSchnell
Copy link

NorbertSchnell commented May 11, 2017

Hm, thinking about this ones more, I cannot make really sense out of a PlayControl that has its own speed. The whole idea of a speed controlled engine (like a PlayControl added to a Transport) is that its speed (and position) is controlled by the master – and the set speed method of the PlayControl should be disabled (i.e. could throw an exception). This means that the bug is the other way around: the speed should always be the speed of the master. This may raise the question why we created the PlayControl as a TimeEngine that implements the speed-controlled interface. Apart from a portion of sophism, the answer is that the PlayControl could serve as an interface between a TimeEngine that does only implement the transported and/or scheduled interface and a master that only accepts TimeEngines that implement the speed-controlled interface – even there is none that I would know of.
However, I guess it would be interesting if the loop could work anyway.

@NorbertSchnell
Copy link

Ones said the above, I have to admit that there is a potential and unimplemented feature of the Transport – a "stretch" parameter – that would control a relative speed for each individual engine added to a given Transport.

@NorbertSchnell
Copy link

Now, there is another (actually implemented :-) feature that allows for defining an "offset" to the internal position of a TimeEngine added to a Transport. This offset is the 4th argument of Transport.add(engine, startPosition = 0, endPosition = Infinity, offsetPosition = 0). While "startPosition" and "endPosition" place (and cut) the added TimeEngine on the Transport's time line, "offsetPosition" would define the actual position inside the TimeEngine (e.g. the SegmentEngine) according to the following calculation: position = transportPosition - startPosition + offsetPosition. The same parameters can be controlled by the method setBoundaries of the object returned by Transport.add, the "Transported".
The method is defined as Transported.setBoundaries(start, duration, offset = 0, stretch = 1). The arguments correspond to the arguments of add as follows: start = startPosition, duration = endPosition - startPosition, offsetPosition = the TimeEngine's (internal) position offset (stretch does simply not work yet :-).
Each time setBoundaries is called on the Transported, the corresponding TimeEngine added to the Transport receives a call of its syncPosition method. This means that you could implement the jumping from one guitar chord to another by calling Transported.setBoundaries on a single SegmentEngine, instead of switching between SegmentEngines created for each chord.
In the case that not all chord snippets are from the same audio buffer, you could change the buffer (and marker array) before calling setBoundaries.

@NorbertSchnell
Copy link

NorbertSchnell commented May 11, 2017

The second comment of this issue says:

Another issue is that if I try too loop just part of the audio buffer,
it will play that part but then continue in silence for as long as the audio buffer is,
ignoring the loop boundaries.

In fact, it seems to work in the example in the third comment.

The code that makes this work actually looks good.
Can you confirm this issue (appart from what is reported in #2)?

@NorbertSchnell
Copy link

The speed issue is fixed by removing the possibility set the speed of a "wrapped" PlayControl.
The "stretch" parameter could become a feature request for the Transport, if you still need it...

@NorbertSchnell
Copy link

To try to address all questions that have been raised in this issue...
I hope you don't need to wrap the different TimeEngines (SegmentPlayer, GranularPlayer, etc.) into a PlayControl to exchange one with the other with the PlayControl.set method. You also should be able to add and remove the TimeEngines directly to/from the Transport. No?

@mdahlgrengadd
Copy link
Author

mdahlgrengadd commented May 12, 2017

Thanks @NorbertSchnell for superb answer!

Hm, thinking about this ones more, I cannot make really sense out of a PlayControl that has its own speed.

The way I first thought it would work, was as a relative speed to its master (now it sets absolute speed), and that would have been very useful. For example: Currently the segment engines have sample buffers recorded at 60 bpm, If I want to play them together in the same transport with a play engine with sample recorded at say 108 bpm, they would of course be out of sync. Setting speed of a segment engine wrapped in a play control to 1.8 would play them in sync.
The stretch factor of transport add would do the same thing.

The speed issue is fixed by removing the possibility set the speed of a "wrapped" PlayControl.
The "stretch" parameter could become a feature request for the Transport, if you still need it...

Yes. I think it could be very useful, especially if speed will be disabled in play control.

@mdahlgrengadd
Copy link
Author

Another issue is that if I try too loop just part of the audio buffer,
it will play that part but then continue in silence for as long as the audio buffer is,
ignoring the loop boundaries.
In fact, it seems to work in the example in the third comment.

The code that makes this work actually looks good.
Can you confirm this issue (appart from what is reported in #2)?

Sorry my mistake, your right!
Play control was not involved. It was when I used cyclic on a segment engine directly added to transport that I got confused. It "cycles" over the audiobuffer.duration, so if you using it a drum loop its logical. But when I used it to play part of a buffer of chords it would play my chord then be silent until the complete buffer of chord reach its end and then loop. I sort of expected it to loop after it reached the last position of markerBuffer.

With drumloop
http://codepen.io/mdahlgrengadd/pen/dWebxB

With a chord buffer
http://codepen.io/mdahlgrengadd/pen/PmeoGb

@NorbertSchnell
Copy link

In order to clearly define a loop when cyclic is enabled there would have to be duration parameter or a loopStart/End couple. The last marker is not working when marker (i.e. segment) durations are given explicitly.
I guess, here is something slightly incoherent. The PlayControl as a (speed-controlled) TimeEngine has a loop option with start/end, but the GranularEngine and the SegmentEngine have a "cyclic" option.

I can see the following solutions here:

  1. we recommend wrapping a TimeEngine into a PlayControl when looping is required
  2. we replace cyclic by a loop option with start/end points like the PlayControl engine has
  3. we add a cyclic option to the Transport

I don't like (1.) since we end up with a Matryoshka and, in addition, the Transported returned by Transport.add is already almost the same as a PlayControl, but less complex.
Both of the other solution make sense. If we adopt (2.), I would advocate for also moving the "stretch" option from the Transported into the TimeEngines as (relative) speed as you expected it. And also offset has to go here to be clear and still it is unclear how these parameters are articulated with the start and duration of Transported.setBoundaries.
In case of (3.), it would be good to unify the parameters of Transport.add and Transported.setBoundaries. We could deprecate Transported.setBoundaries and replace it by Transported.setSegment(startPosition, endPosition, offset, stretch, cyclic), keeping the same parameters for Transport.add.

Yes, my favorite is (3.). What do you think @mdahlgrengadd?

@mdahlgrengadd
Copy link
Author

mdahlgrengadd commented May 12, 2017

  1. we recommend wrapping a TimeEngine into a PlayControl when looping is required
  2. we replace cyclic by a loop option with start/end points like the PlayControl engine has
  3. we add a cyclic option to the Transport

@NorbertSchnell Well, I totally agree that nr 1 feels strange and since you don't like it, I guess its not an option :-) One benefit is that you can easily change relative speed even after the add to transport. Would that be possible with the other solutions?

Maybe its best I describe what I want to do. The scenario I'm trying to achieve is to have a solo instrument recording "followed" by a rhyhm accompainment. And also, the other way around... to "fit" a solist recording to a rhythm (or another solo performance). Seeking on a master transport should then find the correct positions in both solo and rhythm, and changing master speed would slow down engines so they still sync. To achieve this, engines added to transport would have to have individual "stretch" possibility that can be changed during playback, because solo performance speed would fluctuate over time.

In case of (3.), it would be good to unify the parameters of Transport.add and Transported.setBoundaries. We could deprecate Transported.setBoundaries and replace it by Transported.setSegment(startPosition, endPosition, offset, stretch, cyclic), keeping the same parameters for Transport.add.

So with this option would it be possible to change stretch anytime without "retriggering" segment?

@mdahlgrengadd
Copy link
Author

mdahlgrengadd commented May 12, 2017

Each time setBoundaries is called on the Transported, the corresponding TimeEngine added to the Transport receives a call of its syncPosition method. This means that you could implement the jumping from one guitar chord to another by calling Transported.setBoundaries on a single SegmentEngine, instead of switching between SegmentEngines created for each chord.

Hmm, struggling with this. So do I create i big markerbuffer of all chords?
Perhaps you can give example of how to play the second chord in a series where each have a duration of 12. To make it work I had to do this:

        var obj = this.transport.add(this.transportedSegmentEngine);
        obj.setBoundaries(0, 12, -12.01,  1);

The decimal was required because otherwise there was a double triggering of chords (both previous segment and current I think). Also negative offset? Is this correct? (Not tried the devel branch, though)

@NorbertSchnell
Copy link

Would you mind putting an example of this on codepen?

@mdahlgrengadd
Copy link
Author

Sorry, of course!

http://codepen.io/mdahlgrengadd/pen/NjMNJb

In this example the "decimal" thing wont help, because then I would need to add an extra segment with 0 duration at each new chord position in the markerBuffer (so this sounds like its the already mention "segment 1-2"-bug.) You can also hear a "retriggering" of a segment when seeking, maybe that is also fixed in dev-branch. Will try soon.

@NorbertSchnell
Copy link

Ouch, this is my double fault: 1: I got the design wrong, 2: I forgot about it...
While, as stated above, startPosition and endPosition determine the position on the Transport's time-line where it sounds, other than stated above, offset is the TimeEngine's position on the Transport's time-line. So, to play the second chord at position 0 instead of 12, you have to shift the TimeEngine 12s to the left, which is -12.
This is not great but might be ok if nobody tells you contrary. However, it will become a little complicated when we introduce the stretch parameter.
We'll see how we can fix that... We might introduce names parameters as an object with key names as an alternative to the current positional parameters, which also allows for start/end or start/duration.

@mdahlgrengadd
Copy link
Author

@NorbertSchnell
Tested the develop branch and segment engine is working as expected! Many thanks!

Just one question :-) I'm using your suggestion of moving the segment engine on the transport with the transported object returned from transport.add(). Since the segment engine has to be ready with the next chord on correct beat (or it will miss triggering the first segment), I move the engine after the last played segment. If I dont add a little "headroom" to the segment engine boundary, it will stop...

transportedObject.setBoundaries(pos, 12, -rand * 12, 1); // Dont work, stops at next chord.
transportedObject.setBoundaries(pos, 14, -rand * 12, 1); // Works!

Heres a pen:
http://codepen.io/mdahlgrengadd/pen/xdjRey

I know its naive implementation, but still don't understand why the extra "headroom" is needed?

@NorbertSchnell
Copy link

Well, you are abusing :-) positionDisplay.syncPosition, which is called in series with the syncPosition method of your SegmentEngine to call setBoundaries, which will cause another syncPosition of your SegmentEngine – just before or after. The expected behaviour is undefined and I am not surprised that this actually doesn't work (even if it could by accident).
I'd really recommend to schedule the chord change with a TimeEngine very similar to the positionDisplay, just that would return 11.9 in its syncPosition (12 doesn't really exist) and set the next boundaries in the advancePosition method (hoping that this works :-).

Even better would be to create an extension of the SegmentEngine that reimplements its syncPosition method to generate the offset and use it in seeking by calling super.syncPosition. In addition, the extension would reimplement advancePosition and add the offset before calling super.advancePosition.
If in addition you take the Transport's position coming with the syncPosition/advancePosition in the extended SegmentEngine with a modulo 12 you would not have to loop the Transport. This would be the elegant way to go.

(P.S.: Referring to the comments I just deleted: sorry, I did not open the right example.)

@mdahlgrengadd
Copy link
Author

Well, you are abusing :-) positionDisplay.syncPosition, which is called in series with the syncPosition method of your SegmentEngine to call setBoundaries

Ok, sorry! this was only for looping to "work". You can remove those lines in syncPosition, the other problem still exist.

Look at line 91-92 in this updated code pen:

http://codepen.io/mdahlgrengadd/pen/aWGaKm

Anyway, I will code it like you suggest instead. Just thought this could be something wrong in segment engine.

@mdahlgrengadd
Copy link
Author

mdahlgrengadd commented May 15, 2017

Apologies for not understanding the code @NorbertSchnell.

I managed to bypass ( or hack :-) the unexpected halting of segment engine on transport doing this:

var transportedObject = transport.add(segmentEngine, 99999);

transportedObject.advancePosition = function(time, position, speed) {
        position = this.__offsetPosition + this.__engine.advancePosition(time, position - this.__offsetPosition, speed);

        if (speed > 0 && position < this.__endPosition || speed < 0 && position >= this.__startPosition)
            return position;
      
        // Returning Infinite causes a halt, again trying a big number instead.
        return 99999;//Infinity * speed;
}.bind(transportedObject);

So now it acts as more or less like how I expect (not the same as correct, though).

Here's a codepen with a "sequencer" subclassed from segment engine doing this :-)
http://codepen.io/mdahlgrengadd/pen/RVJxdx

@NorbertSchnell
Copy link

Look at line 91-92 in this updated code pen:
http://codepen.io/mdahlgrengadd/pen/aWGaKm
Anyway, I will code it like you suggest instead. Just thought this could be something wrong in segment engine.

No, no, the problem is clearly not in the SegmentEngine, but somewhere in the Transport. Its related to a confusion between jumping back and stopping. Stopping would be ok if it restarted right away when the position loops back – which it doesn't.
Anyway, I have a hard time to figure this out, for the moment, but it's clearly a bug to be fixed (we can leave it associated to this issue).

Given that you are any way jumping back, the 14 is ok, but 12.00001 is also enough. The most important is not to stop before the jump and an elegant value should be Infinity.

@NorbertSchnell
Copy link

I managed to bypass ( or hack :-) the unexpected halting of segment engine on transport doing this...

Hm, not a good hack. I'll try to make a proposition in code next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants