-
Notifications
You must be signed in to change notification settings - Fork 2k
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
refine: erase unneed audio operation & fix potential bug #15884
Conversation
cocos/audio/audio-source.ts
Outdated
@@ -38,6 +38,11 @@ enum AudioSourceEventType { | |||
ENDED = 'ended', | |||
} | |||
|
|||
class OperationInfo { |
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.
where we used this class, can we use the OperationInfo
exported from operation-queue.ts
? or should it be an interface ?
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.
Used by _operationsBeforeLoading
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.
AudioSource only needs two variables, a function name and a parameter; it does not require as many parameter requirements as operation-queue.
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.
could it be an interface instead of a class, I see you only use it as an interface, which is only a type
but class will exist in runtime
pal/audio/operation-queue.ts
Outdated
instance._operationQueue.splice(index, 1); | ||
}); | ||
instance._operationQueue.forEach((opInfo): void => { | ||
instance._eventTarget.emit(opInfo.id.toString()); |
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 emit message means the operation is finished, are you sure ?
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 known, what potential problems will it cause?
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.
Currently, only the play interface has a then processing.
this.node?.emit(AudioSourceEventType.STARTED, this);
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.
we only emit the finished message when the operation is finished, see the line no 49
cocos-engine/pal/audio/operation-queue.ts
Lines 46 to 52 in f024d3f
opInfo.func.call(target, ...opInfo.args).then(() => { | |
opInfo.invoking = false; | |
target._operationQueue.shift(); | |
target._eventTarget.emit(opInfo.id.toString()); | |
const nextOpInfo: OperationInfo = target._operationQueue[0]; | |
nextOpInfo && _tryCallingRecursively(target, nextOpInfo); | |
}).catch((e) => {}); |
Otherwise, the call order of the queue will be messed up.
cocos/audio/audio-source.ts
Outdated
this._operationsBeforeLoading.forEach((opInfo): void => { | ||
if (opInfo.op === 'seek') { | ||
this._cachedCurrentTime = opInfo.params as number; | ||
this._player?.seek(this._cachedCurrentTime).catch((e): void => {}); |
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.
_player? -> _player as it is checked in if condition.
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 please fix the ESLint error.
It is better to use state machine for these operations in future. |
Re: # https://github.com/cocos/3d-tasks/issues/7318
Changelog
Continuous Integration
This pull request:
Compatibility Check
This pull request: