-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adds some new nodes #49
base: develop
Are you sure you want to change the base?
Conversation
I created something similar to
|
@tamlyn Very interesting! I wondered if it was possible, thanks for that. It's up to @nick-thompson if this is useful enough then. I don't mind removing any parts of it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakeCoxon excellent PR thank you!! I left two small comments inline, but besides that I'm more than happy to merge this as is. Let me know your thoughts on the comments.
And to @tamlyn's (great!) point,
const counter = el.accum(el.and(el.eq(el.z(input), 0), el.eq(input, 1)), reset)
Indeed yea you can do this one in user-space. My intention as the core library grows is to do as many things as possible in user space (or, in the js frontend), and only commit new native nodes when necessary. But that idea makes a known tradeoff in favor of composition, readability, maintainability at the cost of (sometimes) known CPU overhead. In this case I think it's a fair bet that the user-space implementation of counter2 would have relatively high overhead compared to the native version, so especially since @JakeCoxon already wrote the native code I'm happy to take it upstream.
|
||
// When the gate is high, count once | ||
if ((FloatType(1.0) - in) <= std::numeric_limits<FloatType>::epsilon()) { | ||
if (!risingEdge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm following this right, any pulse that remains high for > 3 samples will record more than one count– the first 1.0 will count++ and set rising edge = true, the second 1.0 will set rising edge = false, the third 1.0 will set count++ and reset rising edge = true. So I think if this is meant to only count rising edges it will produce the wrong value.
There's a utility that you'll see used in this Core file called Change
which reports the direction of signal change. With that you could do something like if (floatEqual(FloatType(1.0), in) && change(in) > FloatType(0))
which would be true when the input signal is high and the direction of change from last sample to this sample is also high (i.e. rising edge). That change()
won't report another > 0 until the input signal goes low and then high again.
* | ||
* Middle C4 is MIDI note 60, which corresponds to 261.63 Hz. | ||
*/ | ||
export function midi2frequency(midi: ElemNode): NodeRepr_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpick, but I'd prefer midi2freq
and freq2midi
here just for brevity. When composing larger graphs I find short (but understandable) names to be helpful
Any thoughts or changes please let me know