mathias / hum

ClojureScript wrappers around the Web Audio API
https://clojars.org/hum
Eclipse Public License 1.0
57 stars 7 forks source link

note-on doesn't work with time parameter #2

Closed graue closed 10 years ago

graue commented 10 years ago

The time parameter doesn't work because time is an array. Here's a fix:

(defn my-note-on [ctx output osc freq & time]
  (let [time (get time 0 (hum/curr-time ctx))]
    (.setValueAtTime (.-frequency osc) freq time)
    (.linearRampToValueAtTime (.-gain output) 1.0 (+ time 0.1))))

This is not the cleanest way to do it, but is the workaround I'm using for today (I'm at a music hackathon).

mathias commented 10 years ago

Ah, right. I missed that since I haven't used note-on with time yet. Good catch.

I don't know whether this is gross or not, but I've seen this sort of thing used for optional parameters with a default in Clojure code:

(defn note-on [ctx output osc freq & {:keys [time] 
    :or {time (curr-time ctx)}}] 
    <fnbody>)

Thoughts?

graue commented 10 years ago

Another option I've seen in the standard library is

(defn note-on
  [ctx output osc freq]
  (note-on ctx output osc freq (curr-time ctx))
  [ctx output osc freq time]
  <oldfnbody>)

but your version might be better, as 5 is a lot of positional parameters... starting to get overwhelming.

Also, you could then have alternative ways of specifying the time, e.g. :in 5 to turn the note on 5 seconds in the future, or :at 300 to turn the note on at time 300 seconds. I think wanting to specify a relative time will be a common use case.

To cut down on the number of parameters, it might be nice to have a default context that can be set and then used for all functions unless you explicitly specify :context other-ctx. (I'm not sure if there is much of any reason to do this in Web Audio?) And then, the 'output' is really only used to be able to set the amplitude, right? So maybe have an oscillator object that includes its own gain. Then note-on might look like:

(deftype HumOsc [osc output])
;; ...maybe a nice constructor for HumOsc ...

(defn note-on [osc freq & {:keys [in at context]
  (let [ctx (or context @default-context)
        time (cond
                at at
                in (+ (curr-time ctx) in)
                :else (curr-time ctx))]
    (.setValueAtTime (.-frequency (.-osc osc)) freq time)
    (.linearRampToValueAtTime (.-gain (.-output osc)) 1.0 (+ time 0.1))))

and you could do:

(note-on my-sine 440 :in 0.5)

This is totally untested. Maybe a more Clojurian(?) way would be to define a protocol with a method to play a note? And, we'll probably eventually want the ramp-up time of 100ms to be changeable.

Anyway, I'll stop and get back to my hackathon project but if these ideas sound cool I'd be happy to help implement them at some point.

graue commented 10 years ago

Just noticed you don't need default-context, as you can get that from the oscillator (or the gain, or any audio node).

(.-context some-audio-node)
mathias commented 10 years ago

I think you're right; I didn't want to force people to have a global context that the library provided, but I didn't like that each function basically needs to take a context (like most of the HTML5 Canvas functions do.) Wasn't aware that we can get the context out of each audio node, though, so that's a good find and definitely will help improve the library.

I will take a look at the existing functions and try to come up with a new version that abstracts away context once it is set. Would you be interested in taking a look at that when I've got it in a working-but-in-flux state?

graue commented 10 years ago

Sounds good!

mathias commented 10 years ago

Here's a branch with the suggested multiple arities and removing the need to pass context to everything:

https://github.com/mathias/hum/commit/d1e0c59700f6d1265b339da0313248b6fa0de0ad

Let me know what you think!

This will likely be hum 0.3.0

Edit: There's a few more commits on that branch that clean things up a bit more and fix an issue I ran into with advanced compilation: https://github.com/mathias/hum/blob/3fe787a938d6153c6dcc91540431a4fc24d1efe6/src/hum/core.cljs

graue commented 10 years ago

ctx-for doesn't have to come first if you do

(declare ctx-for)

before using it.

I don't understand putting ^:export on every function. Other CLJS libraries like Dommy don't do this. Are you trying to compile Hum and the application using it separately, then include them with two separate <script> tags? That's the only situation I can think of where the ^:export would be needed, but that isn't how ClojureScript is designed to be used.

create-gain looks good to me.

I find the time and offset-time positional parameters to note-on and note-off confusing. At first I thought offset-time was your name for my :at feature, i.e., "start this note 5 seconds in the future". I had to read the code carefully to realize that's not the case. This should be a keyword arg in my opinion, maybe :ramp-time. It won't be obvious from reading an invocation of note-on what the fifth arg does.

As a more general question, what's the intended use of Hum? What level of abstraction does note-on lie at? If it's just supposed to make the Web Audio API nicer to use from ClojureScript, then I think a function that both sets an oscillator's frequency and a gain's volume level is doing too much. If it's supposed to give us instruments ("play a D on this instrument"), then I think it does much too little. In my hackathon entry, I implemented note playback with ADSR envelopes using Web Audio directly. It doesn't make sense to build ADSR on top of note-on, but conversely, only having a linear fade-in and fade-out is far too simple of an envelope for most applications. I feel note-on and note-off are kind of an awkward API that won't see much use, but I'm interested to know what you use them for.

mathias commented 10 years ago

You're right, I should probably take out the ^:export metadata. I was running into some issues with using this branch of hum for the hum_demo (because its compilation/deployment is complex and should be fixed) but that should go away once the new version of hum is on clojars.

The answer to what hum was originally designed for: I extracted it out of code I was writing for an HTML5 canvas game written in ClojureScript. While I was using it to synthesize sounds, the player doesn't really get an instrument to play. note-on and note-off were the simplistic functions that allowed the game to make some notes during gameplay, but it really doesn't do anything that advanced or try to sequence anything. That said, I realized the potential to create software instruments with it.

As far as hum's intended usage now, I'd like it to be a low-level library for creating instruments on top of the Web Audio API, with ClojureScript-ian wrappers. I'm not nearly ambitious enough to build something like Overtone for the browser with this.

I think my next step for 0.3.0 is to move to keyword arguments, as you suggested. :ramp-time is probably a better name, in any case, since offset is an ambiguous idea. note-on and note-off may only live on as examples of how to implement such a function.

Your hackathon entry is awesome! I'm glad you were able to use hum.

mathias commented 10 years ago

db9bfaa is slightly better, I think.

Edit: Make that db9bfaa. I missed an unused parameter I was thinking of using, and took it out.

graue commented 10 years ago

Getting better, yeah. A few more suggestions:

You're not using the :as params, so you can take that out. And I think you want ramp-time to be an offset from time, right?

(defn note-on
  ([output osc freq]
     (note-on output osc freq {:time (curr-time (ctx-for osc))}))
  ([output osc freq {:keys [time ramp-time]}]
     (let [ramp-time (or ramp-time 0.1)]
       (.setValueAtTime (.-frequency osc) freq time)
       (.linearRampToValueAtTime (.-gain output) 1.0 (+ time ramp-time)))))

Keyword arguments have the or thing built in, so you can write the function more simply and with only one case like this:

(defn note-on
  [output osc freq {:keys [time ramp-time]
                    :or {time (curr-time (ctx-for osc))
                         ramp-time 0.1}}]
  (.setValueAtTime (.-frequency osc) freq time)
  (.linearRampToValueAtTime (.-gain output) 1.0 (+ time ramp-time))))

Finally, the library coding standards suggest it's more idiomatic to "unroll optional named arguments" (keyword args):

(release-sharks 2 :laser-beams true)    ; good
(release-sharks 2 {:laser-beams true})  ; bad

You can accomplish that by putting a & in front of the keyword args:

...
  [output osc freq & {:keys [time ramp-time]
  ...
graue commented 10 years ago

I'm glad you like the hackathon entry. Hum definitely helped me get started quickly, so thank you!

mathias commented 10 years ago

Updated in https://github.com/mathias/hum/commit/d207fcb6fcbe0772e952ba177a109c82ed172f38

Gonna call it a night for this; if we think this is good, I'll start putting out an 0.3.0 release sometime later this week.

Thanks again! :cake:

graue commented 10 years ago

:+1:

Appreciate your patience with all my nitpicking :)

mathias commented 10 years ago

:thumbsup: 0.3.0 released!

mathias commented 10 years ago

@graue Not to reopen an old thread, but what do you think hum should be? A coworker recently sent me this as an inspiration for possibly starting to tackle SuperCollider/Overtone-like features:

https://github.com/mohayonao/CoffeeCollider

Maybe a library on top of hum could implement more Overtone-like features?

graue commented 10 years ago

Since ClojureScript has excellent JS interop, I'm skeptical of libraries that merely wrap an HTML5 API without truly embracing the way of Clojure. I think you should make something you know is needed. Like your original reason for making it was as a simple synth library for a game. Why not just own that? Be a simple synth library for games. It doesn't make sense to me to make a library unless you've extracted the code from some project that demonstrates real use for that code.

triss commented 9 years ago

I'm thinking removal of note-on and note-off may be a good idea....