Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) #2391

georgioskarnas · 2025-05-05T05:49:19Z

For #

Background

Change List

Copilot

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.

Copilot · 2025-05-05T05:50:00Z

modules/shadertools/src/modules/engine/skin/skin.ts

+  },
+
+  getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => {
+    console.log('getUniforms', props, prevUniforms);


Consider removing or guarding the debug logging statement before production to avoid unwanted console output.

Suggested change

console.log('getUniforms', props, prevUniforms);

if (process.env.NODE_ENV === 'development') {

console.log('getUniforms', props, prevUniforms);

}

Copilot · 2025-05-05T05:50:00Z

modules/core/src/portable/uniform-buffer-layout.ts

 * TODO - does this depend on device?
 */
-const minBufferSize: number = 1024;
+const minBufferSize: number = 2048; // TODO FIX THIS HACK

 /**


Clarify or resolve the 'FIX THIS HACK' comment to ensure the minBufferSize value is properly justified for production use.

ibgreen-openai

Big step forward!

I've suggested some possible polish.

And also please give tests some thought, look for functions that could be easy to test, maybe in a follow-up

ibgreen-openai · 2025-05-05T10:57:57Z

modules/gltf/src/gltf/create-scenegraph-from-gltf.ts

+export type ScenegraphsFromGLTF = {
+  scenes: GroupNode[];
+  animator: GLTFAnimator;
+  nodeMap: Map<number | string, GroupNode>;


Can we stringify numbers so we don't need to index the maps with a union type?

ibgreen-openai · 2025-05-05T10:59:32Z

modules/gltf/src/gltf/create-scenegraph-from-gltf.ts

@@ -8,20 +8,22 @@ import {GLTFPostprocessed} from '@loaders.gl/gltf';
 import {parseGLTF, type ParseGLTFOptions} from '../parsers/parse-gltf';
 import {GLTFAnimator} from './gltf-animator';
 import {parseGLTFAnimations} from '../parsers/parse-gltf-animations';
-import {deepCopy} from '../utils/deep-copy';
+
+export type ScenegraphsFromGLTF = {


I am not convinces this type needs to exist, but if it does, we should drop the From.

GLTFScenegraphs or maybe AnimatedScenegraphs?

If the Animation support is truly generic and no longer references GLTF, one could imagine moving it up into engine module and have this gltf module focusing on creating scenegraphs from parsed glTF.

ibgreen-openai · 2025-05-05T11:05:18Z

modules/gltf/src/parsers/parse-gltf-animations.ts

+  if (accessorCache.has(accessor)) {
+    return accessorCache.get(accessor) as number[] | number[][];
+  }


Nit: Use typescript narrowing to avoid the "hard" cast?

Suggested change

if (accessorCache.has(accessor)) {

return accessorCache.get(accessor) as number[] | number[][];

}

const cachedAccessor = accessorCache.get(accessor);

if (cachedAccessor) {

return cacheAccessor;

}

}

ibgreen-openai · 2025-05-05T11:06:02Z

modules/gltf/src/parsers/parse-gltf-animations.ts

    return {name, channels};
  });
 }

-//
-
 function accessorToJsArray(


Can we add a comment to this function. E.g. why does it sometimes return an array of arrays?

ibgreen-openai · 2025-05-05T11:07:23Z

modules/gltf/src/parsers/parse-gltf.ts

+  const meshMap = new Map<number | string, GroupNode>();
+  gltf.meshes.forEach((gltfMesh, idx) => {
+    const newMesh = createMesh(device, gltfMesh, combinedOptions);
+    meshMap.set(idx, newMesh);


Do we really want two entries in the same map? Maintain two maps?

ibgreen-openai · 2025-05-05T11:15:01Z

modules/gltf/src/parsers/parse-gltf.ts

-  topLevelNode._node = gltfNode._node;
-
-  return gltfNode._node;
+  return node;
 }

 function createMesh(


Suggested change

function createMesh(

function createNodeForGLTFMesh(

ibgreen-openai · 2025-05-05T11:16:54Z

modules/gltf/src/parsers/parse-gltf.ts

-        const rotationMatrix = new Matrix4().fromQuaternion(gltfNode.rotation);
-        node.matrix.multiplyRight(rotationMatrix);
-      }
+function createNode(


Maybe the creation functions can be made a little clearer.

createNodeForGLTFNode

ibgreen-openai · 2025-05-05T11:18:04Z

modules/shadertools/src/modules/engine/skin/skin.ts

+    SKIN_MAX_JOINTS
+  },
+
+  getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => {


Some simple test cases for get uniforms would be nice...

ibgreen-openai · 2025-05-05T11:22:30Z

modules/gltf/src/gltf/create-scenegraph-from-gltf.ts

@@ -8,20 +8,22 @@ import {GLTFPostprocessed} from '@loaders.gl/gltf';
 import {parseGLTF, type ParseGLTFOptions} from '../parsers/parse-gltf';
 import {GLTFAnimator} from './gltf-animator';
 import {parseGLTFAnimations} from '../parsers/parse-gltf-animations';
-import {deepCopy} from '../utils/deep-copy';


ibgreen-openai · 2025-05-05T11:23:46Z

modules/gltf/src/gltf/animations/animations.ts

  sampler: GLTFAnimationSampler;
-  target: GLTFNodePostprocessed;
+  target: GroupNode;


Q: could the target be by node key instead of Node?

georgioskarnas added 3 commits May 4, 2025 16:27

skin+refactor _node

6a4f5a3

progress

2c328bc

more progress

9ca5f25

georgioskarnas requested review from ibgreen and Copilot May 5, 2025 05:49

Copilot AI reviewed May 5, 2025

View reviewed changes

ibgreen-openai approved these changes May 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) #2391

Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) #2391

-  if (accessorCache.has(accessor)) {
-    return accessorCache.get(accessor) as number[] | number[][];
-  }
+    const cachedAccessor = accessorCache.get(accessor);
+    if (cachedAccessor) {
+      return cacheAccessor;
+    }
+  }

Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) #2391

Are you sure you want to change the base?

Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) #2391

Conversation

Background

Change List

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment