glTF-Blender-IO icon indicating copy to clipboard operation
glTF-Blender-IO copied to clipboard

Export lights using lumens, candelas, correct `GLTF`/`KHR_punctual_lights` units.

Open will-ca opened this issue 3 years ago • 4 comments

Closes #564.

Export mode selection:

image

Enum options and tooltips:

convert_lighting_mode: EnumProperty(
    name='Lighting Mode',
    items=(
        ('SPEC', 'Standard', 'Use standard glTF lighting units (cd, lx, nt)'),
        ('COMPAT', 'Compatibility', 'Use unitless PBR lighting'),
        ('RAW', 'Raw', 'Use Blender lighting strengths directly'),
    ),
    description='Optional backwards compatibility for non-standard render engines. Applies to lights',# TODO: and emissive materials',
    default='SPEC'
)
Correctness Tests

Blender scene:

image

Spec-compliant export, Khronos viewer with exposure set to compensate for unitless pipeline:

image

Spec-compliant export, Babylon.JS with exposure set to compensate for unitless pipeline:

image

Unitless export, Khronos viewer:

image

Unitless export, Babylon.JS:

image

Unitless export, Three.JS:

image

I also tested re-importing exported files back into Blender, in all three conversion modes, to make sure they still looked right.

Structural changes to wider code:

  • Renamed "Geometry" export subpanel to "Data", as it already contained material and now contains lighting options.
  • Added class ConvertGLTF2_Base, for properties that apply to both import and export.
  • Defined PBR_WATTS_TO_LUMENS in gltf2_blender_conversions, the first constant in that file.
Original Post

I believe this makes this part of the exporter... As spec-compliant as any other software. However.... Watts are huge. Or lumens are tiny. When I was in high school I had this 10W LED headlamp rated for 600lm. At 10% LED efficiency, that works out to around 600 lumens for every 1 watt of emitted light, which is also what Wolfram Alpha tells me to expect at a mid-visible wavelength. Obviously multiplying all exported light intensities by 600 also makes the models look a bit different. To get around that, I added an export_lights_factor setting that can be used to scale exported lumens back down, defaulting to 1.0 (technical physical correctness) but with a tooltip recommending inputting 1/600 for most export targets.

I suppose for a lumens-based workflow with Three.JS, you would want to use WebGLRenderer.physicallyCorrectLights. It is unfortunate, however, that neither of the reference viewers support anything like that, and unclear whether with Three.JS even that will correctly handle GLTF values as lumens per square radians as per the spec or just raw lumens, which would still be incorrect.

~~Note that there is also another implication of the spec that I have not implemented, as none of the reference model viewers implement it either, and I'm not sure it's intentional. I left a code comment explaining this. More info: https://github.com/KhronosGroup/glTF/issues/2213~~

~~I explicitly decided not to include a backwards-compatibility checkbox to continue exporting in watts, as that behaviour was incorrect. I figure it's not part of the spec, and every vendor and tool doing their own thing is how you end up with E.G. Collada.~~ I decided to include it as a "raw" mode for finer user control, rather than a "compatibility" mode to perpetuate incorrect results.

Example/Test case

EEVEE Render:

PunctualRendered

Blender 3.3 export:

PunctualExportedWatts

This PR (w/ "Lights Strength" set to 1/600): PunctualExportedLumens

"Mr. Elephant" by Glenn Melenhorst, from Demo Files on blender.org. Used as a demonstrative— Any problems you see in it are the result of me bodging the export.

will-ca avatar Oct 12 '22 00:10 will-ca

~~Actually, maybe hold off on this until a response has been decided for https://github.com/KhronosGroup/glTF/pull/2214. This is ready for merge, but TL;DR: Lumens are not even a physically based unit in the first place, and that leads to all sorts of problems, of which the conversion trickiness that held up #564 for over three years are only the beginning. I'm just some guy, but it is my opinion that the spec for KHR_punctual_lights needs to be changed to use a different unit, and that due to negligible adoption of the units parts of the spec (possibly due to the natural unsuitability of lumens), changing it may not even cause any problems.~~

~~One thing I did not implement here, also, is converting back to watt-based units on import. Frankly I don't really see a sane way to do that as long as lumens are the spec-conformant unit. It's.... Like converting between "number of apples" and "miles per hour"— Wolfram Alpha, GNU Units, etc. will also in fact just spit out a dimensional compatibility error (possibly with a wild guess based on arbitrary assumptions) if you try to make them do it.~~

~~EDIT: After poking around some more, I think it may not actually be possible to test this because there aren't any model viewers that correctly handle the units as defined in the spec.~~

will-ca avatar Oct 12 '22 03:10 will-ca

After the discussion in https://github.com/KhronosGroup/glTF/pull/2214, I'm wondering what's the "least bad" way to accommodate the, uh, practical reality of unitless workflows in the UI options here.

Does it sound correct to say that if we include the 4π divisor, but not the 683 lumens per watt factor, we'll be providing values that are likely to be comparable (based on your tests) in other engines working with exposure=1 / the unitless workflow?

If so, perhaps we could expose this as a dropdown selection with two options, labeled something like:

  • (A) Absolute | Physical | ...
  • (B) Relative | Unitless | ...

It may be worth throwing in a tooltip about the actual units, the necessity of adjusting exposure to accommodate absolute units, and the fact that relative units are not really valid according to the spec... With that we could probably do with just one option, rather than a checkbox and a scalar.

@emackey — Do you feel OK about this approach? The unfortunate truth is that making punctual light exports "correct" will cause them to look dramatically over-exposed in all viewers today, without adjustments in exposure relevant to https://github.com/KhronosGroup/glTF/pull/2128. That's my motivation for retaining a technically-incorrect "Relative" option above.

donmccurdy avatar Oct 15 '22 03:10 donmccurdy

Does it sound correct to say that if we include the 4π divisor, but not the 683 lumens per watt factor, we'll be providing values that are likely to be comparable (based on your tests) in other engines working with exposure=1 / the unitless workflow?

Yep, that should work, and has now been further confirmed by further testing.

Thanks for working through all this with me, I've learned a few things here. slightly_smiling_face

Thank you for taking the time to read it all!

If so, perhaps we could expose this as a dropdown selection with two options, labeled something like:

  • (A) Absolute | Physical | ...
  • (B) Relative | Unitless | ...

It may be worth throwing in a tooltip about the actual units, the necessity of adjusting exposure to accommodate absolute units, and the fact that relative units are not really valid according to the spec... With that we could probably do with just one option, rather than a checkbox and a scalar.

I've now implemented three modes in an dropdown/enum:

  1. "Standard": Spec-compliant.
  2. "Compatibility": Unitless.
  3. "Raw": The original behaviour without any conversion, in case anyone was relying on that, and for custom workflows where an artist does not care about visual equivalence in EEVEE but does want to be able to manually specify the exact number that will be in the glTF file.

I've chosen names for the three modes that describe less of what the modes actually do, and more of how they should be seen and used. The default, spec-compliant mode is called "Standard", while the unitless mode is called "Compatibility" and the original behaviour is called "Raw". Additionally, whole-enum tooltip also uses vaguely cautionary words like "backwards compatibility" and "non-standard render engines". The per-option tooltip for the "Standard" mode is the only one that uses the word "glTF", and it also uses the word "standard" again too. The per-option tooltips for "Compatibility" and "Raw" imply missing stuff or skipping steps.

convert_lighting_mode: EnumProperty(
    name='Lighting Mode',
    items=(
        ('SPEC', 'Standard', 'Use standard glTF lighting units (cd, lx, nt)'),
        ('COMPAT', 'Compatibility', 'Use unitless PBR lighting'),
        ('RAW', 'Raw', 'Use Blender lighting strengths directly'),
    ),
    description='Optional backwards compatibility for non-standard render engines. Applies to lights',# TODO: and emissive materials',
    default='SPEC'
)

Hopefully that should make it implicitly clear the only the default, "Standard" option is officially/technically supported according to the spec.

The per-option tooltips, meanwhile, describe what they will actually do. So hopefully the UX/information exposure path should go something like:

  • See it says "Standard" and leave it alone.
  • If still curious, mouse over and see "backward compatibility for non-standard render engines", and leave it alone.
  • If looking for a solution for overexposure, try "Compatibility", which should work, without necessarily needing to know the details.
  • If looking for a solution for specialized use cases, only now read the per-option tooltips to learn what they actually do.

Eventually material emission strength will need export conversions too. In choosing the enum and the options for it, I additionally tried to consider what would make sense for controlling behaviour in that context too.

At one point I meant to include a longer-winded explanation of the situation with the specs and the units in a separate collapsible panel:

image image

However, I have removed this, as I don't think it's really necessary, you can't actually fit much text on-screen, and multi-line text is always an ugly hack in Blender.

will-ca avatar Oct 16 '22 00:10 will-ca

Using the name "Standard" rather than "Physical" is fine with me, I'm good with either.

I'm not so sure about "Compatibility". Currently we are basing the assumption of compatibility on evidence from a few viewers, which don't necessarily represent the underlying engines themselves, let alone more established engines like Unreal and Unity. Whether most workflows are unitless workflows seems like anyone's guess, though I suspect it's true for game development at least. I haven't found any units associated with lights in, for example, FBX. Moreover I'm hoping we can fix the behavior in viewers sooner than later, in which case compatibility should be fine under the "Standard" case.

I don't think the current behavior, labeled here as "Raw", is bringing anyone much joy. I believe we should remove it sooner or later. If we prefer to keep it around for now, perhaps we add a "(Deprecated)" disclaimer to that option?

tl;dr - suggested wording:

  • Standard: Physically-based glTF lighting units (cd, lx, nt)
  • Unitless: Non-physical, unitless lighting. Useful when exposure controls are not available
  • Raw (Deprecated): Exports strength of Blender lamps with no conversion

Finally, we may need similar options for the importer as well. We can leave that for another PR of course.

donmccurdy avatar Oct 16 '22 16:10 donmccurdy

I'm basing "Compatibility" off of the term "Compatibility Mode"— I.E., something that may or may not imply wider compatibility, but which is meant to make older/deprecated behaviours that are usually incompatible into something that is compatible. That said, if that's not what it implies, then yeah that should probably be changed.

I felt the issue with just calling it "Unitless" would be that that wouldn't make it clear that it's technically not correct. Although, the new tooltip you suggest would both address that and also clarify its intended use case, so I think that would be a good change to make.

There are already options for the importer. I put the EnumProperty annotation on a common base class of the importer and exporter modal operators, so the same dropdown and tooltips get used in both— That does also mean I had to avoid using the word "export" in the tooltips, but reducing code duplication/having a single canonical definition of the modes is worth it IMO.

About "Raw".... My feeling is that on some level the energies are just a float value, and we should still be able to export that into the output file without any transforms added on it. Yes it means that EEVEE, Cycles, and Blender units will look different from glTF spec, but that's only an issue if (1) we take EEVEE/Cycles as the standard and (2) we assume that direct, in-GUI visual equivalence is always the goal. For applications like archviz, or scientific simulation of scene luminances, for example, I could see it being more important to be able to directly type in e.g. "700.00 exactly", than to tweak values based on how they visually look in Blender. I don't know if that's a workflow anyone else would use— I can imagine myself having or inventing use cases where I would want to be able to do that, so I added the option. (Though perhaps the more appropriate solution would be for Blender to allow exposing lumens as the lighting unit— Not sure.)

will-ca avatar Oct 17 '22 01:10 will-ca

Thanks, I see your reasoning on "Compatibility." I'd be fine with either here, but I would use the word "unitless" in the description if not as the label.

My worry (with non-spec-compliant options in general) is creating confusion in the glTF ecosystem. E.g. we add these three options, another tool tries to support these three modes and adds a fourth, etc. until there are multiple de facto behaviors outside of the spec. I'm mostly inclined to leave the dedicated "just edit the JSON" workflow to the dedicated tools like Gestaltor or the VSCode extension.

But, no need to make further changes to address my comments here. If @julienduroure, @emackey, and/or @scurest are happy with this wording then so am I. :)

donmccurdy avatar Oct 17 '22 02:10 donmccurdy

Just a suggestion:

  • Physical
  • Unitless
  • Raw (deprecated)

This way we're not asking folks to look up the spec just to understand one of the options (physical). Same with "compatibility," there's no need for the user to understand what's compatible with what, just let them choose "physical" or "unitless."

emackey avatar Oct 17 '22 13:10 emackey

Currently:

https://github.com/KhronosGroup/glTF-Blender-IO/blob/5c3ff6de1245e295c5d1e3f232b4db2c8f729f76/addons/io_scene_gltf2/init.py#L115-L124

@emackey I'm not against adding "(deprecated)" to "Raw". Upon donmccurdy's further explanation, I agree it might be a good idea as anyone with more specialized use cases should probably be ready to have have other/their own tools anyway.

This way we're not asking folks to look up the spec just to understand one of the options (physical). Same with "compatibility," there's no need for the user to understand what's compatible with what, just let them choose "physical" or "unitless."

I actually also think this but with the opposite conclusion. IMO "Physical" is meaningless unless you also know the watts-to-lumens and steradian conversions, exposure requirements, etc., with all the theory, implementation details, and specs that go with that. So I thought "Standard" and "Compatibility" would communicate the intended use of each mode more clearly— "Stick with the default and it should work, and try the fallback if it doesn't"— Without requiring users to look up the spec and formulae for what "Physical" and "Unitless" actually mean in this context. I.E. From the perspective of the artist, I don't perceive/intend "Standard" to just mean "technically spec-compliant", but more "This is the one you should use".

Also I feel that if one of them isn't clearly marked as "this is the Standard, spec-compliant official default you're supposed to use and everyone is supposed to support", then that's just asking for fragmentation later down the line.

Worth pointing out that regardless of whether we call it "Standard" or "Physical" or "Compatibility" or "Unitless", the tooltips still explain in greater detail what they mean anyway.

will-ca avatar Oct 17 '22 15:10 will-ca

Ah, OK. I'm fine with this.

emackey avatar Oct 17 '22 15:10 emackey

Cool. Should be good to go, then.

Also debating whether the conversion coefficients should be refactored as constants shared in export and import. But I've already exhaustively tested the current behaviour (see top comment), so if so that should probably be a separate PR I think.

will-ca avatar Oct 17 '22 15:10 will-ca

Ok, seems everyone agreed this PR. I will merge before end of the week, it will be in Blender 3.4

julienduroure avatar Oct 20 '22 18:10 julienduroure