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

Several PMREMGenerator fixes #18045

Merged
merged 5 commits into from
Dec 3, 2019
Merged

Several PMREMGenerator fixes #18045

merged 5 commits into from
Dec 3, 2019

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Dec 2, 2019

Fixes #18029

I found a better way to fix the above issue was correctly using the scissor test, thus preserving any desired clearing behavior. In addition I found several uses of the cube shader I had forgotten to update, so this fixes several examples that were broken by my last change.

I also added a Generated option to the webgl_materials_envmaps_hdr example to serve as an example for using PMREMGenerator.fromScene() and to act as a test for the issue that was fixed here.

@mrdoob mrdoob added this to the r112 milestone Dec 3, 2019
@mrdoob mrdoob merged commit 8825762 into mrdoob:dev Dec 3, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 3, 2019

Thanks!

@elalish elalish deleted the autoClear branch December 3, 2019 18:15
envScene.add(mainLight);

var lightMaterial = new THREE.MeshBasicMaterial();
lightMaterial.color.setScalar(20);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't set colors to [ 20, 20, 20 ]. Color is the diffuse reflectance of the material, it is unit-less, and takes values in [ 0, 1 ].

Copy link
Owner

Choose a reason for hiding this comment

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

As far as I understand this is emulating a very bright light.
So there is no other way to do this than by going over 1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting the diffuse reflectance to a value greater than 1 means that more light is reflected than is incident. We should not be doing that.

You can add a light to the scene and increase the light's intensity.

You can also set material.emissive ( in [0, 1] ) and material.emissiveIntensity, which is unbounded.

Copy link
Owner

@mrdoob mrdoob Dec 4, 2019

Choose a reason for hiding this comment

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

Hmm, MeshBasicMaterial doesn't have emissive...

So in order to create "RectAreaLights" when generating envMaps you recomend doing this:

new THREE.MeshStandardMaterial( { emissive: 0xffffff, emissiveIntensity: 20 } );

Instead of this:

new THREE.MeshBasicMaterial( { color: new THREE.Color().setScalar( 20 ) } );

Right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I wrote a RectAreaLightHelper, but I specifically set the rectangle color so it wouldn't over-bright or hue shift as the light intensity increases. I think you will have to experiment with it so you will understand what I am referring to. :-)

Tip: RectAreaLightHelper must be added as a child of the light.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need tone-mapping and exposure control.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... Even when tone-mapped, it is hue-shifting to white unless the exposure is very low... Bummer.

Well, that is a separate issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the discussion; should I change this to MeshLambertMaterial then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or RectAreaLightHelper?

Copy link
Owner

Choose a reason for hiding this comment

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

Using MeshLambertMaterial sounds good to me.

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.

problem in new PMREMGenerator.js
3 participants