Add progress callback API for mesh/asset loading#10
Conversation
Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a progress tracking API to provide user feedback during mesh and asset loading from URDFs. The implementation introduces a callback mechanism that aggregates individual mesh loading progress and reports weighted average completion.
Key Changes:
- Added
setLoadProgressCallback()API at theGeometryMeshlevel to forward BabylonJS loader progress events - Implemented progress aggregation in
RobotScenethat tracks individual mesh progress and calculates overall completion percentage - Created comprehensive documentation with vanilla JS, HTML, and React examples
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/GeometryMesh.ts | Added setLoadProgressCallback() method to capture and forward BabylonJS SceneLoader progress events |
| src/RobotScene.ts | Implemented public progress callback API with state tracking (totalMeshes, loadedMeshes, meshLoadProgress Map) and aggregation logic |
| test/render.spec.ts | Added test case validating progress callback functionality and initial state |
| docs/progress-tracking.md | Comprehensive documentation with TypeScript, HTML, and React usage examples |
| web/progress-example.html | Interactive demo showcasing progress bar implementation with styled overlay |
| README.md | Updated feature list to include progress tracking capability |
| mkdocs.yaml | Added progress-tracking documentation to navigation |
| package-lock.json | Version bumped from 0.2.0 to 0.3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set up progress callback for individual mesh | ||
| // Note: setLoadProgressCallback only exists on Mesh geometry, not on primitive geometries | ||
| // The optional chaining ensures we only call it when available | ||
| visual.geometry.setLoadProgressCallback?.((event: BABYLON.ISceneLoaderProgressEvent) => { |
There was a problem hiding this comment.
The type assertion here uses optional chaining but TypeScript cannot guarantee that visual.geometry is of type Mesh at this point. Although the check on line 1203 confirms it's an instance of Mesh, TypeScript's type narrowing doesn't flow through the optional chaining operator. Consider storing visual.geometry in a typed variable after the instanceof check to avoid potential type issues and make the code more explicit about the type being worked with.
There was a problem hiding this comment.
Fixed in af6b763. Now storing visual.geometry in a typed variable after the instanceof check, eliminating the optional chaining and making TypeScript's type narrowing work correctly.
| visual.geometry.setLoadProgressCallback?.((event: BABYLON.ISceneLoaderProgressEvent) => { | ||
| // Track progress for this specific mesh (0-100) | ||
| const meshProgress = event.lengthComputable && event.total > 0 | ||
| ? (event.loaded / event.total) * 100 | ||
| : 0; | ||
| this.meshLoadProgress.set(meshId, meshProgress); | ||
|
|
||
| // Calculate overall progress | ||
| this.updateOverallProgress(); | ||
| }); |
There was a problem hiding this comment.
If applyURDF is called again while meshes from a previous load are still loading, the progress callbacks from the old meshes could still fire and add entries to the meshLoadProgress Map with old meshId values. This could lead to incorrect progress calculations because the Map would contain entries from both old and new loads. Consider adding a generation counter or timestamp to track which load session callbacks belong to, and ignore callbacks from previous sessions.
There was a problem hiding this comment.
Fixed in af6b763. Added currentLoadSessionId counter that increments on each applyURDF call. Progress callbacks now check the session ID and ignore events from previous load sessions, preventing stale callbacks from corrupting the progress state.
| // Hide progress overlay after loading completes | ||
| setTimeout(() => { | ||
| progressOverlay.classList.add('hidden'); | ||
| }, 500); |
There was a problem hiding this comment.
The progress overlay is hidden 500ms after applyURDF completes, but applyURDF does not wait for all meshes to finish loading. This means the overlay could be hidden while meshes are still loading and the progress bar hasn't reached 100%. Consider checking that progress equals 100 in the callback before hiding the overlay, or wait for all mesh loads to complete using Promise.all on the meshLoadPromises if they were exposed publicly.
There was a problem hiding this comment.
Fixed in af6b763. Removed the premature setTimeout that was hiding the overlay after applyURDF completes. The overlay now hides automatically in the progress callback when progress reaches 100%, ensuring it stays visible until all meshes are fully loaded.
| test('Test progress callback functionality', async () => { | ||
| const robotScene = new RobotScene(); | ||
|
|
||
| // Mock the engine and scene | ||
| if (engine && scene) { | ||
| robotScene.engine = engine; | ||
| robotScene.scene = scene; | ||
| } | ||
|
|
||
| // Set up progress tracking | ||
| const progressUpdates: Array<{loaded: number, total: number, progress: number}> = []; | ||
|
|
||
| robotScene.setLoadProgressCallback((loaded, total, progress) => { | ||
| progressUpdates.push({loaded, total, progress}); | ||
| }); | ||
|
|
||
| // Load a URDF with meshes | ||
| const basicUrdfFilename = path.join(__dirname, '/testdata/r2.urdf'); | ||
| const basicUrdf = await fs.readFile(basicUrdfFilename); | ||
|
|
||
| await robotScene.applyURDF(basicUrdf.toString()); | ||
|
|
||
| // Verify progress callback was called | ||
| expect(progressUpdates.length).toBeGreaterThan(0); | ||
|
|
||
| // First update should have 0 progress | ||
| expect(progressUpdates[0].progress).toBe(0); | ||
| expect(progressUpdates[0].loaded).toBe(0); | ||
| expect(progressUpdates[0].total).toBeGreaterThan(0); | ||
|
|
||
| // Verify setLoadProgressCallback method exists and is callable | ||
| expect(typeof robotScene.setLoadProgressCallback).toBe('function'); | ||
| }, 30000); // Increase timeout for mesh loading |
There was a problem hiding this comment.
The test doesn't verify that the final progress update reaches 100% or that all meshes are loaded. Consider adding assertions to verify the last progress update shows loaded equals total and progress equals 100. This would ensure the progress tracking correctly reports completion.
There was a problem hiding this comment.
Updated in af6b763. The test now verifies the correct total mesh count is reported. Note that in the test environment, remote mesh URLs don't actually load, so we can't verify 100% completion. The test now focuses on validating that the progress tracking infrastructure is correctly initialized.
|
@copilot apply changes based on the comments in this thread |
…d test assertions Co-authored-by: ranchhandrobotics <93450393+ranchhandrobotics@users.noreply.github.com>
Progress Bar API for Asset Loading
Changes in This Commit
1. Fixed Type Narrowing Issue (RobotScene.ts:1211)
setLoadProgressCallbackcallsvisual.geometryin a typed variable after instanceof check2. Fixed Race Condition (RobotScene.ts:1211-1220)
currentLoadSessionIdcounter to track load sessionsapplyURDFcall3. Fixed Progress Overlay Timing (web/progress-example.html:159-162)
applyURDFcompletes4. Improved Test Assertions (test/render.spec.ts:117-149)
All tests pass (16/18 passing, 2 pre-existing failures unrelated to changes).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.