-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Updated the soundLoop.interval #458
base: main
Are you sure you want to change the base?
Conversation
1150f5f
to
ec95c08
Compare
@therewasaguy Kindly, please review this PR. |
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.
looks good, thank you! Just requesting some small changes.
|
||
let desiredInterval = 0; | ||
|
||
SoundLoop._interval = max(desiredInterval, 0.01); |
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.
hrm, SoundLoop
doesn't exist, instead we have a function called p5.SoundLoop that exists on the p5 object.
We don't want to set _interval on the class, but not on the instance (which is represented by this
).
That is happening on line 66. So we should do this check on that line.
@@ -65,6 +65,12 @@ define(function (require) { | |||
|
|||
this._interval = interval || 1; |
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.
can we do the check on this line? this._interval = Math.max(interval, 0.01)
? And then, the question is how can we set the default? We could set the default in the constructor function on line 57, by saying interval = 1
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters
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.
@parulpriyedarshani the code is iterated a lot in the past year, I am not sure if the soundLoop.interval problem still exists or not, can you please look into this once? due to branch change it has some merge conflicts too that need to be addressed
Solves issue #313
Initialized the desiredInterval to zero and set the soundLoop interval of 0.