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

HDR values for emissiveFactor? #1083

Closed
AurL opened this issue Aug 31, 2017 · 22 comments · Fixed by #1994
Closed

HDR values for emissiveFactor? #1083

AurL opened this issue Aug 31, 2017 · 22 comments · Fixed by #1994

Comments

@AurL
Copy link

AurL commented Aug 31, 2017

Hi,

Would it be possible to allow values greater than 1.0 for emissiveFactor?
I didn't find any thread or discussion about this, maybe it has just not been mentionned before.

The specification enforces emissiveFactor values to be in [0,1], but Unity allows emissive HDR values in it's Standard materials, we also allow values in [0, 10] on Sketchfab, and it almost obvious that other softwares are allowing it too.

For now, exporter are clamping the values to be compliant with glTF specification, but all "glowing" models are not exported as they should be.

Thanks!

@pjcozzi
Copy link
Member

pjcozzi commented Sep 1, 2017

Good question, @AurL!

@mlimper or @bghgary do you remember the reasoning here?

@mlimper
Copy link
Contributor

mlimper commented Sep 4, 2017

I'm not a material expert, not sure what is common, but being able to support bloom effects / HDR ranges seems like a good point.

My two cents here would just be that this affects definition of light sources, so it should probably be aligned with the way in which lights / environments and their respective (HDR) ranges would be specified (which is still missing)?

@bghgary
Copy link
Contributor

bghgary commented Sep 5, 2017

I think this is probably an oversight. I can't see any good reason why emissiveFactor should be restricted like it is specified currently.

@AurL AurL changed the title HDR values for emissiveFactor ? HDR values for emissiveFactor? Sep 13, 2017
@donmccurdy
Copy link
Contributor

My two cents here would just be that this affects definition of light sources, so it should probably be aligned with the way in which lights / environments and their respective (HDR) ranges would be specified (which is still missing)?

We're getting fairly close on KHR_lights, but not environments... anything we should revisit here? Or should we just update the valid range of emissiveFactor for now?

@donmccurdy
Copy link
Contributor

Oh, I just read the updates from @bghgary in #1193

We can't change the schema like this and still meet versioning requirements. For example, older versions of native apps that use the old schema to validate will fail to load these new assets.

The ways I can see this working is if we add another factor value that either replaces the old value if recognized or multiplies against it. The old emissiveFactor must continue to be populated with a reasonable fallback value for older clients.

Tagging this issue as "glTF Next" since it might not be something we can address without a 2.X update. The alternative would be adding a scalar e.g. emissiveIntensity that multiplies against emissiveFactor?

@mlimper
Copy link
Contributor

mlimper commented Mar 5, 2018

Tagging this issue as "glTF Next" since it might not be something we can address without a 2.X update. The alternative would be adding a scalar e.g. emissiveIntensity that multiplies against emissiveFactor?

Hm, I guess if this is not super urgent then it may be a better / cleaner solution to wait until the next release and then just adapt the valid range for emissiveFactor (so we don't make the spec more complicated than necessary)

@AurL Is this a blocker at the moment, or are there currently workarounds in practice / in your workflows?

@donmccurdy
Copy link
Contributor

/bump. This primarily seems like a bug/oversight. Do we think that we need to wait for a future release to fix it? Are there compelling reasons (e.g. __% of usage in Sketchfab) to prioritize this sooner?

@bghgary
Copy link
Contributor

bghgary commented Dec 3, 2018

Do we think that we need to wait for a future release to fix it?

We can't change the scheme and maintain backward compatibility, so I think either we add an extension or wait for a future release.

@MiiBond
Copy link
Contributor

MiiBond commented Dec 4, 2018

I'd just like to add that this was something that I ran into with the Adobe Dimension export. It really wrecks certain scenes to have to clamp emissive to 1.0.
I had toyed with the idea of just ignoring the spec and outputting >1 values from Adobe Dimension when needed but I guess I should just define a simple vendor extension until PBRNext.

@frguthmann
Copy link

"The emissive map controls the color and intensity of the light being emitted by the material"

It would be great not only to change the range of emissive but also to clearly specify the unit for this value. I see two different ways to add emission to a material:

  • As an EV compensation to the current exposure. This is a non physical but artist friendly parameter.
  • As an absolute physical quantity. Probably luminous intensity in this case? (Candela)

@devshgraphicsprogramming

@frguthmann I propose using Luminous Watts/m^2/steradian, its a nice metric which happens to 1:1 match the value that should be displayed on the non-tonemapped non-autoexposed screen (pixel value) for a directly visible emitter without any interreflection (emitter in space)

If you choose any other unit then the surface would get brighter/darker depending on the surface area of the emitter.

@donmccurdy
Copy link
Contributor

I think it is important to include a resolution for this issue somewhere on our "PBR Next" roadmap. It's really quite a major limitation for the emission property, and I consider that a more common feature than many of the PBR properties we're bringing into the material specification now.

@emackey
Copy link
Member

emackey commented Sep 14, 2020

I think we're agreed that an extension should introduce a new value that somehow modifies the existing emission texture * factor equation.

What's the preference here?

  • Simple linear modifier (default 1.0)

    • emit = sRGBtoLinear(emissiveTexture) * emissiveFactor * emissiveExtensionValue
  • EV (exposure compensation) (default 0.0)

    • emit = sRGBtoLinear(emissiveTexture) * emissiveFactor * pow(2, emissiveExtensionValue)
  • Luminous Watts/m^2/steradian

    • emit = ...... ?

For the shader code, I would imagine that the combination of emissiveFactor and emissiveExtensionValue would happen at the app level, and sent via uniform into the shader as a single combined factor that could go well above 1.0.

@emackey
Copy link
Member

emackey commented Sep 14, 2020

One more option: The extension could supply an unclamped emissive factor that outright replaces the glTF core emissive factor, rather than multiplying against it. This could allow independent control of the value for systems that do or do not support the new extension.

@donmccurdy
Copy link
Contributor

@romainguy from the options in https://google.github.io/filament/Materials.html#materialmodels/litmodel/emissive, do you have a preference on emissive units?

@emackey are there form factors other than an emissive-specific extension we could/should consider? For example:

@emackey
Copy link
Member

emackey commented Sep 14, 2020

The plan that I'm working towards (which is open for debate) is that a set of PBR Next material extensions will be developed and released on top of glTF 2.0, but they are not intended to be thought of as a loose collection of individual extensions. Instead, each one carries with it "future core" material properties. Client implementations are welcome to treat these new properties (along with their default values) as part of their core material definition and API, as soon as they are released. The glTF extension mechanism is being used as merely a vehicle to deliver settings that were left out of 2.0 core schema. In other words, the extension itself is merely disposable packaging.

I'd prefer not to release an interim roll-up such as a KHR_materials_principled kind of thing. Instead, individual PBR Next extensions are implemented, tested, vetted, over the course of the coming 6 months to a year or more. By the end of that time, we should have a finely-tuned set of material parameters, and the only remaining problem with them (we hope) is that they are strewn across a set of separate glTF 2.0 extensions.

At that point -- after the PBR Next material settings are well-known, well-implemented, and well-tested -- we'll be ready to introduce glTF 3.0. glTF 3.0 will have breaking schema changes from 2.0, but it won't introduce any radical new PBR concepts. Instead, it will pull all of the well-tested parameters from all of the PBR Next extensions that became successful (by some metric), promoting all of those parameters to be the new 3.0 core material definition. It could drop the emissive factor restriction at the same time, so perhaps a replacement bug-fixed emissive factor is all that's needed on that front in PBR Next.

Client implementations that have followed along with the PBR Next effort shouldn't have to work very hard to upgrade to 3.0, if all that happens is that material properties are lifted from various extensions and relocated to core. glTF 3.0 could also pull in any other fixes needed, for example I think that stricter skinning requirements is a great suggestion.

At least, this is how it all plays out in my head.

@donmccurdy
Copy link
Contributor

As big as the PBR Next changes are, do they actually require a major version increment? We can add schema properties with just a 2.x version, correct?

The overall arch you describe sounds reasonable to me, and I'd be glad to avoid a generic "principled" extension in the meantime. But I think we may want to allow some considerable time before we bundle everything up (i.e. years, not months) and it would be nice if bug fixes weren't dependent on a major release.

@emackey
Copy link
Member

emackey commented Sep 18, 2020

We can ask, but bear in mind this is a file format version, not a software release version. I think major version increments are probably the clearest messaging we have. In the interim we can ship extensions instead of patch/minor releases. By mid-next year it will be 4 years since we had a release. But no rush, we should get all of PBR Next and KTX2 done, deployed, and battle-tested before we think about a major release.

@devshgraphicsprogramming
Copy link

devshgraphicsprogramming commented Sep 21, 2020

Personally I dislike "principled" shading models, there's a bunch of parameters that dont really correspond to anything like "sheen" and "edgetint" which are really hard to map back to proper PBR materials like the ones found in Mitsuba or PBRT.

You should see the pain of inversion of "artist friendly fresnel" for metallics (a F0 and F90 color value) back to 3 components complex Index of Refraction.

@fu5ha
Copy link

fu5ha commented Mar 18, 2021

I don't see how allowing > 1.0 is breaking backward compatibility. Simply allowing it to go greater than 1.0 as a multiplier while keeping the same formula, i.e.

emit = sRGBtoLinear(emissiveTexture) * emissiveFactor

will still provide the same behavior for values in [0, 1]. Going greater than 1.0 (which was technically illegal before) is then effectively exactly the same but much more intuitive than the proposed separate emissiveExtensionValue. Also, the proposed best unit, Watt/m^2/Steradian is essentially the same thing... We interpret the emissiveFactor as W/m^2/Sr., and when an emissiveTexture is present, we interpret that as a unitless scalar multiplication/attenuation, still ending up with W/m^2/Sr. in the end.

@carmandale
Copy link

is there a work-around in the mean-time? or are we clamped to 1 until a new version?

@emackey emackey linked a pull request Apr 24, 2021 that will close this issue
@emackey
Copy link
Member

emackey commented Apr 24, 2021

I don't see how allowing > 1.0 is breaking backward compatibility

The emissiveFactor limit is in the official JSON schema used for validation. Ignoring that would produce assets that don't work across the entire ecosystem, for example they wouldn't load into MS Office and other desktop products that validate against schema for security reasons.

are we clamped to 1 until a new version?

No, this will be fixed for glTF 2.0 using an extension. One draft has appeared in 1971 #1994 for consideration. Making this an optional extension means that it can be safely ignored by apps that don't have it implemented yet.

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

Successfully merging a pull request may close this issue.