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

KHR_audio_emitter: Add pitchSpeed and make audio source's data optional #239

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Dec 5, 2024

This PR has 2 changes in it:

  • Add a new "pitchSpeed" property to audio sources.
    • I swear we discussed this before in Discord meetings, but I guess we forgot about it?
    • It was suggested here: KHR_audio_emitter KhronosGroup/glTF#2137 (comment)
    • I would personally find this field useful, and it's supported in Godot and WebAudio.
    • Having this property aligns with WebAudio in all but name, which calls it playbackRate: https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/playbackRate
    • I don't really care what name we use. I do think having it contain the word "pitch" is valuable because users who aren't familiar with audio may expect it to resample and don't understand why changing the playback rate would "cause the pitch to change", but even then I'm good with whatever consensus we bikeshed over this.
  • Make an audio source's audio data field optional instead of required. Two reasons:
    • This technically breaks extensions such as "OMI_audio_ogg_vorbis" because they provide their own audio data separately in a new field. But if the built-in field is empty, it's technically invalid with the spec in master.
    • Making audio data mandatory means it's not possible to define gain or pitchSpeed or autoPlay on an emitter, because it's not allowed to make a placeholder audio source with no data.
    • Making this property optional fixes both of these problems.

@yankscally yankscally self-requested a review January 3, 2025 16:26
Copy link

@yankscally yankscally left a comment

Choose a reason for hiding this comment

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

pitchSpeed... in one way, the name is illogical, but in another way, I can eyeball it and know what it means - unlike playbackRate. In the audio realm, this has many names also, so there is no concrete naming scheme for this, it's usually whatever makes the most sense in that context.

Approved.

@aaronfranke aaronfranke merged commit b48a264 into omigroup:main Jan 3, 2025
2 checks passed
@aaronfranke aaronfranke deleted the audio-pitch-speed branch January 3, 2025 16:52
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.

2 participants