HDR: Slice D — Skybox + tone-mapping compositor (#470)#782
HDR: Slice D — Skybox + tone-mapping compositor (#470)#782fernandotonon wants to merge 1 commit into
Conversation
Add FP16 scene render + fullscreen tonemap compositor (Reinhard/ACES/AgX) with EV exposure on editor viewports when an HDR environment is loaded. Skybox uses the environment cubemap; per-viewport skies toggle via Viewport::setSkiesEnabled without disabling IBL. Global tonemap defaults live on HDREnvironmentManager; CPU reference tests cover exposure scaling and operator divergence. Closes #470 Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds a complete HDR viewport pipeline: CPU-side Reinhard/ACES/AgX tonemapping math ( ChangesHDR Skybox + Tonemap Compositor Pipeline
Sequence DiagramsequenceDiagram
participant OgreWidget
participant HdrViewportController
participant HdrViewportPipeline
participant HDREnvironmentManager
participant OgreCompositor
OgreWidget->>HdrViewportController: registerWidget(this)
HdrViewportController->>HdrViewportPipeline: new HdrViewportPipeline(widget)
HdrViewportPipeline->>HDREnvironmentManager: hasEnvironment()
HdrViewportPipeline->>OgreCompositor: addCompositor / enable
HDREnvironmentManager->>HdrViewportController: environmentChanged()
HdrViewportController->>HDREnvironmentManager: applySkyBox / removeSkyBox
HdrViewportController->>HdrViewportPipeline: refresh()
HDREnvironmentManager->>HdrViewportController: tonemapChanged()
HdrViewportController->>HdrViewportPipeline: refresh()
OgreWidget->>HdrViewportController: tickActiveViewports()
HdrViewportController->>HdrViewportPipeline: updateTonemapUniforms()
HdrViewportPipeline->>OgreCompositor: setNamedConstant(exposureMul / tonemapOp / whitePoint)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251f723fbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (auto* hdrVp = HdrViewportController::getSingletonPtr()) | ||
| hdrVp->registerWidget(this); |
There was a problem hiding this comment.
registerWidget() is only reached when s_singleton already exists, but the new code never creates the controller on the normal app path (repo-wide references only hit getSingletonPtr() from OgreWidget plus the definition). As a result, loading an HDR environment emits environmentChanged with no controller listening, so neither the skybox nor the compositor pipeline is installed for any viewport.
Useful? React with 👍 / 👎.
|
|
||
| target_output | ||
| { | ||
| input on rtHdr |
There was a problem hiding this comment.
OGRE compositor scripts bind local render textures to the fullscreen material with input <texUnit> <name> inside the pass render_quad; this target-level input on rtHdr does not bind rtHdr to the tonemap shader sampler, and on is not a texture unit. When the HDR compositor is enabled, it will either fail script parsing or run the tonemap material without the FP16 scene texture, so the viewport cannot display the intended tonemapped scene.
Useful? React with 👍 / 👎.
| if (!widget || m_pipelines.find(widget) != m_pipelines.end()) | ||
| return; |
There was a problem hiding this comment.
OgreWidget::rebuildRenderWindow() keeps the same OgreWidget* but destroys and recreates its OGRE viewport; after HDR has been enabled, the subsequent registration hits this early return and leaves the existing HdrViewportPipeline holding the old viewport/compositor pointers. Any render-window rebuild path such as MainWindow::rebuildAllOgreViewports() then loses HDR on the recreated window and can later remove a stale compositor from the wrong viewport.
Useful? React with 👍 / 👎.
| m_skyBoxVisible = HDREnvironmentManager::getSingleton()->defaultSkyBoxVisible(); | ||
| m_viewport->setSkiesEnabled(m_skyBoxVisible); |
There was a problem hiding this comment.
enablePipeline() runs during every tone-map refresh, so this assignment overwrites a viewport hidden through HdrViewportController::setSkyBoxVisible(widget, false) with the global default whenever exposure/operator settings change. That makes the per-viewport sky toggle unstable across unrelated tonemap updates.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/HDR/HDREnvironmentManager.cpp (1)
473-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setDefaultSkyBoxVisiblereusestonemapChangedand skips a breadcrumb.This setter emits
tonemapChanged()even though sky visibility is not a tonemap parameter, conflating two concerns for downstream listeners. The three tonemap setters above also add arender.hdr.tonemapSentry breadcrumb for the user action; this one does not. Consider a dedicated signal/breadcrumb for skybox-visibility changes.All user-facing actions should add a Sentry breadcrumb via the established categories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/HDR/HDREnvironmentManager.cpp` around lines 473 - 479, The `setDefaultSkyBoxVisible` setter is incorrectly using `tonemapChanged()` and does not record a breadcrumb, so update `HDREnvironmentManager::setDefaultSkyBoxVisible` to use a dedicated skybox-visibility notification instead of reusing tonemap state changes. Add a Sentry breadcrumb for this user action using the same established pattern as the other HDR setters, but with an appropriate `render.hdr.*` category for skybox visibility, and keep the change localized so downstream listeners can distinguish skybox updates from tonemap updates.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/HDR/HdrTonemap.cpp`:
- Around line 47-62: The tonemapAgx function is clipping HDR highlights too
early by using clamp01(x) before the AgX transform; change it to preserve values
above 1.0 while still preventing negative inputs, then keep the final
saturate(contrast) clamp on the returned result. Update the AgX input handling
inside tonemapAgx so only the lower bound is clamped before the matrix/contrast
steps.
In `@src/HDR/HdrViewportPipeline.cpp`:
- Around line 82-83: `HdrViewportPipeline::enablePipeline` is resetting
per-viewport skybox state from `HDREnvironmentManager::defaultSkyBoxVisible()`
on every refresh, which overwrites values previously set by
`setSkyBoxVisible(...)`. Update the logic so `m_skyBoxVisible` is initialized
from the global default only once (for example during construction or
registration), and have `enablePipeline` simply reapply the existing
`m_skyBoxVisible` when calling `m_viewport->setSkiesEnabled(...)`. Keep the
per-viewport toggle state owned by `HdrViewportPipeline` rather than re-reading
the global default on each refresh.
- Around line 88-97: The viewport rebuild path leaves HdrViewportPipeline
holding an old compositor, so refresh()/enablePipeline() never reattaches HDR
tonemapping to the new viewport because the addCompositor() guard only checks
m_compositor. Update the rebuild flow in rebuildRenderWindow() and the pipeline
attachment logic in HdrViewportPipeline so the compositor is cleared or
re-created when m_viewport changes, or make the guard validate viewport identity
before skipping addCompositor(). Ensure updateTonemapMaterialConstants() and
m_enabled reflect the newly attached viewport.
In `@src/OgreWidget.cpp`:
- Around line 301-303: The HDR viewport registration in
OgreWidget::initOgreWindow() and OgreWidget::rebuildRenderWindow() only handles
the case where HdrViewportController already exists, so widgets created earlier
are never registered if the controller is instantiated later. Fix this by either
ensuring HdrViewportController is created before any OgreWidget viewport setup,
or by having HdrViewportController’s constructor/init path discover and register
already-existing OgreWidget instances so they get attached to the HDR pipeline.
---
Nitpick comments:
In `@src/HDR/HDREnvironmentManager.cpp`:
- Around line 473-479: The `setDefaultSkyBoxVisible` setter is incorrectly using
`tonemapChanged()` and does not record a breadcrumb, so update
`HDREnvironmentManager::setDefaultSkyBoxVisible` to use a dedicated
skybox-visibility notification instead of reusing tonemap state changes. Add a
Sentry breadcrumb for this user action using the same established pattern as the
other HDR setters, but with an appropriate `render.hdr.*` category for skybox
visibility, and keep the change localized so downstream listeners can
distinguish skybox updates from tonemap updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e947cc6-331c-4b8f-87ab-3ac600624ee4
⛔ Files ignored due to path filters (1)
media/materials/programs/GLSL150/QtMeshHdrTonemapFp.glslis excluded by!**/*.glsl
📒 Files selected for processing (15)
cfg/resources.cfg.incfg/resources_d.cfg.inmedia/compositors/QtMeshHdr.compositormedia/materials/scripts/QtMeshHdr.materialsrc/CMakeLists.txtsrc/HDR/HDREnvironmentManager.cppsrc/HDR/HDREnvironmentManager.hsrc/HDR/HdrTonemap.cppsrc/HDR/HdrTonemap.hsrc/HDR/HdrTonemap_test.cppsrc/HDR/HdrViewportController.cppsrc/HDR/HdrViewportController.hsrc/HDR/HdrViewportPipeline.cppsrc/HDR/HdrViewportPipeline.hsrc/OgreWidget.cpp
| Rgb tonemapAgx(Rgb x) | ||
| { | ||
| // Troy Sobotka's AgX default contrast approximation (Blender 4+ default). | ||
| const Rgb lin = clamp01(x); | ||
| const Rgb agx = { | ||
| 0.224282f * lin.r + 0.130789f * lin.g + 0.044929f * lin.b, | ||
| 0.050223f * lin.r + 0.873461f * lin.g + 0.076316f * lin.b, | ||
| 0.020833f * lin.r + 0.080745f * lin.g + 0.898422f * lin.b, | ||
| }; | ||
| const Rgb contrast = { | ||
| std::pow(std::max(0.f, agx.r), 1.35f), | ||
| std::pow(std::max(0.f, agx.g), 1.35f), | ||
| std::pow(std::max(0.f, agx.b), 1.35f), | ||
| }; | ||
| return saturate(contrast); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files src/HDR/HdrTonemap.cpp
echo
echo "== relevant excerpt =="
nl -ba src/HDR/HdrTonemap.cpp | sed -n '1,220p'
echo
echo "== search for tonemap references =="
rg -n "tonemapAgx|tonemapAces|tonemapReinhard|clamp01|AgX|Agx" src -SRepository: fernandotonon/QtMeshEditor
Length of output: 267
🌐 Web query:
AgX default contrast approximation clamp input 0 1 Blender 4 reference tonemapAgx
💡 Result:
In Blender's AgX implementation, the input is explicitly clamped or constrained to ensure the color processing (particularly the AgX inset matrix and log-space encoding) behaves predictably [1][2]. Key details regarding AgX and input clamping include: 1. Input Handling: AgX implementations, including the one used in Blender, typically perform a conversion to linear Rec.2020 space [1][2]. Preventing negative input values is necessary because negative values can cause the AgX inset matrix to produce undesirable artifacts or darker, more saturated colors than intended [1][2]. Consequently, implementations often use a max(color, 0.0) operation or a similar floor before the inset matrix [1][2]. 2. Log-Space Clamping: After the inset matrix, the values are converted to log2 space [1][2]. The log-transformed data is then clamped within a specific dynamic range (typically defined by min_ev and max_ev, e.g., -10 to +6.5 in some reference implementations) before being passed to the sigmoid approximation function [1][2]. This ensures the input to the contrast approximation remains within a stable range [2]. 3. LUT Output Clamping: In Blender's official AgX view transform, the 3D LUTs used to approximate the tone mapping process are designed to clip output values to the [3] range [4]. Earlier versions of these LUTs sometimes included values slightly above 1.0, which was deemed incorrect for a tone mapper and redundant because Blender's display pipeline clips these values regardless [4]. 4. TonemapAgx References: While standard Blender compositor nodes include a Tonemap node (with Reinhard/Reinhard-Devlin algorithms) [5], AgX itself is implemented as an OCIO-based view transform rather than a standard compositor tone mapping node [3][6][7]. References to tonemapAgx in code (such as in external engines like Godot) are approximations or manual implementations of the AgX logic found in Blender's config [1][2]. In summary, the clamping to [3] is a fundamental part of the AgX output stage to conform to standard SDR display requirements [4][7], while input clamping (at 0.0) is a technical requirement to maintain the integrity of the color math during the transformation process [1][2].
Citations:
- 1: https://github.com/allenwp/AgX-GLSL-Shaders/blob/main/EaryChow-AgX/earychow-agx-simplified.glsl
- 2: godotengine/godot@e4a12a0
- 3: https://projects.blender.org/blender/blender/pulls/106355
- 4: https://projects.blender.org/blender/blender/pulls/113253
- 5: https://docs.blender.org/api/4.2/bpy.types.CompositorNodeTonemap.html
- 6: https://projects.blender.org/blender/blender/pulls/111099
- 7: https://developer.blender.org/docs/release_notes/4.0/color_management/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/HDR/HdrTonemap.cpp excerpt =="
sed -n '1,220p' src/HDR/HdrTonemap.cpp
echo
echo "== clamp01 definition and tonemap references =="
rg -n "clamp01|tonemapAgx|tonemapAces|tonemapReinhard|tonemap" src/HDR -SRepository: fernandotonon/QtMeshEditor
Length of output: 5894
Preserve HDR values before AgX
clamp01(x) clips all highlights above 1.0 before the AgX transform, so the operator cannot roll off HDR intensities. Clamp negatives only here; keep the final output clamp.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/HDR/HdrTonemap.cpp` around lines 47 - 62, The tonemapAgx function is
clipping HDR highlights too early by using clamp01(x) before the AgX transform;
change it to preserve values above 1.0 while still preventing negative inputs,
then keep the final saturate(contrast) clamp on the returned result. Update the
AgX input handling inside tonemapAgx so only the lower bound is clamped before
the matrix/contrast steps.
| m_skyBoxVisible = HDREnvironmentManager::getSingleton()->defaultSkyBoxVisible(); | ||
| m_viewport->setSkiesEnabled(m_skyBoxVisible); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
enablePipeline resets per-viewport skybox visibility on every refresh.
Line 82 overwrites m_skyBoxVisible with the global defaultSkyBoxVisible() each time the pipeline is enabled. Since refresh() runs on every environmentChanged/tonemapChanged (via HdrViewportController::refreshAll), any per-viewport state previously set through setSkyBoxVisible(...) is silently discarded back to the global default. This conflicts with the PR objective of a per-viewport skybox toggle that survives independent of global defaults.
Consider seeding from the default only on first enable, and otherwise re-applying the current m_skyBoxVisible:
♻️ Proposed fix
m_viewport = const_cast<Ogre::Viewport*>(vpConst);
- m_skyBoxVisible = HDREnvironmentManager::getSingleton()->defaultSkyBoxVisible();
- m_viewport->setSkiesEnabled(m_skyBoxVisible);
+ m_viewport->setSkiesEnabled(m_skyBoxVisible);(Initialize m_skyBoxVisible from the default once at construction/registration instead of on every enable.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_skyBoxVisible = HDREnvironmentManager::getSingleton()->defaultSkyBoxVisible(); | |
| m_viewport->setSkiesEnabled(m_skyBoxVisible); | |
| m_viewport->setSkiesEnabled(m_skyBoxVisible); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/HDR/HdrViewportPipeline.cpp` around lines 82 - 83,
`HdrViewportPipeline::enablePipeline` is resetting per-viewport skybox state
from `HDREnvironmentManager::defaultSkyBoxVisible()` on every refresh, which
overwrites values previously set by `setSkyBoxVisible(...)`. Update the logic so
`m_skyBoxVisible` is initialized from the global default only once (for example
during construction or registration), and have `enablePipeline` simply reapply
the existing `m_skyBoxVisible` when calling `m_viewport->setSkiesEnabled(...)`.
Keep the per-viewport toggle state owned by `HdrViewportPipeline` rather than
re-reading the global default on each refresh.
| if (!m_compositor) { | ||
| m_compositor = Ogre::CompositorManager::getSingleton().addCompositor( | ||
| m_viewport, kCompositorName); | ||
| if (m_compositor) | ||
| Ogre::CompositorManager::getSingleton().setCompositorEnabled( | ||
| m_viewport, kCompositorName, true); | ||
| } | ||
|
|
||
| updateTonemapMaterialConstants(); | ||
| m_enabled = m_compositor != nullptr; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the viewport rebuild path tears down the HDR pipeline/compositor first.
rg -nP -C4 '\brebuildRenderWindow\b' src/OgreWidget.cpp
rg -nP -C3 '(unregisterWidget|registerWidget|disablePipeline|refresh)\b' src/HDR/HdrViewportController.cpp src/OgreWidget.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '348,410p' src/OgreWidget.cpp
printf '\n----\n'
sed -n '1,180p' src/HDR/HdrViewportController.cpp
printf '\n----\n'
sed -n '1,180p' src/HDR/HdrViewportPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 8915
Rebuild needs to reattach the compositor to the new viewport. rebuildRenderWindow() recreates the viewport, but the existing HdrViewportPipeline stays registered on the widget, so refresh()/enablePipeline() skips addCompositor() once m_compositor is already set. Clear the pipeline on viewport rebuild or key the guard off viewport identity, otherwise HDR tonemapping stays bound to the old viewport.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/HDR/HdrViewportPipeline.cpp` around lines 88 - 97, The viewport rebuild
path leaves HdrViewportPipeline holding an old compositor, so
refresh()/enablePipeline() never reattaches HDR tonemapping to the new viewport
because the addCompositor() guard only checks m_compositor. Update the rebuild
flow in rebuildRenderWindow() and the pipeline attachment logic in
HdrViewportPipeline so the compositor is cleared or re-created when m_viewport
changes, or make the guard validate viewport identity before skipping
addCompositor(). Ensure updateTonemapMaterialConstants() and m_enabled reflect
the newly attached viewport.
|
|
||
| if (auto* hdrVp = HdrViewportController::getSingletonPtr()) | ||
| hdrVp->registerWidget(this); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Who creates the controller (getSingleton) vs only reads it (getSingletonPtr)?
rg -nP 'HdrViewportController::(getSingleton|getSingletonPtr|kill)\s*\(' src
echo "--- controller ctor / registerWidget call sites ---"
rg -nP '\bregisterWidget\s*\(' src
echo "--- does the controller enumerate existing widgets on creation? ---"
ast-grep outline src/HDR/HdrViewportController.cpp --view expandedRepository: fernandotonon/QtMeshEditor
Length of output: 1251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- src/HDR/HdrViewportController.cpp ---"
wc -l src/HDR/HdrViewportController.cpp
cat -n src/HDR/HdrViewportController.cpp | sed -n '1,220p'
echo "--- src/OgreWidget.cpp around registration sites ---"
wc -l src/OgreWidget.cpp
cat -n src/OgreWidget.cpp | sed -n '120,410p'Repository: fernandotonon/QtMeshEditor
Length of output: 14872
Late controller creation leaves existing viewports unregistered. initOgreWindow() and rebuildRenderWindow() only register when HdrViewportController::getSingletonPtr() is already non-null, and the controller ctor doesn’t pick up already-live OgreWidgets. If the controller is first created after some viewports exist, those widgets never get HDR pipelines. Create the controller before viewport init or register existing widgets when it’s instantiated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OgreWidget.cpp` around lines 301 - 303, The HDR viewport registration in
OgreWidget::initOgreWindow() and OgreWidget::rebuildRenderWindow() only handles
the case where HdrViewportController already exists, so widgets created earlier
are never registered if the controller is instantiated later. Fix this by either
ensuring HdrViewportController is created before any OgreWidget viewport setup,
or by having HdrViewportController’s constructor/init path discover and register
already-existing OgreWidget instances so they get attached to the HDR pipeline.
1 participant
Summary
Implements epic #466 Slice D (#470): HDR viewport skybox + FP16 scene render + tonemap compositor.
HDREnvironmentManager::applySkyBox()binds the loaded cubemap toQtMesh/HdrSkyboxviaSceneManager::setSkyBox. Per-viewport visibility usesViewport::setSkiesEnabled(IBL stays active when skies are hidden).QtMeshHdrPipelinerenders the scene toPF_FLOAT16_RGBA, then a fullscreenrender_quadpass appliesQtMesh/HdrTonemapPass(Reinhard / ACES / AgX + EV exposure + sRGB output).HDREnvironmentManager(exposureEv,tonemapOperator,whitePoint,defaultSkyBoxVisible) withtonemapChangedsignal andrender.hdr.tonemapSentry breadcrumbs.HdrViewportController+HdrViewportPipeline, hooked fromOgreWidget(register on init, disable when no env loaded — SDR path unchanged).HdrTonemap_test.cppvalidates 1-stop exposure scaling and operator output differences on synthetic HDR constants.Not in this slice: Inspector/Material Editor controls (#471), bundled HDRIs (#472), CLI/MCP (#473).
Test plan
./build_local/bin/UnitTests --gtest_filter="HdrTonemapTest.*"unit-tests-linuxHDREnvironmentManager::loadEnvironment(path)→ skybox visible + tonemapped viewport; togglesetSkyBoxVisible(widget, false)hides sky onlyCloses #470
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests