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.

Sign up for GitHub

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

Jump to bottom

[Audio] Fix pausing stream on entering tree #83779

Merged
merged 1 commit into from
Oct 23, 2023
Merged

[Audio] Fix pausing stream on entering tree #83779

merged 1 commit into from
Oct 23, 2023

Conversation

AThousandShips
Copy link
Member

The paused notifications are only sent when pause status is changed, this ensures that streams that are non-processing do not play when added to the tree. Also ensures that the process_mode property applies generally.

@AThousandShips AThousandShips added bug topic:audio cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Oct 22, 2023
@AThousandShips AThousandShips added this to the 4.2 milestone Oct 22, 2023
@AThousandShips AThousandShips requested review from a team as code owners October 22, 2023 14:35
@AThousandShips
Copy link
Member Author

Still some quirks to work out

@AThousandShips AThousandShips marked this pull request as draft October 22, 2023 14:41
@AThousandShips AThousandShips marked this pull request as ready for review October 22, 2023 14:55
@AThousandShips
Copy link
Member Author

AThousandShips commented Oct 22, 2023 •
edited
Loading

There are some details that don't always work, but that's for a separate workout I think, though I can add some details to that too, currently play overrides the process mode, I'm unsure if this is desired, but leaving at such for now.

Essentially: This just ensures that the pause status of the stream is correctly assigned on entering the tree, but not changing any behavior involving play etc.

KoBeWi
KoBeWi approved these changes Oct 22, 2023
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The stream_paused property has questionable design. It's both user-controlled and depends on whether the node is paused. I think this should be decoupled (not in this PR).

The fix itself looks ok. I'd say the comments aren't really necessary.

@AThousandShips
Copy link
Member Author

I'll remove the comments!

@AThousandShips
[Audio] Fix pausing stream on entering tree
72fccd8 
The paused notifications are only sent when pause status is changed,
this ensures that streams that are non-processing do not play when added
to the tree. Also ensures that the `process_mode` property applies
generally.
@akien-mga akien-mga merged commit 8f16d5f into godotengine:master Oct 23, 2023
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips deleted the audio_pause branch October 23, 2023 11:12
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@theraot theraot theraot approved these changes

Assignees
No one assigned
Labels
bug topic:audio
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Adding the AudioStreamPlayer into the paused tree will let the music play
5 participants
@AThousandShips @akien-mga @YuriSizov @theraot @KoBeWi