Fix reflection component removal#5125
Conversation
src/components/scene/reflection.js
Outdated
| this.cubeCamera = new THREE.CubeCamera(0.1, 1000, this.cubeRenderTarget); | ||
| this.lightingEstimationTexture = (new THREE.WebGLCubeRenderTarget(16)).texture; | ||
| this.needsVREnvironmentUpdate = true; | ||
| this.probeLight = null; |
There was a problem hiding this comment.
I prefer to declare the variable in init like this. Also I check for it in the test.
There was a problem hiding this comment.
No, we don't need to declare it. There is also other variables used on this that are not declared either.
| this.cubeCamera.position.set(0, 1.6, 0); | ||
| this.cubeCamera.update(renderer, scene); | ||
| this.el.object3D.environment = this.cubeRenderTarget.texture; | ||
| scene.environment = this.cubeRenderTarget.texture; |
There was a problem hiding this comment.
We use scene.environment = null three line above, we should use the scene variable here for symmetry :)
src/components/scene/reflection.js
Outdated
| this.removeChild(this.probeLight); | ||
| if (this.probeLight) { | ||
| this.el.removeChild(this.probeLight); | ||
| this.probeLight = null; |
There was a problem hiding this comment.
this.probeLight = null; is not mandatory, but I find it good code hygiene to remove any reference to a DOM element explicitly. This can help the GC too.
There was a problem hiding this comment.
This should probably be undefined. null represents a missing object. In this case we're resetting the variable. Also for style consistency
There was a problem hiding this comment.
I removed the line. This doesn't add much. The component is removed anyway, so the DOM element is removed as well.
There was a problem hiding this comment.
About null vs undefined, please note that in the aframe code base we are currently setting some variable to null in remove like I suggested. In the text component for example:
Lines 126 to 135 in 579473b
|
Thank you! |
Description:
We get a
Uncaught TypeError: this.removeChild is not a functionerror when removing the scene if we had the reflection component on it and we didn't enter vr mode.Changes proposed:
this.probeLightexists before trying to remove it, also usethis.el.removeChildinstead of non existingthis.removeChild