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

setRoomProperties throws an error #16

Open
brianchirls opened this issue Aug 28, 2018 · 11 comments
Open

setRoomProperties throws an error #16

brianchirls opened this issue Aug 28, 2018 · 11 comments
Labels

Comments

@brianchirls
Copy link

The error happens in LateReflections.prototype.setDurations.

Per the ConvolverNode spec, you can only set the buffer once. If you try to set it a second time, it throws an InvalidStateError. Firefox ignores this, and it is only recently implemented in Chrome, which is probably why the problem didn't show up earlier.

I believe you'll need to create a new ConvolverNode when you want to change the buffer.

@brianchirls
Copy link
Author

This doesn't seem to be an issue until Chrome 69, which is currently in Beta, so you might not see it in the current stable version. The next upgrade is scheduled for early next week, so it will go mainstream in a few days.

@arpu
Copy link

arpu commented Sep 7, 2018

looks like i hit this problem with latest stable chrome Version 69.0.3497.81 (Offizieller Build) (64-Bit)

core:a-node:error Failure loading node: DOMException: Failed to set the 'buffer' property on 'ConvolverNode': Cannot set buffer to non-null after it has been already been set to a non-null buffer at LateReflections.setDurations (webpack:///./node_modules/resonance-audio/build/resonance-audio.js?:3382:26) at Room.setProperties (webpack:///./node_modules/resonance-audio/build/resonance-audio.js?:3110:13) at ResonanceAudio.setRoomProperties (webpack:///./node_modules/resonance-audio/build/resonance-audio.js?:3898:14) at NewComponent.setUpRoom (webpack:///./src/components/aframe-resonance.js?:131:33) at NewComponent.updateProps (webpack:///./src/components/aframe-resonance.js?:107:10) at NewComponent.init (webpack:///./src/components/aframe-resonance.js?:37:10) at NewComponent.updateProperties (webpack:///./node_modules/aframe/dist/aframe-master.js?:75069:12) at HTMLElement.value (webpack:///./node_modules/aframe/dist/aframe-master.js?:73830:19) at HTMLElement.value (webpack:///./node_modules/aframe/dist/aframe-master.js?:73804:14) at entityLoadCallback (webpack:///./node_modules/aframe/dist/aframe-master.js?:73600:14) aframe-master.js:3068 core:schema:warn Unknown property `map` for component/system `material`.

this breaks my app with the new chrome update

@arpu
Copy link

arpu commented Sep 7, 2018

here is a online sample https://etiennepinchon.github.io/aframe-resonance/

@brianchirls
Copy link
Author

@arpu I can confirm that I get the same error on my machine (OSX, same Chrome build version) with your link. This version came out of beta and went stable a few days ago, so a plurality if not a majority of Chrome users should be running this version pretty soon.

As a workaround until this gets fixed, I think you may need to recreate your resonance instance whenever the room dimensions change. You can pass the parameters to the constructor.

@ianpetrarca
Copy link

ianpetrarca commented Oct 10, 2018

@drewbitllama @mgorzel It would be great to get some clarity on this issue. The official Resonance Examples page is completely broken along with about every other website that uses Resonance when viewed on the latest Chrome. Resonance now works better on Firefox which is a bit ironic.

Link to broken examples page:

https://cdn.rawgit.com/resonance-audio/resonance-audio-web-sdk/master/examples/hello-world.html

@hoch
Copy link
Collaborator

hoch commented Oct 14, 2018

@brianchirls is correct.

This is because of the recent change in Chrome's Convolver node. We (AudioWG) reverted the spec decision due to the reasonable amount of breakage. Here's the revert CL for Chrome:
https://chromium-review.googlesource.com/c/chromium/src/+/1249275

The revert patch was landed at 71.0.3569.0, so perhaps @drewbitllama needs to make a temporary fix that recreates a convolver node whenever a buffer changes. When 71 hits the stable, we can remove the temporary fix.

@arpu
Copy link

arpu commented Oct 21, 2018

@hoch do you know a workaround for chrome < 71.0.3569.0 ?

@hoch
Copy link
Collaborator

hoch commented Oct 21, 2018

This line:

this._convolver.buffer = buffer;

Can be replaced with the following:

this._predelay.disconnect();
this._convolver.disconnect();
this._convolver = this._context.createConvolver();
this._convolver.buffer = buffer;
this._predelay.connect(this._convolver);
this._convolver.connect(this.output);

FYI I have not tested the code locally. (traveling atm)

@arpu
Copy link

arpu commented Oct 22, 2018

@hoch thx a lot works!

@ianpetrarca
Copy link

@hoch I tested that code on Mac 10.12.16 Chrome Version 70.0.3538.67 and it fixed the Convolver error but it is now no longer outputting late-reflections for me. I am testing on this repo . I simply swapped your fix into the late reflections src.

@hoch
Copy link
Collaborator

hoch commented Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants