-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SkeletonHelper: add optional AxesHelpers for each bone #28314
base: dev
Are you sure you want to change the base?
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
The e2e tests may need an update to account for the new visuals. I cannot run e2e tests locally: e2e error:
Can you please update e2e tests if needed? |
5a54cbc
to
ff335e5
Compare
@@ -13,14 +14,15 @@ const _matrixWorldInv = /*@__PURE__*/ new Matrix4(); | |||
|
|||
class SkeletonHelper extends LineSegments { | |||
|
|||
constructor( object ) { | |||
constructor( object, options = { axesHelperSize: 0 } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want to use the options
approach anymore. So please inline the new parameter:
constructor( object, axesHelperSize = 0 ) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want to use the options approach anymore.
Can you please link to the thread where that was decided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
- The advantage of this is that then we don't have this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
@@ -38,6 +40,16 @@ class SkeletonHelper extends LineSegments { | |||
|
|||
} | |||
|
|||
if ( options.axesHelperSize > 0 ) { | |||
|
|||
const helper = new AxesHelper( options.axesHelperSize ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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 } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this feature is optional, please update only a single demo. I suggest to use the new feature in webgl_loader_bvh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger, I can do that. I found it very helpful to see it in other demos too.
Please ignore the E2E test for now, see #28296. |
Thank you so much for reviewing this! Another idea regarding sizing, is to make it automatic:
Usage could be 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. |
I would leave auto-sizing out for now and focus on making the implementation more optimized, see #28314 (comment). |
Sounds good. Marked as draft until I circle back. |
Related issue:
.retarget()
and.retargetClip()
error with documentedSkeletonHelper
params #25751 (axes helpers will help debugging animation/retargeting issues with bone transforms)Description
Adds an options paramters to
SkeletonHelper
with a propertyaxesHelperSize
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 thanaxesHelperSize
?This contribution is funded by Lume, 3D HTML