-
Notifications
You must be signed in to change notification settings - Fork 85
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
Prevent external audio stopping #25
Conversation
@@ -40,12 +40,6 @@ class AudioPlayerManager { | |||
for _ in 0..<AudioPlayerManager.defaultStartingPlayerCount { | |||
players.append(createNewPlayerAttachedToEngine()) | |||
} | |||
|
|||
do { | |||
try engine.start() |
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.
If you remove the engine.start, it can cause a lag on slower devices that will make the sound play unsynchronized. However, engine.start to mute the other sound inputs, instead of engine.start, engine.prepare() should be called.
|
||
player.play(audio, identifier: sound) | ||
do { | ||
engine.connect(player.node, to: engine.mainMixerNode, format: nil) |
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.
connecting the player to the engine before playing the sound could cause sound delay.
@@ -144,7 +145,9 @@ class AudioPlayerManager { | |||
private func createNewPlayerAttachedToEngine() -> AudioPlayer { | |||
let player = AudioPlayer() | |||
engine.attach(player.node) | |||
engine.connect(player.node, to: engine.mainMixerNode, format: nil) | |||
// according to this: https://stackoverflow.com/a/55505717/3687801, we will defer connecting nodes to engine up until play command of player node |
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.
not a good idea to remove the connection to engine from this method. It can cause sound delay.
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.
There are a lot of changes for the problem that could be easily fixed on the client-side, however, some remarks have their place.
Some solutions are not thought through to the end and do not consider the entire architecture of the project.
|
||
/// Pause sound(s) | ||
func pauseAllSound() { | ||
audioPlayerManager.pauseAll() |
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.
Since sound playback is optional, it should be guarded first, check if the sound is playable or not before calling the pause method. rename the method so that it is immediately clear that it is optional.
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.
Hmm. I guess I got your point. Let's revisit this when I've spare time.
@@ -415,7 +415,9 @@ public extension SwiftFortuneWheel { | |||
self.animator.addRotationAnimation(fullRotationsCount: 0, | |||
animationDuration: animationDuration, | |||
rotationOffset: rotation, | |||
completionBlock: nil) | |||
completionBlock: { _ in | |||
self.pauseAllSound() |
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.
Calling pauseAllSound after the rotation animation stops is a bad idea. Same audio effects could have more than 1-second duration, and stopping all sound will cut the sound instantly, making sound playback unnatural.
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'll come up with other suggestion if there is any from my side
Closing this based on the discussion of #24 |
This PR aims to fix #24