HDR: Slice A — environment loader & cubemap conversion (#467)#778
Conversation
Add HDREnvironmentManager and a pure-data HdrEquirect pipeline so .hdr/.exr equirectangular maps bake to Ogre float cubemaps with in-memory SHA-1 caching. No shader, GUI, or CLI surface yet — foundation for epic #466 IBL work. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds HDR environment loading with CPU equirect-to-cubemap baking, optional OpenEXR support, a singleton Ogre cubemap manager, CMake dependency wiring, and tests for baking, hashing, loading, and texture lifecycle. ChangesHDR Environment Pipeline
Sequence Diagram(s)sequenceDiagram
participant Client
participant HDREnvironmentManager
participant HdrEquirectLoader
participant OgreTextureManager
Client->>HDREnvironmentManager: loadEnvironment(path)
HDREnvironmentManager->>HdrEquirectLoader: sha1HexOfFile(resolvedPath)
HDREnvironmentManager->>HdrEquirectLoader: loadFromFile + bakeEquirectToCubemap
HDREnvironmentManager->>OgreTextureManager: remove old texture
HDREnvironmentManager->>OgreTextureManager: create/load cube texture
HDREnvironmentManager->>Client: environmentChanged()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: fdb551615d
ℹ️ 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".
| #endif | ||
|
|
||
| for (const QString& candidate : candidates) { | ||
| const QString canon = QDir(candidate).canonicalPath(); |
There was a problem hiding this comment.
When the caller passes a bundled name such as studio.hdr, each candidate is a full file path, but QDir(candidate).canonicalPath() treats that path as a directory and returns empty unless there is a directory named studio.hdr. As a result loadEnvironment("studio.hdr") never finds files under media/hdri/; use QFileInfo(candidate).canonicalFilePath()/exists() for the candidate file instead.
Useful? React with 👍 / 👎.
| if (texMgr.resourceExists(texName)) | ||
| texMgr.remove(texName); | ||
|
|
||
| m_cubemap = texMgr.createManual( |
There was a problem hiding this comment.
When switching from one HDR to a different HDR, this overwrites m_cubemap after only removing a resource with the new texture name. The old HdrEnv_* resource remains registered in Ogre::TextureManager (and the destructor only removes the current texture), so repeatedly trying environments leaks large PF_FLOAT32_RGB cube maps/GPU memory until shutdown.
Useful? React with 👍 / 👎.
| const float phi = std::atan2(dir[2], dir[0]); | ||
| const float theta = std::asin(std::max(-1.f, std::min(1.f, dir[1]))); | ||
| u = phi / kTwoPi + 0.5f; | ||
| v = theta / kPi + 0.5f; |
There was a problem hiding this comment.
For standard equirectangular HDRIs, the top row (v=0) is the north/+Y pole, but this maps dir[1] == +1 to v=1 and dir[1] == -1 to v=0. Real .hdr/.exr environments will therefore bake upside down, inverting sky/ground lighting; the existing spherical projection code in src/UvProject.cpp uses the opposite 0.5f - asin(...) convention.
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: 7
🧹 Nitpick comments (1)
src/HDREnvironmentManager_test.cpp (1)
79-91: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftThis cache test is too timing-dependent to prove a cache hit.
secondMs < firstMs + 50still passes when the second load is slower, and wall-clock checks are noisy in CI. Please assert a deterministic cache-hit signal instead, even if that means adding a small test seam around the bake cache.🤖 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/HDREnvironmentManager_test.cpp` around lines 79 - 91, The cache test in HDREnvironmentManager_test is relying on elapsed wall-clock timing, which is flaky and does not deterministically prove a cache hit. Update the test to assert a stable cache-hit signal instead by adding a small test seam around the bake cache or by checking a direct cache-related observable from loadEnvironment/HDR environment state, and keep the current symbols like mgr->loadEnvironment, currentCacheKey, and HdrEquirect::sha1HexOfFile as the main anchors for locating the logic.
🤖 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 `@CMakeLists.txt`:
- Around line 343-347: The stb FetchContent declaration is using a mutable
branch tag, which makes builds non-reproducible; update the FetchContent_Declare
block for stb to use an immutable release tag or commit SHA instead of master,
following the same pinning approach used by the tinyexr declaration.
In `@src/HDREnvironmentManager_test.cpp`:
- Around line 32-66: The HDR environment tests are repeating Ogre setup inline
instead of enforcing the standard Ogre test prerequisites in a fixture. Move the
shared initialization for HDREnvironmentManagerTest into SetUp() on an
Ogre-specific fixture and have it assert both tryInitOgre() and
canLoadMeshFiles() before any test body runs. Then remove the ad hoc setup from
LoadEnvironment_CreatesCubeMapTexture and ReloadSameFile_UsesBakeCache so the
Ogre-backed cases consistently fail loudly when prerequisites are missing.
In `@src/HDREnvironmentManager.cpp`:
- Around line 63-66: The macOS-specific HDRI path fallback in
HDREnvironmentManager should use the Qt OS guard convention instead of the OGRE
platform macro. Update the conditional around the candidate path construction in
the relevant block of HDREnvironmentManager to use Q_OS_MACOS so it matches the
repo’s platform-guarding rules and stays consistent with other platform-specific
code.
- Around line 136-189: HDREnvironmentManager::loadEnvironment() is only
breadcrumbing the success path and uses an unsupported category, so move the
SentryReporter::addBreadcrumb call to cover the full import attempt and switch
it to the repo’s allowed file-import category. Add breadcrumb logging for
failure returns in loadEnvironment() and ensure both success and failure cases
emit file.import with a clear message so imports are visible in Sentry and
existing dashboards.
- Around line 50-57: The absolute-path handling in
HDREnvironmentManager::loadEnvironment should stop immediately when an absolute
QFileInfo path does not exist instead of continuing into the bundled-name
lookup. Update the early path checks around QFileInfo::isAbsolute(),
QFileInfo::exists(), and the subsequent fileName-based fallback so that only
non-absolute inputs can search bundled HDRIs, preserving the contract for
missing absolute files.
- Around line 103-133: The cubemap construction in
HDREnvironmentManager::buildCubemap is mutating m_cubemap directly, which can
leave the old Ogre::TextureManager resource registered on cacheKey changes and
also clears the active environment on mid-build failures. Build the new texture
into a temporary Ogre::TexturePtr first, validate and fill all six faces there,
and only swap it into m_cubemap after load succeeds; on failure, clean up the
temporary texture and preserve the existing cubemap state.
In `@src/HdrEquirectLoader.cpp`:
- Around line 142-149: The filename-based EXR loading in loadExrTiny still
relies on LoadEXR with QFile::encodeName(path), which can break Unicode paths on
Windows. Update loadExrTiny to avoid passing a narrow encoded filename to
tinyexr: read the EXR via QFile and load from memory, or switch to a
Windows-wide path/opening approach, while keeping the existing error handling
and FloatImage population logic intact.
---
Nitpick comments:
In `@src/HDREnvironmentManager_test.cpp`:
- Around line 79-91: The cache test in HDREnvironmentManager_test is relying on
elapsed wall-clock timing, which is flaky and does not deterministically prove a
cache hit. Update the test to assert a stable cache-hit signal instead by adding
a small test seam around the bake cache or by checking a direct cache-related
observable from loadEnvironment/HDR environment state, and keep the current
symbols like mgr->loadEnvironment, currentCacheKey, and
HdrEquirect::sha1HexOfFile as the main anchors for locating the logic.
🪄 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: 1ed985c5-8ad3-43b1-94de-60c3b8cd6e13
📒 Files selected for processing (9)
CMakeLists.txtmedia/hdri/.gitkeepsrc/CMakeLists.txtsrc/HDREnvironmentManager.cppsrc/HDREnvironmentManager.hsrc/HDREnvironmentManager_test.cppsrc/HdrEquirectLoader.cppsrc/HdrEquirectLoader.hsrc/HdrEquirectLoader_test.cpp
| bool HDREnvironmentManager::loadEnvironment(const QString& pathOrBundledName) | ||
| { | ||
| QElapsedTimer timer; | ||
| timer.start(); | ||
|
|
||
| const QString resolved = resolvePath(pathOrBundledName); | ||
| if (resolved.isEmpty()) | ||
| return false; | ||
|
|
||
| const QString cacheKey = HdrEquirect::sha1HexOfFile(resolved); | ||
| if (cacheKey.isEmpty()) | ||
| return false; | ||
|
|
||
| const qint64 fileSize = QFileInfo(resolved).size(); | ||
|
|
||
| CachedBake* bake = nullptr; | ||
| const bool fromBakeCache = m_bakeCache.contains(cacheKey); | ||
| if (fromBakeCache) { | ||
| bake = &m_bakeCache[cacheKey]; | ||
| } else { | ||
| HdrEquirect::FloatImage equirect; | ||
| QString loadError; | ||
| if (!HdrEquirect::loadFromFile(resolved, equirect, loadError)) | ||
| return false; | ||
|
|
||
| const int faceSize = HdrEquirect::defaultFaceSizeForEquirect(equirect.width); | ||
| CachedBake entry; | ||
| entry.sourcePath = resolved; | ||
| entry.cacheKey = cacheKey; | ||
| entry.faceSize = faceSize; | ||
| QString bakeError; | ||
| if (!HdrEquirect::bakeEquirectToCubemap(equirect, faceSize, entry.faces, bakeError)) | ||
| return false; | ||
| entry.textureName = QStringLiteral("HdrEnv_%1").arg(cacheKey.left(16)); | ||
| m_bakeCache.insert(cacheKey, std::move(entry)); | ||
| bake = &m_bakeCache[cacheKey]; | ||
| } | ||
|
|
||
| QString ogreError; | ||
| if (!createOgreCubemap(cacheKey, bake->faces, ogreError)) | ||
| return false; | ||
|
|
||
| m_currentPath = resolved; | ||
| m_cacheKey = cacheKey; | ||
| m_faceSize = bake->faceSize; | ||
|
|
||
| SentryReporter::addBreadcrumb( | ||
| QStringLiteral("render.hdr.load"), | ||
| QStringLiteral("path=%1 bytes=%2 faceSize=%3 loadMs=%4 cached=%5") | ||
| .arg(QFileInfo(resolved).fileName()) | ||
| .arg(fileSize) | ||
| .arg(m_faceSize) | ||
| .arg(timer.elapsed()) | ||
| .arg(fromBakeCache ? QStringLiteral("yes") : QStringLiteral("no"))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Instrument this import path with the repo’s breadcrumb contract.
loadEnvironment() is a user-facing file import, but only the success path is logged and the category render.hdr.load is outside the repo’s allowed set. Failed imports will disappear from Sentry, and the current category will miss the existing file.import dashboards. As per coding guidelines, all user-facing actions and significant operations must add a Sentry breadcrumb via SentryReporter::addBreadcrumb(category, message) using the established categories (ui.action, ai.tool_call, file.import, file.export).
🤖 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/HDREnvironmentManager.cpp` around lines 136 - 189,
HDREnvironmentManager::loadEnvironment() is only breadcrumbing the success path
and uses an unsupported category, so move the SentryReporter::addBreadcrumb call
to cover the full import attempt and switch it to the repo’s allowed file-import
category. Add breadcrumb logging for failure returns in loadEnvironment() and
ensure both success and failure cases emit file.import with a clear message so
imports are visible in Sentry and existing dashboards.
Source: Coding guidelines
FetchContent_MakeAvailable pulled in tinyexr's test build, which fails on macOS with -Wpoison-system-directories. We only need the header + miniz. Co-authored-by: Cursor <cursoragent@cursor.com>
- Flip equirect V to match Radiance/UvProject pole convention - Resolve bundled HDRI paths as files; reject missing absolute paths - Release previous Ogre cubemap when switching environments - Load EXR from memory (Unicode-safe); pin stb to commit SHA - Add Ogre test fixture and cubemap-release regression test Co-authored-by: Cursor <cursoragent@cursor.com>
Group HDREnvironmentManager and HdrEquirectLoader (plus tests) under src/HDR/ with PS1-style HDR/… includes for a clearer epic layout. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/HDR/HDREnvironmentManager_test.cpp (1)
83-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid timing as the cache oracle.
EXPECT_LT(secondMs, firstMs + 50)is both loose and CI-sensitive; it can pass without proving a bake-cache hit. Prefer a deterministic observable/test hook for cache reuse, or assert a counter/stat emitted by the manager.🤖 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_test.cpp` around lines 83 - 94, The cache reuse check in HDREnvironmentManager_test is relying on elapsed time, which is flaky and doesn’t deterministically prove a cache hit. Replace the timing-based EXPECT_LT on firstMs/secondMs with a stable assertion using an observable from HDREnvironmentManager or HdrEquirect, such as a cache-hit counter, bake/reuse stat, or another test hook that confirms the second loadEnvironment(path) call reused the cached result.
🤖 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.
Nitpick comments:
In `@src/HDR/HDREnvironmentManager_test.cpp`:
- Around line 83-94: The cache reuse check in HDREnvironmentManager_test is
relying on elapsed time, which is flaky and doesn’t deterministically prove a
cache hit. Replace the timing-based EXPECT_LT on firstMs/secondMs with a stable
assertion using an observable from HDREnvironmentManager or HdrEquirect, such as
a cache-hit counter, bake/reuse stat, or another test hook that confirms the
second loadEnvironment(path) call reused the cached result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d346bd6-772d-4708-b8e6-ff338e4da71e
📒 Files selected for processing (8)
CMakeLists.txtsrc/CMakeLists.txtsrc/HDR/HDREnvironmentManager.cppsrc/HDR/HDREnvironmentManager.hsrc/HDR/HDREnvironmentManager_test.cppsrc/HDR/HdrEquirectLoader.cppsrc/HDR/HdrEquirectLoader.hsrc/HDR/HdrEquirectLoader_test.cpp
💤 Files with no reviewable changes (1)
- src/HDR/HdrEquirectLoader.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CMakeLists.txt
- CMakeLists.txt
Address EXR load undefined-data (S836), RAII cleanup, deprecated isNull, singleton ownership, const_cast on cubemap upload, and C-style array usage so the PR passes the SonarCloud reliability gate. Co-authored-by: Cursor <cursoragent@cursor.com>
Copy TinyEXR pixels into std::vector before free, use init-statement ifs, and match project singleton NOSONAR pattern so S836 no longer blocks the gate. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
HDREnvironmentManagersingleton: loads.hdr(stb) /.exr(tinyexr) equirectangular maps, CPU-bakes a 6-face float cubemap, registers it withOgre::TextureManager, and caches bakes in-memory by SHA-1 of source bytes.HdrEquirecthelpers (load, equirect→cubemap, face-mean utilities) for unit testing without GL.ENABLE_OPENEXR(default ON).Parent epic: #466 · Closes #467
Out of scope for this slice (later PRs): IBL precompute (B), RTSS wiring (C), skybox + tonemap compositor (D), Material Editor controls (E), bundled HDRIs (F), CLI/MCP parity (G).
Test plan
xvfb-run -a ./UnitTests --gtest_filter="HdrEquirectLoaderTest.*:HDREnvironmentManagerTest.*"(8 tests)unit-tests-linuxgreenMade with Cursor
Summary by CodeRabbit
.hdrfiles, plus optional.exrloading when HDR OpenEXR support is enabled in the build.