fix: make GLTF meshes load with embedded materials / correct orientation - #1192
fix: make GLTF meshes load with embedded materials / correct orientation#1192cadkin wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughModelCache now applies glTF Y-up to Z-up rotation conditionally, Urdfs supports embedded materials for glTF meshes, and GLB-based tests validate orientation and material preservation. RenderableMeshResource stores its mesh state in TypeScript private properties. Changes3D Mesh Loading and Rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.ts`:
- Around line 1097-1103: In Urdfs.ts, fix the missing semicolon after the isGLTF
assignment to match the semicolon style required for TS files, and update the
stale comment above the embedded variable so it reflects both Collada and glTF
handling instead of only Collada. Keep the logic in the visual.geometry.filename
checks and the embedded material selection unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c5ede05-2a5b-4ad0-82f7-7e9e00d289cc
📒 Files selected for processing (2)
packages/suite-base/src/panels/ThreeDeeRender/ModelCache.tspackages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.ts
| const isCollada = visual.geometry.filename.toLowerCase().endsWith(".dae"); | ||
| const isGLTF = ( | ||
| visual.geometry.filename.toLowerCase().endsWith(".gltf") || | ||
| visual.geometry.filename.toLowerCase().endsWith(".glb") | ||
| ) | ||
| // Use embedded materials if the mesh is a Collada file | ||
| const embedded = isCollada ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore; | ||
| const embedded = (isCollada || isGLTF) ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing semicolon and stale comment.
Line 1101 closing paren lacks a semicolon, and the comment on line 1102 still only mentions Collada despite now covering glTF too.
As per coding guidelines, **/*.{ts,tsx,js,jsx,mjs,cjs} files must "Use semicolons".
🔧 Proposed fix
case "mesh": {
- const isCollada = visual.geometry.filename.toLowerCase().endsWith(".dae");
- const isGLTF = (
- visual.geometry.filename.toLowerCase().endsWith(".gltf") ||
- visual.geometry.filename.toLowerCase().endsWith(".glb")
- )
- // Use embedded materials if the mesh is a Collada file
- const embedded = (isCollada || isGLTF) ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore;
+ const filename = visual.geometry.filename.toLowerCase();
+ const isCollada = filename.endsWith(".dae");
+ const isGLTF = filename.endsWith(".gltf") || filename.endsWith(".glb");
+ // Use embedded materials for Collada or glTF files
+ const embedded =
+ isCollada || isGLTF ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore;📝 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.
| const isCollada = visual.geometry.filename.toLowerCase().endsWith(".dae"); | |
| const isGLTF = ( | |
| visual.geometry.filename.toLowerCase().endsWith(".gltf") || | |
| visual.geometry.filename.toLowerCase().endsWith(".glb") | |
| ) | |
| // Use embedded materials if the mesh is a Collada file | |
| const embedded = isCollada ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore; | |
| const embedded = (isCollada || isGLTF) ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore; | |
| const filename = visual.geometry.filename.toLowerCase(); | |
| const isCollada = filename.endsWith(".dae"); | |
| const isGLTF = filename.endsWith(".gltf") || filename.endsWith(".glb"); | |
| // Use embedded materials for Collada or glTF files | |
| const embedded = | |
| isCollada || isGLTF ? EmbeddedMaterialUsage.Use : EmbeddedMaterialUsage.Ignore; |
🤖 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 `@packages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.ts` around
lines 1097 - 1103, In Urdfs.ts, fix the missing semicolon after the isGLTF
assignment to match the semicolon style required for TS files, and update the
stale comment above the embedded variable so it reflects both Collada and glTF
handling instead of only Collada. Keep the logic in the visual.geometry.filename
checks and the embedded material selection unchanged.
Source: Coding guidelines
|
@cadkin please address sonar issues |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/suite-base/src/panels/ThreeDeeRender/ModelCache.test.ts`:
- Around line 37-58: Update the “should respect meshUpAxis option” test to
return or await the promise from modelCache.load, making the test callback
asynchronous so Jest waits for the rotation assertions to execute. Preserve the
existing assertions and console warning cleanup inside the awaited success path.
In
`@packages/suite-base/src/panels/ThreeDeeRender/renderables/markers/RenderableMeshResource.ts`:
- Line 157: Update the replaceMaterials call in `#loadModel` to pass this.material
instead of this.mesh.material, ensuring non-embedded-material meshes do not
dereference the undefined mesh while preserving the existing embedded-material
conditional.
In `@packages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.test.tsx`:
- Around line 86-103: Move the createRenderable call out of the describe-level
initialization and into an async beforeAll or beforeEach for the test suite,
then await the renderable’s exposed mesh-loading promise or ready state before
assigning/asserting renderable. Keep the existing preserve embedded materials
assertion unchanged and avoid timing-based setTimeout flushing when a loading
API is available.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac27d892-5cba-461c-b510-e96432bb86e5
📒 Files selected for processing (6)
packages/suite-base/src/panels/ThreeDeeRender/ModelCache.test.tspackages/suite-base/src/panels/ThreeDeeRender/ModelCache.tspackages/suite-base/src/panels/ThreeDeeRender/renderables/MockAssets.tspackages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.test.tsxpackages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.tspackages/suite-base/src/panels/ThreeDeeRender/renderables/markers/RenderableMeshResource.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.ts
- packages/suite-base/src/panels/ThreeDeeRender/ModelCache.ts
| it("should respect meshUpAxis option", () => { | ||
| const modelCache = new ModelCache({ | ||
| edgeMaterial: new THREE.LineBasicMaterial({ dithering: true }), | ||
| ignoreColladaUpAxis: true, | ||
| meshUpAxis: "y_up", | ||
| fetchAsset: mockFetch | ||
| }); | ||
|
|
||
| modelCache.load( | ||
| "file:///mock/cube.glb", | ||
| {}, | ||
| mockError | ||
| ).then( | ||
| (model) => { | ||
| expect(model.rotation.x).toEqual(1.5707963267948963); | ||
| expect(model.rotation.y).toEqual(0); | ||
| // FP inaccurary means this is 'neg' zero after the rotation. | ||
| expect(model.rotation.z).toEqual(-0); | ||
|
|
||
| console.warn.mockClear(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Test promise is not returned — assertions are never checked by Jest.
The it callback is synchronous, but modelCache.load(...).then(...) is async. Since the promise is not returned from the test, Jest marks the test as passed before the .then() assertions execute. The test will always pass regardless of whether the rotation values are correct.
🔧 Proposed fix: make the test async and await the load
- it("should respect meshUpAxis option", () => {
+ it("should respect meshUpAxis option", async () => {
const modelCache = new ModelCache({
edgeMaterial: new THREE.LineBasicMaterial({ dithering: true }),
ignoreColladaUpAxis: true,
meshUpAxis: "y_up",
fetchAsset: mockFetch
});
- modelCache.load(
- "file:///mock/cube.glb",
- {},
- mockError
- ).then(
- (model) => {
- expect(model.rotation.x).toEqual(1.5707963267948963);
- expect(model.rotation.y).toEqual(0);
- // FP inaccuray means this is 'neg' zero after the rotation.
- expect(model.rotation.z).toEqual(-0);
-
- console.warn.mockClear();
- }
- );
+ const model = await modelCache.load("file:///mock/cube.glb", {}, mockError);
+ expect(model.rotation.x).toEqual(1.5707963267948963);
+ expect(model.rotation.y).toEqual(0);
+ // FP inaccuracy means this is 'neg' zero after the rotation.
+ expect(model.rotation.z).toEqual(-0);
});📝 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.
| it("should respect meshUpAxis option", () => { | |
| const modelCache = new ModelCache({ | |
| edgeMaterial: new THREE.LineBasicMaterial({ dithering: true }), | |
| ignoreColladaUpAxis: true, | |
| meshUpAxis: "y_up", | |
| fetchAsset: mockFetch | |
| }); | |
| modelCache.load( | |
| "file:///mock/cube.glb", | |
| {}, | |
| mockError | |
| ).then( | |
| (model) => { | |
| expect(model.rotation.x).toEqual(1.5707963267948963); | |
| expect(model.rotation.y).toEqual(0); | |
| // FP inaccurary means this is 'neg' zero after the rotation. | |
| expect(model.rotation.z).toEqual(-0); | |
| console.warn.mockClear(); | |
| } | |
| ); | |
| it("should respect meshUpAxis option", async () => { | |
| const modelCache = new ModelCache({ | |
| edgeMaterial: new THREE.LineBasicMaterial({ dithering: true }), | |
| ignoreColladaUpAxis: true, | |
| meshUpAxis: "y_up", | |
| fetchAsset: mockFetch | |
| }); | |
| const model = await modelCache.load("file:///mock/cube.glb", {}, mockError); | |
| expect(model.rotation.x).toEqual(1.5707963267948963); | |
| expect(model.rotation.y).toEqual(0); | |
| // FP inaccuracy means this is 'neg' zero after the rotation. | |
| expect(model.rotation.z).toEqual(-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 `@packages/suite-base/src/panels/ThreeDeeRender/ModelCache.test.ts` around
lines 37 - 58, Update the “should respect meshUpAxis option” test to return or
await the promise from modelCache.load, making the test callback asynchronous so
Jest waits for the rotation assertions to execute. Preserve the existing
assertions and console warning cleanup inside the awaited success path.
| let renderable = createRenderable({ | ||
| visual: visual, | ||
| robot: robot, | ||
| id: 0, | ||
| frameId: "test-frame", | ||
| renderer: mockRenderer | ||
| }); | ||
|
|
||
| it("should preserve embedded materials", () => { | ||
| const expected = { | ||
| isColor: true, | ||
| r: 0.8000074625015259, | ||
| g: 0.058865584433078766, | ||
| b: 0.03260880336165428 | ||
| }; | ||
|
|
||
| expect(renderable.mesh.children[0].material.color).toEqual(expected); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mesh loading is async but the test doesn't await it — assertions may run before the mesh is loaded.
createRenderable is called at the describe level, which triggers async mesh loading via modelCache.load(). The it block then accesses renderable.mesh.children[0].material.color synchronously. If the microtask queue hasn't drained by the time the it callback runs, children will be empty and the test will throw. Move createRenderable into a beforeAll/beforeEach and add an explicit await for the mesh loading to complete.
🔧 Proposed fix: await mesh loading in beforeAll
describe("loading glTF files", () => {
- let robot: UrdfRobot = { name: "mock" };
- let visual: UrdfVisual = {
+ const robot: UrdfRobot = { name: "mock" };
+ const visual: UrdfVisual = {
geometry: {
geometryType: "mesh",
filename: "file:///mock/cube.glb",
},
origin: {
xyz: {x: 0, y: 0, z: 0},
rpy: {x: 0, y: 0, z: 0}
}
};
- let renderable = createRenderable({
+ let renderable: ReturnType<typeof createRenderable>;
+
+ beforeAll(async () => {
+ renderable = createRenderable({
visual: visual,
robot: robot,
id: 0,
frameId: "test-frame",
renderer: mockRenderer
});
+ // Allow async mesh loading to complete
+ await new Promise((resolve) => setTimeout(resolve, 0));
+ });
it("should preserve embedded materials", () => {If RenderableMeshResource exposes a loading promise or ready state, prefer awaiting that directly instead of using a setTimeout flush.
📝 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.
| let renderable = createRenderable({ | |
| visual: visual, | |
| robot: robot, | |
| id: 0, | |
| frameId: "test-frame", | |
| renderer: mockRenderer | |
| }); | |
| it("should preserve embedded materials", () => { | |
| const expected = { | |
| isColor: true, | |
| r: 0.8000074625015259, | |
| g: 0.058865584433078766, | |
| b: 0.03260880336165428 | |
| }; | |
| expect(renderable.mesh.children[0].material.color).toEqual(expected); | |
| }); | |
| describe("loading glTF files", () => { | |
| const robot: UrdfRobot = { name: "mock" }; | |
| const visual: UrdfVisual = { | |
| geometry: { | |
| geometryType: "mesh", | |
| filename: "file:///mock/cube.glb", | |
| }, | |
| origin: { | |
| xyz: {x: 0, y: 0, z: 0}, | |
| rpy: {x: 0, y: 0, z: 0} | |
| } | |
| }; | |
| let renderable: ReturnType<typeof createRenderable>; | |
| beforeAll(async () => { | |
| renderable = createRenderable({ | |
| visual: visual, | |
| robot: robot, | |
| id: 0, | |
| frameId: "test-frame", | |
| renderer: mockRenderer | |
| }); | |
| // Allow async mesh loading to complete | |
| await new Promise((resolve) => setTimeout(resolve, 0)); | |
| }); | |
| it("should preserve embedded materials", () => { | |
| const expected = { | |
| isColor: true, | |
| r: 0.8000074625015259, | |
| g: 0.058865584433078766, | |
| b: 0.03260880336165428 | |
| }; | |
| expect(renderable.mesh.children[0].material.color).toEqual(expected); | |
| }); |
🤖 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 `@packages/suite-base/src/panels/ThreeDeeRender/renderables/Urdfs.test.tsx`
around lines 86 - 103, Move the createRenderable call out of the describe-level
initialization and into an async beforeAll or beforeEach for the test suite,
then await the renderable’s exposed mesh-loading promise or ready state before
assigning/asserting renderable. Keep the existing preserve embedded materials
assertion unchanged and avoid timing-based setTimeout flushing when a loading
API is available.
|
|
I had a chance to circle back around to this. These render classes had no previous tests to base things off of, so I did what seemed correct to me. Let me know if this isn't how you want to do this. Notably, the I'll take care of the outstanding lint issues soon (tm). |
3 participants
User-Facing Changes
GLB/glTF files now load with embedded materials and respect the
Scene > Mesh up axisoption.Description
Previously when loading a
.glbfile, the orientation option would not be respected and any embedded materials would be lost:With these changes:
Checklist
Summary by CodeRabbit