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

EXT_lights_image_based extension #1377

Merged
merged 7 commits into from Nov 7, 2018

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Jul 5, 2018

Here's a super rough pass of the IBL extension that I've been working on with Microsoft (@bghgary).

It needs a lot more info added, of course, but we thought we'd get it out there early.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 7, 2018

Should the name be EXT_lights_image_based?

Seems consistent with WebGL extensions: https://www.khronos.org/registry/webgl/extensions/

We basically followed this naming convention for glTF extensions, except for KHR_materials_pbrSpecularGlossiness (not sure if there was a reasonable, perhaps just an oversight). https://github.com/KhronosGroup/glTF/blob/master/extensions/README.md

Also, if we expect quick and broad adoption, perhaps just start with KHR_ prefix and it will be draft until ratified.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 7, 2018

@ggetz @lilleyse @bagnell please take a look for Cesium.

@MiiBond
Copy link
Contributor Author

MiiBond commented Jul 9, 2018

@pjcozzi I did talk to @bghgary about the naming and I believe we landed on the camelCase for imageBased because of the pbrSpecularGlossiness extension. I'm fine with changing it as I do think all lower case looks nicer.

If the support is strong for this extension, renaming to KHR would be great. I think we were just concerned that it's one very specific way of storing IBL prefiltered maps and maybe not all runtimes would want to go this route to support them.

@McNopper
Copy link
Contributor

Can we please put the definition of the HDR textures in another extension?
If one later wants to use other formats, this should not affect this extension.

@McNopper
Copy link
Contributor

Is there a reason, not to provide the whole mip map chain?

@McNopper
Copy link
Contributor

Finally, I would prefer to encode specularImages in such a way, that later e.g. one image can be referenced having all sides plus mip map levels.

@gnagyusa
Copy link

This is great. One request: it would be awesome to support per-mesh IBL maps, a.k.a. "light probes" / "reflection probes". This would allow much more realistic reflections, with objects reflecting nearby objects in the scene.
We could do this, simply by referencing an IBL light from a "material", as this is part of the shader state change. This would keep all shader state changes encapsulated:

{
  "pbrMetallicRoughness":
  {
    "baseColorTexture":  { "index":4 },
    "metallicRoughnessTexture": { "index":7 }
  },
  "normalTexture": { "index":5 }
  "extensions" :
  {
    "EXT_lights_imageBased" : { "light" : 0 }
  }
}

Thanks!

@gnagyusa
Copy link

gnagyusa commented Jul 16, 2018

Or, an even better way would be to allow IBL environment image changes per transform node:

"nodes":
[
  {
    "name":"MyNode1"
    "extensions" :
    {
      "EXT_lights_imageBased" : { "light" : 0 }
    }
  }
]

@bghgary
Copy link
Contributor

bghgary commented Jul 17, 2018

@pjcozzi

We basically followed this naming convention for glTF extensions, except for KHR_materials_pbrSpecularGlossiness (not sure if there was a reasonable, perhaps just an oversight). https://github.com/KhronosGroup/glTF/blob/master/extensions/README.md

@MiiBond

I did talk to @bghgary about the naming and I believe we landed on the camelCase for imageBased because of the pbrSpecularGlossiness extension. I'm fine with changing it as I do think all lower case looks nicer.

I think when we added the KHR_materials_pbrSpecularGlossiness, we were (or at least I was) thinking the pattern was <KHR|EXT|vendor>_<some category>_<something specific about that category>. We also called the metal/rough object pbrMetallicRoughness in the core spec, and the casing for pbrSpecularGlossiness would be more consistent than pbr_specular_glossiness.

That said, +1 to using snake_case for image_based.

@bghgary
Copy link
Contributor

bghgary commented Jul 17, 2018

@McNopper

Can we please put the definition of the HDR textures in another extension?
If one later wants to use other formats, this should not affect this extension.

We were thinking that an extension isn't necessary for HDR textures, just like we don't need a flag specifying the color space of a texture. It is implied by the object referencing the texture/image. The original proposal was to do this but I found that implementing it in Babylon was more difficult if we used an extension. Thoughts?

Is there a reason, not to provide the whole mip map chain?

We went back a forth a few times on this. Most of the time, we don't want the whole mip map chain because the smallest mip maps don't have enough detail for the environment. It's the offset in some of the environment map creation tools (e.g. Lys). By not specifying all the levels, you implicitly also specify the offset and don't need to provide the unused images. The down-side is that some browsers don't support uploading partial chains and thus you have to fill the missing chains manually.

Finally, I would prefer to encode specularImages in such a way, that later e.g. one image can be referenced having all sides plus mip map levels.

Can you elaborate on this? I'm not sure what you mean.

@bghgary
Copy link
Contributor

bghgary commented Jul 17, 2018

@gnagyusa

support per-mesh IBL maps, a.k.a. "light probes" / "reflection probes"
allow IBL environment image changes per transform node

I'm not an expert in this, but I would think this should be a separate extension.

@lexaknyazev
Copy link
Member

We were thinking that an extension isn't necessary for HDR textures, just like we don't need a flag specifying the color space of a texture. It is implied by the object referencing the texture/image.

I think we can start with a special PNG encoding that can be used without additional extensions (implying that encoding scheme and image/png are enforced by this extension). But I'd like us to have a way to use true HDR data in the future without deprecating this extension.

@bghgary
Copy link
Contributor

bghgary commented Jul 17, 2018

I'd like us to have a way to use true HDR data in the future without deprecating this extension.

+1. Does KTX2 help at all with this?

@lexaknyazev
Copy link
Member

+1. Does KTX2 help at all with this?

Sure, even KTX1 can hold BC6H cube map with mip levels.

@bghgary
Copy link
Contributor

bghgary commented Jul 17, 2018

I mean the texture transmission extension in conjunction with KTX2. Are there plans to support HDR?

@lexaknyazev
Copy link
Member

Are there plans to support HDR?

Ultimately, yes. Can't say anything about timeline, though.

@mjcarterca
Copy link

mjcarterca commented Jul 18, 2018

Great! I'm a 3d Artist over at Electronic Arts, we've just started experimenting with the gltf2.0 format and are very excited about it's potential

To that end, I'd echo the comments from @gnagyusa on per-mesh / per-transform IBL map assignment. This would be a fantastic way of faking real-time reflections and allow for greater control over lighting, so we can better match the fidelity of our game and cinematic assets.

@McNopper
Copy link
Contributor

McNopper commented Jul 18, 2018

After this discussion, I recommend that specularImages links to one KTX1 file.
Having one KTX1 file solves the following problems:

  • Possibility to have true HDR now
  • Provide whole mip-map chain
  • specularImageSize can be removed
  • specularImages gets simpler

@McNopper
Copy link
Contributor

Strongly disagree just having PNG for specular images, as the color channel resolution and finally the quality is too low.
Also, recommend to call this extension EXT_lights_environment or something similar, as using spherical harmonics is not image based.

@McNopper
Copy link
Contributor

Finally, providing the whole mip map chain and going down to 1x1 pixel should be sufficient. Please comapre with this tool:
https://github.com/derkreature/IBLBaker

@MiiBond
Copy link
Contributor Author

MiiBond commented Jul 18, 2018

For the IBL-per-node idea, we did discuss that and I can't remember why we didn't allow it. Maybe lack of runtime support? I think Babylon and Three.js should both be able to handle this.

EXT_lights_imageBased -> EXT_lights_environment
I don't have a strong opinion here. The spherical harmonics are generated from an image but, yeah, they aren't an image themselves. I could go either way.

Leaving the door open for using other texture types would be nice. We just didn't want to support a long list of formats and layouts. e.g. equirectangular or cube layout, RGBE, RGBM, RGBD, logLuv-encoded PNG's, .hdr, .exr, etc.

As for the mip chain, I believe the common practice is to have an offset that specifies which mip represents a fully convolved (i.e. corresponding to roughness = 1.0) reflection. Then the shader interpolates down to that mip. Using the 1x1 mip for this isn't useful. So, we either specify an offset or we specify only a portion of the mip chain and let that implicitly define which mip corresponds to roughness = 1. Gary and I discussed both options but I'm curious what others think.

@bghgary
Copy link
Contributor

bghgary commented Jul 18, 2018

For the IBL-per-node idea, we did discuss that and I can't remember why we didn't allow it. Maybe lack of runtime support? I think Babylon and Three.js should both be able to handle this.

I don't know this part of Babylon well, but my understanding is that this is handled very differently.

@UX3D-nopper
Copy link
Contributor

Regarding EXT_lights_imageBased -> EXT_lights_environment:

Unity: https://docs.unity3d.com/Manual/GlobalIllumination.html#Environment
Unreal: https://docs.unrealengine.com/en-US/engine/rendering/lightingandshadows/lighttypes/skylight
On quick look I found environment or sky light.

I personally would prefer environment, as it describes the ligth type and note the technology/algorithm used,

@UX3D-nopper
Copy link
Contributor

Images do become a texture:

"specularEnvironmentFactor" : 1.0,
"specularEnvironmentTexture" : {
   "index": 0
}

@UX3D-nopper
Copy link
Contributor

Finally, we need an extension for textures:

{
   "sampler" : 0, 
   "source" : 0
},
{
   "sampler" : 1, 
   "source" : [
      1, 2, 3, 4, 5, 6
   ]
},
{
   "sampler" : 2, 
   "source" : [
      [7, ..., n],
      [...],
      [...],
      [...],
      [...],
      [...]
   ]
}

First texture is for the existing use case or if the image contains all 6 sides plus all mip levels.
Second texture is for separate 6 sides plus all mip levels.
Third texture is for separate 6 sides plus separate mip levels.

This would allow to still use .png and .jpeg but also fiel formats like .hdr, .exr, .dds or .ktx

@UX3D-nopper
Copy link
Contributor

Regarding the mip chain, I am not sure if this a common approach. Which engines are using it?

Thought everyone is doing it like this (see 20.4 Mipmap Filtered Samples):
https://developer.nvidia.com/gpugems/GPUGems3/gpugems3_ch20.html

In my opinion, information/precision is lost, as textures are "missing" and values have more to be interpolated.
Do you have some papers/articles on this?

@pjcozzi pjcozzi added the needs discussion Issue or PR requires working group discussion to resolve. label Sep 27, 2018
@MiiBond
Copy link
Contributor Author

MiiBond commented Oct 12, 2018

Should this extension be under the Khronos or Vendor folder? Or should there be a Multi-Vendor folder added for EXT extensions?

@donmccurdy
Copy link
Contributor

I think for now it should go in Vendor/, not Khronos/. I wouldn't be opposed to creating a third folder at some point in the future but it seems early now.

@MiiBond
Copy link
Contributor Author

MiiBond commented Oct 12, 2018

Thanks. Since support for this extension is now live in Babylon and shipping on Monday with Dimension 2.0, should this PR be merged?

@OmarShehata
Copy link
Contributor

Also @MiiBond it would help to provide a sample glTF with this extension to test with for people implementing it.

For explaining how to generate the spherical harmonics, @TimvanScherpenzeel's tool (https://github.com/timvanscherpenzeel/cubegen/tree/octagen) is a wrapper around Filament's cmgen (https://github.com/google/filament#running-the-native-samples) which generates pre-filtered maps as well as the spherical harmonic coefficients.

@MiiBond
Copy link
Contributor Author

MiiBond commented Oct 31, 2018

Updated the spec to address @emackey 's comments.
@OmarShehata , thanks for the Filament links. I'll take a look. Does the cmgen tool work on macOS?
For your other questions posted above, yeah, I think we could probably extend this to support IBL per node as well as CTTF (KTX) support when they become available.
This extension is already supported in Babylon.js and in Adobe Dimension's web-export feature but I don't think there would be big backwards compatibility issues. @bghgary, what do you think?

@bghgary
Copy link
Contributor

bghgary commented Oct 31, 2018

We haven't tried to update an existing extension before. I'm not sure what the implications are. I think it might be okay to do it if the update is both forward and backwards compatible, like glTF 2.0 core.

@MiiBond
Copy link
Contributor Author

MiiBond commented Nov 5, 2018

Excellent feedback, @lexaknyazev. Thank you!

@OmarShehata
Copy link
Contributor

@MiiBond I haven't tried compiling cmgen on a mac but they do have prebuilt releases for all OS's (https://github.com/google/filament/releases).

Also CesiumJS does have support for this almost ready which I think would help provide a second reference implementation. We've just been waiting to get our hands on some glTF models with this extension to test with. I know Adobe Dimensions can export it but do you know of any freely available models for testing?

@MiiBond
Copy link
Contributor Author

MiiBond commented Nov 7, 2018

Thanks, @OmarShehata .

I'll see if I can export some examples from Dimension today.
@pjcozzi, would we be able to merge this extension in today?

@pjcozzi pjcozzi merged commit 079476b into KhronosGroup:master Nov 7, 2018
@emackey
Copy link
Member

emackey commented Nov 7, 2018

@MiiBond for this version of the extension, can it be applied only to scene objects in glTF?

@emackey
Copy link
Member

emackey commented Nov 7, 2018

@MiiBond Actually it looks like the schema can be applied to the root or the scene, is that correct?

@lexaknyazev
Copy link
Member

The root gets an array of light definitions (like in punctual lights), the scene gets a light instance.

@emackey
Copy link
Member

emackey commented Nov 7, 2018

@lexaknyazev Ah, of course. Thanks!

@TimvanScherpenzeel
Copy link

@MiiBond I haven't tried compiling cmgen on a mac but they do have prebuilt releases for all OS's (https://github.com/google/filament/releases).

Just a heads up if you are going to use the spherical harmonics generated by cmgen: the authors have recently made a change that included 1 / PI in the spherical harmonics. This means the SH reconstruction in the shaders shouldn't include the Lambert BRDF.

"title": "light",
"type": "object",
"description": "An image-based lighting environment.",
"allOf" : [ { "$ref" : "../../../../specification/2.0/schema/glTFChildOfRootProperty.schema.json" } ],
Copy link
Member

Choose a reason for hiding this comment

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

I know this just got merged, but I need to ask one more question. None of our other extensions put this whole relative path on here, they all just say glTFChildOfRootProperty.schema.json without the path. Would it be OK to remove the path for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

Further discussion migrated to #1480.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can make that change and do another pull request. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@MiiBond I needed it for VSCode, so I made the change in #1480. Can you take a quick look at that? Thanks!

@MiiBond
Copy link
Contributor Author

MiiBond commented Nov 7, 2018

@OmarShehata I added a sample asset here:
KhronosGroup/glTF-Sample-Models#201

@lexaknyazev lexaknyazev removed the needs discussion Issue or PR requires working group discussion to resolve. label Dec 6, 2018
@vorg
Copy link

vorg commented Apr 26, 2019

@MiiBond Is there more info on "principled multi-scatter GGX normal distribution" or any reference code on how to generate assets compatible with this extension? Tried to have a look at BabylonJS repo but can't find anything.

@MiiBond
Copy link
Contributor Author

MiiBond commented Apr 29, 2019

Actually, we don't really have any consistency in the exact lighting model or roughness curve used for generation or display of this data. However, we're currently in talks about defining a Khronos-ratified version of this extension which will spell-out, more concretely, the equations to use when generating the IBL maps and displaying them in the runtime.
I expect that we will have a rough spec written in the next few weeks. Stay tuned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet