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.

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

Overhaul AudioStreamPlayer's documentation #81858

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 18, 2023

Closes #76324.
Documents #71122 's behavior.

Continuation of similar past efforts with the same goal of refreshing the documentation up a notch.

AudioStreamPlayer's reference smelled old. A lot of information is missing. Not everything has to be documented, of course. That would severely impact readability, by making the class look more complicated than it actually is. This is a pretty common class for most users, and we have got to be careful.

PRs for AudioStreamPlayer2D and AudioStreamPlayer3D will come at a later date, after this one is merged.


  • The wording now matches other classes are documented a lot more closely;
  • A lot of missing information is added;
    • Unfortunately, unlike previous PRs, a bit of extra verbosity is necessary;
  • Mention the ability to play more than one sound more clearly thorough the reference;
  • Mention the importance of the stream property;
  • Mention when the finished signal is emitted more explicitly;
    • The previous description failed to explain when this signal was not emitted, as much as it may seem like it should.
  • Add note about MIX_TARGET_STEREO being the default mix target.

Methods

get_playback_position

get_stream_playback

has_stream_playback

  • Simplify when this method returns true;

play

  • Be a little more explicit.
    • The original description quickly talks about the optional parameter. It doesn't make much sense to me, given that it's entirely optional.

seek

  • Be a little more explicit;
  • Imply there can be more than one AudioStreamPlayback (sound).

stop

  • Be a little more explicit.
    • "Stops all audio" is it like... my entire system becomes deaf or something?

Properties

autoplay

  • Be a little more explicit.
    • The previous description was fairly awkward.

bus

  • Simplified the note considerably.
    • What a mouthful... More experienced users already know the bus layout can be changed. It's also overwhelming information for AudioStreamPlayer. It's only necessary to describe the fall back behavior.

max_polyphony

  • Be a little more explicit.

mix_target

  • Shuffled wording around to be clearer;
  • Mention how to get the configured speaker mode yourself.

pitch_scale

  • Add example on how this property affects the audio.

playing

  • Describe what happens when setting this property.

stream

  • Describe what happens when setting this property.

stream_paused

  • Imply there can be more than one AudioStreamPlayback (sound).
  • Mention that this property is changed automatically on a bunch of occasions.
    • I can't find it, but I believe there was an issue about this.

volume_db

  • Be a little more explicit.
    • The concept of audio volume is so widely understood it probably doesn't need further explanations. (such as "Higher values make the sound louder"). This is fine.
  • Add note about the linear/db conversion functions.

I don't think this PR is fully complete. The leading description needs a bit more to it. But, I wanted to express this PR's intentions as soon as possible. Feedback is very, very welcome.

In particular, I attempted to reduce verbosity by replacing "audio" when referring to the individual AudioStreamPlaybacks with "sounds". Saying "playback" all the time does not quite convince me as it's repetitive, and is not something that the common user will ever have to think about, so it's a puzzling situation.

@Mickeon Mickeon requested a review from a team as a code owner September 18, 2023 11:52
@Mickeon Mickeon force-pushed the doc-peeves-that-will-be-merged-in-2030 branch 2 times, most recently from 1aac2a3 to 5e70322 Compare September 18, 2023 11:57
@AThousandShips AThousandShips added this to the 4.x milestone Sep 18, 2023
doc/classes/AudioStreamPlayer.xml Outdated Show resolved Hide resolved
doc/classes/AudioStreamPlayer.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-that-will-be-merged-in-2030 branch from 5e70322 to 184faec Compare September 18, 2023 13:34
doc/classes/AudioStreamPlayer.xml Outdated Show resolved Hide resolved
doc/classes/AudioStreamPlayer.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-that-will-be-merged-in-2030 branch from 184faec to 04e3ae8 Compare September 18, 2023 18:26
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 18, 2023

Updated PR to apply most of the aforementioned suggestions.

@ronyeh
Copy link
Contributor

ronyeh commented Sep 20, 2023

Looks good. Improving documentation is very valuable work, and helps out new developers!

@Mickeon Mickeon force-pushed the doc-peeves-that-will-be-merged-in-2030 branch from 04e3ae8 to 2cd756c Compare December 30, 2023 12:18
@Mickeon Mickeon force-pushed the doc-peeves-that-will-be-merged-in-2030 branch from 2cd756c to 292982a Compare December 30, 2023 12:20
@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 30, 2023

Rebased after minor conflict. Would be nice for this to be looked at sooner than later, so that both AudioStreamPlayer2D and AudioStreamPlayer3D may receive a similar treatment

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 7, 2024
@akien-mga akien-mga merged commit e1050e2 into godotengine:master Apr 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the doc-peeves-that-will-be-merged-in-2030 branch April 8, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioStreamPlayer.get_stream_playback() returns null
7 participants