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

Move AudioLoader to bitecs #5949

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Move AudioLoader to bitecs #5949

merged 2 commits into from
Mar 16, 2023

Conversation

takahirox
Copy link
Contributor

This PR adds AudioLoader to bitecs.

This PR focuses on just works with minimal change. It may need be rewritten elegantly later.

Rendering audio icon is not supported yet. It is rendered as black for now. Supporting it may require some refactoring and should be in another PR.

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

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

This seems to support audio files that are copy/pasted into the room.

Was this also meant to include code to load audio from gltf/scene files (via a gltfInflator)?

// TODO: VideoTexture.image must be content that be played
// in video-system. And it is also used to render.
// It is audio here so the object will be rendered as
// black. Audio icon must be rendered. Fix this.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it helps, we have icons in the video menu system that can be used as an example for how to do this. You could add one on each side of the entity below (as children) to display an image.

Copy link
Contributor

Choose a reason for hiding this comment

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

a single double sided material would be better. In the old code we use this https://github.com/mozilla/hubs/blob/aaa4584a822545a8ea998c3c14edc62078fb542b/src/components/media-video.js#L30

Ideally we would just do that in an inflator for "audio" and change how we are getting the video element reference so we don't need to do any weird workaround.

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

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

I don't love the workaround for texture, but its shipable. As John mentioned this does not seem to include a loader for the GLTF "audio" component. Similar to others this one should be mapped to something like an "audioLoaderInflator" that expects a src instead of a loaded video element.

export function* loadAudio(world: HubsWorld, url: string) {
const { texture, ratio }: { texture: VideoTexture; ratio: number } = yield loadAudioTexture(url);

// TODO: VideoTexture.image must be content that be played
Copy link
Contributor

Choose a reason for hiding this comment

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

video-system gets the video element off of obj.material.map.image but it doesn't have to, we can store the video element elsewhere so that we don't have to fake the texture like this.

Copy link
Contributor Author

@takahirox takahirox Mar 16, 2023

Choose a reason for hiding this comment

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

video-system gets the video element off of obj.material.map.image

Yes, and it blocked me to write audio loader elegantly and properly.

we can store the video element elsewhere

I think media files to play can be stored in component. I didn't have time to test in this cycle. I want to try in next cycle.

// TODO: VideoTexture.image must be content that be played
// in video-system. And it is also used to render.
// It is audio here so the object will be rendered as
// black. Audio icon must be rendered. Fix this.
Copy link
Contributor

Choose a reason for hiding this comment

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

a single double sided material would be better. In the old code we use this https://github.com/mozilla/hubs/blob/aaa4584a822545a8ea998c3c14edc62078fb542b/src/components/media-video.js#L30

Ideally we would just do that in an inflator for "audio" and change how we are getting the video element reference so we don't need to do any weird workaround.

grabbable={{ cursor: true, hand: false }}
// Audio and Video are handled very similarly in 3D scene
// so create as video
video={{
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we want to change the visual representation it might be nicer to have an audio inflator. It could end up using the MediaVideo component if it needs to, but it could create a different mesh with a static image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could create a different mesh with a static image.

If I'm right, currently the video system is designed that object.material.map is used both for rendering and playing media so it's impossible to render a static image for audio. I didn't want to add workaround in the video system or write a separated audio system (because most of the processing would be like the same), and didn't have time to refactor the design. I want to think of properer design/implementation in the next cycle.

This commit adds AudioLoader to bitecs.

This commit focuses on just works with minimal change. It may
need be rewritten elegantly later.

Rendering audio icon is not supported yet. It is rendered as
black for now. Supporting it may require some refactoring and
should be in another commit.
@takahirox
Copy link
Contributor Author

Was this also meant to include code to load audio from gltf/scene files (via a gltfInflator)?

I was just missing it. I added to gltfInflator. I think perhaps video loader inflator can be reused for now. I haven't tested it tho because I don't have a test asset. I want to test later.

And in the next (or later) cycle I want to refactor the video system and revisit the audio inflators.

@takahirox takahirox merged commit 7ab0ceb into master Mar 16, 2023
@takahirox takahirox deleted the BitECSAudioLoader branch March 16, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants