Change tempo to be in beats per minute by kasbah · Pull Request #1 · freitagbr/step-sequencer

kasbah · 2016-09-21T21:28:44Z

I am using this to make a nice step sequencer out of my old Novation Launchpad. Thanks very much!

To me it makes more sense to have the tempo be beats per minute and independant of the number of divisions, so I adapted it. What do you think?

If you are changing the divisions you could get into a situation where the current step is out of bounds.

freitagbr

Thanks for contributing!

freitagbr · 2016-09-24T17:34:42Z

index.js

 function setSequence(division, sequence) {
  if (typeof division !== 'number') throw new TypeError('Division must be a number');
-  if (!(sequence instanceof Array)) throw new TypeError('Sequence must be an array');
+  if (sequence && !(sequence instanceof Array)) throw new TypeError('Sequence must be an array');


While you're here, let's change sequence instanceof Array to Array.isArray(sequence).

freitagbr · 2016-09-24T17:39:54Z

index.js

 }

 function advance() {
+  if (this.step >= this.division) this.step = 0;


Good catch, I think this makes more sense as well.

Though, I think this should be:

if (this.step >= this.sequence.length) this.step = 0;

Well, I am working under the assumption that the sequence can be empty because that's the way I have been using this. If we init it the way you describe below then it's different.

freitagbr · 2016-09-24T17:59:00Z

index.js

-function calcTimeout(tempo, division) {
-  return Math.floor((60 / tempo * division) * 10e8) + 'n';
+function calcTimeout(tempo) {
+  return Math.floor((60 / tempo) * 10e8) + 'n';


The way I had it initially, you could have multiple subdivisions per beat, i.e. you could say the tempo is 120, and the division is 4, and you would get 4 emits per beat. This is useful if you want eighth, sixteenth, half notes, etc.

This new way only emits once per beat. I guess you could change the tempo to get the effect of having subdivisions, but I would prefer to have subdivisions.

Thoughts?

freitagbr · 2016-09-24T17:59:11Z

index.js

  this.division = division;
  this.sequence = sequence;
-  this.timeout = calcTimeout(this.tempo, this.division)
+  this.timeout = calcTimeout(this.tempo)


freitagbr · 2016-09-24T17:59:18Z

index.js

  this.step = 0;
  this.timer = new NanoTimer();
-  this.timeout = calcTimeout(this.tempo, this.division)
+  this.timeout = calcTimeout(this.tempo)


freitagbr · 2016-09-24T18:05:57Z

index.js


  this.division = division;
-  this.sequence = sequence;
+  this.sequence = sequence || [];


Sure, we could make the sequence optional. But maybe we should default to an Array that is the same length as the number of subdivisions (or maybe 16, typical step-sequencer length)

this.sequence = sequence || Array(divisions); // or this.sequence = sequence || Array(16);

I think I copied the initialization from the constructor.

kasbah · 2016-09-24T19:34:40Z

Thanks for reviewing! There are two features mixed together on this branch now because I have been using my fork in my project and needed all these changes. I should have sent separate PRs from feature branches. Sorry about that.

This could be split into the following, maybe you could comment on both.

Change tempo to always be steps per minute and remove dependency on the number of divisions
Make the sequence argument optional in all cases

To quote what you said about 1. already.

The way I had it initially, you could have multiple subdivisions per beat, i.e. you could say the tempo is 120, and the division is 4, and you would get 4 emits per beat. This is useful if you want eighth, sixteenth, half notes, etc.

This new way only emits once per beat. I guess you could change the tempo to get the effect of having subdivisions, but I would prefer to have subdivisions.

In my step sequencer I have 3 arguments: tempo, numberOfSteps and stepsPerBeat. I am not sure if I could change all three independently if it works the way you are describing. Currently I change the tempo according to the number of beats, like you say:

stepSequencer = new StepSequencer(tempo * stepsPerBeat, numberOfSteps)

If I change one of those variables I do:

stepSequencer.setTempo(tempo * stepsPerBeat)
stepSequencer.setSequence(numberOfSteps)
stepSequencer.removeAllListeners()
registerAllListeners() //loops through range numberOfSteps to register all the event handlers

I found this way easier to reason about and I am still not sure I could do it if tempo was tied to divisions.

kasbah · 2016-09-24T19:52:00Z

Would you be happy to change the constructor to?

StepSequencer(tempo, stepsPerBeat, numberOfSteps [, sequence])

Though I am not quite sure what to do about checking the size of the sequence array if we allow people to setStepsPerBeat and setNumberOfSteps. Frankly I don't understand the use for the sequence array 😆 .

kasbah · 2016-09-29T14:01:39Z

I fixed the semi-colons. Any thoughts on this? If I can't get this merged in some way I'll have to release a fork as I'd like to put my Launchpad step sequencer up on NPM.

freitagbr · 2016-09-29T18:41:48Z

Hey @kasbah, I'll take a look at this when I get home from work. Hopefully we can get this merged in the next day or two.

freitagbr · 2016-09-30T14:43:34Z

Looking at the constructor you posted earlier:

StepSequencer(tempo, stepsPerBeat, numberOfSteps [, sequence])

I like this. stepsPerBeat is more descriptive than division, so let's go with that. numberOfSteps is sort of the same as sequence.length, so let me explain what I was using sequence for.

I wrote this project while trying to create a little step sequencer for a Raspberry Pi/Teenage Engineering Pocket Operator thing. In the sequence array, I stored true (on) or false (off). On your launchpad, true would mean the pad is lit up, false would mean the pad is off. So, creating a 4-on-the-floor beat might look like this:

var sequence = [
    true, false, false, false,
    true, false, false, false,
    true, false, false, false,
    true, false, false, false
];
var sequencer = new StepSequencer(120, 4, 16, sequence);
// On you launchpad:
// [X] [ ] [ ] [ ] [X] [ ] [ ] [ ]
// [X] [ ] [ ] [ ] [X] [ ] [ ] [ ]

Of course, you could keep the sequence outside of the StepSequencer itself, and just let it handle the timing. Anyways, that's what I used sequence for.

Anyways, you can change the constructor as you suggested. There would need to be an assert or something to make sure numberOfSteps === sequence.length.

kasbah · 2016-09-30T16:43:03Z

Yes, that does make sense. In my application I prefer to keep the sequence
outside of this module as I need to change it all the time and it is part
of my application state (which I manage with redux).

We could make use of dynamic typing and make the last argument either
numberOfSteps or sequence so you specify one but never both. What do
you think?

On 30 September 2016 at 15:43, Brandon Freitag notifications@github.com
wrote:

Looking at the constructor you posted earlier:

StepSequencer(tempo, stepsPerBeat, numberOfSteps [, sequence])

I like this. stepsPerBeat is more descriptive than division, so let's go
with that. numberOfSteps is sort of the same as sequence.length, so let
me explain what I was using sequence for.

I wrote this project while trying to create a little step sequencer for a
Raspberry Pi/Teenage Engineering Pocket Operator thing. In the sequence
array, I stored true (on) or false (off). On your launchpad, true would
mean the pad is lit up, false would mean the pad is off. So, creating a
4-on-the-floor beat might look like this:

var sequence = [
true, false, false, false,
true, false, false, false,
true, false, false, false,
true, false, false, false
];var sequencer = new StepSequencer(120, 4, 16, sequence);// On you launchpad:// [X] [ ] [ ] [ ] [X] [ ] [ ] [ ]// [X] [ ] [ ] [ ] [X] [ ] [ ] [ ]

Of course, you could keep the sequence outside of the StepSequencer
itself, and just let it handle the timing. Anyways, that's what I used
sequence for.

Anyways, you can change the constructor as you suggested. There would need
to be an assert or something to make sure numberOfSteps ===
sequence.length.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMoBoRK3NGBf93gu0Z5MSh89b-P4u9zks5qvSAXgaJpZM4KDTIZ
.

freitagbr · 2016-10-01T17:50:35Z

Ok, so if the last argument is a number, treat it as numberOfSteps, but if it is an array, treat it as sequence, deriving the number of steps from the length of the array. Sounds good to me!

kasbah added 6 commits September 21, 2016 22:21

Refactor timeout calculation into function

a5dfeae

Adapt tempo to be beats per minute

7662b4b

Document beats per minute change

eab50f5

Make providing sequence array optional

d9a612d

Make sequence optional in setSequence

a845e19

Change bounds check in advance function

54e0526

If you are changing the divisions you could get into a situation where the current step is out of bounds.

kasbah force-pushed the master branch from 9e3a555 to 54e0526 Compare September 24, 2016 15:55

freitagbr requested changes Sep 24, 2016

View reviewed changes

Add ommited semicolons

0683531

Conversation

kasbah commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freitagbr left a comment

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasbah Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

freitagbr Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

kasbah Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

kasbah commented Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasbah commented Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasbah commented Sep 29, 2016

Uh oh!

freitagbr commented Sep 29, 2016

Uh oh!

freitagbr commented Sep 30, 2016

Uh oh!

kasbah commented Sep 30, 2016

Uh oh!

freitagbr commented Oct 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kasbah commented Sep 21, 2016 •

edited

Loading

freitagbr Sep 24, 2016 •

edited

Loading

kasbah commented Sep 24, 2016 •

edited

Loading

kasbah commented Sep 24, 2016 •

edited

Loading