-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 darkened camera view #5577
Fix darkened camera view #5577
Conversation
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.
This LGTM. I rewrote the camera system in the new entity system PR so I will need to apply this same change there. I think it still makes sense to ship this.
src/components/camera-tool.js
Outdated
@@ -128,8 +128,7 @@ AFRAME.registerComponent("camera-tool", { | |||
material.toneMapped = false; | |||
|
|||
// Bit of a hack here to only update the renderTarget when the screens are in view |
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.
We can remove this comment now
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.
Not sure if it is a hack anymore but I think only update the renderTarget when the screens are in view
comment is still helpful. What do you think?
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.
Yep that seems good.
this.screen.matrixNeedsUpdate = true; | ||
this.el.setObject3D("screen", this.screen); | ||
|
||
this.selfieScreen = new THREE.Mesh(geometry, material); | ||
this.selfieScreen.position.set(0, 0.4, 0); | ||
this.selfieScreen.scale.set(-2, 2, 2); | ||
this.selfieScreen.onBeforeRender = onBeforeRender; |
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.
Hmm don't remember why we didn't do this originally... I bet we were doing the actual rendering work in the update
callback before and we didn't want to do it twice so we applied it to the material instead of the object. This definitely makes more sense now though given its only setting a flag anyway.
The recent Three.js relies on WebGL for sRGB - Linear color space conversion. But it does in shader for video textures for a performance reason. camera-tool sets isVideoTexture = true for the screen render target texture to update the render target only when the screens are in view as hack. But this hack causes a wrong color space conversion and the camera views are darkened. This commit resolves the problem by removing the hack and rewriting it more properly.
69b815c
to
b0ad205
Compare
Fixes #5571
The recent
Three.js
relies onWebGL
for sRGB - Linear color space conversion. But it still does it in shader for video textures for a performance reason.camera-tool
setsisVideoTexture = true
for the screen render target texture to update the render target only when the screens are in view as hack. But this hack causes a wrong color space conversion and the camera views are darkened.This PR resolves the problem by removing the hack and rewriting it more properly.