Refactor conversion of glTF nodes to luma.gl nodes, add skin capability #2391
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the conversion of glTF nodes to luma.gl nodes and adds initial support for skinning. It also updates related parsing, animation, and scenegraph logic to integrate the new skin capabilities.
- Introduces a new skin shader module with skin uniform handling.
- Refactors glTF parsers to return additional maps (nodeMap and meshMap) and adapts animation and scenegraph functions.
- Updates scenegraph transformations and traversal methods along with various minor improvements.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/shadertools/src/modules/engine/skin/skin.ts | Added a new skin shader module with functions for joint matrix computation and skinning uniforms. |
| modules/shadertools/src/lib/shader-module/shader-module.ts | Extended the type for defines from boolean to boolean |
| modules/gltf/src/parsers/parse-pbr-material.ts | Added a condition to set a skinning define when JOINTS_0 and WEIGHTS_0 attributes are present. |
| modules/gltf/src/parsers/parse-gltf.ts | Refactored parsing to return scenes along with nodeMap and meshMap; improved scene and mesh attachment. |
| modules/gltf/src/parsers/parse-gltf-animations.ts | Updated animation parsing to use a nodeMap and implement caching via accessorCache. |
| modules/gltf/src/gltf/gltf-animator.ts | Removed unused transformation update function to simplify animator logic. |
| modules/gltf/src/gltf/create-scenegraph-from-gltf.ts | Adjusted scenegraph creation logic to return a comprehensive structure for scenes, nodeMap, and meshMap. |
| modules/gltf/src/gltf/create-gltf-model.ts | Integrated the skin module into model creation and updated vertex processing to account for skinning. |
| modules/gltf/src/gltf/animations/interpolate.ts | Refactored animation interpolation logic to use updateTargetPath for smoother target transformations. |
| modules/engine/src/scenegraph/scenegraph-node.ts | Enhanced assertion checks in transformation setters to support quaternion rotations. |
| modules/engine/src/scenegraph/group-node.ts | Added a preorderTraversal helper to simplify scenegraph traversal. |
| modules/core/src/portable/uniform-buffer-layout.ts | Increased the minimum uniform buffer size with a temporary hack noted in a comment. |
| examples/tutorials/hello-gltf/app.ts | Updated scene graph handling and UI initialization to reflect the new scenegraph structure. |
| SKIN_MAX_JOINTS | ||
| }, | ||
|
|
||
| getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => { |
There was a problem hiding this comment.
Some simple test cases for get uniforms would be nice...
| sampler: GLTFAnimationSampler; | ||
| target: GLTFNodePostprocessed; | ||
| target: GroupNode; |
There was a problem hiding this comment.
Q: could the target be by node key instead of Node?
There was a problem hiding this comment.
Using GroupNode makes this generic and detaches it from glTF. What is node key?
There was a problem hiding this comment.
Using GroupNode makes this generic and detaches it from glTF. What is node key?
It detaches it from glTF but links it to the "dynamic" Node structure, forcing apps to keep track of generated nodes.
Referencing a node by key would decouple it further, in much more declarative way, if that is possible.
Since the glTF keys are not guaranteed to be unique, one option is that we mint our own unique keys on nodes, or maybe defined a path concept of keys through the hierarcy.
That would be for a separate PR, obviously.
georgioskarnas
changed the title
|
Remaining issues:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the conversion of glTF data into luma.gl scenegraphs and introduces skinning support throughout the pipeline.
- Adds a new
skinshader module with joint‐matrix uniforms and integrates it into the PBR pipeline. - Revises
parseGLTF/animation logic to return and consume maps of nodes and meshes, improving traversal and update flows. - Updates example app, scenegraph nodes, and traversal utilities to support skinned mesh transforms.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/shadertools/src/modules/engine/skin/skin.ts | Implements skinning GLSL module and uniform generator |
| modules/shadertools/src/lib/shader-module/shader-module.ts | Expands defines type to accept numeric values |
| modules/shadertools/src/index.ts | Exports new skin module |
| modules/gltf/src/parsers/parse-pbr-material.ts | Adds HAS_SKIN define when joint/weight attributes exist |
| modules/gltf/src/parsers/parse-gltf.ts | Refactors to build and return mesh/node maps |
| modules/gltf/src/parsers/parse-gltf-animations.ts | Adapts animation parsing to use node maps and caches |
| modules/gltf/src/gltf/create-scenegraph-from-gltf.ts | Passes new maps and full glTF data to the animator |
| modules/gltf/src/gltf/create-gltf-model.ts | Enables joint/weight attributes and getSkinMatrix usage |
| modules/engine/src/scenegraph/scenegraph-node.ts | Adds asserts, quaternion rotation support, update logic |
| modules/engine/src/scenegraph/group-node.ts | Adds preorderTraversal with world‐matrix context |
| modules/core/src/portable/uniform-buffer-layout.ts | Temporarily increases min uniform‐buffer size |
| examples/tutorials/hello-gltf/app.ts | Adapts example to new scenegraph return shape & skin |
Comments suppressed due to low confidence (3)
modules/shadertools/src/modules/engine/skin/skin.ts:86
- Accessing matrix elements via
Z[j]may not work sinceMatrix4stores values inZ.elements. Consider usingZ.elements[j]to populate the float array.
for (let j = 0; j < 16; j++) { mats[off + j] = Z[j]; }
modules/shadertools/src/modules/engine/skin/skin.ts:61
- [nitpick] The variable name
countibis unclear; renaming to something likeinverseBindMatrixCountwould improve readability.
const countib = inverseBindMatrices.value.length / 16;
modules/shadertools/src/modules/engine/skin/skin.ts:51
- The new uniform-generation logic for skinning is not covered by existing tests. Consider adding unit tests to validate
jointMatrixcontents and length under various input scenarios.
getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => {
| @@ -16,7 +16,7 @@ import {log} from '../utils/log'; | |||
| * Smallest buffer size that can be used for uniform buffers. | |||
| * TODO - does this depend on device? | |||
| */ | |||
| const minBufferSize: number = 1024; | |||
| const minBufferSize: number = 2048; // TODO FIX THIS HACK | |||
There was a problem hiding this comment.
The comment indicates a temporary workaround. Consider documenting the rationale or replacing this hack with a configurable parameter.
Copilot uses AI. Check for mistakes.
| center = [0, 0, 0]; | ||
| cameraPos = [0, 0, 0]; | ||
| time: number = 0; | ||
| mouseCameraTime: number = 0; |
There was a problem hiding this comment.
Nit: Maybe a TSDoc on this line (what is mouse camera time)?
| @@ -43,11 +42,10 @@ const lightSources = { | |||
|
|
|||
| export default class AppAnimationLoopTemplate extends AnimationLoopTemplate { | |||
| device: Device; | |||
| scenes: GroupNode[] = []; | |||
| animator?: GLTFAnimator; | |||
| scenegraphsFromGLTF?: ReturnType<typeof createScenegraphsFromGLTF>; | |||
There was a problem hiding this comment.
Nit: That is an ugly type, maybe we can import a type instead of brute force extracting from the function?
There was a problem hiding this comment.
That's what I did before but you said that type is not useful.
| const renderPass = device.beginRenderPass({clearColor: [0, 0, 0, 1], clearDepth: 1}); | ||
|
|
||
| const far = 2 * this.cameraPos[0]; | ||
| const near = far / 1000; | ||
| const projectionMatrix = new Matrix4().perspective({fovy: Math.PI / 3, aspect, near, far}); | ||
| const cameraTime = this.options['cameraAnimation'] ? time : 0; | ||
| const cameraTime = this.options['cameraAnimation'] ? time : this.mouseCameraTime; |
There was a problem hiding this comment.
Nit: Can we type the options so that we don't need index signatures?
| @@ -119,6 +124,11 @@ export default class AppAnimationLoopTemplate extends AnimationLoopTemplate { | |||
| modelViewProjectionMatrix, | |||
| modelMatrix, | |||
| normalMatrix: new Matrix4(modelMatrix).invert().transpose() | |||
| }, | |||
| skin: { | |||
| // TODO: This is required to trigger getUniforms() of skin. | |||
There was a problem hiding this comment.
Add to glTF tracker task?
| @@ -167,6 +174,8 @@ export default class AppAnimationLoopTemplate extends AnimationLoopTemplate { | |||
| canvas.style.opacity = '1'; | |||
| showError(); | |||
| } catch (error) { | |||
| // eslint-disable-next-line no-console | |||
| console.error(error); | |||
There was a problem hiding this comment.
Nit: Do we need both console and showError?
There was a problem hiding this comment.
Yes! because the showError() doesn't have stack trace.
| @@ -5,6 +5,12 @@ | |||
| import {Vector3, Matrix4, NumericArray} from '@math.gl/core'; | |||
| import {uid} from '../utils/uid'; | |||
|
|
|||
| function assert(condition: boolean, message?: string): asserts condition { | |||
There was a problem hiding this comment.
Doesn't luma.gl/core export an assert function?
| return this; | ||
| } | ||
|
|
||
| update(options: {position?: any; rotation?: any; scale?: any} = {}): this { | ||
| const {position, rotation, scale} = options; | ||
| update({position, rotation, scale}: {position?: any; rotation?: any; scale?: any} = {}): this { |
There was a problem hiding this comment.
Nit: Maybe add TSDoc to each method?
| for (let i = 0; i < value.length; i++) { | ||
| target[path][i] = value[i]; | ||
| } | ||
| function stepInterpolate(target: GroupNode, path: GLTFAnimationPath, value: number[]) { |
There was a problem hiding this comment.
Nit: TSDoc on functions...
| @@ -17,11 +16,20 @@ export function createScenegraphsFromGLTF( | |||
| ): { | |||
| scenes: GroupNode[]; | |||
| animator: GLTFAnimator; | |||
|
|
|||
| gltfMeshIdToNodeMap: Map<string, GroupNode>; | |||
There was a problem hiding this comment.
Here we do seem to have useful ids, see comment above.
There was a problem hiding this comment.
For the gltf side yes but not the luma side.
| const gltfAnimations = gltf.animations || []; | ||
| const accessorCache1D = new Map<GLTFAccessorPostprocessed, number[]>(); |
There was a problem hiding this comment.
Add some comments about why we introduce these caches?
georgioskarnas
force-pushed
the
gk/skinanim_node
branch
from
e33ccfe to
acec46c
Compare
Background
This PR refactors the conversion of glTF nodes to luma.gl nodes and adds support for skinning. It also updates related parsing, animation, and scenegraph logic to integrate the new skin capabilities.
Change List