Fix GLB export to keep parented children for wrapper models#478
Merged
tracygardner merged 4 commits intomainfrom Mar 27, 2026
Merged
Fix GLB export to keep parented children for wrapper models#478tracygardner merged 4 commits intomainfrom
tracygardner merged 4 commits intomainfrom
Conversation
Deploying flockdev with
|
| Latest commit: |
b1d9dec
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f269eb2.flockdev.pages.dev |
| Branch Preview URL: | https://codex-update-glb-export-to-i.flockdev.pages.dev |
📝 WalkthroughWalkthroughTightened GLB export node-selection to an explicit allowlist of anchor meshes and their direct children, temporarily substituting transparent "ghost" materials for wrapper nodes during export, and restoring state in a finally block. Added a new XR export test suite and tests that assert the updated Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Flock as flock.exportMesh
participant Mesh as Scene/Meshes
participant GLB as EXPORT.GLTF2Export.GLBAsync
Test->>Flock: call exportMesh(targetMesh, "GLB")
Flock->>Mesh: identify anchorMesh (mesh) and optional direct `__root__` child
Flock->>Mesh: build allowedNodes = anchors + each anchor's direct children
Flock->>Mesh: detect wrapper nodes with direct `__root__` child
Flock->>Mesh: replace wrapper materials with ghost PBRMaterial
Flock->>GLB: invoke GLBAsync with options.shouldExportNode(uses allowedNodes)
GLB-->>Flock: performs export (calls shouldExportNode per node)
Flock->>Mesh: restore original materials and face orientation (finally)
Flock-->>Test: export completes / throws (resources cleaned up)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/xr-export.test.js (1)
60-92: Consider adding test cleanup to avoid potential test pollution.The tests create objects but don't dispose them after completion. While the unique
Date.now()IDs prevent naming collisions, leftover meshes could accumulate in the scene across test runs.💡 Suggested cleanup pattern
it("keeps regular single-model export coverage", async function () { const treeId = flock.createObject({ modelName: "tree.glb", modelId: `tree.glb__single_export_${Date.now()}`, position: { x: -2, y: 0, z: 0 }, }); const treeMesh = await waitForModel(flock, treeId); const originalGLBAsync = flock.EXPORT.GLTF2Export.GLBAsync; let shouldExportNode; flock.EXPORT.GLTF2Export.GLBAsync = async ( _scene, _filename, options, ) => { shouldExportNode = options?.shouldExportNode; return { downloadFiles() {} }; }; try { await flock.exportMesh(treeId, "GLB"); + } finally { + flock.EXPORT.GLTF2Export.GLBAsync = originalGLBAsync; + } + + expect(shouldExportNode).to.be.a("function"); + expect(shouldExportNode(treeMesh)).to.equal(true); + + const exportedDescendants = treeMesh + .getChildMeshes(false) + .filter((mesh) => shouldExportNode(mesh)); + expect(exportedDescendants.length).to.be.greaterThan(0); + + // Cleanup + try { + if (typeof flock.deleteObject === "function") { + flock.deleteObject(treeId); + } } finally { - flock.EXPORT.GLTF2Export.GLBAsync = originalGLBAsync; + // Ensure cleanup doesn't fail the test } - - expect(shouldExportNode).to.be.a("function"); - expect(shouldExportNode(treeMesh)).to.equal(true); - - const exportedDescendants = treeMesh - .getChildMeshes(false) - .filter((mesh) => shouldExportNode(mesh)); - expect(exportedDescendants.length).to.be.greaterThan(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/xr-export.test.js` around lines 60 - 92, The test "keeps regular single-model export coverage" creates objects (via flock.createObject and treeMesh from waitForModel) but never cleans them up; to prevent cross-test pollution, add teardown logic in that test (or an afterEach) to remove/dispose the created resources: delete the created object by its treeId (use flock.deleteObject(treeId) or the project's object removal API) and also call dispose() or remove the mesh instance (treeMesh.dispose() or equivalent) to free geometry/materials; ensure cleanup happens in the finally block after the export so it always runs even if the test fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/xr-export.test.js`:
- Around line 60-92: The test "keeps regular single-model export coverage"
creates objects (via flock.createObject and treeMesh from waitForModel) but
never cleans them up; to prevent cross-test pollution, add teardown logic in
that test (or an afterEach) to remove/dispose the created resources: delete the
created object by its treeId (use flock.deleteObject(treeId) or the project's
object removal API) and also call dispose() or remove the mesh instance
(treeMesh.dispose() or equivalent) to free geometry/materials; ensure cleanup
happens in the finally block after the export so it always runs even if the test
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff29f8b5-feb6-4675-98a4-d074d0da6b4e
📒 Files selected for processing (4)
api/xr.jsscripts/run-api-tests.mjstests/tests.htmltests/xr-export.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/xr.js (1)
287-300:⚠️ Potential issue | 🟡 MinorMissing error handling for
flipFacesrestoration on export failure.If
GLBAsyncrejects, the.then()callback won't execute and the mesh faces will remain flipped. While this pattern pre-dates this PR, the changes touch this code path.🛡️ Suggested fix using try/finally pattern
} else if (format === "GLB") { mesh.flipFaces(); - await flock.EXPORT.GLTF2Export.GLBAsync( - flock.scene, - mesh.name + ".glb", - { - shouldExportNode: (node) => - allowedNodes.has(node) && - !(node.name !== "__root__" && hasRootDescendant(node)), - }, - ).then((glb) => { + try { + const glb = await flock.EXPORT.GLTF2Export.GLBAsync( + flock.scene, + mesh.name + ".glb", + { + shouldExportNode: (node) => + allowedNodes.has(node) && + !(node.name !== "__root__" && hasRootDescendant(node)), + }, + ); + glb.downloadFiles(); + } finally { mesh.flipFaces(); - glb.downloadFiles(); - }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/xr.js` around lines 287 - 300, The GLB export flips mesh faces before calling flock.EXPORT.GLTF2Export.GLBAsync but restores them only in the .then() path, so if GLBAsync rejects the faces remain flipped; change this to use async/await with a try/finally around the await flock.EXPORT.GLTF2Export.GLBAsync(...) call so mesh.flipFaces() is called again in the finally block to guarantee restoration, then call glb.downloadFiles() after a successful await and rethrow or handle the error as appropriate; refer to the format === "GLB" branch, mesh.flipFaces(), flock.EXPORT.GLTF2Export.GLBAsync, and glb.downloadFiles() when making the change.
🧹 Nitpick comments (1)
api/xr.js (1)
271-273: Consider simplifying the predicate for improved readability.The helper function is correctly implemented, but the double negation in the
shouldExportNodecallback (line 294-295) combined with this function makes the logic harder to follow. Consider renaming or restructuring for clarity.♻️ Optional: Rename helper to clarify intent and simplify condition
- const hasRootDescendant = (node) => + const isWrapperMesh = (node) => + node.name !== "__root__" && typeof node?.getChildMeshes === "function" && node.getChildMeshes(false).some((child) => child.name === "__root__"); // ... later in shouldExportNode: shouldExportNode: (node) => - allowedNodes.has(node) && - !(node.name !== "__root__" && hasRootDescendant(node)), + allowedNodes.has(node) && !isWrapperMesh(node),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/xr.js` around lines 271 - 273, The helper function hasRootDescendant is correct but its name and the double-negation use in shouldExportNode makes intent unclear; rename the helper to something explicit like hasRootChildMesh (or hasNoRootDescendant if you prefer a negative) and update its usage in shouldExportNode so the condition reads positively (e.g., if (hasRootChildMesh(node)) { ... } or if (!hasNoRootDescendant(node)) { ... }) to remove the double negation and improve readability—update the function name wherever getChildMeshes-based predicate is referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/xr.js`:
- Around line 253-269: The non-scene export builds allowedNodes using
anchorMesh.getChildMeshes(false) which excludes TransformNode instances and
causes shouldExportNode to drop intermediate TransformNodes; update the
exportAnchors traversal (anchored around symbols anchorMesh and allowedNodes) to
include TransformNodes either by replacing getChildMeshes(false) with
getChildren() when collecting descendants or by additionally iterating
getChildren() and adding nodes whose constructor.name === "TransformNode" (or
checking instance types) so allowedNodes contains the full hierarchy used by
shouldExportNode.
---
Outside diff comments:
In `@api/xr.js`:
- Around line 287-300: The GLB export flips mesh faces before calling
flock.EXPORT.GLTF2Export.GLBAsync but restores them only in the .then() path, so
if GLBAsync rejects the faces remain flipped; change this to use async/await
with a try/finally around the await flock.EXPORT.GLTF2Export.GLBAsync(...) call
so mesh.flipFaces() is called again in the finally block to guarantee
restoration, then call glb.downloadFiles() after a successful await and rethrow
or handle the error as appropriate; refer to the format === "GLB" branch,
mesh.flipFaces(), flock.EXPORT.GLTF2Export.GLBAsync, and glb.downloadFiles()
when making the change.
---
Nitpick comments:
In `@api/xr.js`:
- Around line 271-273: The helper function hasRootDescendant is correct but its
name and the double-negation use in shouldExportNode makes intent unclear;
rename the helper to something explicit like hasRootChildMesh (or
hasNoRootDescendant if you prefer a negative) and update its usage in
shouldExportNode so the condition reads positively (e.g., if
(hasRootChildMesh(node)) { ... } or if (!hasNoRootDescendant(node)) { ... }) to
remove the double negation and improve readability—update the function name
wherever getChildMeshes-based predicate is referenced.
🪄 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: 966a619a-2513-4a44-b8aa-ac90e6cf47e9
📒 Files selected for processing (2)
api/xr.jstests/xr-export.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/xr-export.test.js
Comment on lines
+253
to
269
| const anchorMesh = mesh; | ||
| const rootChild = anchorMesh | ||
| .getChildMeshes() | ||
| .find((child) => child.name === "__root__"); | ||
|
|
||
| const exportAnchors = [anchorMesh]; | ||
| if (rootChild) { | ||
| mesh = rootChild; | ||
| exportAnchors.push(rootChild); | ||
| } | ||
|
|
||
| const allowedNodes = new Set(); | ||
| for (const anchor of exportAnchors) { | ||
| allowedNodes.add(anchor); | ||
| anchor | ||
| .getChildMeshes(false) | ||
| .forEach((childMesh) => allowedNodes.add(childMesh)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if TransformNodes are present in typical mesh hierarchies in this codebase
# and whether previous export logic handled them differently
# Search for TransformNode usage patterns in the codebase
echo "=== TransformNode usage patterns ==="
rg -n "TransformNode" --type js -C2
# Check if there are any tests that verify hierarchy preservation
echo -e "\n=== Test patterns for hierarchy/parent-child ==="
rg -n "parent|hierarchy|TransformNode" --glob "*test*.js" -C2Repository: flipcomputing/flock
Length of output: 50377
🏁 Script executed:
sed -n '220,295p' api/xr.js | cat -nRepository: flipcomputing/flock
Length of output: 2952
🏁 Script executed:
# Check Babylon.js getChildMeshes behavior and TransformNode usage
# Look for TransformNode being added as children
rg "addChild|parent\s*=" api/xr.js -A2 -B2 | head -40
# Check if there are any TransformNodes in model hierarchies
rg "getChildren|getDescendants" api/xr.js -A2 -B2Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Check for getChildren usage (alternative to getChildMeshes that might include TransformNodes)
rg "getChildren\(" api/ -B2 -A2
# Look for any documentation about getChildMeshes in comments
rg "getChildMeshes" api/xr.js -B3 -A3
# Check if there's documentation about Babylon mesh hierarchy
rg "Mesh|TransformNode" docs/ README.md 2>/dev/null || echo "No docs found"Repository: flipcomputing/flock
Length of output: 2417
🏁 Script executed:
# Search for any import of GLB files to understand typical model structure
find . -name "*.glb" -o -name "*.gltf" 2>/dev/null | head -20
# Check for any comments or TODOs about TransformNode handling in export
rg "TODO|FIXME|BUG|TransformNode.*export|export.*TransformNode" api/ -i
# Look at the shouldExportNode call to understand what it's passed
sed -n '74,80p' api/xr.jsRepository: flipcomputing/flock
Length of output: 3199
🏁 Script executed:
# Check if there's any documentation or usage of TransformNode in imported models
rg "AssimpJSON|SceneLoader|importMesh" api/ -B2 -A2 | head -50
# Look at the full export function to understand scope better
sed -n '200,280p' api/xr.js | grep -n "function\|export\|return\|const\|getChild"Repository: flipcomputing/flock
Length of output: 2284
TransformNodes are excluded from non-scene GLB exports, creating an inconsistency with the scene export path.
The scene export branch (lines 220–235) explicitly preserves TransformNodes via if (c === "TransformNode") return true;, but the non-scene branch (lines 253–276) builds allowedNodes using only getChildMeshes(false), which returns only AbstractMesh instances and excludes TransformNodes. When shouldExportNode filters nodes at export time, any TransformNode not in allowedNodes will be rejected, potentially causing the GLTF exporter to skip intermediate hierarchy nodes and re-parent their children unexpectedly. This divergence is problematic since imported GLB files commonly include TransformNodes for non-geometry glTF nodes. Either explicitly include TransformNodes in allowedNodes (as the scene case does), or use getChildren() instead of getChildMeshes() to capture the full hierarchy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/xr.js` around lines 253 - 269, The non-scene export builds allowedNodes
using anchorMesh.getChildMeshes(false) which excludes TransformNode instances
and causes shouldExportNode to drop intermediate TransformNodes; update the
exportAnchors traversal (anchored around symbols anchorMesh and allowedNodes) to
include TransformNodes either by replacing getChildMeshes(false) with
getChildren() when collecting descendants or by additionally iterating
getChildren() and adding nodes whose constructor.name === "TransformNode" (or
checking instance types) so allowedNodes contains the full hierarchy used by
shouldExportNode.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/xr.js (1)
287-300:⚠️ Potential issue | 🟡 Minor
flipFaces()restoration is not guaranteed on export failure.If
GLBAsyncrejects, the.then()callback won't execute andmesh.flipFaces()won't be called, leaving the mesh in a flipped state. The scene export branch (line 242) correctly uses.finally()for cleanup.🛡️ Suggested fix using .finally()
mesh.flipFaces(); - await flock.EXPORT.GLTF2Export.GLBAsync( + flock.EXPORT.GLTF2Export.GLBAsync( flock.scene, mesh.name + ".glb", { shouldExportNode: (node) => allowedNodes.has(node) && !(node.name !== "__root__" && hasDirectRootChild(node)), }, - ).then((glb) => { - mesh.flipFaces(); - glb.downloadFiles(); - }); + ) + .then((glb) => glb.downloadFiles()) + .finally(() => mesh.flipFaces());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/xr.js` around lines 287 - 300, The mesh.flipFaces() call before EXPORT.GLTF2Export.GLBAsync is not restored if GLBAsync rejects; update the GLB export code around EXPORT.GLTF2Export.GLBAsync (the GLBAsync call and its .then callback) to ensure mesh.flipFaces() is always called (use a .finally handler or a try/finally around await) so the mesh state is restored on both success and failure; keep the glb.downloadFiles() behavior on success but move cleanup (mesh.flipFaces()) into the finally block to guarantee restoration.
🧹 Nitpick comments (1)
api/xr.js (1)
293-295: Consider simplifying the double-negative condition for readability.The predicate logic is correct but the double negation makes it harder to parse at a glance.
♻️ Optional refactor for clarity
- shouldExportNode: (node) => - allowedNodes.has(node) && - !(node.name !== "__root__" && hasDirectRootChild(node)), + shouldExportNode: (node) => { + if (!allowedNodes.has(node)) return false; + // Exclude wrapper meshes (those with a direct __root__ child) + // unless the node itself is __root__ + if (node.name === "__root__") return true; + return !hasDirectRootChild(node); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/xr.js` around lines 293 - 295, The predicate in shouldExportNode uses a double-negative which reduces readability; replace the condition allowedNodes.has(node) && !(node.name !== "__root__" && hasDirectRootChild(node)) with an equivalent clearer expression such as checking allowedNodes.has(node) and then explicitly allowing the root name or the absence of a direct root child (e.g., allowedNodes.has(node) && (node.name === "__root__" || !hasDirectRootChild(node))). Update the shouldExportNode definition accordingly, keeping the same logic but making the node.name and hasDirectRootChild checks explicit and easy to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/xr.js`:
- Around line 287-300: The mesh.flipFaces() call before
EXPORT.GLTF2Export.GLBAsync is not restored if GLBAsync rejects; update the GLB
export code around EXPORT.GLTF2Export.GLBAsync (the GLBAsync call and its .then
callback) to ensure mesh.flipFaces() is always called (use a .finally handler or
a try/finally around await) so the mesh state is restored on both success and
failure; keep the glb.downloadFiles() behavior on success but move cleanup
(mesh.flipFaces()) into the finally block to guarantee restoration.
---
Nitpick comments:
In `@api/xr.js`:
- Around line 293-295: The predicate in shouldExportNode uses a double-negative
which reduces readability; replace the condition allowedNodes.has(node) &&
!(node.name !== "__root__" && hasDirectRootChild(node)) with an equivalent
clearer expression such as checking allowedNodes.has(node) and then explicitly
allowing the root name or the absence of a direct root child (e.g.,
allowedNodes.has(node) && (node.name === "__root__" ||
!hasDirectRootChild(node))). Update the shouldExportNode definition accordingly,
keeping the same logic but making the node.name and hasDirectRootChild checks
explicit and easy to read.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/xr.js (1)
314-322: Redundantawaitwith.then()chaining.Using
awaitwith.then()is unnecessary and can be simplified for clarity.♻️ Suggested simplification
- try { - await flock.EXPORT.GLTF2Export.GLBAsync( - flock.scene, - mesh.name + ".glb", - { - shouldExportNode: (node) => allowedNodes.has(node), - }, - ).then((glb) => { - glb.downloadFiles(); - }); - } finally { + try { + const glb = await flock.EXPORT.GLTF2Export.GLBAsync( + flock.scene, + mesh.name + ".glb", + { + shouldExportNode: (node) => allowedNodes.has(node), + }, + ); + glb.downloadFiles(); + } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/xr.js` around lines 314 - 322, The code is redundantly using await together with .then(); change the call to await the promise returned by flock.EXPORT.GLTF2Export.GLBAsync and then call glb.downloadFiles() directly: assign the awaited result to a local (e.g., const glb = await flock.EXPORT.GLTF2Export.GLBAsync(flock.scene, mesh.name + ".glb", { shouldExportNode: (node) => allowedNodes.has(node) });) and then call glb.downloadFiles(); this removes the unnecessary .then() chaining and keeps the same behavior.tests/xr-export.test.js (1)
13-68: Test correctly validates parented children are exported.The test sets up a parent-child relationship via
setParent, stubsGLBAsyncto capture theshouldExportNodepredicate, and verifies both the parent and child meshes satisfy the filter. Thetry/finallyensures the stub is restored.However, the created objects (
hutId,treeId) are not cleaned up after the test, which could affect test isolation if tests share state.♻️ Consider adding afterEach cleanup
describe("XR exportMesh GLB tests `@new`", function () { this.timeout(20000); + + const createdIds = []; + + afterEach(function () { + for (const id of createdIds) { + if (typeof flock.deleteObject === "function") { + flock.deleteObject(id); + } + } + createdIds.length = 0; + }); it("preserves attached children when exporting a wrapper model", async function () { const hutId = flock.createObject({ modelName: "hut.glb", modelId: `hut.glb__export_parent_${Date.now()}`, position: { x: 0, y: 0, z: 0 }, }); + createdIds.push(hutId); const treeId = flock.createObject({ modelName: "tree.glb", modelId: `tree.glb__export_child_${Date.now()}`, position: { x: 2, y: 0, z: 0 }, }); + createdIds.push(treeId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/xr-export.test.js` around lines 13 - 68, The test creates objects hutId and treeId but never cleans them up; modify the test to remove those objects after running (e.g., in the existing try/finally or an afterEach) by calling the platform cleanup API (e.g., flock.deleteObject(hutId) and flock.deleteObject(treeId) or the equivalent remove method) so state is isolated; keep the GLBAsync restore as-is and perform the deletions in the same finally block (or add an afterEach that deletes any created hutId/treeId) to ensure cleanup even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/xr.js`:
- Around line 314-322: The code is redundantly using await together with
.then(); change the call to await the promise returned by
flock.EXPORT.GLTF2Export.GLBAsync and then call glb.downloadFiles() directly:
assign the awaited result to a local (e.g., const glb = await
flock.EXPORT.GLTF2Export.GLBAsync(flock.scene, mesh.name + ".glb", {
shouldExportNode: (node) => allowedNodes.has(node) });) and then call
glb.downloadFiles(); this removes the unnecessary .then() chaining and keeps the
same behavior.
In `@tests/xr-export.test.js`:
- Around line 13-68: The test creates objects hutId and treeId but never cleans
them up; modify the test to remove those objects after running (e.g., in the
existing try/finally or an afterEach) by calling the platform cleanup API (e.g.,
flock.deleteObject(hutId) and flock.deleteObject(treeId) or the equivalent
remove method) so state is isolated; keep the GLBAsync restore as-is and perform
the deletions in the same finally block (or add an afterEach that deletes any
created hutId/treeId) to ensure cleanup even on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b2aa920-872d-457f-8bbd-e1ea8a06b33f
📒 Files selected for processing (2)
api/xr.jstests/xr-export.test.js
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.
Motivation
exportMesh(meshId, "GLB")could drop valid children that were attached viasetParent(parent, child)because the export logic replaced the original anchor with a child named__root__and exported only that subtree.Description
__root__child exists, add it as an additional export anchor instead of replacing the original anchor (changes inapi/xr.js).Set(allowedNodes) containing both anchors and all their descendant meshes and use it inGLTF2Export.GLBAsync(..., { shouldExportNode })so GLB exporting includes wrapper-level attachments.STL/OBJlogic intact and only modify the non-sceneGLBbranch to avoid altering other export flows.tests/xr-export.test.jsthat verifiessetParent(parent, child)attachments are preserved duringexportMesh(..., "GLB")and a sanity check for single-model exports, and register the suite in the browser harness (tests/tests.html) and CLI runner (scripts/run-api-tests.mjs).Testing
npx prettier --checkon the affected files and fixed formatting withnpx prettier --write, which succeeded.npx eslintwhich surfaced preexisting/no-op errors in other files but did not indicate a problem in the new export selection logic.npm run test:api xr-export, but the headless Playwright Chromium binary was not available in this environment so the run failed (Playwright needsnpx playwright install).Codex Task
Summary by CodeRabbit
Bug Fixes
Tests