Fast RectAreaLight implementation using Linearly Transformed Cosines #9234
Conversation
|
I favor acceptance and we can further improve it once it is in dev. I do notice there is a weird artifact on the floor I see on my laptop (Windows 10, Chrome latest, NVIDIA 960M):
|
|
Here is another shot of the artifacts, may be caused by my extreme light shape:
I've upped the contrast a bit with a regular light shape and I can see this: I think we can still accept this PR because it is the best area light we can get, and we should aim to improve its quality once it is accepted. |
|
@bhouston very interesting. will see if i can repro, and investigate. perhaps it has to do with interpolation of the matrix values |
|
@bhouston I re-introduced a tweak to how the texture coords are calculated. i'm having trouble reproducing on my mac. i see a small amount of banding, but nothing like the artifacts you have in your screenshot Does this link exhibit the same artifacts at the one in the PR description? |
|
@bhouston it could also be useful to know if you can repro the same artifacts on the original webgl demo: |
|
The "With tweak" has the pattern: Here is a screenshot of the original from blog.selfshadow.com with an extreme light and the contrast enhanced, it is actually quite smooth and artifact free (there will always be some banding because the screen has limited bit depth): |
|
I found out that I was actually using an " Intel® HD Graphics 530" GPU that has the artifacts, not the NVIDIA 960M -- my laptop was using the integrated GPU for Chrome. I tested the same examples on my desktop with an NVIDIA GPU and it works without any artifacts. |
| rectAreaLength, | ||
| hemiLength, | ||
| _lights.shadows.length | ||
| ].join(','); |
There was a problem hiding this comment.
Not sure if this is something closure compiler cleans up, but the original intention of the code was to avoid the array allocation.
There was a problem hiding this comment.
Ah, yeah, I made that change primarily for readability. Happy to switch it back. In general, I'd try to promote the practice of adding comments around code choices made for non-intuitive optimization purposes.
|
Minor thing but... How about |
|
@mrdoob, the name was a suggestion from @bhouston, and I think it makes sense given that a few other types of AreaLights might be worth pursuing, e.g. The shader render equation is tailored, in this case, to handle a quad-shaped light. The params that the constructor takes are also specific to it being a rectangle ( |
|
One key question to consider: This implementation hard-codes the look-up texture values for the Rect light calculation in arrays inside |
|
RE the intensity question, I'm curious what others think. Should intensity represent (a) the irradiance of the entire rect light or (b) the radiance from any given point on the surface of the light? With option (a), the scene should look equally bright regardless of the size of the area light. Larger rect lights would have larger, but dimmer highlights than smaller rect lights With option (b), specular highlights will appear the same brightness, but overall scene brightness will increase with the size of the rect. |
|
@bhouston take a look at the example now. scaling intensity by the rect area feels much more natural. also cool to see scene stay roughly the same but watch the specular highlight size change as you change the dims of the rect |
Argh, that is huge. Any way to precompute that easily? Maybe the first time it needs them, it calls a function that computes these values. Maybe there is code hanging around somewhere that already does that -- or ask the paper authors for a copy of it. If not, we can not include that in the base ThreeJS but as an add on inside of |
|
@bhouston Their pre-computed values are the result of a "fitting" process, whereby they regress on the parameters that result in a distribution that most closely matches the GGX distribution. I do not believe (but could be wrong) that it's feasible to pre-compute the value on the fly each time three.js boots up. Also, and unfortunately, the size of the hard-coded pre-computed values in plain-text is closer to 200k. Compressed is 75k. We can perhaps get in touch, also asking how the results compare when using a look-up table of 32x32 instead of 64x64, etc. Can also play w/ the precision of the floats in the file, seeing how reducing precision affects quality |
|
Interesting. For a while ThreeJS was < 100KB compressed. I think it is around 110KB compressed right now. I believe the tables are smooth and thus one can likely fit some polynomials to it via matlab: http://www.mathworks.com/help/matlab/ref/polyfit.html Or one could approximate the table by piece-wise linear or cubic patches, again assuming it is smooth. |
|
The core concept and trick of the paper is that they are approximating the GGX BRDF as a product of a simple clamped cosine distribution with some linear transformation applied to it. They use the fact that it's a linear transformation to simplify the integral over a polygonal region. The linear transformation applied depends on the roughness and the view angle. For any given (roughness, theta_v) pair, they represent the linear transformation as a matrix, which has 4 non-zero-or-one values, hence storing 4 (really, 5) float values in the texture for each (roughness, theta_v) pair. So what you're proposing is to come up w/ a fitted function of f(a, t) => (a, b, c, d, e), i.e. a function of two parameters that returns a vector or 5 values. I'm open to ideas. What about loading the look-up table asynchronously, only when the feature is needed? |
|
@mrdoob can you provide a little more context on the link you just posted? |
|
@abelnation http://threejsworker.com/ is just our handy way of viewing/sharing PRs. |
|
gotcha. thanks |
|
@WestLangley does this PR look good to you? |
|
This PR currently increases the size of the file "three.js" by 209K characters because it contains two large lookup tables: At minimum I would suggest that we move those tables into a file in "examples/js" that filles in those two variables. And when one tries to use the area light class in the renderer it checks if those variables are defined and if they are not it say it is dependent upon this extra file being included. |
|
Oh, I missed that. Would base64ing those arrays help? |
|
No. Compressing the data still adds 80kb. I'm exploring coming up w/ fitted polynomial based functions to estimate the lookup table. With this option, the idea would be to compute the look-up tables on the fly if/when the feature is needed. The other option is to on-the-fly fetch the look-up tables as a separate file if/when the feature is needed. Does three.js have other areas where initialization is paused while certain cpu/network-intensive activities are performed? |
That sounds like the best option to me.
Not that I know of. |
abelnation
force-pushed
the
rect-area-light-with-ltc-approximation
branch
2 times, most recently
from
d44fe8f to
2b1bab1
Compare
|
Ok. FYI:
Aside from how we fetch the pre-computed values, the rest of the PR is good to go for providing feedback. Let me know if you guys have best practices for rebasing that I'm not following. |
|
@abelnation wrote:
The PMREM generator is dynamic and creates blurred cubemaps on demand. We are running into issues on some older (e.g. crap) mobile devices where this can take upwards of 5 seconds and it blocks devices leading to a very poor user experience. So we are looking to have the option to pre-calculated these. I think that people can optionally include "examples/js/AreaLightTables.js" after "three.min.js" if they want to use area lights. That would be the easiest solution. We already do this pattern here: I think dynamic fetch isn't a great idea as you do not know what someone's deployment looks like and fetching else where is problematic because of customized ThreeJS versions and whatnot. |
|
Ok, well then I will plan to update this PR to treat RectAreaLight as an extra. I think that makes more sense than hiding the LTC tables inside an example. |
|
@mrdoob @bhouston can someone clarify for me where the appropriate place would be for the LTC data? It appears to me that On another note, one issue with the current system of includes is that an "optional" package can only be added to the source file before or after the entire "common" package. It seems there may be some cases where including an optional feature would require place it's definitions in specific locations interleaved with the common files. Do you guys have thoughts on this? |
|
I'd just throw it into examples/js And make it clear in the code that it is required. We are switching hopefully to a ES6 module system with tree shaking so this will become moot in the near term, do not get caught up on it too much. |
abelnation
force-pushed
the
rect-area-light-with-ltc-approximation
branch
from
247a137 to
cfd1a21
Compare
|
ok, @bhouston i've moved the BRDF texture data to a file in examples. i also rebased on top of dev to make the PR mergeable again. |
abelnation
changed the title
|
@abelnation, I asked the authors about the fresnel term, and they confirmed that in the current github code, it is missing. You can see the GGX evaluate code to see that. They are going to update the code soon, with the fresnel back. But that's just for an update. Your work is Awesome! |
|
bump because id like to implement it in the editor, but it is currently not being mergable. is it dead? |
|
Yeah, the modules change probably broke this... @abelnation Could you try pulling from |
|
Can try to take a look sometime. Would prefer to get a commitment from the maintainers that they intend to merge this code. This PR was blocked on @WestLangley, and it doesn't feel worth my time if it's ultimately going to continue to stagnate. |
|
@abelnation wrote:
Blocked? I do not feel that statement is accurate. I wrote this:
|
|
@WestLangley, ah, my mistake. I assumed that was the reason this PR was not merged by the team. If that is the case, what are all the next steps to work towards merging this PR (aside from resolving conflicts)? |
|
The buck stops at @mrdoob so you should get his feedback. I like this PR as long as the tables are optional. |
|
@mrdoob Can this please be revived? I would really love to use these lights. |
|
@EskelCz this PR is blocked on me, unless someone wants to put in the work of resolving the merge conflicts to get up to date with the latest changes. trying to find some time to get this done |
|
@mrdoob @abelnation @EskelCz Does anyone know what this is about? Note: I am a webpack guy |
|
Update: JS is now building... |
|
Ok, so I've got it working... All demos including the rect area lights are running on my laptop's GTX 960M but I'm having problems with git. I hope to have code checked in soon! |
|
Ok, I have two branches that can be merged without conflicts...
I suspect that the better of the two options is no2. @abelnation is this correct? There are a few GLSL warnings and bugs in the RectAreaLightHelper that I want to resolve before I submit a PR. Note: These issues are probably due to the extreme merging that had to be done! |
|
I'm rooting for you guys, this will be a killer feature in ThreeJS once it hits a release :) |
|
Ok, here it is! |
* sketch of spherical distributions with linearly transformed cosine * adding prelim example sketches for cosine dists * makeSkew method added * add polygon light sandbox example to repo for reference * formatting * notes files * adding skeleton files for area point light * debug directional light integrated into area light example page * initial AreaLightHelper functional in example * makeShape functions for Polygon and add as choices to example * annotating TODO's with name. partial work on AreaLight shader components * adding TODOs and placeholders for all places where AreaLight code needs to be added * fix typo * RectAreaLight shading works in preliminary fashion * updates to example * Preliminary RectAreaLight implementation (no shadows, no distance/decay) * Integrate RectAreaLight with MeshStandardMaterial * moving rectarealight brdf data to an example file. rest of implementation left in place * TubeBufferGeometry: Removed invisible char (#9943) * Remove reference to THREE in IcosahedronGeometry.js (#9945) Fix broken references to THREE namespace in geometries. * Updated builds. * Updated package.json. * Resolved some issues from the first merge * Added demo from #9234 Noticed that the code from @abelnation is not his latest... more merging to come!!! * Fixed issues from merge * Fixed issues from merge... webgl_lights_arealight improved RectAreaLightHelper update bugs fixed * Removing built js files that cause conflicts * Use FileLoader.setMimeType() from MMDLoader (#9990) * MMDPhysics improvement (#9989) * MMDPhysics improvement * Add property defined check * Shoe physic bodies in mmd example by default. * Updated builds. * Improved documentation for constants / Materials (#9993) * Improved documentation for Materials / Material (#9994) * Added defaults to docs / perspectiveCamera (#10007) * added constanst / animation (#10005) * Fixed error in <head> for Docs / AnimationAction, AnimationClip, and AnimationMixer (#10004) * Added default values for zoom, near and far properties of docs / orthographic camera (#10006) * Improved documentation for Constants / Textures (#10001) * Improved documentation for Materials / Material * Moved Texture Combine Operations to constanst / materials * updated Basic, Lambert and Phong .combine property to point to Material constant page * Improved documentation for constants / textures * Added Encoding constants to Textures constants page * Ccdik solver optimization (#10010) * Optimize CCDIKSolver * Remove lines I should have not commit * Remove lines I should have not committed * added missing toJSON method (#10020) * Added missing toJson method (#10019) * added missing toJSON method (#10018) * created documentation for VideoTexture (#10016) * Created doc for CanvasTexture (#10015) * Add CCDIKHelper (#9996) * Add CCDIKHelper * Clean up MMDPhysics.js * Fix typo * Update OBJLoader.html (#10009) Spelling correction. * Fix typo in comment (#10021) * Improved documentation for docs / Texture (#10012) * Improved documentation for docs / Texture * Removed duplicate needsUpate * Improved docs for Clock (#10008) * Created new document page Constants / Renderer (#10002) * Created constants / renderer * renamed Renderer.html to WebGLRenderer.html * Improved documentation for CompressedTexture (#10014) * AudioContext: Added getContext() and setContext(). * Updated builds. * Simplified AudioContext. * Updated builds. * BufferAttribute.onUpload() clean up. * Renamed docs /constants / WebGLRenderer to Renderer (#10028) * fixes #10026 (#10027) * capture bufferAttribute.array properties at first upload (#9972) * save typed array info in attribute properties * use saved attribute properties * remove unused variable * Discard attribute typed arrays for buffered geometries that are not modified after initial rendering (#9512) * add setDiscardBuffer method to BufferGeometry * added discard support to BufferAttribute * add mechanism for discard of BufferAttribute TypedArrays * use more elegant method for creating dummy typed array. * fix typo * Update BufferGeometry.js fix brain fade * rework to use callbacks (phase 1) * rework part 2 * remove build file * support setting onUploadCallback from Geometry * remove repeated calculation from renderer * remove now redundant getter * remove geoemtry interface * document discard mechanism. * merge fixes * restore return.this * drop unneeded call() * rename discard() method to disposeArray() * Improved documentation for WebGLRenderer (#10030) * added missing methods * Finished add methods * redid changed to WebGLRenderer.html * removed unused texture.sourceFile property (#10024) * glTFLoader: Removed hack. See #10024. * Updated builds. * add link to project wiki in README.md (#9987) * Added deprecated msg/fixed link (#10025) * Created documentation for DepthTexture (#10017) * Created documentation for DepthTexture * pulled upstream * added missing comma to docs/list.js * Deprecated UniformsUtils. See #8016. * Updated builds. * MeshBasicMaterial: Add support for lightMap (#9975) * Updated builds.
|
🎉 |
Addresses #8718
Working example:
http://groovemechanic.net/three.js.release/examples/webgl_lights_rectarealight.html
This PR presents a fast implementation of rectangular area lights. The implementation is heavily based on work from a recent ACM SIGGRAPH 2016 publication. Currently, the implementation is integrated into the BlinnPhong material model, however, I will be working to integrate the Standard/Physical material models as well while feedback is gathered.
This implementation does not include the following:
RectAreaLightdecay/distanceparamsI am considering this PR a work in progress, but hope to integrate feedback to drive towards a merge-able implementation.
Work to be done as Follow-up PR's
RectAreaLightdistance/decayparameters