Skip to content

Fix AudioStreamPlayer.play() ignores get_tree().paused#114764

Open
neclor wants to merge 2 commits intogodotengine:masterfrom
neclor:audio-stream-player-paused-fix
Open

Fix AudioStreamPlayer.play() ignores get_tree().paused#114764
neclor wants to merge 2 commits intogodotengine:masterfrom
neclor:audio-stream-player-paused-fix

Conversation

@neclor
Copy link
Copy Markdown

@neclor neclor commented Jan 8, 2026

Problem

AudioStreamPlayerInternal::play_basic() does not respect Node pause mode.
As a result, a AudioStreamPlayer can start playback while the SceneTree is paused.

This change prevents play from starting when node->can_process() is false.

Issue

Fixes #114694

@neclor neclor requested a review from a team as a code owner January 8, 2026 22:29
@AThousandShips AThousandShips added bug topic:audio cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release cherrypick:4.6 Considered for cherry-picking into a future 4.6.x release labels Jan 9, 2026
@AThousandShips AThousandShips added this to the 4.7 milestone Jan 9, 2026
@wlsnmrk
Copy link
Copy Markdown
Contributor

wlsnmrk commented Jan 16, 2026

Thanks for putting this fix together!

A suggestion: it might be preferable, instead of returning early, to let the playback construction proceed and then call set_stream_paused(!node->can_process()) at the end of play_basic(), before the return.

Advantages:

  • It leaves the playback queued for when the tree resumes, instead of discarding the call to play_basic()
  • It's analogous to the behavior in notification() at line 97
  • I believe it would fix the TODO in set_stream_paused() at line 178 (181 in your modified version), which seems likely to be the root of AudioStreamPlayer.play() ignores get_tree().paused #114694

@neclor
Copy link
Copy Markdown
Author

neclor commented Jan 17, 2026

You're right, your suggestion makes more sense.

I noticed that VideoStreamPlayer handles this by manually triggering the pause notification:

void VideoStreamPlayer::play() {
	//...
	if (!can_process()) {
		_notification(NOTIFICATION_PAUSED);
	}
}

I think we should follow the same pattern in AudioStreamPlayerInternal
Something like:

if (!node->can_process()) {
	notification(Node::NOTIFICATION_PAUSED);
}

@neclor
Copy link
Copy Markdown
Author

neclor commented Jan 17, 2026

I don't think this will resolve the TODO in set_stream_paused(). To fix it, we would need a new variable to save the pause state even when there is no stream.

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

Labels

bug cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release cherrypick:4.6 Considered for cherry-picking into a future 4.6.x release topic:audio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AudioStreamPlayer.play() ignores get_tree().paused

3 participants