Improve light render parity with blender #2549
Conversation
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
| float clampedDist = max(dist, 0.00001); | ||
| light.fallOff = clamp(1.0 / (clampedDist * clampedDist), 0.0, 1.0); | ||
| light.fallOff = clamp(light.fallOff, 1.0 - posLight, 1.0); | ||
| light.fallOff *= step(dist, radius); |
There was a problem hiding this comment.
this is the inverse square falloff used by blender with a radius cut-off, since jme allows to set the radius of lights
| @@ -739,7 +739,9 @@ public static ColorRGBA getAsColor(JsonObject parent, String name) { | |||
| return null; | |||
| } | |||
| JsonArray color = el.getAsJsonArray(); | |||
| return new ColorRGBA(color.get(0).getAsFloat(), color.get(1).getAsFloat(), color.get(2).getAsFloat(), color.size() > 3 ? color.get(3).getAsFloat() : 1f); | |||
| return new ColorRGBA().setAsSrgb( | |||
| color.get(0).getAsFloat(), color.get(1).getAsFloat(), color.get(2).getAsFloat(), color.size() > 3 ? color.get(3).getAsFloat() : 1f | |||
| @@ -56,6 +56,9 @@ | |||
| * Created by Trevor Flynn - 3/23/2021 | |||
| */ | |||
| public class LightsPunctualExtensionLoader implements ExtensionLoader { | |||
| private static final boolean COMPUTE_LIGHT_RANGE = true; | |||
| private static final boolean APPLY_INTENSITY_CONVERSION = true; | |||
| private static final boolean SKIP_HDR_CONVERSION = true; | |||
There was a problem hiding this comment.
you can use these toggles to test with and without the patches
| intensity = intensity / solidAngle; | ||
| } | ||
|
|
||
| float range = obj.has("range") ? obj.get("range").getAsFloat() : (COMPUTE_LIGHT_RANGE ? getCutoffDistance(color, intensity) : Float.POSITIVE_INFINITY); |
There was a problem hiding this comment.
The POSITIVE_INFINITY range was an error to begin with and required a patch to turn it into FLOAT_MAX_VALUE #2058 , but it ends up introducing some floating point errors with large numbers that makes the light calculation a bit off.
Truth is: jme doesn't support infinite lights very well.
So this patch calculates the right range based on the falloff function.
jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java
Outdated
Show resolved
Hide resolved
| vec3 spotDir = normalize(light.spotDirection); | ||
| float cosA = dot(-L, spotDir) ; | ||
| float spotAtten = clamp((cosA - outerAngleCos) / (innerAngleCos - outerAngleCos), 0.0, 1.0); | ||
| light.fallOff *= pow(spotAtten, sharpnessFactor); |
There was a problem hiding this comment.
This smooths out the angular attenuation. It doesn't look exactly like blender's.
|
Screenshots look much nicer. With the exception of my comment on the println: looks good! |
Maybe in the short term a parameter that defaults to false and at some point (on a major version change) the default flips to true and then a few versions later the parameter gets deleted |
riccardobl
added
the
defect
|
I never knew this was an issue, but I have certainly noticed that JME's point lights illuminate the scene way too much, and it is near impossible to tweak them properly in dark scenes without ruining the darkness. I could never make a proper bright torch in a dark room without it completely brightening the entire room. Then if I turn the brightness or radius down to fix that issue, the torch becomes too dim close up and looks bad. Its like I could never properly tweak point lights. So I am very excited for this PR!
This sounds like a good solution. We could probably even make it true by default right away in the next alpha version, that would be beneficial in the short term for alpha and beta testing, that way testers will be forced to test this (in my experience with 3.8, its better to force new changes on people using alpha versions, otherwise people will overlook the togglable option and it won't get tested as much). Then (depending on feedback) we can decide whether it should default to true/false for the next stable release. |
When importing a gltf model, lights look nothing like in blender.
Turns out jme doesn't use proper physically based light attenuation, and that blender might be doing some shenanigans in spot lights.
In this PR i've patched jme to make it much more closer to blender lighting.
See screenshot below:
Blender:
JME Without this PR:
JME With this PR:
It is still not perfect, but much better i think.
This PR needs more work, mostly to figure out how to merge it, since it will break all the existing scenes setup.
I am looking for suggestions on this regard (maybe we can make it toggleable with a PBRLight param?)