Skip to content
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

Feat: setup audio component #1587

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Feat: setup audio component #1587

wants to merge 21 commits into from

Conversation

JujieX
Copy link
Member

@JujieX JujieX commented Jun 3, 2023

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

setup audio feature
UML API design
image

Other information:

Playground: galacean/galacean.github.io#752

@JujieX JujieX marked this pull request as draft June 3, 2023 12:05
@JujieX JujieX self-assigned this Jun 6, 2023
@JujieX JujieX added this to the 1.2 milestone Jun 6, 2023
@JujieX JujieX added the engine label Jun 6, 2023
return new AssetPromise((resolve) => {
this.request<ArrayBuffer>(item.url, { type: "arraybuffer" }).then((arrayBuffer) => {
AudioManager.context.decodeAudioData(arrayBuffer).then((result) => {
resolve(result);
Copy link
Collaborator

Choose a reason for hiding this comment

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

reject

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

/** play the sound from the very beginning */
public play() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete public

Copy link
Collaborator

Choose a reason for hiding this comment

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

: void

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

/** Current playback progress, in seconds */
get position(): number {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Position?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to "time"

packages/core/src/audio/AudioSource.ts Show resolved Hide resolved
* Plays the clip.
*/
play(): void {
if (!this._clip || !this.clip.duration || this.isPlaying) return;
Copy link
Member

Choose a reason for hiding this comment

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

this.clip.duration why not this._clip.duration?

if (this.startTime > this._clip.duration || this.startTime < 0) return;
if (this._duration && this._duration < 0) return;

this._pausedTime = null;
Copy link
Member

Choose a reason for hiding this comment

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

_pausedTime type is number

@deepClone
private _endTime: number = null;
@deepClone
private _duration: number = null;
Copy link
Member

Choose a reason for hiding this comment

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

number init null is not good

Copy link
Member

Choose a reason for hiding this comment

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

+1

@GuoLei1990 GuoLei1990 added the enhancement New feature or request label Jun 17, 2023
/**
* the number of discrete audio channels
*/
get channels(): Readonly<number> {
Copy link
Member

Choose a reason for hiding this comment

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

get channels(): number is OK

private _audioBuffer: AudioBuffer;

/**
* the number of discrete audio channels
Copy link
Member

@GuoLei1990 GuoLei1990 Jun 17, 2023

Choose a reason for hiding this comment

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

  • Comments need to add a period,param comments do not need
  • Capitalize the first letter

Copy link
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

Unit test must add!

/**
* get the clip's audio buffer
*/
getData(): AudioBuffer {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid web native interfaces as much as possible

Copy link
Member Author

Choose a reason for hiding this comment

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

I think getData and setData is a must

@@ -0,0 +1,4 @@
export { AudioManager } from "./AudioManager";
Copy link
Member

Choose a reason for hiding this comment

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

AudioManager is internal

Copy link
Member Author

Choose a reason for hiding this comment

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

no export now

@GuoLei1990 GuoLei1990 removed the engine label Jun 17, 2023
super(entity);
const gain = AudioManager.context.createGain();
gain.connect(AudioManager.context.destination);
AudioManager.listener = gain;
Copy link
Member

Choose a reason for hiding this comment

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

Only allowed one listener?

constructor(entity: Entity) {
super(entity);
const gain = AudioManager.context.createGain();
gain.connect(AudioManager.context.destination);
Copy link
Member

Choose a reason for hiding this comment

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

should be encapsulated

AudioManager._context = new window.AudioContext();
}
if (AudioManager._context.state !== "running") {
window.document.addEventListener("pointerdown", AudioManager._unlock, true);
Copy link
Member

Choose a reason for hiding this comment

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

Should do this when call context every time?

return new AssetPromise((resolve, reject) => {
this.request<ArrayBuffer>(item.url, { type: "arraybuffer" }).then((arrayBuffer) => {
AudioManager.context
.decodeAudioData(arrayBuffer)
Copy link
Member

Choose a reason for hiding this comment

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

should do some encapsulation

Copy link
Member Author

Choose a reason for hiding this comment

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

now it loads onto audioClip, not audio buffer

const duration = this.endTime ? this.endTime - this._pausedTime : null;
this._play(this._pausedTime, duration);
}
}
Copy link
Member

@GuoLei1990 GuoLei1990 Jun 17, 2023

Choose a reason for hiding this comment

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

What is the unique value that is different from play from the perspective of usage

if (this.isPlaying) {
return this._pausedTime
? this.engine.time.elapsedTime - this._absoluteStartTime + this._pausedTime
: this.engine.time.elapsedTime - this._absoluteStartTime + this.startTime;
Copy link
Member

Choose a reason for hiding this comment

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

looks a bit complicated

@deepClone
private _endTime: number = null;
@deepClone
private _duration: number = null;
Copy link
Member

Choose a reason for hiding this comment

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

+1

/**
* Stops playing the clip.
*/
stop(): void {
Copy link
Member

Choose a reason for hiding this comment

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

What is the unique value that is different from pause from the perspective of usage? It's call play form 0 next time?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

now only has play stop and pause, removed unpause

return this._playbackRate;
}

set playbackRate(value: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Your comments use speed, API use rate,try to use the unified concept

Copy link
Member Author

Choose a reason for hiding this comment

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

use rate

set endTime(value: number) {
this._endTime = value;
this._duration = this._endTime - this._startTime;
}
Copy link
Member

Choose a reason for hiding this comment

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

You mean clip start and end time?

@GuoLei1990 GuoLei1990 marked this pull request as ready for review June 19, 2023 06:16
@GuoLei1990 GuoLei1990 changed the base branch from dev/1.1 to main September 15, 2023 09:34
@GuoLei1990 GuoLei1990 added the high priority High priority issue label Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio enhancement New feature or request high priority High priority issue
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants