Add RTSS normal mapping with bundled shader resources - #192
Merged
Conversation
Integrate Ogre's Real-Time Shader System (RTSS) for normal map support on imported FBX/glTF models. Bundle all required RTSS shader files (RTShaderLib/ and Main/) directly in the repo so deployed builds work without the Ogre SDK Media directory. Key changes: - RTShaderHelper: RTSS initialization, scheme resolver, normal map API - MaterialProcessor: auto-detect and apply normal maps from imported models - MeshProcessor: float4 tangents with handedness for correct TBN basis - FBXExporter: texture blend mode property for round-trip fidelity - MaterialEditorQML: expose normal map controls in QML material editor - OgreWidget: enable RTSS material scheme on viewports - Bundled media/RTShaderLib/ (20 shader files) and media/Main/ (13 files) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds shadow volume rendering and normal mapping support to QtMeshEditor. It introduces a unified cross-language shader header for HLSL/CG/Metal/GLSL compatibility, implements shadow extrusion shaders for various light types, establishes Real-Time Shader System (RTSS) initialization infrastructure, updates mesh processing to represent tangent handedness as a vector component, and integrates normal map support into material processing workflows. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes The review requires understanding new RTSS infrastructure (300+ lines), following integration points across multiple existing systems (Manager, MaterialEditor, MeshProcessor), comprehending tangent data structure changes affecting multiple files, and validating the substantial new shader and material declarations. The heterogeneous nature of changes—spanning C++ logic, shaders, materials, QML, and tests—demands separate reasoning for each cohort. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 91baa07d08
ℹ️ 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
+44
to
+45
| Ogre::LogManager::getSingleton().logMessage("MaterialProcessor: Applying RTSS normal map '" + normalFilename + "' to existing material '" + materialName + "'"); | ||
| applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()); |
There was a problem hiding this comment.
In MaterialProcessor::processMaterial, when a material name already exists, this branch applies the imported normal map directly to that shared material and returns it. Because Ogre materials are globally keyed by name, importing a second asset that reuses a common material name (e.g. from a .material script or another model) can silently overwrite the first asset’s RTSS normal-map setup and change rendering across existing entities.
Useful? React with 👍 / 👎.
Comment on lines
+100
to
+101
| if(!normalTexPtr) { | ||
| normalTexPtr = loadTexture(normalFilename, normalPath, scene); |
There was a problem hiding this comment.
The new-material path loads the normal texture without any error handling, so a missing or unreadable normal/height map now raises an Ogre exception and aborts the entire import flow. This is especially likely for assets with stale texture references, and it regresses robustness because normal mapping should be optional rather than fatal when the texture cannot be loaded.
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 (5)
qml/PassPropertiesPanel.qml (1)
320-333: Implementation looks correct; minor signature inconsistency.The refactored Shininess control with factor-based scaling and the
updatingguard to prevent recursive updates is well implemented. The math correctly ensures values converge (e.g., slider 64.5 → SpinBox 6450 → slider 64.5).One minor inconsistency:
textFromValueandvalueFromTexthere omit thelocaleparameter, whereas similar controls in this file (e.g., diffuseAlpha at lines 270-276) include it:// Existing pattern (lines 270-276) textFromValue: function(value, locale) { return value + "%" }This works fine in JavaScript but is inconsistent with the rest of the file.
,
Optional: add locale parameter for consistency
- textFromValue: function(value) { return (value / factor).toFixed(2); } - valueFromText: function(text) { return Math.round(parseFloat(text) * factor); } + textFromValue: function(value, locale) { return (value / factor).toFixed(2); } + valueFromText: function(text, locale) { return Math.round(parseFloat(text) * factor); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/PassPropertiesPanel.qml` around lines 320 - 333, The textFromValue and valueFromText signatures in the Shininess control are inconsistent with other controls — update their function signatures to include the locale parameter (e.g., textFromValue: function(value, locale) { ... } and valueFromText: function(text, locale) { ... }) while keeping the same logic, and ensure the onValueChanged/updating/shininessSlider interactions remain unchanged so the scaling behavior using property factor and the updating guard continues to work.src/Assimp/MeshProcessor_test.cpp (1)
76-80: Cover the positive-handedness path too.These assertions only pin the
-1branch. Please add one case that expectsw == 1, and ideally onecreateMesh()assertion thatVES_TANGENTis emitted asVET_FLOAT4, so a regression in the actual Ogre vertex declaration does not slip through.As per coding guidelines, "Add Google Test unit tests for new functionality in the
src/directory with the_test.cppsuffix."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Assimp/MeshProcessor_test.cpp` around lines 76 - 80, Add a positive-handedness test and a vertex-declaration check: extend MeshProcessor_test.cpp to include a createMesh() invocation that yields a tangent with w == 1 and add an EXPECT_EQ(resultData->tangents[i], Ogre::Vector4(x, y, z, 1)) for that case (use resultData->tangents to find the tangent index), and also assert that the created mesh's vertex declaration emits VES_TANGENT as VET_FLOAT4 (check the vertex declaration returned by createMesh() or the mesh's vertex declaration APIs for VES_TANGENT == VET_FLOAT4). Ensure the new assertions live in the same test file (suffix _test.cpp) alongside the existing negative-handedness checks.src/Assimp/MaterialProcessor.h (1)
16-16: Consider dropping this one-line wrapper.
src/Assimp/MaterialProcessor.cppjust forwards this helper toRTShaderHelper::applyNormalMap, so keeping a separate private declaration adds another symbol to maintain without adding behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Assimp/MaterialProcessor.h` at line 16, Drop the one-line wrapper applyRTSSNormalMap declared in MaterialProcessor.h (and its implementation in MaterialProcessor.cpp); instead, update callers to call RTShaderHelper::applyNormalMap directly and remove the private declaration and any `#includes` only used for that forward-through function to avoid an extra maintained symbol. Ensure you search for occurrences of MaterialProcessor::applyRTSSNormalMap and replace them with RTShaderHelper::applyNormalMap(mat, normalMapName) (or appropriate argument ordering) and then delete the now-unused method and its declaration.media/Main/ShadowExtrudeDirLight.vert (1)
12-15: Clarify the encoded-wconvention here.Line 13 talks about
w, but the branchless extrusion actually keys offuv0.x. A short note thatuv0.xcarries the original homogeneouswwould make this shader much easier to maintain in isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@media/Main/ShadowExtrudeDirLight.vert` around lines 12 - 15, The comment is misleading about `w`—the extrusion is branchless and keyed off uv0.x, not the vertex position.w; update the comment near the newpos calculation to state that uv0.x carries the original homogeneous w (uv0.x==1 => vertex preserved, uv0.x==0 => vertex extruded) and mention that the expression (uv0.xxxx * (position + light_position_object_space)) - light_position_object_space uses uv0.x as that encoded w to select between extruded and non‑extruded positions; reference uv0, newpos, position and light_position_object_space when adding the clarifying note.src/RTShaderHelper_test.cpp (1)
146-152: Avoid pinning the sanity check to an exact upstream file count.The per-file assertions above already verify the required bundle.
EXPECT_EQ(entries.size(), 20)will fail on any harmless upstream Ogre addition/removal, so upgrades become needlessly noisy.💡 Suggested tweak
- EXPECT_EQ(entries.size(), 20) << "Expected 20 files in RTShaderLib/"; + EXPECT_GE(entries.size(), 20) << "Expected at least 20 files in RTShaderLib/";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RTShaderHelper_test.cpp` around lines 146 - 152, The test RTShaderLibHasExpectedFileCount in RTSSResourcesTest currently asserts equality on entries.size() (EXPECT_EQ(entries.size(), 20)), which is brittle against harmless upstream file count changes; update the assertion to a non-strict check such as EXPECT_GE(entries.size(), 20) (or remove this sanity check entirely) so the test only fails if files are missing rather than when extras are added; locate the test function name RTShaderLibHasExpectedFileCount and replace the EXPECT_EQ call that references entries.size() with EXPECT_GE(entries.size(), 20) (or delete the line if you prefer relying on the per-file asserts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@media/Main/OgreUnifiedShader.h`:
- Around line 116-130: The Metal branch under OGRE_METAL lacks shader-entry
macros required by ShadowBlend shaders; add Metal equivalents for
OGRE_UNIFORMS_BEGIN / OGRE_UNIFORMS_END, MAIN_PARAMETERS, MAIN_DECLARATION,
OUT(decl, sem), and mul(a,b) (matching how GLSL/HLSL provide them) so
ShadowBlend.vert and ShadowBlend.frag expand to valid Metal, or alternatively
add a platform guard that prevents compiling ShadowBlend.* for Metal; update the
OGRE_METAL block (where vec2/vec3/etc. and IN() are defined) to include these
macro definitions using Metal/metal:: types and attribute syntax.
In `@src/Assimp/MaterialProcessor.cpp`:
- Around line 92-103: The importer currently assumes normalTexPtr is non-null
and dereferences it after attempting to load a normal map (from
material->GetTexture via hasNormalMap and loadTexture), which can abort import
if loadTexture fails; change the logic so that after attempting to get or load
the texture (normalTexPtr), you check for null/empty (or
Ogre::TexturePtr::isNull()) and only call applyRTSSNormalMap(ogreMaterial,
normalTexPtr->getName()) when normalTexPtr is valid — otherwise skip applying
the normal map and continue with the base material; keep the texture lookup
sequence (getByName then loadTexture) but avoid any unconditional dereference of
normalTexPtr.
- Around line 27-46: Existing shared materials fetched via
Ogre::MaterialManager::getSingleton().getByName(materialName) are mutated in
place by applyRTSSNormalMap, which causes later imports to overwrite earlier
models; to fix, detect when an existingMaterial will be modified by an
import-specific normal (the aiTextureType_NORMALS/HEIGHT path) and clone the
material instead of mutating the shared one: create a unique clone (e.g., using
Ogre::MaterialManager::getSingleton().create or existingMaterial->clone with a
generated name like materialName + "_" + importId), load the normal via
loadTexture into normalTexPtr, and call applyRTSSNormalMap on the cloned
material (and use the clone for the mesh) or alternatively only reuse the shared
existingMaterial when its texture set already matches the incoming normal;
update references so the mesh uses the clone (not the original existingMaterial)
when a clone was made.
In `@src/FBX/FBXExporter.cpp`:
- Around line 1665-1691: The texture-to-material mapping in FBXExporter.cpp
currently only treats tus->getName() == "normal_map" as a normal map; update the
check in the block that builds texMatPairs (the loop over sub entities / pass /
TextureUnitState) so that it treats both "normal_map" and "NormalMap" as normal
maps (i.e. test tus->getName() == "normal_map" || tus->getName() == "NormalMap")
and set fbxProp to "NormalMap" in that case, otherwise use "DiffuseColor" when
inserting tuples used by writeConnection("OP", ...).
In `@src/MaterialEditorQML.cpp`:
- Around line 228-243: The rebind loop uses editedMatName (m_materialName) which
is the new name, so renamed materials won't match existing sub-entities; capture
the original pre-edit material name (e.g., oldMatName) before changes and in the
loop compare each sub-entity's getMaterialName() against both oldMatName and
editedMatName (m_materialName.toStdString()), and if either matches call
setMaterialName(editedMatName); update references in the code around
Manager::getSingleton()->getSceneNodes(), the loop over Ogre::Entity objects,
Entity::getNumSubEntities, getSubEntity and setMaterialName to perform this
dual-name match.
In `@src/MeshImporterExporter.cpp`:
- Around line 221-235: The export currently only treats tus->getName() ==
"normal_map" as a normal map and falls back to diffuse for everything else;
update the texture-name handling in the loop that iterates
pass->getTextureUnitState(ti) so it recognizes the other naming conventions used
elsewhere (e.g., "NormalMap", "normalmap", and known specular names) instead of
defaulting to diffuse; add a case or case-insensitive comparison for normal-map
names to call aiMat->AddProperty(..., AI_MATKEY_TEXTURE(aiTextureType_NORMALS,
normalIdx)) and similarly detect specular-map names to use
aiTextureType_SPECULAR with a specular index variable, incrementing the correct
index counters (normalIdx, specIdx, diffuseIdx) so texture semantics are
preserved on export while still falling back to diffuse only for truly unknown
names.
In `@src/RTShaderHelper.cpp`:
- Around line 95-103: The macOS candidate list currently uses appDir +
"/../../../media/RTShaderLib" which resolves outside the .app bundle; update the
logic that builds candidates (variable candidates and appDir) to also include
the bundle-internal path by using the macBundlePath() root on macOS—e.g., add a
candidate built from macBundlePath() + "/media/RTShaderLib" (or equivalent) so
the RTShaderLib inside the .app/media is discovered; modify the OGRE_PLATFORM ==
OGRE_PLATFORM_APPLE block to push that additional candidate alongside the
existing "../../../media/RTShaderLib" entry.
---
Nitpick comments:
In `@media/Main/ShadowExtrudeDirLight.vert`:
- Around line 12-15: The comment is misleading about `w`—the extrusion is
branchless and keyed off uv0.x, not the vertex position.w; update the comment
near the newpos calculation to state that uv0.x carries the original homogeneous
w (uv0.x==1 => vertex preserved, uv0.x==0 => vertex extruded) and mention that
the expression (uv0.xxxx * (position + light_position_object_space)) -
light_position_object_space uses uv0.x as that encoded w to select between
extruded and non‑extruded positions; reference uv0, newpos, position and
light_position_object_space when adding the clarifying note.
In `@qml/PassPropertiesPanel.qml`:
- Around line 320-333: The textFromValue and valueFromText signatures in the
Shininess control are inconsistent with other controls — update their function
signatures to include the locale parameter (e.g., textFromValue: function(value,
locale) { ... } and valueFromText: function(text, locale) { ... }) while keeping
the same logic, and ensure the onValueChanged/updating/shininessSlider
interactions remain unchanged so the scaling behavior using property factor and
the updating guard continues to work.
In `@src/Assimp/MaterialProcessor.h`:
- Line 16: Drop the one-line wrapper applyRTSSNormalMap declared in
MaterialProcessor.h (and its implementation in MaterialProcessor.cpp); instead,
update callers to call RTShaderHelper::applyNormalMap directly and remove the
private declaration and any `#includes` only used for that forward-through
function to avoid an extra maintained symbol. Ensure you search for occurrences
of MaterialProcessor::applyRTSSNormalMap and replace them with
RTShaderHelper::applyNormalMap(mat, normalMapName) (or appropriate argument
ordering) and then delete the now-unused method and its declaration.
In `@src/Assimp/MeshProcessor_test.cpp`:
- Around line 76-80: Add a positive-handedness test and a vertex-declaration
check: extend MeshProcessor_test.cpp to include a createMesh() invocation that
yields a tangent with w == 1 and add an EXPECT_EQ(resultData->tangents[i],
Ogre::Vector4(x, y, z, 1)) for that case (use resultData->tangents to find the
tangent index), and also assert that the created mesh's vertex declaration emits
VES_TANGENT as VET_FLOAT4 (check the vertex declaration returned by createMesh()
or the mesh's vertex declaration APIs for VES_TANGENT == VET_FLOAT4). Ensure the
new assertions live in the same test file (suffix _test.cpp) alongside the
existing negative-handedness checks.
In `@src/RTShaderHelper_test.cpp`:
- Around line 146-152: The test RTShaderLibHasExpectedFileCount in
RTSSResourcesTest currently asserts equality on entries.size()
(EXPECT_EQ(entries.size(), 20)), which is brittle against harmless upstream file
count changes; update the assertion to a non-strict check such as
EXPECT_GE(entries.size(), 20) (or remove this sanity check entirely) so the test
only fails if files are missing rather than when extras are added; locate the
test function name RTShaderLibHasExpectedFileCount and replace the EXPECT_EQ
call that references entries.size() with EXPECT_GE(entries.size(), 20) (or
delete the line if you prefer relying on the per-file asserts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22d9076d-2920-4813-b29e-9f58bf43382e
⛔ Files ignored due to path filters (22)
media/Main/GLSL_GL3Support.glslis excluded by!**/*.glslmedia/Main/HLSL_SM4Support.hlslis excluded by!**/*.hlslmedia/Main/spot_shadow_fade.ddsis excluded by!**/*.ddsmedia/RTShaderLib/FFPLib_AlphaTest.glslis excluded by!**/*.glslmedia/RTShaderLib/FFPLib_Fog.glslis excluded by!**/*.glslmedia/RTShaderLib/FFPLib_Texturing.glslis excluded by!**/*.glslmedia/RTShaderLib/FFPLib_Transform.glslis excluded by!**/*.glslmedia/RTShaderLib/RTSLib_Colour.glslis excluded by!**/*.glslmedia/RTShaderLib/RTSLib_IBL.glslis excluded by!**/*.glslmedia/RTShaderLib/RTSLib_LTC.glslis excluded by!**/*.glslmedia/RTShaderLib/RTSLib_Lighting.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_CookTorrance.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_DualQuaternion.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_IntegratedPSSM.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_LayeredBlending.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_NormalMap.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_PerPixelLighting.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_TriplanarTexturing.glslis excluded by!**/*.glslmedia/RTShaderLib/SGXLib_WBOIT.glslis excluded by!**/*.glslmedia/RTShaderLib/dfgLUTmultiscatter.ddsis excluded by!**/*.ddsmedia/RTShaderLib/ltc_1.ddsis excluded by!**/*.ddsmedia/RTShaderLib/ltc_2.ddsis excluded by!**/*.dds
📒 Files selected for processing (29)
CMakeLists.txtmedia/Main/DefaultShaders.metalmedia/Main/OgreUnifiedShader.hmedia/Main/Shadow.materialmedia/Main/ShadowBlend.fragmedia/Main/ShadowBlend.vertmedia/Main/ShadowExtrudeDirLight.vertmedia/Main/ShadowExtrudeDirLightFinite.vertmedia/Main/ShadowExtrudePointLight.vertmedia/Main/ShadowExtrudePointLightFinite.vertmedia/Main/ShadowVolumeExtude.programmedia/RTShaderLib/RTSSamplers.materialqml/PassPropertiesPanel.qmlsrc/Assimp/MaterialProcessor.cppsrc/Assimp/MaterialProcessor.hsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/Assimp/MeshProcessor_test.cppsrc/CMakeLists.txtsrc/FBX/FBXExporter.cppsrc/Manager.cppsrc/Manager.hsrc/MaterialEditorQML.cppsrc/MeshImporterExporter.cppsrc/OgreWidget.cppsrc/RTShaderHelper.cppsrc/RTShaderHelper.hsrc/RTShaderHelper_stub.cppsrc/RTShaderHelper_test.cpp
Comment on lines
+116
to
+130
| #elif defined(OGRE_METAL) | ||
|
|
||
| #define vec2 float2 | ||
| #define vec3 float3 | ||
| #define vec4 float4 | ||
| #define mat3 metal::float3x3 | ||
| #define mat4 metal::float4x4 | ||
|
|
||
| #define float32_t float | ||
| #define f32vec2 float2 | ||
| #define f32vec3 float3 | ||
| #define f32vec4 float4 | ||
|
|
||
| #define IN(decl, sem) decl [[ attribute(sem) ]]; | ||
| #else |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
hdr_path = Path("media/Main/OgreUnifiedShader.h")
hdr = hdr_path.read_text()
metal = hdr.split("#elif defined(OGRE_METAL)", 1)[1].split("#else", 1)[0]
required = [
"OGRE_UNIFORMS_BEGIN",
"OGRE_UNIFORMS_END",
"MAIN_PARAMETERS",
"MAIN_DECLARATION",
"OUT(",
"mul("
]
print("Expected result: one or more required symbols are missing from the OGRE_METAL block.\n")
print("Definitions inside OGRE_METAL block:")
for name in required:
print(f" {name}: {'present' if name in metal else 'MISSING'}")
print("\nUsages in the new shared shaders:")
for rel in ["media/Main/ShadowBlend.vert", "media/Main/ShadowBlend.frag"]:
text = Path(rel).read_text()
print(f"\n== {rel} ==")
for i, line in enumerate(text.splitlines(), 1):
if any(tok in line for tok in ["OGRE_UNIFORMS(", "MAIN_PARAMETERS", "MAIN_DECLARATION", "mul("]):
print(f"{i}: {line}")
print("\nOther media/Main files including OgreUnifiedShader.h:")
for p in sorted(Path("media/Main").glob("*")):
if p.is_file():
text = p.read_text(errors="ignore")
if 'OgreUnifiedShader.h' in text and str(p) not in {
"media/Main/ShadowBlend.vert",
"media/Main/ShadowBlend.frag",
"media/Main/OgreUnifiedShader.h",
}:
print(f" {p}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Macro definitions in OgreUnifiedShader.h ==="
cat -n media/Main/OgreUnifiedShader.h | head -200 | tail -100
echo -e "\n=== Check if ShadowBlend shaders have any platform guards ==="
head -20 media/Main/ShadowBlend.vert
echo "---"
head -20 media/Main/ShadowBlend.frag
echo -e "\n=== Check other .vert files for macro usage ==="
for f in media/Main/ShadowExtrudeDirLight.vert media/Main/ShadowExtrudePointLight.vert; do
if [ -f "$f" ]; then
echo "=== $f ==="
grep -n "OGRE_UNIFORMS\|MAIN_PARAMETERS\|MAIN_DECLARATION\|OUT(\|mul(" "$f" | head -5 || echo "No matches"
fi
done
echo -e "\n=== Check DefaultShaders.metal ==="
head -50 media/Main/DefaultShaders.metalRepository: fernandotonon/QtMeshEditor
Length of output: 4860
The OGRE_METAL branch is missing required shader entry-point macros used by ShadowBlend shaders.
The Metal branch (lines 116-129) only defines type aliases and the IN() macro, but lacks:
OGRE_UNIFORMS_BEGIN/OGRE_UNIFORMS_ENDMAIN_PARAMETERSMAIN_DECLARATIONOUT()mul()
The ShadowBlend.vert and ShadowBlend.frag shaders depend on these macros (confirmed in lines 3, 7, 9, 11 and 3, 7, 8 respectively). If compiled for macOS, these will expand to invalid Metal code. The GLSL branch defines MAIN_DECLARATION and mul(), and the HLSL branch defines all required symbols. Add equivalent Metal definitions or guard the ShadowBlend shaders to prevent Metal compilation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@media/Main/OgreUnifiedShader.h` around lines 116 - 130, The Metal branch
under OGRE_METAL lacks shader-entry macros required by ShadowBlend shaders; add
Metal equivalents for OGRE_UNIFORMS_BEGIN / OGRE_UNIFORMS_END, MAIN_PARAMETERS,
MAIN_DECLARATION, OUT(decl, sem), and mul(a,b) (matching how GLSL/HLSL provide
them) so ShadowBlend.vert and ShadowBlend.frag expand to valid Metal, or
alternatively add a platform guard that prevents compiling ShadowBlend.* for
Metal; update the OGRE_METAL block (where vec2/vec3/etc. and IN() are defined)
to include these macro definitions using Metal/metal:: types and attribute
syntax.
Comment on lines
+27
to
+46
| if(auto existingMaterial = Ogre::MaterialManager::getSingleton().getByName(materialName)) { | ||
| // Material already exists (e.g. from a .material script), but still apply | ||
| // normal maps from Assimp if present, since scripts rarely include RTSS directives. | ||
| aiString existingNormalPath; | ||
| if(AI_SUCCESS == material->GetTexture(aiTextureType_NORMALS, 0, &existingNormalPath) | ||
| || AI_SUCCESS == material->GetTexture(aiTextureType_HEIGHT, 0, &existingNormalPath)) { | ||
| std::string normalTexPath = existingNormalPath.C_Str(); | ||
| std::string normalFilename = normalTexPath.substr(normalTexPath.find_last_of("/\\") + 1); | ||
| Ogre::TexturePtr normalTexPtr = Ogre::TextureManager::getSingleton().getByName(normalFilename); | ||
| if(!normalTexPtr) { | ||
| try { | ||
| normalTexPtr = loadTexture(normalFilename, existingNormalPath, scene); | ||
| } catch (...) { | ||
| Ogre::LogManager::getSingleton().logMessage("MaterialProcessor: Failed to load normal map '" + normalFilename + "' for existing material '" + materialName + "'"); | ||
| } | ||
| } | ||
| if(normalTexPtr) { | ||
| Ogre::LogManager::getSingleton().logMessage("MaterialProcessor: Applying RTSS normal map '" + normalFilename + "' to existing material '" + materialName + "'"); | ||
| applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()); | ||
| } |
There was a problem hiding this comment.
Don't rewrite a shared material with import-specific normal maps.
getByName(materialName) returns the existing resource, and RTShaderHelper::applyNormalMap() mutates it in place. If two imports reuse a generic material name but carry different normal maps, the later import silently changes the earlier model too. Clone the material before applying an asset-specific normal map, or only reuse the shared material when the texture set already matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Assimp/MaterialProcessor.cpp` around lines 27 - 46, Existing shared
materials fetched via
Ogre::MaterialManager::getSingleton().getByName(materialName) are mutated in
place by applyRTSSNormalMap, which causes later imports to overwrite earlier
models; to fix, detect when an existingMaterial will be modified by an
import-specific normal (the aiTextureType_NORMALS/HEIGHT path) and clone the
material instead of mutating the shared one: create a unique clone (e.g., using
Ogre::MaterialManager::getSingleton().create or existingMaterial->clone with a
generated name like materialName + "_" + importId), load the normal via
loadTexture into normalTexPtr, and call applyRTSSNormalMap on the cloned
material (and use the clone for the mesh) or alternatively only reuse the shared
existingMaterial when its texture set already matches the incoming normal;
update references so the mesh uses the clone (not the original existingMaterial)
when a clone was made.
Comment on lines
+1665
to
1691
| // Texture → Material (OP) — connect to ALL materials that use each texture | ||
| { | ||
| std::set<std::pair<std::string, std::string>> texMatPairs; | ||
| // Track (texName, matName, fbxProperty) tuples | ||
| std::set<std::tuple<std::string, std::string, std::string>> texMatPairs; | ||
| for (const auto* sub : m_entity->getSubEntities()) | ||
| { | ||
| auto mat = sub->getMaterial(); | ||
| if (mat->getNumTechniques() == 0 || mat->getTechnique(0)->getNumPasses() == 0) continue; | ||
| auto* pass = mat->getTechnique(0)->getPass(0); | ||
| for (unsigned short ti = 0; ti < pass->getNumTextureUnitStates(); ++ti) | ||
| { | ||
| std::string texName = pass->getTextureUnitState(ti)->getTextureName(); | ||
| if (!texName.empty()) | ||
| texMatPairs.insert({texName, mat->getName()}); | ||
| auto* tus = pass->getTextureUnitState(ti); | ||
| std::string texName = tus->getTextureName(); | ||
| if (!texName.empty()) { | ||
| std::string fbxProp = (tus->getName() == "normal_map") ? "NormalMap" : "DiffuseColor"; | ||
| texMatPairs.insert({texName, mat->getName(), fbxProp}); | ||
| } | ||
| } | ||
| } | ||
| for (const auto& [texName, matName] : texMatPairs) | ||
| for (const auto& [texName, matName, fbxProp] : texMatPairs) | ||
| { | ||
| auto texIt = m_textureIds.find(texName); | ||
| auto matIt = m_materialIds.find(matName); | ||
| if (texIt != m_textureIds.end() && matIt != m_materialIds.end()) | ||
| writeConnection("OP", texIt->second, matIt->second, "DiffuseColor"); | ||
| writeConnection("OP", texIt->second, matIt->second, fbxProp); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where "normal_map" is set as a texture unit state name
rg -n -C3 '"normal_map"' --type=cpp
# Also check for setName calls on TextureUnitState to understand naming patterns
ast-grep --pattern 'setName($$$"normal_map"$$$)'Repository: fernandotonon/QtMeshEditor
Length of output: 3681
🏁 Script executed:
#!/bin/bash
# Search for setName calls that might set "NormalMap"
rg -n "setName.*NormalMap" --type=cpp
# Search for where NormalMap is set (not just checked)
rg -n "\"NormalMap\"" --type=cpp -B2 -A2
# Check if there are any imports or material scripts that might use NormalMap
fd -e material -e script | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1615
🏁 Script executed:
#!/bin/bash
# Search for material script parsing or where TUS names might come from material definitions
rg -n "NormalMap" --type=material
# Check if there's any material parsing that might preserve arbitrary TUS names
rg -n "texture_unit\|TextureUnitState" --type=cpp | grep -i "name\|setName" | head -20
# Search for where materials might be loaded from scripts
rg -n "parseScript\|MaterialManager::load" --type=cpp -B2 -A2 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for material loading and parsing code
rg -n "material\|texture_unit" --type=cpp -A3 -B3 | grep -i "parse\|load\|name\|setname" | head -40
# Check content of material scripts directly
find media -name "*.material" -o -name "*.mtl" | head -5 | xargs cat | head -100
# Search for where materials come from during import (FBX, OBJ, etc.)
rg -n "MaterialProcessor\|getMaterial\|setName" src/Material* --type=cpp -B2 -A2 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Check where entities get their materials from in the FBX context
rg -n "m_entity\|getMaterial\|SubEntity" src/FBX/FBXExporter.cpp -B3 -A3 | head -60
# Look for all places where setName is called on TextureUnitState
rg -n "setName.*textureUnit\|tus->setName\|tusName\|TUS.*setName" --type=cpp
# Search specifically for NormalMap being set (not just checked)
rg -n "setName.*NormalMap" --type=cpp
# Check MaterialProcessor to see how it creates/modifies materials
rg -n "class MaterialProcessor\|void.*process.*[Mm]aterial" src/ --type=cpp -A10 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find FBXExporter class definition and initialization
rg -n "class FBXExporter\|FBXExporter.*m_entity\|FBXExporter::" src/FBX/FBXExporter.cpp -B2 -A5 | head -80
# Check the constructor to see where m_entity comes from
rg -n "FBXExporter::" src/FBX/FBXExporter.cpp | head -20
# Search for where normal maps are actually applied during import
rg -n "applyNormalMap\|NormalMap" src/ --type=cpp -B3 -A3Repository: fernandotonon/QtMeshEditor
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Find where exportFBX is called and what entities are passed
rg -n "exportFBX" src/ --type=cpp -B3 -A3
# Check git history for why RTShaderHelper checks both variants
git log --oneline -n 50 -- src/RTShaderHelper.cpp 2>/dev/null | head -10
# Search for any material loading from external sources that might not normalize names
rg -n "load.*[Mm]aterial\|import.*[Mm]aterial" src/ --type=cpp -B2 -A2 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 7270
🏁 Script executed:
#!/bin/bash
# Check the specific commit that added RTSS normal mapping
git log -p --all -S "NormalMap" -- src/RTShaderHelper.cpp 2>/dev/null | head -150
# Also check MaterialEditorQML for context
git log -p --all -S "normal_map.*NormalMap" -- src/MaterialEditorQML.cpp 2>/dev/null | head -100
# Look for any test or documentation about TUS naming conventions
rg -n "texture.*name\|normal.*map" --type=cpp src/FBX/FBXExporter_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Search for any comments explaining the normal_map naming convention
rg -n "normal_map\|NormalMap" src/ --type=cpp -B3 | grep -E "//|/\*" | head -30
# Check test files for expected behavior
rg -n "normal_map\|NormalMap" src/FBX/FBXExporter_test.cpp -B2 -A2
# Look at the RTShaderHelper implementation more carefully for context
sed -n '185,230p' src/RTShaderHelper.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2533
🏁 Script executed:
#!/bin/bash
# Check if materials go through RTShaderHelper before FBXExporter processes them
rg -n "createShaderBasedTechnique\|applyNormalMap\|RTShaderHelper" src/FBX/FBXExporter.cpp
# Look at how the entity's materials are set up before export
rg -n "getMaterial\|setMaterial\|SubEntity" src/FBX/FBXExporter.cpp -B2 -A2 | head -50
# Check if there's any material setup in the export flow
sed -n '1960,2000p' src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 714
🏁 Script executed:
#!/bin/bash
# Find FBXDocumentBuilder to see what it does with materials
rg -n "class FBXDocumentBuilder\|FBXDocumentBuilder::build" src/FBX/FBXExporter.cpp -A15 | head -50
# Check if materials are pre-processed before export
rg -n "build.*entity\|processMaterial\|material.*process" src/FBX/FBXExporter.cpp -B2 -A5 | head -60
# Check the full flow from line 1665 where the issue is
sed -n '1665,1695p' src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1662
Handle both "normal_map" and "NormalMap" texture unit state naming conventions.
At line 1679, the code checks only tus->getName() == "normal_map" to detect normal maps. However, RTShaderHelper.cpp (lines 193, 213, 223) explicitly handles both "normal_map" and "NormalMap" naming variants with the comment "handles both naming conventions" and notes that TUS names can come from .material scripts or previous imports. FBXExporter should apply the same defensive check to ensure normal maps are correctly mapped to the "NormalMap" FBX property regardless of how the texture unit state was originally named:
std::string fbxProp = (tus->getName() == "normal_map" || tus->getName() == "NormalMap") ? "NormalMap" : "DiffuseColor";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter.cpp` around lines 1665 - 1691, The texture-to-material
mapping in FBXExporter.cpp currently only treats tus->getName() == "normal_map"
as a normal map; update the check in the block that builds texMatPairs (the loop
over sub entities / pass / TextureUnitState) so that it treats both "normal_map"
and "NormalMap" as normal maps (i.e. test tus->getName() == "normal_map" ||
tus->getName() == "NormalMap") and set fbxProp to "NormalMap" in that case,
otherwise use "DiffuseColor" when inserting tuples used by writeConnection("OP",
...).
Comment on lines
+228
to
+243
| // Re-apply the edited material to sub-entities that use it. | ||
| // Only update sub-entities whose material name matches — otherwise | ||
| // Entity::setMaterialName would override ALL sub-entities with one material. | ||
| std::string editedMatName = m_materialName.toStdString(); | ||
| for (Ogre::SceneNode* sn : Manager::getSingleton()->getSceneNodes()) { | ||
| if (!sn->getName().empty() && !sn->getAttachedObjects().empty()) { | ||
| Ogre::Entity *e = static_cast<Ogre::Entity *>(sn->getAttachedObject(0)); | ||
| e->setMaterialName(e->getSubEntity(0)->getMaterialName()); | ||
| if (sn->getName().empty() || sn->getAttachedObjects().empty()) | ||
| continue; | ||
| for (auto* obj : sn->getAttachedObjects()) { | ||
| if (obj->getMovableType() != "Entity") continue; | ||
| auto* entity = static_cast<Ogre::Entity*>(obj); | ||
| for (unsigned int si = 0; si < entity->getNumSubEntities(); ++si) { | ||
| if (entity->getSubEntity(si)->getMaterialName() == editedMatName) | ||
| entity->getSubEntity(si)->setMaterialName(editedMatName); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle material renames when rebinding sub-entities.
editedMatName is already the new name here. If the script renames the material, every sub-entity still reports the old name, so this loop matches nothing and the mesh keeps referencing the removed resource. Capture the pre-edit name and match both the old and new names during the rebind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialEditorQML.cpp` around lines 228 - 243, The rebind loop uses
editedMatName (m_materialName) which is the new name, so renamed materials won't
match existing sub-entities; capture the original pre-edit material name (e.g.,
oldMatName) before changes and in the loop compare each sub-entity's
getMaterialName() against both oldMatName and editedMatName
(m_materialName.toStdString()), and if either matches call
setMaterialName(editedMatName); update references in the code around
Manager::getSingleton()->getSceneNodes(), the loop over Ogre::Entity objects,
Entity::getNumSubEntities, getSubEntity and setMaterialName to perform this
dual-name match.
Comment on lines
+221
to
+235
| unsigned short diffuseIdx = 0; | ||
| unsigned short normalIdx = 0; | ||
| for (unsigned short ti = 0; ti < pass->getNumTextureUnitStates(); ++ti) | ||
| { | ||
| auto* tus = pass->getTextureUnitState(ti); | ||
| if (tus->getContentType() == Ogre::TextureUnitState::CONTENT_NAMED) | ||
| { | ||
| aiString texPath(tus->getTextureName()); | ||
| aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, ti)); | ||
| if (tus->getName() == "normal_map") { | ||
| aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_NORMALS, normalIdx)); | ||
| ++normalIdx; | ||
| } else { | ||
| aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, diffuseIdx)); | ||
| ++diffuseIdx; | ||
| } |
There was a problem hiding this comment.
Preserve non-diffuse texture semantics during export.
Lines 229-235 only special-case "normal_map". Elsewhere in this PR, RTShaderHelper already accepts "NormalMap" and specular-map names, so those materials will still round-trip as diffuse here and lose the correct slot on export.
Possible fix
- unsigned short diffuseIdx = 0;
- unsigned short normalIdx = 0;
+ unsigned short diffuseIdx = 0;
+ unsigned short normalIdx = 0;
+ unsigned short specularIdx = 0;
for (unsigned short ti = 0; ti < pass->getNumTextureUnitStates(); ++ti)
{
auto* tus = pass->getTextureUnitState(ti);
if (tus->getContentType() == Ogre::TextureUnitState::CONTENT_NAMED)
{
aiString texPath(tus->getTextureName());
- if (tus->getName() == "normal_map") {
+ const auto& tusName = tus->getName();
+ if (tusName == "normal_map" || tusName == "NormalMap") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_NORMALS, normalIdx));
++normalIdx;
+ } else if (tusName == "specular_map" || tusName == "SpecularMap") {
+ aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_SPECULAR, specularIdx));
+ ++specularIdx;
} else {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, diffuseIdx));
++diffuseIdx;
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 221 - 235, The export currently
only treats tus->getName() == "normal_map" as a normal map and falls back to
diffuse for everything else; update the texture-name handling in the loop that
iterates pass->getTextureUnitState(ti) so it recognizes the other naming
conventions used elsewhere (e.g., "NormalMap", "normalmap", and known specular
names) instead of defaulting to diffuse; add a case or case-insensitive
comparison for normal-map names to call aiMat->AddProperty(...,
AI_MATKEY_TEXTURE(aiTextureType_NORMALS, normalIdx)) and similarly detect
specular-map names to use aiTextureType_SPECULAR with a specular index variable,
incrementing the correct index counters (normalIdx, specIdx, diffuseIdx) so
texture semantics are preserved on export while still falling back to diffuse
only for truly unknown names.
Comment on lines
+95
to
+103
| // Bundled locations (covers macOS .app bundle and Linux/Windows layouts) | ||
| candidates << appDir + "/media/RTShaderLib" | ||
| << appDir + "/../media/RTShaderLib"; | ||
|
|
||
| #if OGRE_PLATFORM == OGRE_PLATFORM_APPLE | ||
| // macOS dev builds: cmake --install puts media at bin/media/ while the | ||
| // binary is 3 levels deeper at bin/QtMeshEditor.app/Contents/MacOS/ | ||
| candidates << appDir + "/../../../media/RTShaderLib"; | ||
| #endif |
There was a problem hiding this comment.
Add the .app/media candidate on macOS.
Inside a bundle, QCoreApplication::applicationDirPath() points to .../Contents/MacOS. The current ../../../media/RTShaderLib candidate resolves next to the app bundle, not inside it, so installed macOS apps won't find the bundled RTSS files.
💡 Suggested fix
`#if` OGRE_PLATFORM == OGRE_PLATFORM_APPLE
// macOS dev builds: cmake --install puts media at bin/media/ while the
// binary is 3 levels deeper at bin/QtMeshEditor.app/Contents/MacOS/
- candidates << appDir + "/../../../media/RTShaderLib";
+ candidates << QDir(appDir + "/../..").absolutePath() + "/media/RTShaderLib"
+ << appDir + "/../../../media/RTShaderLib";
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RTShaderHelper.cpp` around lines 95 - 103, The macOS candidate list
currently uses appDir + "/../../../media/RTShaderLib" which resolves outside the
.app bundle; update the logic that builds candidates (variable candidates and
appDir) to also include the bundle-internal path by using the macBundlePath()
root on macOS—e.g., add a candidate built from macBundlePath() +
"/media/RTShaderLib" (or equivalent) so the RTShaderLib inside the .app/media is
discovered; modify the OGRE_PLATFORM == OGRE_PLATFORM_APPLE block to push that
additional candidate alongside the existing "../../../media/RTShaderLib" entry.
- Add RTShaderHelper_stub.cpp to tests/CMakeLists.txt for MaterialEditorQML test targets (fixes undefined reference errors) - Guard normalTexPtr dereference in MaterialProcessor (null check) - Add macOS bundle-root media path candidate in addRTSSResources - Change RTShaderLib file count assertion from EQ to GE (less brittle) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/RTShaderHelper.cpp (2)
113-122: Empty canonical path handling is correct but consider logging.
QDir::canonicalPath()returns an empty string if the path doesn't exist. Theif (canon.isEmpty() ...)check correctly handles this, but silently skipping non-existent paths could make debugging harder in edge cases.💡 Optional verbose logging
for (const auto& path : withMain) { QString canon = QDir(path).canonicalPath(); - if (canon.isEmpty() || added.contains(canon)) + if (canon.isEmpty()) { + // Path doesn't exist — skip silently (expected for unused candidates) + continue; + } + if (added.contains(canon)) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RTShaderHelper.cpp` around lines 113 - 122, Silent skips of non-existent paths make debugging harder; modify the loop that processes withMain (the block using QSet<QString> added, QDir(path).canonicalPath(), log.logMessage, and rgm.addResourceLocation) to log a warning when QDir(path).canonicalPath() returns empty, including the original path string, and still continue skipping; keep existing duplicate suppression (added.contains(canon)) behavior and only log for the empty canonicalPath case to aid debugging.
83-83: Single scene manager limitation due to static listener.The static
sListenerpointer means only oneSceneManagercan be registered with RTSS at a time. Ifinitialize()is called twice with different scene managers, the first listener is orphaned (memory leak) and only the second scene manager receives scheme resolution.If multiple scene managers are needed in the future, consider storing the listener per scene manager or asserting single-use.
💡 Defensive assertion
void RTShaderHelper::initialize(Ogre::SceneManager* sceneMgr) { + if (sListener) { + Ogre::LogManager::getSingleton().logMessage( + "RTSS: Already initialized — call shutdown() first"); + return; + } + if (!Ogre::RTShader::ShaderGenerator::initialize())Also applies to: 126-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RTShaderHelper.cpp` at line 83, The static sListener causes a single global SchemeResolverListener and leaks/orphans the previous listener when initialize() is called again; change this by associating a listener with each SceneManager (e.g., a std::unordered_map<SceneManager*, SchemeResolverListener*>) or, if multi-SceneManager support isn't needed, add a defensive assert and properly delete the old listener before replacing it. Update usages in initialize(), shutdown() and any cleanup code to look up/remove the listener by the SceneManager pointer (or assert single-use), and ensure you delete the SchemeResolverListener instance to avoid memory leaks; reference symbols: sListener, SchemeResolverListener, initialize(), shutdown().src/RTShaderHelper_test.cpp (1)
20-38: Consider adding macOS.appbundle path candidate.The test
SetUp()checksappDir + "/media/RTShaderLib"andappDir + "/../media/RTShaderLib", but on macOS when running from within a.appbundle, the binary is atContents/MacOS/and media may be at../../media/RTShaderLib(bundle root). This mirrors the production code path added inRTShaderHelper.cpp.💡 Suggested addition
if (!QDir(rtssDir).exists()) { // Try sibling path (Linux .deb layout) QString alt = appDir + "/../media/RTShaderLib"; if (QDir(alt).exists()) { rtssDir = QDir(alt).canonicalPath(); mainDir = QDir(appDir + "/../media/Main").canonicalPath(); + } else { + // Try macOS .app bundle layout (appDir is Contents/MacOS/) + alt = appDir + "/../../media/RTShaderLib"; + if (QDir(alt).exists()) { + rtssDir = QDir(alt).canonicalPath(); + mainDir = QDir(appDir + "/../../media/Main").canonicalPath(); + } } else { GTEST_SKIP() << "RTSS media not installed (run cmake --install)"; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RTShaderHelper_test.cpp` around lines 20 - 38, The SetUp() test currently checks only appDir + "/media/RTShaderLib" and appDir + "/../media/RTShaderLib"; add the macOS .app bundle candidate by also trying appDir + "/../../media/RTShaderLib" (and corresponding mainDir at appDir + "/../../media/Main") when QCoreApplication::applicationDirPath() points into Contents/MacOS, updating rtssDir and mainDir if that path exists before skipping; modify the if (!QDir(rtssDir).exists()) branch in SetUp() to test this bundle-relative path (referencing SetUp(), rtssDir, mainDir, and QCoreApplication::applicationDirPath()) so tests run when executed from a .app bundle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/CMakeLists.txt`:
- Around line 60-62: TEST_SRC_FILES currently contains a duplicate entry for
QMLMaterialHighlighter.cpp which will produce duplicate symbol/linker errors;
remove the redundant mention so QMLMaterialHighlighter.cpp appears only once in
TEST_SRC_FILES (ensure the remaining reference is the correct path and keep the
other listed sources like RTShaderHelper_stub.cpp intact).
---
Nitpick comments:
In `@src/RTShaderHelper_test.cpp`:
- Around line 20-38: The SetUp() test currently checks only appDir +
"/media/RTShaderLib" and appDir + "/../media/RTShaderLib"; add the macOS .app
bundle candidate by also trying appDir + "/../../media/RTShaderLib" (and
corresponding mainDir at appDir + "/../../media/Main") when
QCoreApplication::applicationDirPath() points into Contents/MacOS, updating
rtssDir and mainDir if that path exists before skipping; modify the if
(!QDir(rtssDir).exists()) branch in SetUp() to test this bundle-relative path
(referencing SetUp(), rtssDir, mainDir, and
QCoreApplication::applicationDirPath()) so tests run when executed from a .app
bundle.
In `@src/RTShaderHelper.cpp`:
- Around line 113-122: Silent skips of non-existent paths make debugging harder;
modify the loop that processes withMain (the block using QSet<QString> added,
QDir(path).canonicalPath(), log.logMessage, and rgm.addResourceLocation) to log
a warning when QDir(path).canonicalPath() returns empty, including the original
path string, and still continue skipping; keep existing duplicate suppression
(added.contains(canon)) behavior and only log for the empty canonicalPath case
to aid debugging.
- Line 83: The static sListener causes a single global SchemeResolverListener
and leaks/orphans the previous listener when initialize() is called again;
change this by associating a listener with each SceneManager (e.g., a
std::unordered_map<SceneManager*, SchemeResolverListener*>) or, if
multi-SceneManager support isn't needed, add a defensive assert and properly
delete the old listener before replacing it. Update usages in initialize(),
shutdown() and any cleanup code to look up/remove the listener by the
SceneManager pointer (or assert single-use), and ensure you delete the
SchemeResolverListener instance to avoid memory leaks; reference symbols:
sListener, SchemeResolverListener, initialize(), shutdown().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a296932f-79bd-4857-916f-1c2bee8f6316
📒 Files selected for processing (4)
src/Assimp/MaterialProcessor.cppsrc/RTShaderHelper.cppsrc/RTShaderHelper_test.cpptests/CMakeLists.txt
Comment on lines
60
to
62
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/RTShaderHelper_stub.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/QMLMaterialHighlighter.cpp | ||
| ) |
There was a problem hiding this comment.
Duplicate source file entry will cause linker errors.
QMLMaterialHighlighter.cpp is listed twice in TEST_SRC_FILES (lines 47 and 61). This will result in duplicate symbol errors during linking.
🔧 Proposed fix
${CMAKE_CURRENT_SOURCE_DIR}/../src/CLIPipeline.cpp
${CMAKE_CURRENT_SOURCE_DIR}/../src/RTShaderHelper_stub.cpp
- ${CMAKE_CURRENT_SOURCE_DIR}/../src/QMLMaterialHighlighter.cpp
)📝 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
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/RTShaderHelper_stub.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/QMLMaterialHighlighter.cpp | |
| ) | |
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/RTShaderHelper_stub.cpp | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/CMakeLists.txt` around lines 60 - 62, TEST_SRC_FILES currently contains
a duplicate entry for QMLMaterialHighlighter.cpp which will produce duplicate
symbol/linker errors; remove the redundant mention so QMLMaterialHighlighter.cpp
appears only once in TEST_SRC_FILES (ensure the remaining reference is the
correct path and keep the other listed sources like RTShaderHelper_stub.cpp
intact).
OgreRTShaderSystem is already linked to UnitTests, so the stub providing empty implementations is unnecessary. Using the real RTShaderHelper.cpp avoids maintaining duplicate symbol definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/CMakeLists.txt (1)
60-61:⚠️ Potential issue | 🔴 CriticalRemove the second
QMLMaterialHighlighter.cppentry.Line 61 still duplicates Line 47 in
TEST_SRC_FILES, so each test target compiles the same translation unit twice and will fail at link time.Suggested fix
${CMAKE_CURRENT_SOURCE_DIR}/../src/CLIPipeline.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/RTShaderHelper.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/../src/QMLMaterialHighlighter.cpp )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CMakeLists.txt` around lines 60 - 61, The TEST_SRC_FILES list contains a duplicate entry for QMLMaterialHighlighter.cpp which causes the same translation unit to be compiled twice; remove the redundant QMLMaterialHighlighter.cpp line from tests/CMakeLists.txt (so TEST_SRC_FILES contains unique source entries) and verify no other duplicate source filenames remain in that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/CMakeLists.txt`:
- Around line 60-61: The TEST_SRC_FILES list contains a duplicate entry for
QMLMaterialHighlighter.cpp which causes the same translation unit to be compiled
twice; remove the redundant QMLMaterialHighlighter.cpp line from
tests/CMakeLists.txt (so TEST_SRC_FILES contains unique source entries) and
verify no other duplicate source filenames remain in that variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 426868ba-e983-4cb2-844d-a6e787be9ed9
📒 Files selected for processing (2)
src/CMakeLists.txttests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CMakeLists.txt
|
This was referenced Mar 13, 2026
7 tasks
5 tasks
5 tasks
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
1 participant
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
media/RTShaderLib/andmedia/Main/) directly in the repo so deployed builds work without the Ogre SDK Media directoryKey files
src/RTShaderHelper.h/cpp(init, scheme resolver, normal map API)src/RTShaderHelper_stub.cpp(for unit test binary)src/RTShaderHelper_test.cpp(21 tests verifying bundled resources)media/RTShaderLib/(20 files),media/Main/(13 files)src/Assimp/MaterialProcessor.cpp,MeshProcessor.cppsrc/FBX/FBXExporter.cppqml/PassPropertiesPanel.qml,src/MaterialEditorQML.cppsrc/Manager.cpp/h,src/OgreWidget.cppTest plan
RTSSResourcesTesttests pass (verify bundled shader files exist)MeshProcessorTestupdated for float4 tangents🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes