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

scene graph based light probe #16270

Conversation

richardmonette
Copy link
Contributor

@richardmonette richardmonette commented Apr 16, 2019

related: #16223

Per #16223 (comment)

I really like @mrdoob's recommendation is that LightProbe is derived from Object3D.

I've changed LightProbe to be Object3D based.

Per #16199 (comment)

Thus I do want baby steps that may not be the perfect solution. This is one of them.

Just change the interpolation to per mesh rather than per vertex for now.

I do agree with that. Per-mesh interpolation, even with a naive interpolation method, should be a great basis to build from.

I've changed from using the lighting system directly to a baby step towards making the interpolated light probe per object.

We will still need to do:

  • A better interpolation scheme
  • A proper dirty state tracking system which determine when an object needs it's interpolated SH information updated (e.g. either the object or one or more light probes has moved in the previous frame)

This gets us a tiny bit closer to our goal however, as it will at least in some basic way take into account the relative position of the light probe(s) and objects.

@WestLangley @donmccurdy @bhouston

@bhouston
Copy link
Contributor

I believe this is conceptually in the right direction.

@richardmonette
Copy link
Contributor Author

@WestLangley If you are OK with this, then I will:

  1. Resolve conflicts
  2. Update the example so the sphere is moving. Even with the super basic hard-cutover nearest neighbour "interpolation", we should be able to see the SH contribution changing as an object moves relative to the light probes.

@@ -1414,6 +1439,8 @@ function WebGLRenderer( parameters ) {
object.modelViewMatrix.multiplyMatrices( camera.matrixWorldInverse, object.matrixWorld );
object.normalMatrix.getNormalMatrix( object.modelViewMatrix );

object.interpolatedDiffuseLightProbe = computeInterpolatedLightProbe( object, scene );
Copy link
Collaborator

@donmccurdy donmccurdy Apr 16, 2019

Choose a reason for hiding this comment

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

Maybe this should be interpolatedDiffuseSH rather than a LightProbe instance? 🤔

Once we're actually doing interpolation between probes, we won't need a LightProbe to represent the interpolated result of probes in the scene. For example:

// Update .interpolatedDiffuseSH (instance of SphericalHarmonics3)
computeInterpolatedSH( object, scene, object.interpolatedDiffuseSH );

@donmccurdy
Copy link
Collaborator

donmccurdy commented Apr 16, 2019

One suggestion inline, but I agree it's OK to merge this without a more complex interpolation scheme. Per #16223 (comment) I'm willing to work on interpolation, but @richardmonette if you were planning to do that I won't complain. 😇

@WestLangley
Copy link
Collaborator

WestLangley commented Apr 17, 2019

@donmccurdy Do you think the renderer should be interpolating the scene light probes?

To me, that is an app-level responsibility, which can be applied in mesh.onBeforeRender() based on the mesh's position, and at the developers desired update frequency -- not necessarily every frame.

Plus, there are other issues we haven't discussed. One is, how do you set the intensity of the interpolated probe knowing it must be proportional to the intensity of the mesh's environment map? It is the developer's responsibility to do that in a reasonable way.

@WestLangley
Copy link
Collaborator

@richardmonette I mean this with all due respect, but I would prefer if you would allow me to respond to feedback from the PRs I file, rather than you doing it. This is leading to file conflicts, and confusion. I hope you understand.

I am, of course, open to your suggestions.

@donmccurdy
Copy link
Collaborator

Do you think the renderer should be interpolating the scene light probes?

On one hand – robust and efficient interpolation methods exist, and there's no point in the community maintaining multiple alternative implementations. The proposed method is likely to be a good one for all scenes with multiple probes, and I think it should be provided by three.js – whether as part of the renderer or by some other means.

That said, I agree there are downsides to interpolating in the renderer... In addition to the update frequency and intensity issues you mentioned:

  • Managing the light probe volume is complex enough to very easily add 500-1000 LOC to the library.
  • If the renderer applies diffuse SH automatically, we'll need new material properties to opt out for static meshes. That could be avoided by just letting the application set mesh.material.lightProbe, or not.

As discussed in another thread, I think an .update( mesh ) pattern may be a good choice:

import { LightProbeManager } from 'three'; // or three/examples/js/...?

var probeManager = new LightProbeManager();
probeManager.setProbes( [ probe1, ..., probeN ] );

mesh.onBeforeRender = function () {

  // Update mesh1.material.lightProbe (or .diffuseSH?)
  probeManager.update( mesh );

}

function animate () {

  renderer.render( scene, camera );

}

I wish that didn't require onBeforeRender. 😕

@donmccurdy
Copy link
Collaborator

donmccurdy commented Apr 17, 2019

One is, how do you set the intensity of the interpolated probe knowing it must be proportional to the intensity of the mesh's environment map?

Suddenly I like the idea of using mesh.material.lightProbeIntensity or sh.multiplyScalar(0.5) sh.scale(0.5) and removing lightProbe.intensity 😓

Maybe I don't fully understand your point - the mesh may not have an environment map at all, and I'm not sure why interpolation should need to adjust probe intensity when there is one.

@WestLangley
Copy link
Collaborator

I think it should be provided by three.js – whether as part of the renderer or by some other means.

Not as part of the renderer, in my opinion. But definitely as part of the examples.

@WestLangley
Copy link
Collaborator

But I'm not sure I fully understand your point - the mesh may not have an environment map at all, and I'm not sure why interpolation should need to adjust probe intensity when there is one.

If the environment map exists, and the mesh environment map intensity is reduced, then for consistency, the light probe intensity (used in the shader) should reduce, too... Well, at least I would hope so, but I guess one would not have to enforce that.

@donmccurdy
Copy link
Collaborator

I see, thanks. Perhaps envMapIntensity should just always apply to the light probes' interpolated result (no need for material.lightProbeIntensity) and at some point perhaps we rename it more appropriately.

@bhouston
Copy link
Contributor

bhouston commented Apr 17, 2019 via email

@richardmonette
Copy link
Contributor Author

#16228 (comment) looks like a better implementation

@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2019

@richardmonette thanks for giving a push to all this 🙌

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.

5 participants