SkeletonHelper: add optional AxesHelpers for each bone #28314

trusktr · 2024-05-08T08:48:13Z

Related issue:

SkeletonUtils .retarget() and .retargetClip() error with documented SkeletonHelper params #25751 (axes helpers will help debugging animation/retargeting issues with bone transforms)

Description

Adds an options paramters to SkeletonHelper with a property axesHelperSize that when greater than zero will show AxesHelpers of the given size for each bone.

Screenshots

Do you prefer a single axesHelperSize number parameter instead of an options object parameter? And do you prefer a different name than axesHelperSize?

This contribution is funded by Lume, 3D HTML

github-actions · 2024-05-08T08:50:18Z

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize `dev`	Filesize PR	Diff
678.5 kB (168.1 kB)	678.9 kB (168.2 kB)	+388 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize `dev`	Filesize PR	Diff
457.6 kB (110.4 kB)	457.6 kB (110.4 kB)	+0 B

trusktr · 2024-05-08T08:50:54Z

The e2e tests may need an update to account for the new visuals. I cannot run e2e tests locally:

e2e error:

$ npm run test-e2e

> three@0.164.1 test-e2e
> node test/e2e/puppeteer.js

file:///Users/trusktr/src/mrdoob+three.js/node_modules/@puppeteer/browsers/lib/esm/launch.js:301
                reject(new Error([
                       ^

Error: Failed to launch the browser process!


TROUBLESHOOTING: https://pptr.dev/troubleshooting

    at Interface.onClose (file:///Users/trusktr/src/mrdoob+three.js/node_modules/@puppeteer/browsers/lib/esm/launch.js:301:24)
    at Interface.emit (node:events:526:35)
    at Interface.close (node:internal/readline/interface:527:10)
    at Socket.onend (node:internal/readline/interface:253:10)
    at Socket.emit (node:events:526:35)
    at endReadableNT (node:internal/streams/readable:1376:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.6.1

Can you please update e2e tests if needed?

Mugen87 · 2024-05-08T12:55:31Z

src/helpers/SkeletonHelper.js

@@ -13,14 +14,15 @@ const _matrixWorldInv = /*@__PURE__*/ new Matrix4();

 class SkeletonHelper extends LineSegments {

-	constructor( object ) {
+	constructor( object, options = { axesHelperSize: 0 } ) {


We do not want to use the options approach anymore. So please inline the new parameter:

constructor( object, axesHelperSize = 0 ) {

We do not want to use the options approach anymore.

Can you please link to the thread where that was decided?

I do not know the issue where this topic was mentioned. If you don't like an additional parameter, setAxesHelperSize() could be added as well.

To avoid hijacking this thread, if removing options is something that is going to happen, I think a separate thread with a plan of action makes sense.

Roger, happy to make the change. (I'm not an owner, your preference is my command)

The reasoning I use is this:

for permanent required things, separate parameters is fine

for optional things ("options") use a trailing last parameter with all the rest of the "options"

The advantage of this is that then we don't have this:
foo(requiredValue, undefined, undefined, someValue, undefined, otherValue)

Then let's use a setter for now. When adding color configuration to axes helper, a setter was @mrdoob's preferred option, see #23829 (comment).

@WestLangley I should have expressed myself more clearly in #28314 (comment), sorry. When thinking about it, we did never explicitly state to not use options anymore. I should have said using a single parameter is more in-line with other helpers and a setter more compatible compared to latest changes like in CameraHelper.

Mugen87 · 2024-05-08T12:57:12Z

src/helpers/SkeletonHelper.js

@@ -38,6 +40,16 @@ class SkeletonHelper extends LineSegments {

 			}

+			if ( options.axesHelperSize > 0 ) {
+
+				const helper = new AxesHelper( options.axesHelperSize );


Visualizing the axes per bone is indeed helpful but using AxesHelper is unfortunately an inefficient approach since this will produce potentially way too many draw calls. I think the feature must be implement without additional 3D objects by enhancing the line segments with additional data.

I agree, but this is for debug, not for production, so maybe that's not critical here. This is already super useful in debugging animations (retargeting stuff).

Its a bit more work to reconstruct all the lines manually, but doable.

Does BatchedMesh help here perhaps?

The problem is devs might use SkeletonHelper for all skinned meshes in their scenes. If they have 10 complex skinned meshes with 100 bones, you end up with 1010 draw calls instead of 10. Even helpers should not be that inefficient.

Does BatchedMesh help here perhaps?

Yes. If it makes it easier to manage the bone axes, give BatchedMesh a try. Otherwise the helper can manage the geometry data on its own like CameraHelper.

Mugen87 · 2024-05-08T12:57:55Z

examples/webgl_loader_bvh.html

@@ -47,7 +47,7 @@
 			const loader = new BVHLoader();
 			loader.load( 'models/bvh/pirouette.bvh', function ( result ) {

-				const skeletonHelper = new THREE.SkeletonHelper( result.skeleton.bones[ 0 ] );
+				const skeletonHelper = new THREE.SkeletonHelper( result.skeleton.bones[ 0 ], { axesHelperSize: 5 } );


Since this feature is optional, please update only a single demo. I suggest to use the new feature in webgl_loader_bvh.

Roger, I can do that. I found it very helpful to see it in other demos too.

Mugen87 · 2024-05-08T13:01:09Z

Can you please update e2e tests if needed?

Please ignore the E2E test for now, see #28296.

trusktr · 2024-05-08T21:02:33Z

Thank you so much for reviewing this!

Another idea regarding sizing, is to make it automatic:

get bounding box of the skeleton
take average size of the width, height, and depth of the box (or similar)
make the axes size be a specific proportion of this average size (f.e. 0.1)

Usage could be new SkeletonHelper(object, true), or if specific size is still desired then new SkeletonHelper(object, 5).

An auto-sizing would help with the Editor (because the size of models in unknown up front).

I made the feature optional because size is currently not auto-detected. I think it would be nicer to have the feature enabled by default, with auto-sizing.

Mugen87 · 2024-05-09T08:42:32Z

I would leave auto-sizing out for now and focus on making the implementation more optimized, see #28314 (comment).

trusktr · 2024-05-11T22:28:23Z

Sounds good. Marked as draft until I circle back.

trusktr marked this pull request as ready for review May 8, 2024 08:53

SkeletonHelper: add optional AxesHelpers for each bone

ff335e5

trusktr force-pushed the joe/SkeletonHelper-AxesHelpers branch from 5a54cbc to ff335e5 Compare May 8, 2024 09:02

Mugen87 reviewed May 8, 2024

View reviewed changes

trusktr marked this pull request as draft May 11, 2024 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SkeletonHelper: add optional AxesHelpers for each bone #28314

SkeletonHelper: add optional AxesHelpers for each bone #28314

trusktr commented May 8, 2024

github-actions bot commented May 8, 2024 •

edited

trusktr commented May 8, 2024 •

edited

Mugen87 May 8, 2024

WestLangley May 8, 2024

Mugen87 May 8, 2024 •

edited

WestLangley May 8, 2024

trusktr May 8, 2024

Mugen87 May 8, 2024

Mugen87 May 9, 2024 •

edited

Mugen87 May 8, 2024 •

edited

trusktr May 8, 2024 •

edited

Mugen87 May 8, 2024 •

edited

Mugen87 May 8, 2024 •

edited

trusktr May 8, 2024

Mugen87 commented May 8, 2024

trusktr commented May 8, 2024 •

edited

Mugen87 commented May 9, 2024

trusktr commented May 11, 2024

SkeletonHelper: add optional AxesHelpers for each bone #28314

Are you sure you want to change the base?

SkeletonHelper: add optional AxesHelpers for each bone #28314

Conversation

trusktr commented May 8, 2024

github-actions bot commented May 8, 2024 • edited

📦 Bundle size

🌳 Bundle size after tree-shaking

trusktr commented May 8, 2024 • edited

Mugen87 May 8, 2024

Choose a reason for hiding this comment

WestLangley May 8, 2024

Choose a reason for hiding this comment

Mugen87 May 8, 2024 • edited

Choose a reason for hiding this comment

WestLangley May 8, 2024

Choose a reason for hiding this comment

trusktr May 8, 2024

Choose a reason for hiding this comment

Mugen87 May 8, 2024

Choose a reason for hiding this comment

Mugen87 May 9, 2024 • edited

Choose a reason for hiding this comment

Mugen87 May 8, 2024 • edited

Choose a reason for hiding this comment

trusktr May 8, 2024 • edited

Choose a reason for hiding this comment

Mugen87 May 8, 2024 • edited

Choose a reason for hiding this comment

Mugen87 May 8, 2024 • edited

Choose a reason for hiding this comment

trusktr May 8, 2024

Choose a reason for hiding this comment

Mugen87 commented May 8, 2024

trusktr commented May 8, 2024 • edited

Mugen87 commented May 9, 2024

trusktr commented May 11, 2024

github-actions bot commented May 8, 2024 •

edited

trusktr commented May 8, 2024 •

edited

Mugen87 May 8, 2024 •

edited

Mugen87 May 9, 2024 •

edited

Mugen87 May 8, 2024 •

edited

trusktr May 8, 2024 •

edited

Mugen87 May 8, 2024 •

edited

Mugen87 May 8, 2024 •

edited

trusktr commented May 8, 2024 •

edited