Lights Slice G: scene round-trip for project & glTF (#489) - #824
Conversation
…idecar (#489). Persist rig-grouped lights through scene save/load using qtmesh.scene.lights metadata, export FBX with a .lights.json sidecar for bit-exact restore, and surface light properties in qtmesh info --json. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds SceneLightsIO for capturing, serializing, importing, and exporting scene lights, then wires those paths into mesh import/export, CLI JSON output, rig metadata, build configuration, and tests. ChangesScene Lights I/O Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: e6e4b76c36
ℹ️ 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".
| scene->mMetaData->Add(kSceneLightsMetadataKey, | ||
| aiString(documentToJson(doc).constData())); |
There was a problem hiding this comment.
For a scene that uses the default three-point rig (or any light document whose compact JSON exceeds 1023 bytes), this aiString construction truncates documentToJson(doc) before the metadata is attached because Assimp strings are capped at 1024 bytes. readDocumentFromAiScene() then sees invalid JSON and falls back to the lossy aiLight path, so the advertised bit-exact glTF/project round-trip loses rig grouping, shadow fields, and ambient data.
Useful? React with 👍 / 👎.
| const float importedIntensity = std::max(SceneLightsIO::ogreLuminance(snapshot.diffuse), 1e-6f); | ||
| snapshot.powerScale = 1.0f; |
There was a problem hiding this comment.
When importing any file that lacks qtmesh.scene.lights metadata, Assimp light intensity is represented by the magnitude of mColorDiffuse (the export path above scales diffuse by powerScaleToGltfIntensity()), but this code computes that luminance and then forces powerScale to 1. The restored scene and qtmesh info therefore report/export intensity near 1 instead of the source value, making third-party glTF/FBX lights much dimmer or brighter than intended.
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MeshImporterExporter.cpp (1)
2962-2968: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFBX export path is missing a
file.exportSentry breadcrumb.The TMD (Line 2972) and RSD (Line 3311) branches emit
SentryReporter::addBreadcrumb("file.export", ...), but the FBX branch does not, even though FBX export is a significant user-facing operation. Additionally, thewriteLightsSidecar(_uri)return value is ignored — a failed sidecar write silently breaks the lights round-trip.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 such as ...file.export."🔧 Proposed fix
} else if (_format == "FBX Binary (*.fbx)") { bool ok = FBXExporter::exportFBX(e, _uri); // FBXExporter embeds textures (Video.Content) so avoid emitting sidecar // .material and extracted image files next to the FBX. if (!ok) return -1; - SceneLightsIO::writeLightsSidecar(_uri); + if (!SceneLightsIO::writeLightsSidecar(_uri)) + Ogre::LogManager::getSingleton().logWarning( + "FBX lights sidecar write failed for " + _uri.toStdString()); + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("Exported FBX: %1").arg(_uri)); } else if (_format == QStringLiteral("PlayStation TMD (*.tmd)")) {🤖 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/MeshImporterExporter.cpp` around lines 2962 - 2968, Add the missing file.export Sentry breadcrumb in the FBX export branch of MeshImporterExporter::export before or around the FBXExporter::exportFBX call, matching the pattern used by the TMD and RSD branches. Also handle the SceneLightsIO::writeLightsSidecar(_uri) result instead of ignoring it, so a failed sidecar write is detected and propagated consistently with the existing export error handling in this method.Source: Coding guidelines
🧹 Nitpick comments (3)
src/SceneLightsIO_test.cpp (1)
92-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the snapshot comparison loop into a helper.
The near-identical name-matching comparison loop appears in both
SceneGltfRoundTripPreservesLightsandFbxSidecarRoundTripPreservesLights. Extracting a helper would eliminate duplication and ensure both tests stay consistent if the comparison logic evolves.♻️ Suggested helper extraction
+// Add to SceneLightsIOTest or a TestHelpers utility: +static void expectSnapshotsMatch(const QList<LightSnapshot>& before, + const QList<LightSnapshot>& after) +{ + ASSERT_EQ(after.size(), before.size()); + for (const LightSnapshot& snapshot : before) + { + bool found = false; + for (const LightSnapshot& imported : after) + { + if (imported.name == snapshot.name) + { + EXPECT_EQ(imported, snapshot) << snapshot.name.toStdString(); + found = true; + break; + } + } + EXPECT_TRUE(found) << snapshot.name.toStdString(); + } +}Then replace both loops with:
- ASSERT_EQ(after.size(), before.size()); - for (const LightSnapshot& snapshot : before) - { - bool found = false; - for (const LightSnapshot& imported : after) - { - if (imported.name == snapshot.name) - { - EXPECT_EQ(imported, snapshot) << snapshot.name.toStdString(); - found = true; - break; - } - } - EXPECT_TRUE(found) << snapshot.name.toStdString(); - } + expectSnapshotsMatch(before, after);Also applies to: 183-196
🤖 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/SceneLightsIO_test.cpp` around lines 92 - 105, The snapshot name-matching comparison loop is duplicated across SceneGltfRoundTripPreservesLights and FbxSidecarRoundTripPreservesLights; extract it into a shared helper near the existing LightSnapshot test utilities in SceneLightsIO_test.cpp. Move the logic that scans before/after, matches by snapshot.name, and asserts equality into a reusable function, then replace both test-local loops with calls to that helper so the comparison behavior stays identical in both tests.src/SceneLightsIO.cpp (1)
399-415: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
appendChildNodereallocates per child (O(n²)); thenewRootChildrenbatch path is dead.For a non-null parent this grows
parent->mChildrenby one element on every call, reallocating and copying the whole array each time. Since callers always pass a non-null parent (scene->mRootNodeor a rig node), therootChildrenaccumulation and the batch-append block at Lines 685-694 never contribute anything, making that code effectively dead while still reallocating the root child array a second time. Consider collecting children per parent and appending once, or reserving capacity up front.🤖 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/SceneLightsIO.cpp` around lines 399 - 415, The current appendChildNode path grows aiNode::mChildren one child at a time, causing repeated reallocations and copies, and the rootChildren accumulation/newRootChildren batch append logic is effectively unused because callers always provide a non-null parent. Update appendChildNode and its callers to collect children per parent and append once (or reserve the final size up front), and remove or simplify the dead batch-append handling so the child arrays are built in a single pass.src/CLIPipeline.cpp (1)
1621-1626: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the extra full-file parse in text mode.
lightsInfoJsonFromFileperforms a fullAssimp::Importer::ReadFile(seeSceneLightsIO.cpp:711-774), yethasLightsInFile/lightsPayloadare only consumed on thejsonOutputpaths. In text mode this is a wasted second parse of the input file on top of theMeshImporterExporter::importerload at Line 1630. Gate the call onjsonOutput.♻️ Guard lights extraction on jsonOutput
- QString lightError; - const QJsonObject lightsPayload = - SceneLightsIO::lightsInfoJsonFromFile(fi.absoluteFilePath(), &lightError); - const int lightsInFile = lightsPayload.value(QStringLiteral("lightCount")).toInt(); - const bool hasLightsInFile = lightsInFile > 0; + QJsonObject lightsPayload; + int lightsInFile = 0; + if (jsonOutput) { + QString lightError; + lightsPayload = SceneLightsIO::lightsInfoJsonFromFile(fi.absoluteFilePath(), &lightError); + lightsInFile = lightsPayload.value(QStringLiteral("lightCount")).toInt(); + } + const bool hasLightsInFile = lightsInFile > 0;🤖 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/CLIPipeline.cpp` around lines 1621 - 1626, `CLIPipeline` is doing an unnecessary second full-file parse through `SceneLightsIO::lightsInfoJsonFromFile` even when text output is being used. Move the `lightsPayload`/`hasLightsInFile` computation behind the `jsonOutput` check in `CLIPipeline` so the extra import only happens on JSON paths, and keep the existing `MeshImporterExporter::importer` load as the only parse in text mode.
🤖 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/CLIPipeline.cpp`:
- Around line 1691-1708: The multi-entity JSON path in
CLIPipeline::infoJsonChanged-style output is wrapping arrays into an object
unconditionally, which changes the schema for consumers of info --json. Update
the branch that handles arr.size() > 1 so it preserves the original bare-array
output when hasLightsInFile is false, and only emits the {"meshes": ...} wrapper
when lights metadata must be included; keep the single-entity and animation-only
paths aligned with this schema choice.
In `@src/MeshImporterExporter.cpp`:
- Around line 2680-2683: The additive import path in MeshImporterExporter is
clearing and replacing existing scene lights through
SceneLightsIO::importLightsSidecar() and SceneLightsIO::importFromAssimpScene(),
even when geometry import may fail. Update this branch so lights are only
merged/restored when the import succeeds and never cleared before the
mesh/import validity check, preserving user lights during additive imports.
In `@src/SceneLightsIO.cpp`:
- Around line 776-807: Add Sentry breadcrumbs to the significant file operations
in writeLightsSidecar and importLightsSidecar so they report user-visible
activity. In writeLightsSidecar, emit SentryReporter::addBreadcrumb with the
file.export category before/around the sidecar write path, and in
importLightsSidecar emit SentryReporter::addBreadcrumb with the file.import
category before/around the sidecar read/apply path. Keep the breadcrumbs tied to
these exact helpers so the export/import actions are traceable even when the
file open or JSON parsing steps fail.
- Around line 18-26: The include list in SceneLightsIO.cpp is missing direct
headers for symbols used in this file. Add the missing includes for QFile and
std::max by updating the top-level include block in SceneLightsIO.cpp, alongside
the existing Qt and STL headers, so the file no longer depends on transitive
includes.
- Around line 449-475: The rig-group lookup in the scene export code is storing
raw pointers to items inside doc.rigGroups, which can become invalid after later
appends. Update the logic in the rig-group collection and light-assignment flow
so groupByNode uses a stable identifier such as the rig group index instead of
&doc.rigGroups.last(), and then use that stable reference when appending lights
to each group. Make the change in the loop that builds RigGroupExport entries
and the loop that matches LightHandle parents to groups.
- Around line 638-646: The metadata payload in appendLightsToAiScene is too
large for aiString and gets truncated, breaking qtmesh.scene.lights round-trips.
Change the SceneLightsIO path so it does not store the full JSON in a single
metadata string: either split the lights data across multiple metadata keys in
aiMetadata or move the full payload to the existing .lights.json sidecar
handling, and keep the kSceneLightsMetadataKey entry limited to a small
reference if needed.
---
Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 2962-2968: Add the missing file.export Sentry breadcrumb in the
FBX export branch of MeshImporterExporter::export before or around the
FBXExporter::exportFBX call, matching the pattern used by the TMD and RSD
branches. Also handle the SceneLightsIO::writeLightsSidecar(_uri) result instead
of ignoring it, so a failed sidecar write is detected and propagated
consistently with the existing export error handling in this method.
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 1621-1626: `CLIPipeline` is doing an unnecessary second full-file
parse through `SceneLightsIO::lightsInfoJsonFromFile` even when text output is
being used. Move the `lightsPayload`/`hasLightsInFile` computation behind the
`jsonOutput` check in `CLIPipeline` so the extra import only happens on JSON
paths, and keep the existing `MeshImporterExporter::importer` load as the only
parse in text mode.
In `@src/SceneLightsIO_test.cpp`:
- Around line 92-105: The snapshot name-matching comparison loop is duplicated
across SceneGltfRoundTripPreservesLights and FbxSidecarRoundTripPreservesLights;
extract it into a shared helper near the existing LightSnapshot test utilities
in SceneLightsIO_test.cpp. Move the logic that scans before/after, matches by
snapshot.name, and asserts equality into a reusable function, then replace both
test-local loops with calls to that helper so the comparison behavior stays
identical in both tests.
In `@src/SceneLightsIO.cpp`:
- Around line 399-415: The current appendChildNode path grows aiNode::mChildren
one child at a time, causing repeated reallocations and copies, and the
rootChildren accumulation/newRootChildren batch append logic is effectively
unused because callers always provide a non-null parent. Update appendChildNode
and its callers to collect children per parent and append once (or reserve the
final size up front), and remove or simplify the dead batch-append handling so
the child arrays are built in a single pass.
🪄 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: 4d9690da-13dd-448a-b4d2-b2bd9413755d
📒 Files selected for processing (9)
src/Assimp/Importer.hsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/LightRigLibrary.cppsrc/MeshImporterExporter.cppsrc/SceneLightsIO.cppsrc/SceneLightsIO.hsrc/SceneLightsIO_test.cpptests/CMakeLists.txt
| #include <QJsonArray> | ||
| #include <QJsonDocument> | ||
| #include <QJsonObject> | ||
| #include <QFileInfo> | ||
| #include <QDir> | ||
|
|
||
| #include <cmath> | ||
| #include <map> | ||
| #include <vector> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the include block and the referenced usages.
sed -n '1,60p' src/SceneLightsIO.cpp
printf '\n--- QFile references ---\n'
rg -n '\bQFile\b' src/SceneLightsIO.cpp
printf '\n--- std::max references ---\n'
rg -n '\bstd::max\b' src/SceneLightsIO.cpp
printf '\n--- Include lines ---\n'
rg -n '^`#include`' src/SceneLightsIO.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2547
Add the direct includes this file uses. QFile and std::max are used here, but <QFile> and <algorithm> are missing from the include list; relying on transitive includes is brittle.
♻️ Proposed include additions
`#include` <QJsonArray>
`#include` <QJsonDocument>
`#include` <QJsonObject>
`#include` <QFileInfo>
+#include <QFile>
`#include` <QDir>
+#include <algorithm>
`#include` <cmath>
`#include` <map>
`#include` <vector>📝 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.
| #include <QJsonArray> | |
| #include <QJsonDocument> | |
| #include <QJsonObject> | |
| #include <QFileInfo> | |
| #include <QDir> | |
| #include <cmath> | |
| #include <map> | |
| #include <vector> | |
| `#include` <QJsonArray> | |
| `#include` <QJsonDocument> | |
| `#include` <QJsonObject> | |
| `#include` <QFileInfo> | |
| `#include` <QFile> | |
| `#include` <QDir> | |
| `#include` <algorithm> | |
| `#include` <cmath> | |
| `#include` <map> | |
| `#include` <vector> |
🤖 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/SceneLightsIO.cpp` around lines 18 - 26, The include list in
SceneLightsIO.cpp is missing direct headers for symbols used in this file. Add
the missing includes for QFile and std::max by updating the top-level include
block in SceneLightsIO.cpp, alongside the existing Qt and STL headers, so the
file no longer depends on transitive includes.
…rip. Chunk large qtmesh.scene.lights metadata across Assimp aiString limits, fix QList invalidation when capturing rig groups, preserve imported Assimp light intensity, keep multi-entity info JSON backward-compatible, and update empty-scene importer tests for restored default lighting. Co-authored-by: Cursor <cursoragent@cursor.com>
Scene import now restores light rig nodes alongside mesh entities; SceneSaveLoadTest assertions that compared raw scene-node counts were failing in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Clear-scene was destroying rig-group light children without unregistering them from LightManager (SIGSEGV). Scene .glb export now writes a lights sidecar like FBX, and scene import prefers that sidecar so user-added lights round-trip reliably. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SceneLightsIO.cpp (1)
827-844: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sourcemislabels sidecar-sourced lights asassimp. When the embedded metadata block is absent, lights are loaded from the.lights.jsonsidecar (Lines 797-804), buthasQtMeshBlockonly inspectsscene->mMetaData, sosourceis reported as"assimp"even though the data is QtMeshEditor's own bit-exact sidecar. Consider distinguishing a"sidecar"source so CLI consumers can tell lossy Assimp reconstruction from exact QtMeshEditor state.🤖 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/SceneLightsIO.cpp` around lines 827 - 844, The source tag in SceneLightsIO::write/export logic is too coarse because hasQtMeshBlock only checks embedded scene metadata, so sidecar-loaded lights are mislabeled as assimp. Update the source selection near the root JSON construction to distinguish the .lights.json sidecar path from the embedded qtmesh.scene.lights block, and keep assimp only for reconstructed fallback data. Use the existing symbols hasQtMeshBlock, kSceneLightsMetadataKey, and kSceneLightsChunkCountKey to locate the current detection logic, and add a separate sidecar-aware source value for exact QtMeshEditor state.src/MeshImporterExporter.cpp (1)
4064-4066: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep light teardown and rebuild in the same rollback path
deleteAllUserLights()runs before thetry, but lights are only re-imported on the success path. If reconstruction throws, the function returnsfalsewith the scene left unlit. Move the delete/restore into thetry/catchflow or add rollback in the catch.🤖 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/MeshImporterExporter.cpp` around lines 4064 - 4066, The light teardown in MeshImporterExporter is happening before the protected rebuild flow, so a throw can leave the scene without restored lights. Move the deleteAllUserLights() and any light re-import logic into the same try/catch rollback path used by the scene rebuild, or add explicit restoration in the catch so failed reconstruction leaves lights in a consistent state. Use the existing LightManager::getSingletonPtr() handling and the import/rebuild block around getSceneNodes() as the place to keep teardown and recovery together.
🧹 Nitpick comments (1)
src/SceneLightsIO.cpp (1)
883-900: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
file.importbreadcrumb to the direct-Assimp import branch. When the sidecar path fails,importLightsFromFilereads the mesh via Assimp and applies lights (Lines 888-899) without emitting a breadcrumb, unlikeimportLightsSidecar. This significant file-import operation should be traceable.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 such asui.action,ai.tool_call,file.import, andfile.export."♻️ Proposed breadcrumb
SceneLightsDocument doc; if (!readDocumentFromAiScene(scene, doc)) return false; + SentryReporter::addBreadcrumb(QStringLiteral("file.import"), + QStringLiteral("Imported lights from %1").arg(path)); return applyToLightManager(doc, useDefaultWhenEmpty);🤖 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/SceneLightsIO.cpp` around lines 883 - 900, The direct-Assimp branch in importLightsFromFile currently performs a significant file import without adding a breadcrumb. Add a Sentry breadcrumb in the path that runs after importLightsSidecar fails and before/around the Assimp::Importer ReadFile/readDocumentFromAiScene/applyToLightManager flow, using SentryReporter::addBreadcrumb with the file.import category and a clear message so this import operation is traceable like the sidecar path.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.
Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 4064-4066: The light teardown in MeshImporterExporter is happening
before the protected rebuild flow, so a throw can leave the scene without
restored lights. Move the deleteAllUserLights() and any light re-import logic
into the same try/catch rollback path used by the scene rebuild, or add explicit
restoration in the catch so failed reconstruction leaves lights in a consistent
state. Use the existing LightManager::getSingletonPtr() handling and the
import/rebuild block around getSceneNodes() as the place to keep teardown and
recovery together.
In `@src/SceneLightsIO.cpp`:
- Around line 827-844: The source tag in SceneLightsIO::write/export logic is
too coarse because hasQtMeshBlock only checks embedded scene metadata, so
sidecar-loaded lights are mislabeled as assimp. Update the source selection near
the root JSON construction to distinguish the .lights.json sidecar path from the
embedded qtmesh.scene.lights block, and keep assimp only for reconstructed
fallback data. Use the existing symbols hasQtMeshBlock, kSceneLightsMetadataKey,
and kSceneLightsChunkCountKey to locate the current detection logic, and add a
separate sidecar-aware source value for exact QtMeshEditor state.
---
Nitpick comments:
In `@src/SceneLightsIO.cpp`:
- Around line 883-900: The direct-Assimp branch in importLightsFromFile
currently performs a significant file import without adding a breadcrumb. Add a
Sentry breadcrumb in the path that runs after importLightsSidecar fails and
before/around the Assimp::Importer
ReadFile/readDocumentFromAiScene/applyToLightManager flow, using
SentryReporter::addBreadcrumb with the file.import category and a clear message
so this import operation is traceable like the sidecar path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6887ffe9-a8d5-4be1-9e1e-e15c6c1aa176
📒 Files selected for processing (9)
src/CLIPipeline.cppsrc/Manager.cppsrc/Manager_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/PropertiesPanelController.cppsrc/SceneLightsIO.cppsrc/SceneLightsIO.hsrc/SceneLightsIO_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CLIPipeline.cpp
- src/SceneLightsIO.h
Defer lights metadata parsing in `info` to JSON output only, restore mesh lights after successful geometry load, check scene sidecar write failures, and add the missing <algorithm> include. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
Scene lighting (rig groups, standalone lights, ambient, shadows) now round-trips through project/scene save and mesh export.
Closes #489
Technical Details
SceneLightsIOmodule captures/restores lights via JSON, Assimpqtmesh.scene.lightsmetadata (chunked when >1023 bytes), and companion*.lights.jsonsidecarssceneExporter/sceneImporterembed metadata and write/read the sidecar (glb2 may drop custom metadata).lights.jsonsidecar; mesh import restores lights only after geometry loads successfullyLightRigLibrarypersistsrigIdon rig-group nodes viaSceneLightsIO::kRigIdUserKeyqtmesh info <file> --jsonaddslights,ambient,lightCountwhen present (bare array preserved for multi-entity files without lights)Features
.scene.gltf/.scene.glb) preserves default rigs, user-added lights, and rig groupinginfo --jsonreports embedded/sidecar lighting metadataBugfixes
deleteAllUserLightsbefore node teardown).scene.glbsave via sidecar (Assimp glb metadata alone is unreliable)Test plan
UnitTests --gtest_filter=SceneLightsIOTest.*:SceneLightsIOOgreTest.*(7/7 pass locally)ManagerHeadlessTest.DestroyAllUserRootNodes_WithLighting_DoesNotCrash