Make nested playbacks accessible for synced audio #109381
Conversation
| @@ -262,6 +262,13 @@ int AudioStreamPlaybackSynchronized::mix(AudioFrame *p_buffer, float p_rate_scal | |||
| return p_frames; | |||
| } | |||
|
|
|||
| Ref<AudioStreamPlayback> AudioStreamPlaybackSynchronized::get_playback(int p_index) { | |||
| ERR_FAIL_INDEX_V_MSG(p_index, stream->stream_count, 0, "Stream index out of bounds"); | |||
| ERR_FAIL_COND_V_MSG(!playback[p_index].is_valid(), 0, "Stream at index is not valid"); | |||
There was a problem hiding this comment.
| ERR_FAIL_COND_V_MSG(!playback[p_index].is_valid(), 0, "Stream at index is not valid"); | |
| ERR_FAIL_COND_V_MSG(playback[p_index].is_null(), Ref<AudioStreamPlayback>(), "Stream at index is not valid."); |
Please be explicit about return types, this is pretty rudindant though, did you take this from another method? I don't think this is needed it'll just be noisy and return the same value
There was a problem hiding this comment.
Thanks for the review.
Regarding return types: Specifying Ref<AudioStreamPlayback>() makes more sense. I was trying to get a "null object". Providing 0 actually resulted in the same object being constructed, probably due to implicit conversion? Anyway, your version is definitely the cleaner one.
Regarding is_valid() / is_null(): I looked around in AudioStreamPlaybackSynchronized how playback was being used in code and it seems like every time it's being accessed is_valid() is used to check if the reference is valid, that is why I adopted it as an extra check here as well. My guess is that the playback of a sub-stream can fail/break/stop and then might get freed while others work, resulting in gaps, but... that's only a guess. So if you tell me the check actually is redundant I can take it out of the code. What do you think?
There was a problem hiding this comment.
The result should still be checked as it will return a null so using it without a check outside of this will ceash regardless, so id remove it
| @@ -262,6 +262,13 @@ int AudioStreamPlaybackSynchronized::mix(AudioFrame *p_buffer, float p_rate_scal | |||
| return p_frames; | |||
| } | |||
|
|
|||
| Ref<AudioStreamPlayback> AudioStreamPlaybackSynchronized::get_playback(int p_index) { | |||
| ERR_FAIL_INDEX_V_MSG(p_index, stream->stream_count, 0, "Stream index out of bounds"); | |||
There was a problem hiding this comment.
| ERR_FAIL_INDEX_V_MSG(p_index, stream->stream_count, 0, "Stream index out of bounds"); | |
| ERR_FAIL_INDEX_V_MSG(p_index, stream->stream_count, Ref<AudioStreamPlayback>(), "Stream index out of bounds."); |
Add AudioStreamPlaybackSynchronized.get_playback() to make the internal playbacks of AudioStreamPlaybackSynchronized accessible. This can be used to get the playback of an AudioStreamPlaybackInteractive nested inside an AudioStreamPlaybackSynchronized for switching the current clip it is playing. This implements option 1 of godotengine/godot-proposals#12957
sebageek
force-pushed
the
make-nested-playbacks-accessible
branch
from
96269c4 to
80b4cb4
Compare
sebageek
changed the title
Add AudioStreamPlayback.get_playback() to make the internal playbacks of AudioStreamPlaybackSynchronized accessible. This allows us for example to get the playback of a AudioStreamPlaybackInteractive nested inside an AudioStreamPlaybackSynchronized for switching the current clip its playing.
This implements option 1 of godotengine/godot-proposals#12957
Draft of what I was trying out to solve my problem. See godotengine/godot-proposals#12957 for details. I'm not sure if this is the better option of the two.