godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.81k stars 20.14k forks source link

GLTF Lights import with wrong value due to specification change #73624

Open RevoluPowered opened 1 year ago

RevoluPowered commented 1 year ago

Godot version

4.0 - rc2

Issue description

In blender 3.3 the light output in a GLTF file is in watts in blender 3.4 the light output is in a new unit due to a specification change in GLTF

This means that when you import a light with blender 3.3 it will work properly in godot 4, but when you use blender 4.4 it will export the light power in a very high value.

The specification for GLTF is here and contains how the punctual lights now use the new units of measurement: https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_lights_punctual/README.md

The change in blender is here: https://github.com/KhronosGroup/glTF-Blender-IO/commit/3cf4d007bbf5ae5d9266e4be53e3200fdfcf8fc5

The discussion on why this was changed is here: https://github.com/KhronosGroup/glTF/pull/2214

For the devs of GLTF: I would like if we had unit column in the .glb for the light unit used, as now files are incompatible when we make this change and or will require a user to export again, which makes original uploaders have to ensure a blender version, or the engine to check the file version.

From a user perspective it will break for them. I made this note here so that its shown on GLTF github too.

Perhaps even though a small change like this should have been a major version change.

Steps to reproduce

  1. Open Godot 4 RC1/RC2 and import the file provided.
  2. Open the light up, the light units will be wrongly set to a much higher value than 500 watts.

You can reproduce the issue running portable blender 2.3 and exporting a light, and also exporting another light from 3.4.

Minimal reproduction project

A 500 watt light from blender 3.4 lamp-big-500W-on.zip

RevoluPowered commented 1 year ago

Tagged as rendering too as @reduz or @clayjohn might want to use the new lighting unit too, but unsure.

PZerua commented 1 year ago

I looked into this a few weeks ago but ended up convincing myself that it made no sense to import lights from blender and better create them directly in Godot. When exporting lights from Blender 3.4 there are now three modes: Standard, Unitless and Raw (how it used to export it). In Godot there used to be a line to convert Blender light units to Godot units but was removed in a refactoring (https://github.com/godotengine/godot/commit/3d76b91229c469879ec0eb42f54b5e2667270209#diff-fdf33f6fcd2b129d540ebd0e70fd04982b68e279cebfd9e96036d2fd7561c31fL5153).

The thing is, Godot by default uses a light unit that is more "artistic driven", so a direct conversion might be difficult. We could try and estimate a "good" conversion factor that looks good enough, which I tried, but It would look different depending on which mode you choose to export. Right now it just looks completely broken because the light is way too bright (another option is tweaking the exposure of your game so everything looks right with brighter lights, if all your lights are imported from Blender).

My rabbit hole ended when I tried again but with "Physically light units" activated, but it still looked broken. I think it should look right there, so there is probably a bug.

clayjohn commented 1 year ago

Ah, wonderful they finally moved to using physically based units.

On our end it should make it much simpler to convert intensity values accurately. When using physical light units it should be possible to just import the values as-is. When not using physical light units we can scale the value to something that is reasonable given the fact that there is no exposure by default

daBlesr commented 1 year ago

Without using physical light, assets are overexposed with the current import settings for both the old and the new units of light compared to how it is rendered in Blender. Since it is unfair to users to break backwards compatibility with old Blender assets, may I suggest to parse the version of the package that is used in Blender to export glTF files (https://github.com/KhronosGroup/glTF-Blender-IO), and use scaled or as-is values as @clayjohn suggested, when lux units are used in gtTF file.

The version from which they changed Watts to Lumen is 3.4.40. At the top of the exported glTF file this is shown:

{
    "asset" : {
        "generator" : "Khronos glTF Blender I/O v3.4.50",
        "version" : "2.0"
    },
    ...
}

I have verified that Blender 3.4 uses version 3.4.50 and Blender 3.3 uses v3.3.36.

clayjohn commented 1 year ago

That sounds like an excellent idea

daBlesr commented 1 year ago

A more general solution, is a light intensity slider on the import configuration editor for GLTF files. That would help people independent of Blender versions, or GLTF's exported from other Modelling software, and version changes related to light in the future. So they can always change the intensities for all lights in their imported scene at once.

bluediamond87 commented 1 year ago

Looking forward to thus fix team! I'm currently planning out the pipeline with the artist. I definitely want the option to try to import blender's lights.

Calinou commented 1 year ago

@aaronfranke Do you know why Blender unit conversion code was removed in https://github.com/godotengine/godot/pull/64977?

A more general solution, is a light intensity slider on the import configuration editor for GLTF files. That would help people independent of Blender versions, or GLTF's exported from other Modelling software, and version changes related to light in the future. So they can always change the intensities for all lights in their imported scene at once.

This makes me wonder if light range should have a multiplier too, along with the attenuation curve (since all these parameters tend to work in tandem). Range/attenuation scale would affect only omni/spot lights, while energy would affect all light types. If you don't use physical light units, you generally aim for a visually attractive look which often requires taking liberties with how input units are treated.

It's common for level design tools in general to offer a way to multiply the energy or range of all lights in the scene, so that you can perform quick adjustments if the scene is too dark/bright.

will-ca commented 11 months ago

Right.

I think there's some confusion in this thread, because it's kinda a pretty complicated issue.

And also because I proposed a radical change in a popular specification, then withdrew that proposal, and then instead changed behaviour in a popular authoring tool based on the results of the complicated discussions surrounding that proposal, I suppose.

I guess I'll try to clear that up.


GLTF Lights import with wrong value due to specification change

in blender 3.4 the light output is in a new unit due to a specification change in GLTF

The discussion on why this was changed is here: KhronosGroup/glTF#2214

To my knowledge, the KHR_lights_punctual GLTF specification has not changed as described in this issue #73624. In fact, if you check its commit history, you can see that it has hardly changed at all since 2018.

I proposed the linked PR because there was a lot of confusion and ambiguity across the various renderers about how (and whether) the specification was actually implemented. My own use case was that I simply wanted to be able to export with physically based units from Blender. But implementing the specification required a coefficient that wasn't explicitly defined, and when I both tried to infer it physically and later used a widespread industry standard, experimental exports following that spec were very, very wrong (overexposed) in renderers like Three.JS, Babylon.JS, and even the official Khronos group reference renderer.

Because of the ambiguity and incomplete implementations/documentation around various reference renderers, I concluded in despair at one point that changing the spec would be easier and less disruptive than fixing the Blender exporter.

However, the conclusion of the linked discussion was to not change the specification. Instead, we figured out how to implement the existing specification correctly in Blender, and we figured out why that didn't look right in the reference renderers and how they might handle it better in the future.


For the devs of GLTF: I would like if we had unit column in the .glb for the light unit used, as now files are incompatible when we make this change and or will require a user to export again, which makes original uploaders have to ensure a blender version, or the engine to check the file version.

From a user perspective it will break for them. I made this note here so that its shown on GLTF github too.

Perhaps even though a small change like this should have been a major version change.

To emphasize, the KHR_lights_punctual GLTF extension specification has not changed. This aspect of the GLTF format should still be fully compatible/remains the exact same AFAICT, and should not require either re-export nor adjustment of previously compliant tools.

What has changed is the default ways Blender exports to GLTF. The previous Blender GLTF export behaviour was incorrect. The new way now follows the spec correctly by default.


In Godot there used to be a line to convert Blender light units to Godot units but was removed in a refactoring (3d76b91#diff-fdf33f6fcd2b129d540ebd0e70fd04982b68e279cebfd9e96036d2fd7561c31fL5153).

When exporting lights from Blender 3.4 there are now three modes: Standard, Unitless and Raw (how it used to export it).

We could try and estimate a "good" conversion factor that looks good enough, which I tried, but It would look different depending on which mode you choose to export.

IIRC (and confirmed by re-reading the code):

"Standard" is the name I chose/convinced them to accept for the new, spec-compliant behaviour. The name is meant to suggest that this is the right one, because the other modes are actually technically wrong. This mode changes only two things compared to the previous behaviour: (1) We convert from total radiated power for punctual light sources, which Blender uses, to radiated power per steradian, which KHR_lights_punctual specifies, and (2) we then multiply by exactly 683 to convert from Blender's watts to GLTF/KHR_lights_punctual's lumens, which is a semi-arbitrary industry standard coefficient based on human peak perceptual luminous efficiency at 555nm green light.

"Unitless" is a new compatibility mode added because we discovered that the default exposure settings in basically all renderers (except for Filament) also didn't make much sense for compliant KHR_lights_punctual GLTF files. This mode applies the same Step 1 W→W/sr conversions as "Standard" for point-like light sources, but skips the Step 2 ×683 W→lm conversion. I.E. 1.0 in the GLTF is expected to map more or less directly to 1.0 RGB in your shaders.

"Raw" is the old, incorrect behaviour. All it does is just take the numbers you see in Blender, and save those directly in the GLTF file. If you reintroduce the removed light import code for Blender, and it worked before, then it should still work with "Raw". But again, this behaviour is incorrect. It ignores the units, and it also makes points lights significantly brighter than they should be in comparison to directional lights. If it worked correctly with Godot's GLTF import before, then that means that Godot's GLTF import was also non-compliant with KHR_lights_punctual.

To summarize. "Standard" is the only correct mode. "Unitless" is a compatibility mode for other renderers that aren't 100% to spec either. "Raw" is the old behaviour.


The relationships between the three export modes can be described as some very simple coefficients per light type. Using the old (incorrect, "Raw") Blender exporter behaviour, and internal light strengths (S), as a baseline:

Light Type "Standard"-compliant (unit) "Unitless" (unit) "Raw" old behaviour (unit)
Point-like S / (4π) * 683 (lm/sr) S / (4π) (W/sr, or no unit) S (W/sphere)
Directional S * 683 (lm/m²) S (W/m², or no unit) S (W/m²)

All of this is implemented in def __gather_intensity() at lines 82 to 96 of exp/gltf2_blender_gather_lights.py in the glTF-Blender-IO code.

So assuming the aforementioned Blender-to-Godot lighting converter worked "correctly" for the (fundamentally incorrect) old/"Raw" Blender GLTF export behaviour, you could revive it simply by multiplying by (1/683) for directional lights, and ((4*pi)/683) for point-like light sources.


Right now it just looks completely broken because the light is way too bright (another option is tweaking the exposure of your game so everything looks right with brighter lights, if all your lights are imported from Blender).

Without using physical light, assets are overexposed with the current import settings for both the old and the new units of light compared to how it is rendered in Blender.

If I had to guess, without any investigation or Godot lighting knowledge, I'd say this is probably because you're skipping the 683 coefficient needed on import to bring large lumens values down to the 0-1.0 range that most engines are scaled to show. I.E. You may be using what we eventually decided to call the "Unitless" workflow.

A typical houselamp is actually like 700 lumens (or 56lm/sr), which is naturally going to be way overexposed if an engine is scaled for "1.0" to be a reasonable light strength.


Since it is unfair to users to break backwards compatibility with old Blender assets, may I suggest to parse the version of the package that is used in Blender to export glTF files (https://github.com/KhronosGroup/glTF-Blender-IO), and use scaled or as-is values as @clayjohn suggested, when lux units are used in gtTF file.

Seeing as I initiated the discussions and wrote the changes linked to from this issue, I feel I should offer a recommendation:

  1. A compliant GLTF importer should work by default with the "Standard" option provided by Blender. The "Standard" option is the only spec-compliant export mode for GLTF/KHR_lights_punctual in Blender. If your GLTF importer does not handle this format correctly, then your GLTF importer is technically broken according to the spec. (Whether it's just overexposed (the 683 term), which is just an issue with defaults and easily changed, or conflating steradians with full spheres (the 4pi term), which breaks internal consistency across light types.)
  2. To maintain backwards compatibility, files that are labelled as having been exported from versions of Blender that didn't have the spec-compliant "Standard" option can instead be interpreted according to the old/"Raw" behaviour, using the coefficients in the table above.

OR: Alternatively, if you are indeed already using a unitless workflow, you could stick with that, bring back the old Blender converter for old files, and maybe add a note in the docs about choosing "Unitless" when exporting new files from Blender.

will-ca commented 11 months ago

A more general solution, is a light intensity slider on the import configuration editor for GLTF files. That would help people independent of Blender versions, or GLTF's exported from other Modelling software, and version changes related to light in the future. So they can always change the intensities for all lights in their imported scene at once.

…When I originally added three different options to Blender's exporter, donmccurdy had some concerns about diluting the spec:

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.

As a user I generally want my software to have options, but I do think leaving flexibility 100% open-ended in an importer also kinda defeats the whole purpose of having a standard file format. In this specific case, we decided that adding the two compatibility options to the Blender exporter was worth it, as long as we kept "Standard" as the default and labelled it as the only spec-compliant option. But I didn't mean that to create ambiguity which propagates through the ecosystem, and I would feel partly responsible for weakening the standard if that does end up happening.

It's common for level design tools in general to offer a way to multiply the energy or range of all lights in the scene, so that you can perform quick adjustments if the scene is too dark/bright.

As somebody who largely doesn't use Godot, but has used other 3D software, I think "light intensity slider" for multiplying all selected lights would be a more generally useful tool to have when working on a entire scene in general though, not just when importing.

will-ca commented 11 months ago

Oh also IMO somebody should edit the title/OP of this issue so that other people who try to figure out GLTF lighting in the future don't get the wrong idea that the specification changed spontaneously one day.

aaronfranke commented 11 months ago

@aaronfranke Do you know why Blender unit conversion code was removed in https://github.com/godotengine/godot/pull/64977?

It was an ugly hack that I hated, at first I thought it was worth it anyway but now I think we should instead fix Blender. Godot should only aim to support what's actually in the spec. As long as Godot is able to import the Khronos test model correctly, Godot is correct. If Blender uses a different unit, Blender is wrong and needs fixing. https://github.com/KhronosGroup/glTF-Sample-Models/tree/adf2abe718a61a6859b29fab481bfbbdfc05df24/2.0/Lights

will-ca commented 11 months ago

As long as Godot is able to import the Khronos test model correctly, Godot is correct.

Actually, the lighting test models in that repository tend to be misleading in that they are generally designed around unitless light strength values, without any regard for whether the values they specify actually make any sense for a physically based scene.


My own comment https://github.com/KhronosGroup/glTF/pull/2214#issuecomment-1279494303:

Note that this also means that all of the example files— If they display "correctly" in the reference viewers— Are all actually ridiculous dark— And that is compensated for by all of the rendering engines having default exposures that are way too bright. E.G. the EmissiveStrengthTest file above has emissiveStrengths of 2, 4, 8, and 16. If these are in cd/m**2 (nits) as per the spec, then according to WolframAlpha the darkest one is lower than the "luminance of a twilight sky" and the brightest is still six times darker than the "a heavy overcast day". The lowest value is so dark, actually, that apparently it is outside of the range of human sight, at "0.7 × minimal luminance for human color vision". For comparison, LCD panels seem to range from 200 nits to over 1,000 nits. So the results of that example file definitely shouldn't look as bright and bloom-y as they do, with any kind of sane default exposure settings assuming units per spec.

In the file you linked:

        "KHR_lights_punctual" : {
            "lights" : [
                {
                    "intensity" : 5,
                    "…": "…"
                {
                    "intensity" : 10,
                    "…": "…"
                },
                {
                    "intensity" : 10,
                    "…": "…"
                },
                {
                    "intensity" : 1.0000003576278687,
                    "…": "…"
                }
            ]
        }

KHR_lights_punctual specification:

Property Description Required
intensity Brightness of light in. The units that this is defined in depend on the type of light. point and spot lights use luminous intensity in candela (lm/sr) while directional lights use illuminance in lux (lm/m2) No, Default: 1.0

If we interpret the light intensities in the reference file you're using, according to the units in the specification:

Intensity Equivalent To
Red Point Light 5 lm/sr 5 candles
Green Spot Light 10 lm/sr 10 candles
Blue Spot Light 10 lm/sr 10 candles
Cyan Directional Light 1 lm/m^2 4 full moons

For comparison, a typical home lightbulb should be around 60 lm/sr, while direct sunlight is around 100,000 lm/m^2.

Basically, the test file is very dim compared to reasonable real-world light sources according to the spec. And if Godot wants to expose defaults that will look good with real-world-aligned physically based GLTF lights, then the image you posted is way too bright.


For reference, this is how gltf_viewer from Filament renders that test model on default camera settings when you disable its built-in IBL and Sun:

Image.

To get it to look anything look anything like how Godot (and the reference README) shows it, you have to crank the exposure time, ISO, and aperture to basically ridiculous levels:

Image.


It was an ugly hack that I hated, at first I thought it was worth it anyway but now I think we should instead fix Blender. […] If Blender uses a different unit, Blender is wrong and needs fixing.

As the person who wrote the current implementation of KHR_lights_punctual intensity conversions for Blender, and having spent a lot of time talking with somebody from the GLTF spec and Three.JS project about it in the process, I am confident in saying that the current KHR_lights_punctual export from Blender is "right", and compliant to the specification, when exported using the default "Standard" mode.

aaronfranke commented 11 months ago

@will-ca Makes sense then, if the test asset is not conforming to to the specification for reasonable camera settings, then the test asset is wrong and we need a better test asset. Once we have that, we can fix Godot to match Blender, since as you say Blender matches the written spec even though it does not match the example file.

fire commented 8 months ago

@aaronfranke @RevoluPowered This got lost and suggestions how to remedy this?

aaronfranke commented 8 months ago

@fire We need an example file in order to proceed.

fire commented 7 months ago

Like https://github.com/godotengine/godot/files/10783648/lamp-big-500W-on.zip ?

PastMoments commented 6 months ago

This issue also seems to appear when Importing .blend files directly within Godot

aaronfranke commented 2 months ago

@fire We don't just need the .glb file, we also need a screenshot of what it's supposed to look like. Otherwise it's impossible for us to figure out how to import it correctly.

RepComm commented 2 weeks ago

is it possible we can get a scaling value for .blend, .gltf light intensity imports? Not using any lights from blender is kind of an insane 'solution' artistically speaking.

RepComm commented 2 weeks ago

Did it myself with a @tool script https://gist.github.com/RepComm/7be8d605fc393d4a181f2658698d5d24

The script listens to scene child entered, initialization, and "fix now" boolean in editor block. Drag a blend file with lights into the scene (have to release mouse to see effects, scene enter isn't triggered until you fully place the model for some reason).

It works by using an arbitrary "light_energy_max" float (set in editor) to trigger scaling, and uses an arbitrary "light_energy_scale_factor" (set in editor) to multiply by.

PastMoments commented 2 weeks ago

is it possible we can get a scaling value for .blend, .gltf light intensity imports? Not using any lights from blender is kind of an insane 'solution' artistically speaking.

FYI the workaround is to export the .gltf from blender with the lights set to "RAW". This lets you use lights from blender normally.

RepComm commented 2 weeks ago

Appreciate the tip, and in addition for my use case of .blend files the script is still useful. Would love a simple checkbox fix for .blend even if that checkbox is a godot import setting

will-ca commented 3 days ago

FYI the workaround is to export the .gltf from blender with the lights set to "RAW". This lets you use lights from blender normally.

Note this behavior is technically non-compliant with the GLTF spec. And it doesn't just apply a uniform multiplier to light brightness; IIRC it makes point lights proportionally… times too bright, compared to directional lights (steradians vs 360° flux and flux/m²) or something like that?

We debated whether to include the "Raw" option at all, or avoid enabling fragmentative implementations. (I was in favor of having it for compatibility/artist control.)

So "Raw" results in "incorrect" behavior, producing invalid "GLTF files" that won't work right anywhere else. Though maybe Godot's import accounts for it.

aaronfranke commented 3 days ago

@will-ca Thanks for the information. In order to make Godot more compliant, we need test files, along with screenshots of what those are supposed to look like, that are fully compliant with the expected units. Otherwise, we do not have a baseline for what the "correct" behavior is supposed to be.