Skip to content

Conversation

@georgioskarnas
Copy link
Collaborator

@georgioskarnas georgioskarnas commented May 5, 2025

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

  • Introduces a new skin shader module with skin uniform handling.
  • Refactors glTF parsers to return additional maps and adapts animation and scenegraph functions.
  • Updates scenegraph transformations and traversal methods along with various minor improvements.

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

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ibgreen-openai ibgreen-openai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SKIN_MAX_JOINTS
},

getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

path: GLTFAnimationPath;
sampler: GLTFAnimationSampler;
target: GLTFNodePostprocessed;
target: GroupNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using GroupNode makes this generic and detaches it from glTF. What is node key?

Copy link
Collaborator

@ibgreen ibgreen May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll keep GroupNode for this PR.

@georgioskarnas georgioskarnas changed the title Refactor conversion of glTF nodes to luma.gl nodes, add skin capability (WIP) Refactor conversion of glTF nodes to luma.gl nodes, add skin capability May 27, 2025
@georgioskarnas georgioskarnas marked this pull request as ready for review May 27, 2025 03:05
@georgioskarnas
Copy link
Collaborator Author

Remaining issues:

  • assert()
  • uniform-buffer-layout.ts minBufferSize
  • skin.ts should not need any params

@georgioskarnas georgioskarnas requested a review from Copilot May 27, 2025 06:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 skin shader 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 since Matrix4 stores values in Z.elements. Consider using Z.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 countib is unclear; renaming to something like inverseBindMatrixCount would 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 jointMatrix contents and length under various input scenarios.
getUniforms: (props: SkinProps = {}, prevUniforms?: SkinUniforms): SkinUniforms => {

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Add any remaining tasks / ideas to the glTF tracker task.

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 5, 2025

@kargeor

  • I merged latest master and fixed the test failure.
  • It looks like the example is broken because precomputed bounds in the nodes are no longer correct?

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 10, 2025

This PR for skin animations in luma.gl looks almost ready to land.

  • It includes an important refactor that separates animation data from the glTF structure (previous code was mutating the glTF).
  • The (hopefully) final problem seems to be that this PR breaks the bounding box calculations in the hello-gltf example.
    @felixpalmer FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants