-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add xod/waves library #2000
Add xod/waves library #2000
Conversation
I still suggest that the zero value of the period should not cause an error. It is also confusing to use the future tense in the pin descriptions of some nodes. But, in eneral, everything is ok. |
How do you think the node should behave in this case? |
Depending on the node. But, theoretically, if the distance between the extreme vertices of the graph tends to 0, the graph turns into a straight line, output = 1, or true. |
emitValue<output_OUT>(ctx, true); | ||
} else if (!enabled && state->wasEnabled) { | ||
// just paused | ||
// TODO: we can get rid of storing nextSwitchTime if API would |
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.
Sounds good and not complex to implement. How about doing it a part of this PR and remove a TODO?
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.
This particular implementation is just a copy of xod/core/square-wave
.
I'm not sure about growing the API for a single use-case. Do you have some examples where it could also be useful?
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.
Suggest leaving it as is for now to limit the PR scope
|
||
void evaluate(Context ctx) { | ||
auto fractionalPart = fmod(getValue<input_X>(ctx), 1); | ||
auto peakPosition = constrain(getValue<input_PP>(ctx), 0.0, 1.0); |
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.
I got an error in simulation:
error: use of undeclared identifier 'constrain'
auto peakPosition = constrain(getValue<input_PP>(ctx), 0.0, 1.0);
^
1 error generated.
So you probably have to use conditions or make a patch to our wasm package with the implementation of this function (and pray to God that every Arduino package developers add it into core.h
😄 ).
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.
and pray to God that every Arduino package developers add it into core.h 😄
I'm pretty sure they do. It's not like it's some obscure undocumented function: https://www.arduino.cc/reference/en/#functions
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.
Like it 🤗
A few thoughts though:
- Would it be more robust if all waves (except
square
) outputφ
(phase) instead ofN
? If one needs integer, he can triviallyxod/math/floor
. - The
variable-triangle-wave
is cool but somewhat inconsistent in naming:variable-tri-wave
… What if it would be named assaw-wave
withPP
set to0.5
by default. Well, it’s a saw, the saw might be customized.
BTW, what about replacing usage of the deprecated nodes for the new ones in the standard library ( |
and deprecate *-wave nodes from xod/core: xod/core/saw-wave xod/core/saw-wave-map xod/core/sine-wave xod/core/sine-wave-map xod/core/square-wave xod/core/tri-wave xod/core/tri-wave-map
2102f58
to
d5c0cd7
Compare
and deprecate old *-wave nodes from xod/core:
xod/core/saw-wave
xod/core/saw-wave-map
xod/core/sine-wave
xod/core/sine-wave-map
xod/core/square-wave
xod/core/tri-wave
xod/core/tri-wave-map