Improve light render parity with blender by riccardobl · Pull Request #2549 · jMonkeyEngine/jmonkeyengine

riccardobl · 2025-08-10T08:34:45Z

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?)

github-actions · 2025-08-10T08:37:11Z

🖼️ 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:

Go to the (failed run) > Summary > Artifacts > screenshot-test-report
Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html

⚠️ If you didn't expect to change anything visual:
Fix your changes so the screenshot tests pass.

✅ If you did mean to change things:
Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.

✨ If you are creating entirely new tests:
Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.

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

riccardobl · 2025-08-10T08:45:27Z

jme3-core/src/main/resources/Common/ShaderLib/module/pbrlighting/PBRLightingUtils.glsllib

+            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);


this is the inverse square falloff used by blender with a radius cut-off, since jme allows to set the radius of lights

riccardobl · 2025-08-10T08:46:50Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/GltfUtils.java

        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


gltf colors are srgb

riccardobl · 2025-08-10T08:47:09Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

 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;


you can use these toggles to test with and without the patches

riccardobl · 2025-08-10T08:51:36Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

+            intensity = intensity / solidAngle;
+        }
+
+        float range = obj.has("range") ? obj.get("range").getAsFloat() : (COMPUTE_LIGHT_RANGE ? getCutoffDistance(color, intensity) : Float.POSITIVE_INFINITY);


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

riccardobl · 2025-08-10T08:57:51Z

jme3-core/src/main/resources/Common/ShaderLib/module/pbrlighting/PBRLightingUtils.glsllib

+                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);


This smooths out the angular attenuation. It doesn't look exactly like blender's.

richardTingle · 2025-08-10T08:58:57Z

Screenshots look much nicer. With the exception of my comment on the println: looks good!

richardTingle · 2025-08-10T09:01:35Z

I am looking for suggestions on this regard (maybe we can make it toggleable with a PBRLight param?)

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

yaRnMcDonuts · 2025-08-18T18:52:19Z

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!

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

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.

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

capdevon · 2025-08-27T11:10:40Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

+        if(APPLY_INTENSITY_CONVERSION) intensity = intensity / (4.0f * FastMath.PI);
+
+        float range = obj.has("range") ? obj.get("range").getAsFloat() : (COMPUTE_LIGHT_RANGE ? getCutoffDistance(color, intensity) : Float.POSITIVE_INFINITY);
+        color = buildLinearLightColor(color, intensity);


Hi @riccardobl , it seems that the buildLinearLightColor(color, intensity) method is missing. After reviewing the file, it should probably be replaced with the lumensToColor(color, intensity) method call.

thanks, the code got botched when i merged it from my fork. Should be fixed now

capdevon · 2025-08-27T13:11:41Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

    private void addLight(Node parent, Node node, int lightIndex) {
        if (lightDefinitions.containsKey(lightIndex)) {
            Light light = lightDefinitions.get(lightIndex);
+            light = light.clone();


This is quite an important change. What kind of problem did you encounter by not cloning the light object taken from the lightDefinitions map? Could it be that the lightIndex is associated with multiple nodes, causing inconsistencies? Just curious

This is a fix.
The LightControl cannot properly move a light with its parent spatial, if it shared among multiple spatials.

Enabling #2548 by default in the gltf loader, makes the light control obsolete, so it could be removed and this fix could be avoided, but i am not sure that jme can share light instances, anyway.

Based on your explanation, cloning the object seems like the right approach. I'm not sure what effect sharing the same light source among multiple nodes would have, but I suspect it wouldn't be appropriate.
Thanks for the clarification ;)

riccardobl · 2026-03-01T19:58:34Z

@gemini-code-assist do a full review of this

gemini-code-assist

Code Review

This pull request introduces changes to improve lighting parity with Blender, specifically by implementing physically based light attenuation and updating intensity calculations for glTF lights. The changes in the GLSL shader for falloff and the Java code for light loading are significant steps toward more realistic lighting.

My review focuses on ensuring spec compliance and improving the maintainability of the new features. I've pointed out an issue with color space conversion that deviates from the glTF specification and could cause problems with models from other sources. I've also suggested making the new lighting behaviors configurable to provide a non-breaking upgrade path for existing projects, as you mentioned this is a concern. Finally, there's a suggestion to improve code quality by refactoring hardcoded constants.

Overall, this is a great improvement for PBR lighting. Addressing these points will make the changes more robust and easier to integrate.

gemini-code-assist · 2026-03-01T20:00:55Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/GltfUtils.java

+        return new ColorRGBA().setAsSrgb(
+            color.get(0).getAsFloat(), color.get(1).getAsFloat(), color.get(2).getAsFloat(), color.size() > 3 ? color.get(3).getAsFloat() : 1f
+        );


According to the glTF 2.0 specification for the KHR_lights_punctual extension, light colors are defined in linear space. This change incorrectly applies an sRGB-to-linear conversion to them by using setAsSrgb. While this is correct for some material properties like baseColorFactor, it will cause lights from spec-compliant glTF files to appear darker than intended.

To ensure correctness for all glTF files, I recommend creating a separate utility method for sRGB color conversion. For example:

Revert getAsColor to its original implementation (without setAsSrgb).

Create a new method, e.g., getAsSrgbColor, that performs the sRGB conversion.

Update the GltfLoader to use getAsSrgbColor for material properties that are in sRGB space.

This will maintain spec compliance for lights while correctly handling sRGB material colors.

return new ColorRGBA(color.get(0).getAsFloat(), color.get(1).getAsFloat(), color.get(2).getAsFloat(), color.size() > 3 ? color.get(3).getAsFloat() : 1f);

gemini-code-assist · 2026-03-01T20:00:55Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

+    private static final boolean COMPUTE_LIGHT_RANGE = true;
+    private static final boolean APPLY_INTENSITY_CONVERSION = true;
+    private static final boolean SKIP_HDR_CONVERSION = true;


These flags are useful for development. As you noted in the PR description, these changes might break existing scenes. To provide a smoother upgrade path and more flexibility, consider making these features configurable through GltfModelKey instead of hardcoded flags. This would allow users to opt-in to the new lighting model on a per-model basis.

gemini-code-assist · 2026-03-01T20:00:55Z

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java

+        final float visibleThreshold = 0.001f; 
+        final float maxRange = 10000f;


It's good practice to define constants like visibleThreshold and maxRange as private static final fields at the class level. This improves readability and makes them easier to find and modify.

improve light render parity with blender

f3ee817

riccardobl commented Aug 10, 2025

View reviewed changes

richardTingle reviewed Aug 10, 2025

View reviewed changes

jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/LightsPunctualExtensionLoader.java Outdated Show resolved Hide resolved

riccardobl commented Aug 10, 2025

View reviewed changes

cleanup

def0dab

riccardobl added the defect Something that is supposed to work, but doesn't. Less severe than a "bug" label Aug 10, 2025

capdevon reviewed Aug 27, 2025

View reviewed changes

fix merge issue

2f64bba

capdevon reviewed Aug 27, 2025

View reviewed changes

yaRnMcDonuts added this to the v3.9.0 milestone Sep 17, 2025

This was referenced Sep 28, 2025

Possible issue with point light blending in some cases #2565

Closed

Infinite radius point lights might cause type overflow #2566

Closed

yaRnMcDonuts modified the milestones: v3.9.0, v3.10.0 Dec 18, 2025

yaRnMcDonuts mentioned this pull request Jan 21, 2026

Fix PointLight infinite radius shader overflow(#2566) #2570

Merged

yaRnMcDonuts and others added 2 commits January 29, 2026 18:05

Update LightsPunctualExtensionLoader.java

169b58b

Accept changed screenshot reference images for blender light

75a231a

gemini-code-assist bot reviewed Mar 1, 2026

View reviewed changes

		final float visibleThreshold = 0.001f;
		final float maxRange = 10000f;

Uh oh!

Conversation

riccardobl commented Aug 10, 2025

Blender:

JME Without this PR:

JME With this PR:

Uh oh!

github-actions bot commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riccardobl Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

riccardobl Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

riccardobl Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

riccardobl Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riccardobl Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

richardTingle commented Aug 10, 2025

Uh oh!

richardTingle commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaRnMcDonuts commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

capdevon Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

riccardobl Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

capdevon Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riccardobl Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capdevon Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riccardobl commented Mar 1, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Aug 10, 2025 •

edited

Loading

riccardobl Aug 10, 2025 •

edited

Loading

richardTingle commented Aug 10, 2025 •

edited

Loading

yaRnMcDonuts commented Aug 18, 2025 •

edited

Loading

capdevon Aug 27, 2025 •

edited

Loading

riccardobl Aug 27, 2025 •

edited

Loading

capdevon Aug 27, 2025 •

edited

Loading