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

[glTF] Very high light range values of imported lights are causing shadows not to render #92551

Open
passivestar opened this issue May 30, 2024 · 5 comments

Comments

@passivestar
Copy link
Contributor

passivestar commented May 30, 2024

Tested versions

v4.3.beta.custom_build [8bf8f41]

System information

Godot v4.3.beta (8bf8f41) - macOS 14.4.1 - Vulkan (Forward+) - integrated Apple M1 Max - Apple M1 Max (10 Threads)

Issue description

This point light is imported with a range of 4096. Even though it has shadow enabled you can't see it:

image

Shadow appears when you reduce its range all the way down to 300:

image

There's a same problem with spotlights iirc. Perhaps if the imported glTF doesn't provide a range value it should use Godot's default light range instead of an abnormally high one

Steps to reproduce

MRP

Minimal reproduction project (MRP)

MRP_light_range.zip

@Calinou
Copy link
Member

Calinou commented May 30, 2024

Perhaps if the imported glTF doesn't provide a range value it should use Godot's default light range instead of an abnormally high one

If the light is meant to have infinite range in the source glTF, I wouldn't make it use Godot's default range, but a hardcoded value that is higher (probably between 100 and 250). I'd also only do this for lights with shadows enabled, as lights with very high ranges render correctly when shadows are disabled.

@passivestar
Copy link
Contributor Author

I'd also only do this for lights with shadows enabled

That might be a problem because blender doesn't export shadow state, it doesn't even seem to be a part of glTF from what I understand

image

https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_lights_punctual/README.md#light-shared-properties

I'm turning them on myself with an EditorScenePostImportPlugin if a light has a -shadow suffix

@Calinou
Copy link
Member

Calinou commented May 30, 2024

That might be a problem because blender doesn't export shadow state, it doesn't even seem to be a part of glTF from what I understand

Does the glTF specification even have a way to signal that certain lights should have shadows enabled? If not, it doesn't sound very usable for game development where you can't be "all or nothing" in real world projects 🙁

I suppose we could add support for a -shadow import hint and/or offer an import option to set omni/spot shadow range thresholds (where all lights larger than a certain range have shadows enabled), but that's not great to hear.

@passivestar
Copy link
Contributor Author

Does the glTF specification even have a way

No

I suppose we could add support for a -shadow import hint

I can't think of a reason not to

@clayjohn
Copy link
Member

We shouldn't default to such a high range. Every scene i've worked on I've had to manually go reduce the range to something reasonable (oftentimes less than 10).

With proper inverse square falloff, a light with a brightness of 1 won't be visible beyond 20 meters or so (even if the range is significantly higher than that).

We used to calculate attenuation based on range and intensity. Now we don't touch attenuation because GLTF doesn't have a setting for it. This is a bit of a problem. Godot defaults to a linear falloff (attenuation = 1) so lights imported from GLTF are effectively imported with a linear falloff.

If we assigned GLTF lights a proper inverse square falloff (attenuation = 2), then we could easily cap their range at 100 or so when no range is specified (and then scale up the range when there is more intensity). At that range, there would be no visible difference with an increased range. However, with a linear falloff, we need a much larger range before we can cap it.

Technically, we are not compliant with the GLTF spec right now since GLTF lights are not using the inverse square law.

I suggest we do the following:

  1. Import lights with an attenuation of 2 when importing from GLTF
  2. Default to a range of 100. If intensity is specified, but range isn't scale up range based on intensity (I suggest increasing by a factor of clamp(intensity / 100.0, 1.0, 40.96) as below 100 intensity a range of 100 is fine, then it should scale up linearly after that until a max of 4096)

Of course switching to inverse square falloff will technically break compatibility, but we are not compliant with the spec right now. So I think the breakage is justified.

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

No branches or pull requests

3 participants