-
Notifications
You must be signed in to change notification settings - Fork 522
Feature: KHR_audio_emitter #835
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
Conversation
04f3ca6 to
b9729a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 17 out of 26 changed files in this pull request and generated 1 comment.
Files not reviewed (9)
- Runtime/Plugins/GLTFSerialization/Extensions/KHR_audio_emitter.cs.meta: Language not supported
- Runtime/Plugins/GLTFSerialization/Extensions/KHR_audio_emitterFactory.cs.meta: Language not supported
- Runtime/Scripts/Plugins/Audio.meta: Language not supported
- Runtime/Scripts/Plugins/Audio/TempAssignClip.cs.meta: Language not supported
- Runtime/Scripts/Plugins/AudioExport.cs.meta: Language not supported
- Runtime/Scripts/Plugins/AudioImport.cs.meta: Language not supported
- Samples~/KHR_audio.meta: Language not supported
- Samples~/KHR_audio/AudioSourceScriptableObject.cs.meta: Language not supported
- Samples~/KHR_audio/KHRAudioSchemas.cs.meta: Language not supported
Comments suppressed due to low confidence (2)
Runtime/Scripts/Plugins/AudioExport.cs:150
- The property 'Name' is used in the KHR_AudioSource initialization. Please ensure that the KHR_AudioSource schema has a corresponding property definition for 'Name' or remove it if not required.
Name = audioSourceId.Clip.name
Runtime/Scripts/Plugins/AudioExport.cs:185
- The variable 'ermitterId' appears to be a misspelling of 'emitterId'. Renaming it will improve code readability and consistency.
var ermitterId = ProcessAudioSource(false, positionalSources.ToArray(), exporter, gltfRoot);
| var audioClipRequest = UnityWebRequestMultimedia.GetAudioClip(tempFile, GetAudioTypeForMimeType(audio.mimeType)); | ||
| audioClipRequest.SendWebRequest(); | ||
| while (!audioClipRequest.isDone) | ||
| { | ||
| // Wait for the request to complete | ||
| } | ||
|
|
||
| if (audioClipRequest.result != UnityWebRequest.Result.Success) | ||
| { | ||
| Debug.LogError($"Cannot load audio clip for mimeType {audio.mimeType}: {audioClipRequest.error}"); | ||
| continue; | ||
| } | ||
|
|
||
| AudioClip clip = DownloadHandlerAudioClip.GetContent(audioClipRequest); | ||
| if (clip == null || clip.samples == 0) | ||
| { | ||
| Debug.LogError($"Cannot load audio clip for mimeType {audio.mimeType}"); | ||
| continue; | ||
| } | ||
|
|
||
| clip.name = $"audio_{index:D3}"; | ||
|
|
||
| _audioClips.Add(index, clip); |
Copilot
AI
Apr 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a busy-wait loop to wait for the UnityWebRequest to complete may block the main thread. Consider using a coroutine or async/await pattern to handle asynchronous operations more efficiently.
| var audioClipRequest = UnityWebRequestMultimedia.GetAudioClip(tempFile, GetAudioTypeForMimeType(audio.mimeType)); | |
| audioClipRequest.SendWebRequest(); | |
| while (!audioClipRequest.isDone) | |
| { | |
| // Wait for the request to complete | |
| } | |
| if (audioClipRequest.result != UnityWebRequest.Result.Success) | |
| { | |
| Debug.LogError($"Cannot load audio clip for mimeType {audio.mimeType}: {audioClipRequest.error}"); | |
| continue; | |
| } | |
| AudioClip clip = DownloadHandlerAudioClip.GetContent(audioClipRequest); | |
| if (clip == null || clip.samples == 0) | |
| { | |
| Debug.LogError($"Cannot load audio clip for mimeType {audio.mimeType}"); | |
| continue; | |
| } | |
| clip.name = $"audio_{index:D3}"; | |
| _audioClips.Add(index, clip); | |
| StartCoroutine(LoadAudioClipCoroutine(tempFile, audio.mimeType, index)); |
b9729a1 to
638db77
Compare
638db77 to
071063d
Compare
Added support for KHR_audio_emitter extension
Based on glTF PR
Spec > Readme
Current Features:
Supported Parameters:
Work in Progress: