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

Generate lightProbe from CubeCamera #17282

Closed
wants to merge 112 commits into from

Conversation

nipmarsh
Copy link

Add a method to generate a lightProbe from a cubeCamera.
It assumes that the cubeCamera renderTarget is RGBA

vobu and others added 4 commits June 14, 2019 08:46
cordova iOS (as of 5.0) still uses UIWebView,
which provides OffscreenCanvas,
also OffscreenCanvas.getContext("webgl"),
but not OffscreenCanvas.getContext("2d")!
@WestLangley
Copy link
Collaborator

Is there a reason why you do not pass in a WebGLRenderTarget.texture instead of a CubeCamera?

See how WebGLBackground handles this:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLBackground.js#L105-L107

@nipmarsh
Copy link
Author

I could have only pass the renderTarget but the method is named fromCubeCamera so semanthically this is better to pass the CubeCamera

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2019

Live demo: http://raw.githack.com/nipmarsh/three.js/lightprobegenerator_fromCubeCamera/examples/webgl_lightprobe_cubecamera.html

Examples always need to be added to files.js. Otherwise they do not appear in the menu side bar.

@WestLangley
Copy link
Collaborator

but the method is named fromCubeCamera

You can change the name, no? Can you reuse the existing method? See how WebGLBackground handles this.

@WestLangley
Copy link
Collaborator

Your environment map is a Cornell box with colors of the form [ 0, 1, 1 ], so converting to and from sRGB space does nothing. Hence it is difficult to test if your color-space is correct.

@WestLangley
Copy link
Collaborator

WestLangley commented Aug 29, 2019

You are getting there, but there is still something wrong with the way you handle colorspace.

These should produce the same light probe, and it should be in linear colorspace.

lightProbe.copy( LightProbeGenerator.fromRenderTargetCube( renderer, cubeCamera.renderTarget ) );

lightProbe.copy( LightProbeGenerator.fromCubeTexture( cubeTexture ) );

The helper should be rendered in gamma-corrected colorspace.

@@ -116,6 +116,105 @@ THREE.LightProbeGenerator = {

return new THREE.LightProbe( sh );

},

fromCubeRenderTarget: function ( renderer, renderTarget ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fromRenderTargetCube: function ( renderer, renderTargetCube ) {

Copy link
Author

Choose a reason for hiding this comment

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

Can you explain what is wrong for you?

Copy link
Author

Choose a reason for hiding this comment

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

I think color correction in threejs is a little bit messy.
So i should simply assume that the renderTarget is linear. What do you think?
Color correction should be applied as a render pass if the user wants it.
It would be more simple to understand and all buffer would be in linear color space has it should be without wondering in which color space we are working on!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can follow the same pattern in your method I used in #17385 and decode to linear based on the encoding of the WebGLRenderTargetCube.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the function signature should be

fromRenderTargetCube: function ( renderer, renderTargetCube ) {

@WestLangley
Copy link
Collaborator

The texture files are not needed in this PR, right?


var imageWidth = renderTarget.width; // assumed to be square
var data = new Uint8Array(imageWidth * imageWidth * 4);
renderer.readRenderTargetPixels(renderTarget, 0, 0, imageWidth, imageWidth, data, faceIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

whitespace formatting -- multiple places...

> ./node_modules/.bin/eslint --fix <filename>

Mugen87 and others added 24 commits September 6, 2019 11:01
Cleanup Geometry and DirectGeometry TS
Examples: Remove usage of BufferAttribute.setArray().
PointerLockControls: Clean up.
…ra' into lightprobegenerator_fromCubeCamera
@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

I think this PR got messed up...
https://github.com/mrdoob/three.js/pull/17282/files

@nipmarsh
Copy link
Author

I think this PR got messed up...
https://github.com/mrdoob/three.js/pull/17282/files

Ouch, i accidently merge instead of rebase dev branch, sorry...
I will redo a clean pull request and close this one

@nipmarsh nipmarsh closed this Sep 17, 2019
@nipmarsh nipmarsh deleted the lightprobegenerator_fromCubeCamera branch September 17, 2019 07:18
@WestLangley
Copy link
Collaborator

@nipmarsh Would you be willing to redo a clean PR?

@nipmarsh
Copy link
Author

nipmarsh commented Oct 13, 2019 via email

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.