Skip to content

Conversation

@ngokevin
Copy link
Contributor

@ngokevin ngokevin commented Oct 12, 2018

The last audio buffer source node property to support, next to playbackRate.

https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/detune

[page:Audio.pause pause]() etc. methods. Default is *true*.</p>

<h3>[property:Number detune]</h3>
<p>Modify pitch. Default is *0*.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would add the metric which is cent.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 12, 2018

@mrdoob Should we provide .getDetune() and .setDetune() for consistency reasons?

@mrdoob
Copy link
Owner

mrdoob commented Oct 12, 2018

@Mugen87 I think so 👌

@mrdoob mrdoob added this to the r98 milestone Oct 12, 2018
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 14, 2018

Okay, then let's add both methods to this PR.

@mrdoob
Copy link
Owner

mrdoob commented Oct 15, 2018

@ngokevin could you address @Mugen87's comments?

@ngokevin
Copy link
Contributor Author

Updated.


if ( this.isPlaying === true ) {

this.source.detune.setValueAtTime( this.detune, this.context.currentTime );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a special reason for using setValueAtTime()? We normally use setTargetAtTime(), see #11133 (comment)

this.source.detune.setTargetAtTime( this.detune, this.context.currentTime, 0.01 );

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add both new methods to the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the playbackRate code, but I can change both.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that'd be great 😊

@mrdoob mrdoob modified the milestones: r98, r99 Oct 31, 2018
@mrdoob mrdoob closed this Nov 29, 2018
@mrdoob mrdoob removed this from the r99 milestone Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants