Fix/scale HDRI lighting by render.light.intensity by midaksh · Pull Request #3433 · f3d-app/f3d

midaksh · 2026-08-03T13:12:20Z

Describe your changes

When HDRI ambient lighting is enabled (-f / render.hdri.ambient), changing render.light.intensity (console, L / Shift+L) only scaled the explicit VTK lights. The HDRI image-based lighting contribution in the PBR shader was left unchanged, so at intensity 0 the model could still look lit by the HDRI.

This PR scales the IBL diffuse + specular terms by the same factor. The shader substitution is done in vtkF3DRenderPass::PostReplaceShaderValues. The IBLIntensity uniform is declared via VTK's //VTK::CustomUniforms::Dec mechanism and is pushed to every actor in vtkF3DRenderer::UpdateLights each frame, so newly added actors always have it before their shader is compiled.

Tests: Added TestHDRILightIntensityZero (--light-intensity=0.0) and TestHDRILightIntensityBrighter (--light-intensity=5.0), using the same suzanne.ply + shanghai_bund_1k.hdr setup as TestHDRI, with DEPENDS TestHDRI and threshold 0.07.

Before (`master`, bug)

render.light.intensity 0	render.light.intensity 1

At intensity 0 the figure is still lit by the HDRI.

After (this branch, fixed)

render.light.intensity 0	render.light.intensity 1	render.light.intensity 5

At intensity 0 the figure is black. At 1 it matches the previous default.
At 5 it is clearly brighter.

How to test

./bin/f3d --no-config testing/data/RiggedFigure.glb \
  --hdri-file=testing/data/shanghai_bund_1k.hdr -f

open console(Esc):

set render.light.intensity 0
set render.light.intensity 1
set render.light.intensity 5

With the console closed, L / Shift+L also change HDRI lighting live (same as explicit lights). You can confirm the value with:

print render.light.intensity

Issue ticket number and link if any

Fixes: #3312

Checklist for finalizing the PR

I have performed a self-review of my code
I have added tests for new features and bugfixes
I have added documentation for new features
If it is a modifying the libf3d API, I have updated bindings
If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

AI Disclosure

I have not used AI to generate any of the content of this pull request
I have used AI to generate code in this pull request:
- I have carefully read and understood the AI policy.
- I have carefully reviewed and completely understood every generated line.
- I disclose below which parts of the code were generated and with which AI model:

...

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

midaksh · 2026-08-03T13:13:00Z

\ci full

codecov · 2026-08-03T14:54:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.12%. Comparing base (8ae77cf) to head (8ec5f44).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3433      +/-   ##
==========================================
+ Coverage   97.04%   97.12%   +0.07%     
==========================================
  Files         216      216              
  Lines       18808    19041     +233     
==========================================
+ Hits        18253    18494     +241     
+ Misses        555      547       -8

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

midaksh · 2026-08-03T17:52:40Z

Hello @Meakk, let me know if any changes are required.
Thx

mwestphal

Design needs improvements
Missing test
Missing doc update

Let us know if you have any questions!

mwestphal · 2026-08-03T20:06:50Z

: offscreen HDRI tests need a writable cache path (setCachePath) and VTK-version-specific baselines that do not travel cleanly across CI platforms.

What do you mean by that ?

midaksh · 2026-08-04T13:56:58Z

Design needs improvements

Missing test

Missing doc update

Let us know if you have any questions!

Design: moved the shader change into vtkF3DRenderPass (for GLES too), and update the uniform from UpdateLights instead of every frame in the mapper.
Docs: updated both doc/user/03-OPTIONS.md and doc/libf3d/03-OPTIONS.md so light intensity is described as including HDRI.

I had HDRI baseline cases locally, but they were flaky on CI (cache path + GPU-specific baselines), so I dropped them. Happy to add a focused test if you have a preferred p

midaksh · 2026-08-04T13:58:51Z

: offscreen HDRI tests need a writable cache path (setCachePath) and VTK-version-specific baselines that do not travel cleanly across CI platforms.

What do you mean by that ?

that was about an earlier attempt to add HDRI regression images to TestSDKDynamicLightIntensity.
on CI those failed for two reasons:
HDRI needs a writable LUT cache (setCachePath), which wasn’t set up in the offscreen test path.
The baselines differed across VTK versions / GPUs, so they didn’t match cleanly.

So I removed those test cases from this PR. The existing light-intensity tests still pass, and I verified the HDRI behavior manually, if there is some other workaround pattern let me know pls

mwestphal · 2026-08-04T14:36:37Z

I had HDRI baseline cases locally, but they were flaky on CI (cache path + GPU-specific baselines), so I dropped them. Happy to add a focused test if you have a preferred p

We definitely need a test, please add one and we will see how it goes.

Meakk · 2026-08-04T19:02:45Z

The design looks correct :)

midaksh · 2026-08-04T21:17:41Z

@Meakk test changes are implemented, open for review :)

mwestphal · 2026-08-05T08:05:07Z

@midaksh it looks like you are using AI to generate content in this pull request, please update your AI usage in the PR description and keep it accurate at all time at the risk of being banned.

Also remove all mentions of co-authoring from AI agents, you are the sole owner of the code you submit, not the AI agent you use.

midaksh · 2026-08-05T14:15:16Z

@midaksh it looks like you are using AI to generate content in this pull request, please update your AI usage in the PR description and keep it accurate at all time at the risk of being banned.

Also remove all mentions of co-authoring from AI agents, you are the sole owner of the code you submit, not the AI agent you use.

Hi @mwestphal no AI generated code was pushed in any commits, there were just the commits that were pushed via Cursor all the code, testing and design/structure was done by me, I will update all that is needed and remove the automated co-auther thanks.

mwestphal · 2026-08-05T15:16:29Z

there were just the commits that were pushed via Cursor

You may want to fix that in your setup.

When render.light.intensity is set (via L / Shift+L or the console), the HDRI image-based lighting (diffuse irradiance + specular reflections) was not affected, making it appear disconnected from all other lights. Fix by injecting an IBLIntensity uniform into the PBR fragment shader: - Shader substitution is done in vtkF3DRenderPass::PostReplaceShaderValues so it applies to both the standard OpenGL mapper and the GLES mapper used for web and Android builds. - IBLIntensity is pushed to each actor's fragment custom uniforms inside vtkF3DRenderer::UpdateLights, in the same !LightIntensitiesConfigured block that already scales the explicit VTK lights, so the uniform is updated only when the intensity actually changes — not every frame. - Both doc/user and doc/libf3d option descriptions updated to remove the "(except HDRI)" caveat. Closes f3d-app#3312

Add TestHDRILightIntensityZero and TestHDRILightIntensityBrighter to verify that render.light.intensity=0.0 blackens the HDRI ambient contribution and that render.light.intensity=5.0 brightens it. Both tests use the same suzanne.ply + shanghai_bund_1k.hdr setup and the same 0.07 threshold as TestHDRI to tolerate LUT precision differences on GLES. DEPENDS TestHDRI ensures the LUT cache is warm before either test runs. Fixes f3d-app#3312

midaksh · 2026-08-05T17:20:02Z

You may want to fix that in your setup.

I have removed the co-auther also took your advice and changed the set up thanks :)

mwestphal · 2026-08-06T06:56:22Z

+  // VTK's //VTK::CustomUniforms::Dec mechanism then declares the uniform in the
+  // GLSL shader source. The actual GPU upload (glUniform1f) is already performed
+  // each frame by VTK's SetCustomUniforms, so this dict update is negligible.
+  // see https://github.com/f3d-app/f3d/issues/3312


LGTM but I defer to @Meakk on that

mwestphal

LGTM but @Meakk review needed anyway

midaksh requested a review from a team as a code owner August 3, 2026 13:12

mwestphal added the ci:full label Aug 3, 2026

midaksh force-pushed the fix/hdri-intensity-follows-light-intensity branch from d926d1f to 0da41ff Compare August 3, 2026 15:58

mwestphal reviewed Aug 3, 2026

View reviewed changes

Comment thread vtkext/private/module/vtkF3DPolyDataMapper.h Outdated

mwestphal requested changes Aug 3, 2026

View reviewed changes

Meakk reviewed Aug 3, 2026

View reviewed changes

Comment thread vtkext/private/module/vtkF3DPolyDataMapper.cxx Outdated

midaksh force-pushed the fix/hdri-intensity-follows-light-intensity branch 4 times, most recently from d7f0076 to 62d09c3 Compare August 4, 2026 11:30

midaksh requested a review from mwestphal August 4, 2026 13:59

Meakk reviewed Aug 4, 2026

View reviewed changes

Comment thread testing/baselines/TestHDRILightIntensityBrighter.png

midaksh added 3 commits August 5, 2026 22:18

Use 0.1 and 2.0 light intensity instead of 0.0 and 5.0 in HDRI tests

8ec5f44

midaksh force-pushed the fix/hdri-intensity-follows-light-intensity branch from 65de868 to 8ec5f44 Compare August 5, 2026 16:49

mwestphal reviewed Aug 6, 2026

View reviewed changes

mwestphal approved these changes Aug 6, 2026

View reviewed changes

mwestphal requested a review from Meakk August 6, 2026 06:57

Uh oh!

Uh oh!

Conversation

midaksh commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Before (master, bug)

After (this branch, fixed)

How to test

Issue ticket number and link if any

Checklist for finalizing the PR

AI Disclosure

Continuous integration

Uh oh!

midaksh commented Aug 3, 2026

Uh oh!

codecov Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

midaksh commented Aug 3, 2026

Uh oh!

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

mwestphal commented Aug 3, 2026

Uh oh!

Uh oh!

midaksh commented Aug 4, 2026

Uh oh!

midaksh commented Aug 4, 2026

Uh oh!

mwestphal commented Aug 4, 2026

Uh oh!

Uh oh!

Meakk commented Aug 4, 2026

Uh oh!

midaksh commented Aug 4, 2026

Uh oh!

mwestphal commented Aug 5, 2026

Uh oh!

midaksh commented Aug 5, 2026

Uh oh!

mwestphal commented Aug 5, 2026

Uh oh!

midaksh commented Aug 5, 2026

Uh oh!

mwestphal Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

midaksh commented Aug 3, 2026 •

edited

Loading

Before (`master`, bug)

codecov Bot commented Aug 3, 2026 •

edited

Loading