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

Add CubeTexture LOD support and use it for PBR Environment #987

Merged
merged 1 commit into from Mar 25, 2019

Conversation

Projects
None yet
3 participants
@georgios-uber
Copy link
Contributor

georgios-uber commented Mar 15, 2019

No description provided.

@georgios-uber georgios-uber self-assigned this Mar 15, 2019

@georgios-uber georgios-uber requested a review from ibgreen Mar 15, 2019

@georgios-uber georgios-uber force-pushed the tex-cube-lod branch 2 times, most recently from f1ddc94 to 059bce2 Mar 15, 2019

@georgios-uber georgios-uber marked this pull request as ready for review Mar 15, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 15, 2019

Coverage Status

Coverage increased (+0.004%) to 46.393% when pulling d37f94b on tex-cube-lod into 80a9e97 on master.

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

georgios-uber commented Mar 15, 2019

Screen Shot 2019-03-15 at 3 43 44 PM

@ibgreen
Copy link
Contributor

ibgreen left a comment

I like the feature! Just needs a round or two of review and clarification/cleanup...

Show resolved Hide resolved examples/gltf/app.js Outdated
@@ -93,7 +23,7 @@ export default class GLTFMaterialParser {
this.parameters = {};

if (ibl) {
this.env = new GLTFEnv(gl);
this.env = ibl;

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

I feel that ibl is too terse, we need a better name. Type out whatever it means or use another name.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 16, 2019

Author Contributor

Image Based Rendering or IBL : a technique that uses an image as a light source

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 17, 2019

Author Contributor

I replaced all of references to imageBasedLighting. Should I also rename environment?

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

I replaced all of references to imageBasedLighting.

OK thanks, let's go with that for now.

Should I also rename environment?

Yes, maybe ImageBasedLightingEnvironment, or IBLEnvironment.

I think writing a doc about that class, with short explanation, references to papers, references to the tool you mention below would be really helpful.

I assume the PBR module in some sense depends on or "collaborates with" this class, making that relationship crips and clear in the docs (perhaps by updating PBR docs?) would surely cast this PR in a much clearer light.

const facePixels = imageDataMap[face];
return Promise.all(Array.isArray(facePixels) ? facePixels : [facePixels]);
})
).then(resolvedFaces => {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Use async await

return Promise.all(
FACES.map(face => {
const facePixels = imageDataMap[face];
return Promise.all(Array.isArray(facePixels) ? facePixels : [facePixels]);

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Why do we need Promise.all here? Add comment...

gl.texImage2D(face, 0, format, format, type, resolvedFaces[index]);
if (resolvedFaces[index].length > 1 && this.opts.mipmaps !== false) {
log.warn(
`Texture ${

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

We normally avoid long error messages that add to the bundle size but are very seldom used.

Add as comment and log something minimal, user can click their way back here and read the comment.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 16, 2019

Author Contributor

Ok

gl.texImage2D(face, 0, format, width, height, border, format, type, resolvedFaces[index]);
} else {
gl.texImage2D(face, 0, format, format, type, resolvedFaces[index]);
if (resolvedFaces[index].length > 1 && this.opts.mipmaps !== false) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

I think we need some comments here, otherwise this will break in next refactor

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 17, 2019

Author Contributor

Probably need unit test for that too.

@@ -81,15 +84,30 @@ export default class TextureCube extends Texture {
const {gl} = this;
const imageDataMap = pixels || data;

return Promise.all(FACES.map(face => imageDataMap[face])).then(resolvedFaces => {
return Promise.all(

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

I think we need some comments here, otherwise this will break in next refactor

import {Texture2D, TextureCube} from '../../webgl';
import {loadImage} from '../../core/load-file';

export default class GLTFEnvironment {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

I am not quite clear on what this class represents. A number of cube maps that together represents an advanced environment mapping? Could you try to write a doc page for it as a step towards articulating this?

Also, how do we get additional such maps to use? (Apart from the one example we got through glTF demo)

  • Are there libraries of such environments available?
  • Are they part of glTF files?
  • Can we generate them? The classic technique is to set up 6 cameras and generate such maps from our scenegraph by rendering in each direction? Then we can create perfect reflections for any object in the scene.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 16, 2019

Author Contributor

Yes, programs like this one

Show resolved Hide resolved modules/core/src/scenegraph/gltf/gltf-environment.js Outdated
Show resolved Hide resolved modules/core/src/scenegraph/gltf/gltf-environment.js Outdated

@georgios-uber georgios-uber force-pushed the tex-cube-lod branch 2 times, most recently from 746db4c to 471b66c Mar 17, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

georgios-uber commented Mar 17, 2019

Added doc+unit test.

@georgios-uber georgios-uber requested a review from ibgreen Mar 18, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

georgios-uber commented Mar 20, 2019

Ping ping

@ibgreen
Copy link
Contributor

ibgreen left a comment

Looking better, adding another round of comments

}});
```

This class supports _Async Textures_. You can provide promises instead of images on all methods.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Some example of "provide promises instead of images on all methods"? Questions here when reading, can you provide promises for each LOD?, or each array of LOD or does it need to be the pixels arg itself that is a promise?

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Nit: We should update the ### constructor header below to show which params it takes...

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

You can provide multiple individual promises. I'll update.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

I updated please take a second look.

@@ -331,21 +346,21 @@ export class DemoApp {
switch (value) {
case 'exclusive':
Object.assign(this.loadOptions, {
pbrIbl: true,
imageBasedLighting: this.environment,

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Is there any reason not to just call this prop environment?

  • imageBasedLighting sounds like a boolean.
  • imageBasedLightingEnvironment?

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

imageBasedLightingEnvironment

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

it is a specific type of environment

Show resolved Hide resolved modules/core/src/scenegraph/gltf/gltf-environment.js Outdated
return this._SpecularEnvSampler;
}

getBrdfTex() {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Please type out Texture here and in other places.

If we are going to keep brdf as a term:

  1. It needs to be mentioned/explained in the docs
  2. Type in in all caps?

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

Bidirectional reflectance distribution function

so getBidirectionalReflectanceDistributionFunctionLookupTexture() sounds good?

@ibgreen
Copy link
Contributor

ibgreen left a comment

More comments

@ibgreen
Copy link
Contributor

ibgreen left a comment

We need to land this soon to make it into 7.0, try to address comments and add TODOs / open tasks for any missing items.

@georgios-uber georgios-uber force-pushed the tex-cube-lod branch from 471b66c to dc61604 Mar 24, 2019

@georgios-uber georgios-uber force-pushed the tex-cube-lod branch from dc61604 to d37f94b Mar 24, 2019

@georgios-uber georgios-uber merged commit f1ebdf7 into master Mar 25, 2019

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.008%) to 46.393%
Details
license/cla Contributor License Agreement is signed.
Details

@georgios-uber georgios-uber deleted the tex-cube-lod branch Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.