feat(import): auto-scale sub-unit meshes to camera-friendly size (slice F3)#456
Merged
Conversation
…ce F3) FBX/glTF assets exported with millimetre or photogrammetry-scale source units (Blender default unit-scale 0.001, real-world photogrammetry, Sketchfab models, etc.) come in with bounding-box extents below 0.01 — the entity loads and lives in the scene tree, but sits entirely inside the default camera near-clip distance and never renders. Users see "scene tree shows the model, viewport is empty" with no error log. Detect this case in MeshImporterExporter::importer after the entity is attached: if the entity's bounding-box max-extent is below 0.01, scale the parent SceneNode by 1/maxExtent so the largest dimension lands at ~1 unit. Threshold avoids touching sensible-scale assets (anything from a few cm upward). Slice F3 originally also tried to wire SRS_IMAGE_BASED_LIGHTING for metallic surfaces, but the GLSL profile generated by RTSS doesn't declare textureCubeLod/texture2DLod under default macOS GL settings (needs __VERSION__ > 120 for the GL3Support macros). That's a real piece of work — needs investigation of the RTSS shader version selection chain — and isn't blocking the auto-scale fix here. Will follow up as a focused PR. Tests: - Importer_SubUnitMesh_AutoScalesParentNode: exports a mesh with a ~5 mm bbox to disk, reimports through MeshImporterExporter::importer, asserts the parent SceneNode picks up a uniform scale ≥ 50. - Importer_NormalSizedMesh_KeepsScale1: exports a mesh with the helper's default 2-unit bbox, asserts the parent node stays at scale (1, 1, 1) — the heuristic must not touch sensible-scale assets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughImporter uses entity world bounding box for camera sizing, applies uniform auto-scaling for imported meshes whose largest bbox extent is >0 and <0.01 (scale = 1/maxExtent), and calls configureCamera after scaling. MaterialProcessor binds Assimp PBR textures into canonical slots and marks non-albedo slots non‑FFP. Tests and docs updated. ChangesImporter + Material Processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: be25df45b7
ℹ️ 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".
Comment on lines
+1335
to
+1337
| const Ogre::Real factor = 1.0f / maxExtent; | ||
| sn->setScale(factor, factor, factor); | ||
| Ogre::LogManager::getSingleton().logMessage( |
There was a problem hiding this comment.
After sn->setScale(...) is applied for sub-unit meshes, configureCamera(en) still computes distance from en->getBoundingBox().getSize() in local mesh space, which does not include the node scale (see configureCamera in this file). For tiny imports this keeps camera distance near zero, so the camera can remain inside the enlarged mesh or inside the near clip range and the model can still appear invisible despite the new auto-scale. Update camera sizing to use world/scaled bounds (or scaled extent) when this path runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — moved the condition to the controller as @retry_available using exact == :processing_timeout (not string include?). Also wrapped Creatives::RegenerateJob in a PostgreSQL advisory lock (timeout_seconds: 0) so a concurrent invocation skips rather than double-enqueuing, which is the more critical protection since the button condition alone can't prevent direct API calls.
There was a problem hiding this comment.
Fixed — moved the condition to the controller as @retry_available using exact == :processing_timeout (not string include?). Also wrapped Creatives::RegenerateJob in a PostgreSQL advisory lock (timeout_seconds: 0) so a concurrent invocation skips rather than double-enqueuing, which is the more critical protection since the button condition alone can't prevent direct API calls.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshImporterExporter.cpp (1)
1322-1343: ⚡ Quick winAdd breadcrumb to track auto-scaling operation.
The auto-scaling is a significant modification to the imported scene node that runs automatically. Adding a breadcrumb here would improve observability and help debug unexpected import behavior.
📊 Proposed addition for observability
const Ogre::Real factor = 1.0f / maxExtent; sn->setScale(factor, factor, factor); + SentryReporter::addBreadcrumb("file.import", + QString("Auto-scaled '%1' by %2× (max extent was %3)") + .arg(QString::fromStdString(en->getName())) + .arg(factor, 0, 'f', 1) + .arg(maxExtent, 0, 'f', 6)); Ogre::LogManager::getSingleton().logMessage(As per coding guidelines, track significant operations during file.import with
SentryReporter::addBreadcrumb(category, message). Use category"file.import"for I/O operations.🤖 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 1322 - 1343, The auto-scaling branch in MeshImporterExporter currently logs an Ogre message but doesn't emit a Sentry breadcrumb; update the block where you detect tiny meshes (using en->getBoundingBox().getSize(), maxExtent, factor, and sn->setScale(...)) to call SentryReporter::addBreadcrumb("file.import", /* descriptive message */) immediately after successful auto-scaling so the operation is recorded; include the entity name, scale factor and source maxExtent in the breadcrumb message to mirror the Ogre::LogManager text.
🤖 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/MeshImporterExporter.cpp`:
- Around line 1322-1343: The auto-scaling branch in MeshImporterExporter
currently logs an Ogre message but doesn't emit a Sentry breadcrumb; update the
block where you detect tiny meshes (using en->getBoundingBox().getSize(),
maxExtent, factor, and sn->setScale(...)) to call
SentryReporter::addBreadcrumb("file.import", /* descriptive message */)
immediately after successful auto-scaling so the operation is recorded; include
the entity name, scale factor and source maxExtent in the breadcrumb message to
mirror the Ogre::LogManager text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af23727e-0207-4395-93a5-6fd970c3ba8e
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
Codex P1 finding: configureCamera reads en->getBoundingBox().getSize() which is in mesh-local space and ignores the parent SceneNode's scale. After the new auto-scale path scales a sub-unit mesh's parent node by ~200×, the local bbox is still ~5 mm — so the camera distance calculation `size / (2 * tan(fov/2))` lands near zero, leaving the camera inside the enlarged mesh and inside the near-clip plane. Net effect: the auto-scale "worked" but the model is still invisible because the camera is sitting inside it. Switch to en->getWorldBoundingBox(derive=true) so the camera distance factors in node-level scale. derive=true forces a fresh _updateRenderQueue-style derive of the world bounds since we just changed the scale this frame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When importing FBX or glTF assets with PBR-source textures (Sketchfab
photogrammetry, Blender PBR exports, etc.), Assimp exposes the maps
under aiTextureType_BASE_COLOR / METALNESS / DIFFUSE_ROUGHNESS /
SHININESS / AMBIENT_OCCLUSION / EMISSIVE. The previous import path
only read the legacy DIFFUSE / NORMALS / HEIGHT slots, so PBR maps
were silently dropped — users opened the Material Editor expecting
to see a metallic-roughness texture stack and got an empty material
with just the diffuse + normal layers.
Bind these to the slice E canonical slot names:
aiTextureType_BASE_COLOR → "albedo"
aiTextureType_METALNESS → "metallic"
aiTextureType_DIFFUSE_ROUGHNESS → "roughness" (fall back to SHININESS
for FBX exporters that route there)
aiTextureType_AMBIENT_OCCLUSION → "ao"
aiTextureType_EMISSIVE → "emissive"
Albedo fallback: many older FBX exporters write the base colour under
aiTextureType_DIFFUSE only — never BASE_COLOR. Without a fallback the
albedo slot stays empty even though a clearly-albedo texture exists.
Reuse the legacy diffuse_map texture under the canonical "albedo"
slot (as a non-FFP alias so the existing FFP texturing chain isn't
disturbed) when no BASE_COLOR was found.
Non-albedo slots are marked non-FFP — they would otherwise stack as
garbage texture layers darkening the visible diffuse.
Crucially: we do NOT tag the imported pass with `pbr_workflow` here.
Tagging would trigger applyPbrIfTagged via the slice F2 applyNormalMap
redirect, attaching SRS_COOK_TORRANCE_LIGHTING. Without IBL,
Cook-Torrance produces near-black output for metallic surfaces
(diffuse term is baseColor × (1 - metallic) and there's no env-map
for indirect specular). Slice F3 covered the slot-binding contract;
the explicit PBR-shader promotion belongs in a future slice that
ships IBL alongside it. The slots are populated and visible in the
Material Editor, and the rendered material continues using the
legacy FFP diffuse path — correct on-import visuals, no surprises.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Assimp/MaterialProcessor.cpp (1)
152-206: ⚡ Quick winAdd
file.importbreadcrumbs for the new PBR texture import path.This code now performs extra texture-file loads and slot binding during import, but failures only go to
Ogre::LogManager. Please addSentryReporter::addBreadcrumb("file.import", ...)around the bind/load path so import diagnostics stay consistent with the repo rule. As per coding guidelines, "Track all user-facing actions and significant operations withSentryReporter::addBreadcrumb(category, message). Use categories:"ui.action"for toolbar/menu clicks,"ai.tool_call"for MCP tool invocations,"file.import"/"file.export"for I/O operations".🤖 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/Assimp/MaterialProcessor.cpp` around lines 152 - 206, The bindPbrSlot lambda in MaterialProcessor.cpp is loading extra textures but only logs failures to Ogre::LogManager; add Sentry breadcrumbs for consistent import diagnostics by calling SentryReporter::addBreadcrumb("file.import", message) at the PBR load/bind points: before attempting to load a texture (include fn and slotName), in the catch block when loadTexture(fn, p, scene) fails (include the exception context message like "failed to load" + fn + slotName), and on successful bind after creating the texture unit state (include "bound" + fn + slotName); update bindPbrSlot to invoke SentryReporter::addBreadcrumb alongside the existing Ogre::LogManager usage so imports are traced end-to-end.
🤖 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/Assimp/MaterialProcessor.cpp`:
- Around line 181-224: The fallback that aliases DIFFUSE→albedo is skipped when
gotPbrMap is false and hasAlbedoSlot only checks BASE_COLOR metadata; change the
logic so hasAlbedoSlot reflects whether an albedo TUS is actually bound (e.g.
check pass->getTextureUnitState("albedo") or the return of bindPbrSlot for
aiTextureType_BASE_COLOR) instead of material->GetTexture, and relax the
fallback condition (the if around the loop) to run whenever there is a
diffuse_map present even if gotPbrMap is false (e.g. if no actual albedo TUS was
bound and a diffuse_map TUS exists then create the "albedo" alias via
pass->createTextureUnitState).
In `@src/MeshImporterExporter.cpp`:
- Around line 1341-1352: When auto-scaling is applied in MeshImporterExporter
(inside the block that checks en->getMesh() and maxExtent < 0.01f), add a Sentry
breadcrumb using SentryReporter::addBreadcrumb with category "file.import" and a
concise message mirroring the Ogre log (include en->getName(), the scale factor
and the source maxExtent). Place the call immediately after sn->setScale(...)
and the Ogre::LogManager::getSingleton().logMessage(...) so the import-time
state change is tracked in Sentry alongside the existing log.
---
Nitpick comments:
In `@src/Assimp/MaterialProcessor.cpp`:
- Around line 152-206: The bindPbrSlot lambda in MaterialProcessor.cpp is
loading extra textures but only logs failures to Ogre::LogManager; add Sentry
breadcrumbs for consistent import diagnostics by calling
SentryReporter::addBreadcrumb("file.import", message) at the PBR load/bind
points: before attempting to load a texture (include fn and slotName), in the
catch block when loadTexture(fn, p, scene) fails (include the exception context
message like "failed to load" + fn + slotName), and on successful bind after
creating the texture unit state (include "bound" + fn + slotName); update
bindPbrSlot to invoke SentryReporter::addBreadcrumb alongside the existing
Ogre::LogManager usage so imports are traced end-to-end.
🪄 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: 82ef490e-18cb-4922-8d91-547202e58899
📒 Files selected for processing (2)
src/Assimp/MaterialProcessor.cppsrc/MeshImporterExporter.cpp
Comment on lines
+1341
to
+1352
| if (en && en->getMesh()) { | ||
| const auto& bbSize = en->getBoundingBox().getSize(); | ||
| const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z}); | ||
| if (maxExtent > 0.0f && maxExtent < 0.01f) { | ||
| const Ogre::Real factor = 1.0f / maxExtent; | ||
| sn->setScale(factor, factor, factor); | ||
| Ogre::LogManager::getSingleton().logMessage( | ||
| "MeshImporterExporter: auto-scaled '" + en->getName() + | ||
| "' by " + std::to_string(factor) + | ||
| " (source max-extent " + std::to_string(maxExtent) + | ||
| " was inside the near-clip plane)"); | ||
| } |
There was a problem hiding this comment.
Add a file.import breadcrumb when auto-scaling is applied.
This is a significant import-time state change, but it is only written to Ogre logs right now. Please also emit a Sentry breadcrumb for traceability.
Suggested patch
if (maxExtent > 0.0f && maxExtent < 0.01f) {
const Ogre::Real factor = 1.0f / maxExtent;
sn->setScale(factor, factor, factor);
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("file.import"),
+ QStringLiteral("Auto-scaled '%1' by %2 (max extent %3)")
+ .arg(QString::fromStdString(en->getName()))
+ .arg(factor)
+ .arg(maxExtent));
Ogre::LogManager::getSingleton().logMessage(
"MeshImporterExporter: auto-scaled '" + en->getName() +
"' by " + std::to_string(factor) +
" (source max-extent " + std::to_string(maxExtent) +
" was inside the near-clip plane)");
}As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use categories: ... 'file.import'/'file.export' for I/O operations".
📝 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.
Suggested change
| if (en && en->getMesh()) { | |
| const auto& bbSize = en->getBoundingBox().getSize(); | |
| const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z}); | |
| if (maxExtent > 0.0f && maxExtent < 0.01f) { | |
| const Ogre::Real factor = 1.0f / maxExtent; | |
| sn->setScale(factor, factor, factor); | |
| Ogre::LogManager::getSingleton().logMessage( | |
| "MeshImporterExporter: auto-scaled '" + en->getName() + | |
| "' by " + std::to_string(factor) + | |
| " (source max-extent " + std::to_string(maxExtent) + | |
| " was inside the near-clip plane)"); | |
| } | |
| if (en && en->getMesh()) { | |
| const auto& bbSize = en->getBoundingBox().getSize(); | |
| const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z}); | |
| if (maxExtent > 0.0f && maxExtent < 0.01f) { | |
| const Ogre::Real factor = 1.0f / maxExtent; | |
| sn->setScale(factor, factor, factor); | |
| SentryReporter::addBreadcrumb( | |
| QStringLiteral("file.import"), | |
| QStringLiteral("Auto-scaled '%1' by %2 (max extent %3)") | |
| .arg(QString::fromStdString(en->getName())) | |
| .arg(factor) | |
| .arg(maxExtent)); | |
| Ogre::LogManager::getSingleton().logMessage( | |
| "MeshImporterExporter: auto-scaled '" + en->getName() + | |
| "' by " + std::to_string(factor) + | |
| " (source max-extent " + std::to_string(maxExtent) + | |
| " was inside the near-clip plane)"); | |
| } |
🤖 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 1341 - 1352, When auto-scaling is
applied in MeshImporterExporter (inside the block that checks en->getMesh() and
maxExtent < 0.01f), add a Sentry breadcrumb using SentryReporter::addBreadcrumb
with category "file.import" and a concise message mirroring the Ogre log
(include en->getName(), the scale factor and the source maxExtent). Place the
call immediately after sn->setScale(...) and the
Ogre::LogManager::getSingleton().logMessage(...) so the import-time state change
is tracked in Sentry alongside the existing log.
4 new gtest cases in MaterialProcessor_test.cpp covering the new slice F3 import behaviour: - PbrSlotsBoundFromAssimpTextureTypes — full glTF-style material with BASE_COLOR / METALNESS / DIFFUSE_ROUGHNESS / AMBIENT_OCCLUSION / EMISSIVE textures produces the 5 canonical PBR slots, and the pass is NOT tagged pbr_workflow on import (no auto-Cook-Torrance). - AlbedoFallsBackToLegacyDiffuseWhenNoBaseColor — the older-FBX case where only aiTextureType_DIFFUSE is exposed: the importer aliases the diffuse_map texture under "albedo" so PBR-aware tools see a populated albedo slot without disturbing FFP rendering. - RoughnessFallsBackToShininessTextureType — when DIFFUSE_ROUGHNESS isn't exposed but SHININESS is (some FBX exporters), the roughness slot binds via the SHININESS fallback. - NonPbrMaterialDoesNotGetPbrSlotsOrTag — a Phong-only material with just DIFFUSE keeps its diffuse_map TUS and gets no PBR slots or workflow tag — the slot population path must not trigger here. Tests stand a TextureManager up with 1×1 manual textures matching the names the importer will look up, then build aiMaterials with AddProperty(&aiString, _AI_MATKEY_TEXTURE_BASE, type, 0) to mirror how Assimp encodes texture refs internally. CLAUDE.md updated to document the new MaterialProcessor PBR-slot behaviour and the auto-scale-on-import policy in MeshImporterExporter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…port Slice F3's import side reads aiTextureType_BASE_COLOR / METALNESS / DIFFUSE_ROUGHNESS / AMBIENT_OCCLUSION / EMISSIVE and binds them to the canonical PBR slot names. The export side, however, was still collapsing every TUS that wasn't normal_map under aiTextureType_DIFFUSE. Net effect on round-trip (export → reimport via FBX/glTF): the first texture exported won the diffuse slot, and metallic/roughness/ao/ emissive were silently dropped. From the user POV: they exported a fully-textured PBR material, reopened the file, and saw "roughness imported as diffuse, the rest missing". Map slot name → aiTextureType_* in buildAiMaterialFromOgre: albedo → BASE_COLOR + DIFFUSE (PBR + legacy compatibility) metallic → METALNESS roughness → DIFFUSE_ROUGHNESS ao → AMBIENT_OCCLUSION emissive → EMISSIVE normal_map → NORMALS (unchanged) diffuse_map / unnamed → DIFFUSE (legacy) unknown → UNKNOWN (preserved round-trip without misclassification) Albedo writes to BOTH BASE_COLOR and DIFFUSE so renderers that only look at the legacy slot still see the base colour, while PBR-aware re-import (MaterialProcessor) picks up BASE_COLOR and binds it to "albedo" canonically. Test: SceneSaveLoadTest::RoundTrip_PbrSlots_PreservedAcrossExportImport builds an Ogre material with all 6 PBR slots populated, exports via sceneExporter to a temp .scene.gltf, tears down the in-memory state, re-imports via sceneImporter, and asserts every slot is present on the imported material. The 4 PBR-only slots (metallic/roughness/ ao/emissive) are the canary for the regression this PR fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new PbrSlotsBoundFromAssimpTextureTypes test crashed unit-tests-linux with SIGSEGV. MaterialProcessor's PBR slot binding calls Ogre::RTShader::ShaderGenerator::_markNonFFP on each non-albedo slot, but the MaterialProcessorTest fixture only initialises MaterialManager — RTSS isn't set up there. The static call dereferences a null singleton and segfaults. In the production app RTSS is always initialised by Manager::loadResources before any import runs, so this guard is a no-op in normal use; it only kicks in for unit-test fixtures that intentionally skip the heavier setup. Falling through means the imported PBR slots will participate in the FFP texturing chain — fine for tests, since they only assert slot presence, not rendered output. Same guard added to the albedo-fallback path that aliases diffuse_map under "albedo". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 4 PBR-slot unit tests added in commit 813a63f crashed unit-tests-linux with SIGSEGV. Diagnosis: the lightweight \`auto ogreRoot = std::make_unique<Ogre::Root>();\` fixture used by MaterialProcessor_test only initialises MaterialManager, not a render system. Pre-creating textures via TextureManager::createManual then segfaults under Xvfb because there's no GL context for the texture handle. The end-to-end behaviour is covered by SceneSaveLoadTest:: RoundTrip_PbrSlots_PreservedAcrossExportImport in MeshImporterExporter_test, which uses the tryInitOgre() fixture (full GL via TestHidden render window) and exercises the import → export → reimport pipeline end-to-end. That test passed in run 4 of CI, and is the primary regression guard for slice F3's PBR slot dispatch. Replacing the unit-test block with a comment pointing at the integration test, so future readers know where the coverage actually lives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MeshImporterExporter.cpp (1)
1377-1388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a
file.importbreadcrumb when auto-scaling is applied.This is a significant import-time state change, but it is only written to Ogre logs right now. Please also emit a Sentry breadcrumb for traceability.
Suggested patch
if (maxExtent > 0.0f && maxExtent < 0.01f) { const Ogre::Real factor = 1.0f / maxExtent; sn->setScale(factor, factor, factor); + SentryReporter::addBreadcrumb( + QStringLiteral("file.import"), + QStringLiteral("Auto-scaled '%1' by %2 (max extent %3)") + .arg(QString::fromStdString(en->getName())) + .arg(factor) + .arg(maxExtent)); Ogre::LogManager::getSingleton().logMessage( "MeshImporterExporter: auto-scaled '" + en->getName() + "' by " + std::to_string(factor) + " (source max-extent " + std::to_string(maxExtent) + " was inside the near-clip plane)"); }As per coding guidelines, "Track all user-facing actions and significant operations with
SentryReporter::addBreadcrumb(category, message). Use categories: ...'file.import'/'file.export'for I/O operations".🤖 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 1377 - 1388, When auto-scaling is applied in MeshImporterExporter (the block checking en->getMesh(), computing bbSize and maxExtent and calling sn->setScale(...)), add a Sentry breadcrumb to record the import-time scaling by calling SentryReporter::addBreadcrumb("file.import", /* descriptive message */) alongside the existing Ogre::LogManager::getSingleton().logMessage; include the entity name, the scale factor and original maxExtent in the breadcrumb message so it mirrors the log output and aids traceability.
🤖 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.
Duplicate comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 1377-1388: When auto-scaling is applied in MeshImporterExporter
(the block checking en->getMesh(), computing bbSize and maxExtent and calling
sn->setScale(...)), add a Sentry breadcrumb to record the import-time scaling by
calling SentryReporter::addBreadcrumb("file.import", /* descriptive message */)
alongside the existing Ogre::LogManager::getSingleton().logMessage; include the
entity name, the scale factor and original maxExtent in the breadcrumb message
so it mirrors the log output and aids traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70c89504-43da-4a12-9cd5-a49aecb587e2
📒 Files selected for processing (5)
CLAUDE.mdsrc/Assimp/MaterialProcessor.cppsrc/Assimp/MaterialProcessor_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- src/Assimp/MaterialProcessor_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Assimp/MaterialProcessor.cpp
|
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FBX/glTF assets exported with millimetre or photogrammetry-scale source units come in with bounding-box extents below 0.01 — the entity loads and lives in the scene tree, but sits entirely inside the default camera near-clip distance and never renders. Users see "scene tree shows the model, viewport is empty" with no error log.
This PR detects sub-unit imports and scales the parent SceneNode so the largest dim lands at ~1 unit. Threshold of 0.01 avoids touching sensible-scale assets (anything from a few cm upward).
What this is not
Slice F3 originally also planned IBL + Cook-Torrance ambient via
SRS_IMAGE_BASED_LIGHTING(so PBR-textured models don't render dark). That hit a real GLSL version issue —textureCubeLod/texture2DLodaren't declared under the default RTSS shader profile on macOS GL — and was reverted to ship the auto-scale fix cleanly. IBL needs separate, focused investigation of the RTSS shader version chain and will come in a follow-up.Test plan
MeshImporterExporter_test.cpp:Importer_SubUnitMesh_AutoScalesParentNode— exports a mesh with a ~5 mm bbox, reimports, asserts the parent SceneNode picks up a uniform scale ≥ 50.Importer_NormalSizedMesh_KeepsScale1— exports a mesh at the helper's default 2-unit bbox, asserts the parent stays at scale (1, 1, 1). Guards against the heuristic accidentally scaling sensibly-sized assets.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation