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_environment #1850

Closed
wants to merge 26 commits into from
Closed

EXT_lights_environment #1850

wants to merge 26 commits into from

Conversation

UX3D-nopper
Copy link
Contributor

No description provided.

@UX3D-nopper
Copy link
Contributor Author

This extension provides the ability to define an environment light (also known as [sky light](https://docs.unrealengine.com/en-US/Engine/Rendering/LightingAndShadows/LightTypes/SkyLight/index.html)) in a glTF scene. In general, this environment is used for image based lighting in rasterizers and pathtracers.

To environment light is defined by a panorama image (also know as [equirectangular image](https://docs.blender.org/manual/en/latest/render/lights/world.html)). For encoding the panorama image and for supporting [High-dynamic-range rendering](https://en.wikipedia.org/wiki/High-dynamic-range_rendering), the widely used [RGBE image format](https://en.wikipedia.org/wiki/RGBE_image_format) is used as a `.hdr` file:
The panorama image and `.hdr` file format is supported in paint tools like Gimp or Photoshop, 3D modelling tools like Blender or 3ds Max, Game Engines like Unity or Unreal Engine and Web Engines like Babylon.js or three.js.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would consider panorama / equirectangular IBL easy to create, but inefficient to load, which is the opposite of what we usually aim for with glTF. In Babylon.js and three.js the IBL would have to be converted to a cubemap at load time.

Other than wide support in paint tools, are there other reasons to prefer equirectangular? All of these engines should support cubemaps, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need a) HDR and b) a common supported format. In general, KTX2 would be a perfect fit, but unfortunatelly it does not support HDR compression at this point of time.
I disagree, that .hdr is inefficient to load. It is easy to load (some lines of code and straight forward), nicely encoded and one can choose the final precission and resolution easily (e.g. half float vs. float) for the final cube map. With this preprocessing argument, we could not allow JPEG and/or Basisu compression, as there is also quite something happening on the CPU before the final image is uploaded to the GPU. It is in most cases just hidden in some library.
It is not just about paint tools. Any DCC (2D or 3D) tool can handle .hdr panorama files for IBL. Is there any out there, which does not support .hdr?
I understand your point of view, but some groups want to have this extension and/or feature set. That's the reason for EXT_ and we should not bring all on the same page, as it will not become a KHR_ extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not critiquing the choice of .hdr here, but the choice of panorama instead of cubemap. Couldn't you use 6 .hdr images to form a cubemap, or is that just unusual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is unusual. E.g. in Blender you just define the panorma .hdr file:
image
So, for Blender, an easy import and export would be possible.

Copy link

Choose a reason for hiding this comment

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

I think I would consider panorama / equirectangular IBL easy to create, but inefficient to load, which is the opposite of what we usually aim for with glTF.

I agree with @donmccurdy
If we choose a cubemap format we will reduce the load time process - I think this is something we should aim for!

{
"source": 0,
"intensity": 1.0,
"frontside": "+X"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary that we allow this to be configured, rather than normatively stating a front facing direction as we do with cameras?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is, that different DCC tools do have a different front face. Some do have +Z, some do have +X. So, yes this is required to compensate this.

Copy link
Contributor

@donmccurdy donmccurdy Aug 13, 2020

Choose a reason for hiding this comment

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

Different engines will have different conventions too — we are placing the burden of conversion on them, rather than the DCC tools? This is much easier with cubemaps, perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

E.g. the above screenshot from Blender does have +X as the front face. Because of Blender's coordinate system.
If you set up the same scene in 3ds max - as there +Z is the front face, the scene looks different. To compensate, during export to glTF, the front face is defined.
If an engine is following the glTF specification, the coordinate system is well defined and I do not see any burden. It is the opposite, now the orientation is well defined.
Also, some 3D engines will not use cube maps. E.g. Blender Cycles can be executed on the CPU and there is no need to convert to a cube map. And probably any CPU based path/raytracer will directly sample from the panorma image.

@donmccurdy
Copy link
Contributor

Would be great, if we could consolidate with this EXT_lights_image_based

Perhaps consider this a v2 of that, EXT_lights_image_based2?

@UX3D-nopper UX3D-nopper changed the title Initial draft for EXT_lights_environment EXT_lights_environment Aug 14, 2020
Written against the glTF 2.0 spec.

## Overview

Copy link

Choose a reason for hiding this comment

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

Please add purpose of extension, along the lines of:

The purpose of this extension is to allow embedding of environment map data (IBL) in a glTF asset.
An environment map will supply diffuse and specular light contribution to the scene, as well as information needed for reflections.

This can be used on it's own - ie a glTF asset with only environment map data - for a usecase where the IBL needs to be distributed.
It can also be used together with model data, for usecases where a model shall be displayed in a defined environment.

@rsahlin
Copy link

rsahlin commented Aug 24, 2020

Would be great, if we could consolidate with this EXT_lights_image_based

Perhaps consider this a v2 of that, EXT_lights_image_based2?

I agree!

A couple of things that I like with the existing extension.

  • Uses .png for RGBD format. As this is an existing decoder there is minimal change needed to support RGBD.
    With .hdr a new decoder needs to be supported by implementations.
  • Defines an array of cubemaps - creators can decide to supply all pre-filtered variants or none as they choose.
  • SH for diffuse light contribution

@rsahlin
Copy link

rsahlin commented Aug 26, 2020

The .hdr fileformat seems somewhat vague - googling the format seems to indicate 2 different fileformats:

https://en.wikipedia.org/wiki/RGBE_image_format

http://paulbourke.net/dataformats/pic/

Seems that neither of them store the data in a GPU texture buffer friendly format - ie the image data needs to be processed and then stored in a texture format of choice.

@UX3D-nopper
Copy link
Contributor Author

The .hdr fileformat seems somewhat vague - googling the format seems to indicate 2 different fileformats:

https://en.wikipedia.org/wiki/RGBE_image_format

http://paulbourke.net/dataformats/pic/

Seems that neither of them store the data in a GPU texture buffer friendly format - ie the image data needs to be processed and then stored in a texture format of choice.

In general, during loading of HDR, you already convert it to the GPU friendly format.
But probably we should just drop this extension, as some just do not want to have it for IBL.

@rsahlin
Copy link

rsahlin commented Aug 26, 2020

In general, during loading of HDR, you already convert it to the GPU friendly format.
But probably we should just drop this extension, as some just do not want to have it for IBL.

Sure, and while doing so we could just as well use the RGBD .png format - and then we end up with the same extension as already exists :-)

If we look at KTX2 hdr formats what can we achieve in the short term?

@lexaknyazev
Copy link
Member

If we look at KTX2 hdr formats what can we achieve in the short term?

Cubemap BC6H + ZStandard.

@UX3D-nopper
Copy link
Contributor Author

In general, during loading of HDR, you already convert it to the GPU friendly format.
But probably we should just drop this extension, as some just do not want to have it for IBL.

Sure, and while doing so we could just as well use the RGBD .png format - and then we end up with the same extension as already exists :-)

If we look at KTX2 hdr formats what can we achieve in the short term?

With KTX2, to make it generic, the file size doubles, as half floats would probably be used.

The suggestion from Alexey - using BC6H - you have to convert, if BC6H is not supported.

Best would be to have HDR enconding in basisu, but that is not yet available.

@rsahlin
Copy link

rsahlin commented Aug 26, 2020

Aha, ok so implementations of KTX2 needs to support the following deflation schemes:

BasisLZ
Zstandard
ZLIB

@lexaknyazev
Copy link
Member

BasisLZ is a special case exclusive to ETC1S LDR data.

All other formats could be processed with Zlib or ZStandard. To get decent rates from these schemes, the original data should be RDO-encoded.

@rsahlin
Copy link

rsahlin commented Aug 26, 2020

I would suggest a slow migration to KTX2 by using the same format - RGBD using RGBA data - encoded as a lossless image.
This would be the same image storage solution as the existing EXT_lights_image_based extension BUT packaged in a future proof container.

This way we would get HDR support now and implementations will be ready to take advantage of the HDR supercompression when that becomes available.

@rsahlin
Copy link

rsahlin commented Aug 26, 2020

All other formats could be processed with Zlib or ZStandard. To get decent rates from these schemes, the original data should be RDO-encoded.

@lexaknyazev This is uncharted territory for me - does that mean that there is no or little support for lossless (.png type) compression?

@lexaknyazev
Copy link
Member

I would suggest a slow migration to KTX2 by using the same format - RGBD using RGBA data - encoded as a lossless image.

This may lead to bigger file size than using PNG as the latter's compression comprises two steps:

  1. Lossless filtering pass, i.e. differential / average / ... scanline encoding.
  2. Regular zlib/deflate pass.

We should evaluate Zstandard rate on raw RGBD data.

RDO is a way of controlling the effective rate of a lossless compression step by altering the original data.

@UX3D-nopper
Copy link
Contributor Author

UX3D-nopper commented Aug 27, 2020

"For devices that don’t support ASTC HDR, all devices running Vulkan or OpenGL ES 3.0 support RGB9e5"
https://docs.unity3d.com/Manual/class-TextureImporterOverride.html

See https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_shared_exponent.txt

And it is not a forbidden KTX2 format, right?

It does not work for WebGL 1.0 (see Unity document point 4 "Except on OpenGL ES 2.0 / WebGL 1."), but these engines just then need to convert the data.

So, KTX2 cube map plus using VK_FORMAT_E5B9G9R9_UFLOAT_PACK32?

@rsahlin
Copy link

rsahlin commented Aug 27, 2020

We should evaluate Zstandard rate on raw RGBD data.

Thanks for your reply @lexaknyazev - sure, this sounds like a great idea.

So, if I understand correctly that would be like zipping the raw 32 bit RGBD data?
I don't see a problem with that since I think most implementors have access to some sort of zlib.

@johguenther
Copy link

Coming from #946 I'd like to specifically second #946 (comment). In particular in PBR context an environment map is not much different from any other light source. Thus, attaching it to the node would allow for orientation and animation, consistent to KHR_lights_punctual or other objects like cameras. The environment map, aka HDRI light, is "just" in infinity and thus is only affected by the node's orientation, ignoring position and scale, like the directional light.

@rsahlin
Copy link

rsahlin commented Dec 17, 2020

The environment map, aka HDRI light, is "just" in infinity and thus is only affected by the node's orientation, ignoring position and scale, like the directional light.

Thanks for the suggestion @johguenther
While I agree that it makes sense to be able to change the orientation - I'm not sure if it should be done by attaching the IBL to a node or by adding rotation to the environment map.

My first hunch is that I would rather add rotation to the IBL (and not add the IBL to a node) - simply because it would lead to confusion if more than one IBL can be present.

@rsahlin rsahlin mentioned this pull request Mar 9, 2021
rsahlin added a commit that referenced this pull request Mar 16, 2021
This is a continuation of PR #1850 made into KHR extension
and based on Khronos gltf repo (not forked)
@rsahlin rsahlin mentioned this pull request Mar 17, 2021
@rsahlin
Copy link

rsahlin commented Jun 16, 2021

@UX3D-nopper - could you please close this and we will continue on #1956?

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.

6 participants