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

Fix audio engine init issue #15922

Merged

Conversation

docEdub
Copy link
Contributor

@docEdub docEdub commented Dec 2, 2024

When the Engine constructor is called with a WebGLRenderingContext or WebGL2RenderingContext, the audio engine does not get initialized. This change fixes the issue by calling Engine._sharedInit with the WebGL rendering context's canvas.

Note that the WebGL rendering context's canvas may be an OffscreenCanvas and its type is cast to HTMLCanvasElement for the call to Engine._sharedInit because it would be a large change to make AbstractEngine._renderingCanvas support both types. This is done the same way in the WebGPUEngine constructor, with the only consequence for the audio engine being that the mute button stays at the top-left of the document instead of automatically repositioning to the top-left of the canvas.

When the `Engine` constructor is called with a `WebGLRenderingContext` or `WebGL2RenderingContext`, the audio engine does not get initialized. This change fixes the issue by calling `Engine._sharedInit` with the WebGL rendering context's canvas.

Note that the WebGL rendering context's canvas may be an `OffscreenCanvas` and its type is cast to `HTMLCanvasElement` for the call to `Engine._sharedInit` because it would be a large change to make `AbstractEngine.__renderingCanvas` support both types. This is done the same way in the `WebGPUEngine` constructor, with the only consequence for the audio engine being that the mute button stays at the top-left of the document instead of automatically repositioning to the top-left of the canvas.
@docEdub docEdub added the bug label Dec 2, 2024
@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@sebavan
Copy link
Member

sebavan commented Dec 3, 2024

I wonder if instead we could change a tiny bit how it is done ?

In thinEngine, lets remove the canvas assignement in the constructor:
image

And called sharedInit instead:
image

Then in Engine we should be able to remove the _sharedInit call:
image

As well as in WebGPUEngine:
image

I guess this should ensure that _sharedInit would be call consistently ?

@docEdub
Copy link
Contributor Author

docEdub commented Dec 3, 2024

I wonder if instead we could change a tiny bit how it is done ?

I don't think this will work because WebGPUEngine does not inherit ThinEngine.

@deltakosh
Copy link
Contributor

I think we can do both:

  • Do what seb propose
  • But keep the call in WebGPUEngine

@sebavan sebavan merged commit 65aa159 into BabylonJS:master Dec 3, 2024
13 checks passed
@docEdub docEdub deleted the 241202-fix-audio-engine-init-issue branch December 3, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants