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

FB_materials_modmap #1658

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

FB_materials_modmap #1658

wants to merge 5 commits into from

Conversation

idorurez
Copy link

A draft pass at implementing a vendor extension for lightmap functionality

@idorurez idorurez changed the title FBX_materials_lightmap FB_materials_lightmap Aug 12, 2019
@bghgary
Copy link
Contributor

bghgary commented Aug 19, 2019

I haven't looked at either in great detail, but since they both have lightmap in the name, what are the differences between this extension and #1017?

@bghgary
Copy link
Contributor

bghgary commented Aug 19, 2019

Ah, sorry, should have read the other thread first. Never mind.


## Specifying Material extension

The data is defined as an optional extension to a glTF `materials`, by adding an `extensions` property and defining a `FB_materials_lightmap` property with a `lightmapTexture` node inside it:
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 we can confidently say that the lightmapTexture must be sRGB, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe not? See Unity docs and HDR / BC6H references..

Copy link
Contributor

Choose a reason for hiding this comment

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

My thought is that consider whatever platform this gets ingested on, it would be advantageous to leave color space to the adopter's discretion.

We have tried to be specific about the colorspace of other textures in the glTF spec; unless there's a clear reason not to I think we should do the same here. The colorspace could depend on the texture format if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

A lightmap technically can be unbounded in range (thus HDR) because a light map represents a measure of the incoming light.

@donmccurdy
Copy link
Contributor

Thanks @idorurez!

Because this is a draft, I'll respond below on the assumption that FB is not yet using this extension, and is looking for either feedback on the approach or alignment with other vendors. If that isn't the case — e.g. the extension is already in use or locked in — and you'd simply prefer for this to be merged as-is, we can also make that happen.


This does not define how the lightmap will be applied, just that one exists.

For a multi-vendor or Khronos extension, I think we would need to go farther than just identifying the texture as a lightmap. Ideally we would define how it will be applied, assuming some consistency in client applications is possible. If that isn't possible, we would at least want to understand the spectrum of implementations that exist, and provide some documentation and guidance for implementers. For occlusionTexture, as an example, pre-existing implementations have quite a few differences.

  • Unity: See Lightmaps: Technical Information. Offers several quality settings, with the "normal" setting being RGBM.
  • Unreal: Haven't found much. One link suggested lightmaps are greyscale in Unreal?
  • threejs*: RGB channels are multiplied against lightmapIntensity, then added to indirect diffuse. Alpha channel is ignored.

^Not a lot of detail, hopefully others can help improve this! :)


```javascript
lowp vec4 lightmapColor = texture2D(LightmapTexture, oLightmapTexCoord);
color.rgb *= (LightmapFactor.rgb * lightmapColor.rgb);
Copy link
Contributor

Choose a reason for hiding this comment

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

For threejs at least, this would be += rather than *=... do we have other references here?

Copy link
Author

@idorurez idorurez Aug 20, 2019

Choose a reason for hiding this comment

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

Yes, this is a draft for a vendor extension and I'd like to merge it as-is, and it's already been implemented.

However, I'm soliciting a broader range of practices in the event I'm completely missing an important component. Furthermore, my limited research (I'll take a look at the Unity link above also) along with internal discussion hasn't revealed anything beyond a simple multiplicative application for a very basic lightmaps.

I am aware of the additive application, but I am under the assumption that such an application implies that there still needs to be some level of lighting in the color/texture it is applied to. Communicating a basic multiplicative approach is easier to adopters as we don't have to ask them to bake in a base level of lighting that they will always have to account for, which they may not want to do. I'm not beholden to this however, and I'm open to thoughts and ideas!

I'll investigate the lightmap color space. I'm specifically leaving out alpha and keeping it RGB. My thought is that consider whatever platform this gets ingested on, it would be advantageous to leave color space to the adopter's discretion.

Copy link
Contributor

@donmccurdy donmccurdy Aug 20, 2019

Choose a reason for hiding this comment

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

Furthermore, my limited research (I'll take a look at the Unity link above also) along with internal discussion hasn't revealed anything beyond a simple multiplicative application for a very basic lightmaps.

Ok thanks. I need to learn more about this myself, too. Everything I can find describes multiplicative lightmaps, so perhaps threejs is unusual here. We can dismiss the additive use if there are no other implementations.

I'm not sure I understand the advantage of a multiplicative lightmap over an ambient occlusion map – it's RGB, true, but can still only darken the base color?

I am under the assumption that such an application implies that there still needs to be some level of lighting in the color/texture it is applied to...

If this extension is intended to replace other lighting in the scene, it could arguably require KHR_materials_unlit to also be present. But I would prefer to define both cases – both unlit and PBR application of the lightmap.

Copy link
Author

Choose a reason for hiding this comment

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

Ok thanks. I need to learn more about this myself, too. Everything I can find describes multiplicative lightmaps, so perhaps threejs is unusual here. We can dismiss the additive use if there are no other implementations.

Without revealing too much about what we have under our hood, we also have an unpublished spec that uses an additive application. After some consultation, we realized we needed to define a more broader, simple approach with a multiplicative application.

I'm not sure I understand the advantage of a multiplicative lightmap over an ambient occlusion map – it's RGB, true, but can still only darken the base color?

The multiplicative aspect, in general, is the same. The main difference is that the source itself is indirect (occlusion) vs direct. I could easily see a situation where we would want to brighten the environment with the lightmap.

If this extension is intended to replace other lighting in the scene, it could arguably require KHR_materials_unlit to also be present. But I would prefer to define both cases – both unlit and PBR application of the lightmap.

Ah good point. Again, without revealing too much, I have been working in an environment that operates under the assumption that the (PBR) material will always be unlit. What would you suggest we do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

For threejs at least, this would be += rather than *=... do we have other references here?

Light maps, if you are doing them correctly, should be additive with all other light sources.

The reason is pretty simple. Light in general is additive and a light map is not exclusive but rather just one more light source in a scene. This way you can combine dynamic lights with precomputed light maps.

Copy link
Contributor

@bhouston bhouston Aug 21, 2019

Choose a reason for hiding this comment

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

The multiplicative aspect, in general, is the same. The main difference is that the source itself is indirect (occlusion) vs direct. I could easily see a situation where we would want to brighten the environment with the lightmap.

source itself is indirect (occlusion) vs direct

A light source can not be occlusion and also a light source. There are direct light sources and indirect light sources, and there are also occlusions, but all three of these things are separate.

I think you have an occlusion map and are calling it a light map.

Yes, this is a draft for a vendor extension and I'd like to merge it as-is, and it's already been implemented.

If we basically want to say that this is a confused feature and that is okay because it is just a vendor extension, then sure, but then why standardize it at all?

Copy link
Author

Choose a reason for hiding this comment

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

I think you have an occlusion map and are calling it a light map.

That's a fair assessment.

If we basically want to say that this is a confused feature and that is okay because it is just a vendor extension, then sure, but then why standardize it at all?

I'm not asking for a full EXT_, multiparty, approved extension. Does a vendor extension need to be standardized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does a vendor extension need to be standardized?

It does not. Very often contributors opening a "vendor" PR are also proposing standardization, or don't understand the difference, so we do tend to review liberally regardless of the prefix. :)

@donmccurdy
Copy link
Contributor

Stepping back from lightmaps for a moment, there are several variations of lighting that either exist in glTF, or have been suggested at some point:

  • punctual lights
  • ambient occlusion maps
  • ambient lights (not pursuing)
  • area lights
  • lightmaps
  • spherical harmonics
  • light probes / irradiance probes
  • IBL

Looking at this list, I'm really not confident that all of these things belong in the glTF format... Understandably users have different needs; some want efficient solutions for mobile devices today, others want higher quality or more future-proof features. But all the same, if we try to do everything the format will get complex very fast.

Of the not-yet-supported features on that list, where do we think glTF should be — say, 2-5 years out?

/cc @bhouston

@robertlong
Copy link
Contributor

I'd like to see all of those lighting features supported in glTF, although I think most of them should be implemented as extensions. I think that discussion should be moved into another issue.

As for lightmaps, I'd like to see a multi-vendor (EXT) supported lightmap extension, specifically one that was supported in Unity, Unreal, Babylon, and Three.js This one looks good as-is and I would imagine adding support for Three.js with a custom shader that used multiplicative lightmaps.

@idorurez
Copy link
Author

Without getting too detailed (or do we need to?), should we define the limits on the encoded values based on the encoding method and color space?

I'm referring to the Unity Technical Documentation on Lightmaps, where they mention various clamped values for RGBM and dLDR encoding.

Maybe it's worth keeping it to a single encoding method to keep things simple.

@bhouston
Copy link
Contributor

bhouston commented Aug 21, 2019

In three.js parlance (which is based on Frostbite's), lightmaps should be considered indirect diffuse - search for "indirect diffuse" in this PDF:

https://seblagarde.files.wordpress.com/2015/07/course_notes_moving_frostbite_to_pbr_v32.pdf

The implementation that put them into indirect diffuse was this large PR here:

https://github.com/mrdoob/three.js/pull/7324/files#diff-29f010d669a36f5263d5351f4702e64fR62

Although we should upgrade the texture2D( lightMap, vUv2 ).xyz to be lightMapTexelToLinear( lightMap ).rgb so that encoded (e.g. HDR) textures can be supported with ease.

It was discussed how lightMap should interact with other lights (should it occlude other lights?) here:

mrdoob/three.js#7377 (comment)

@bhouston
Copy link
Contributor

lightMaps to be physical should be specified in units at least once combined with the lightMapFactor - and it should be illuminance in lux.

The reason we put light maps into the indirect diffuse component is that it allows us to consider the indirect diffuse component correctly in the material's BRDF rather than just using a simplified material model of just (diffuse * lightMap) for lightmaps.

@idorurez
Copy link
Author

We're getting way into the weeds here, as Three.js has a much involved and proper model for a physically based implementation of lightmaps.

Again, without giving a whole lot away, there is specificity on the unlit texture of what we're expecting to multiply against, so I was considering KHR_materials_unlit as a requirement.

However, @bhouston makes a good point: Don't call it lightmap. Name it something like FB_materials_modulationMap to avoid confusion on implementation, much like the thread he has linked. I think this is a much better nomenclature because it implies it's application.

Coincidentally, I called this FB_materials_multMap before refactoring it as FB_materials_lightmap.

What does everyone think?

@donmccurdy
Copy link
Contributor

On one hand this is "way into the weeds" for a vendor extension that is already sufficient for the vendor's needs. But it's definitely a discussion I would want to have in detail before an EXT_ or KHR_ extension. If threejs has a physically-based lightmap model now, I don't imagine we would implement the multiplicative version, although certainly threejs users can easily do this using custom shaders or THREE.NodeStandardMaterial.

From my perspective we can merge this (as a vendor extension) any time; it is just a public description of what Facebook needed and built. I agree that another name might be better, if that's ok.

Before an EXT_ or KHR_ extension that other tools would implement, I do think we should have a sense for how/if it fits our roadmap (#1658 (comment)) and, if we decide to proceed, agree on a physical model.

@idorurez
Copy link
Author

Thanks Don, your advice and help is appreciated. I'm going to rename this as appropriately after consulting with a few engineers here to be sure everyone's happy and update this pull request.

extensions/2.0/Vendor/FB_materials_lightmap/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/FB_materials_lightmap/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/FB_materials_lightmap/README.md Outdated Show resolved Hide resolved

```javascript
lowp vec4 lightmapColor = texture2D(LightmapTexture, oLightmapTexCoord);
color.rgb *= (LightmapFactor.rgb * lightmapColor.rgb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does a vendor extension need to be standardized?

It does not. Very often contributors opening a "vendor" PR are also proposing standardization, or don't understand the difference, so we do tend to review liberally regardless of the prefix. :)

extensions/2.0/Vendor/FB_materials_lightmap/README.md Outdated Show resolved Hide resolved
@idorurez idorurez changed the title FB_materials_lightmap FB_materials_modmap Sep 4, 2019
@grimdeathr
Copy link

Where are we with this? Are lightmaps fully supported with clearcoat and metalness ? I'm having some issues with configuration

@kitaedesigns
Copy link

Curious why this extension never made it. Is this the same as EPIC_Lightmap extension for gltf?

@donmccurdy
Copy link
Contributor

donmccurdy commented Jan 4, 2024

Note that this extension does not add lightmaps to glTF — it rather adds a broadly specified mathematical operation. It sounds like that structure was useful to Facebook in an (unspecified) project for lighting-related customization, but given the discussion under #1658 (comment), this is not the right approach for a standardized lightmap extension.

Because this was a proposal for a vendor extension, I think it should be merged without further delay if the vendor is still interested in having the extension specification hosted here. If not, the PR can be closed. In either case, design for a lightmap extension probably needs a new PR, with volunteers to work on its definition.

Comment on lines +65 to +67
```javascript
lowp vec4 modmapColor = texture2D(ModmapTexture, oModmapTexCoord);
color.rgb *= (ModmapFactor.rgb * modmapColor.rgb);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like JavaScript. It looks like GLSL.

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.

8 participants