Skip to content
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

New animation / skinning / ik example #24652

Merged
merged 26 commits into from Sep 23, 2022
Merged

New animation / skinning / ik example #24652

merged 26 commits into from Sep 23, 2022

Conversation

abernier
Copy link
Contributor

@abernier abernier commented Sep 16, 2022

Note Merged
see animation / skinning / ik example

Following up my previous PR about CCDIKSolver documentation, I'm now submitting a new animation / skinning / ik example I've made with CCDIKSolver applied on a GLTF skinned mesh:

image

This PR is deployed here with github-pages so you can preview it: https://abernier.github.io/three.js/examples/#webgl_animation_skinning_ik

I've added some useful lil-gui options, so users can play with the IK solver and bones.

Hope this could be useful and fun :)

TODO

  • formatting
  • uncommented lines should be removed
  • remove gsap and use tween.js
  • convert the new glTF asset into a single glb
  • downgrade to lil-gui 0.16
  • add to screenshots' exception list

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2022

The example looks awesome! However, there are some open poinst that have to be fixed: webgl_animation_skinning_ik.html needs some updates to make it more consistent to the other examples. The formatting is off, all uncommented lines should be removed, it would be good to remove gsap and use tween.js from the repository instead. Also convert the new glTF asset into a single glb.

@abernier
Copy link
Contributor Author

abernier commented Sep 17, 2022

@Mugen87 thank you

Sure, I've listed your points in a TODO tasks list in the PR description 👍🏻.

Also, because I needed the latest lil-gui version (v0.17.0), I have imported it from skypack:

// import { GUI } from 'three/addons/libs/lil-gui.module.min.js';
import GUI from "https://cdn.skypack.dev/lil-gui@0.17.0";

-> do you want I'd rather upgrade the jsm/libs/lil-gui.module.min.js directly in this PR, or maybe in a separate one?

@abernier
Copy link
Contributor Author

And just for my info, why not including gsap into jsm/libs: license issue? I find the Timeline feature often very handy...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2022

license issue?

Yes. And because one animation library should be sufficient for the repo (like having lil-gui is sufficient for GUI stuff).

What features are missing in the current lil-gui version that you need for the example?

@abernier
Copy link
Contributor Author

abernier commented Sep 17, 2022

What features are missing in the current lil-gui version that you need for the example?

controller.show() and controller.hide()

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2022

Any chances to rewrite the code or simplify the example so you don't have to use a higher version of lil-gui? I really have no good feeling if we start using external libs from different releases.

@abernier
Copy link
Contributor Author

abernier commented Sep 17, 2022

but can't we just upgrade lil-gui repo lib to its latest 0.17.0 version?

--

otherwise if not possible, yes, what I can do is to put the controllers I wanted to show/hide into a folder (because gui.show()/gui.hide() already existed on folders in 0.16)

I would prefer not to but I will, if you confirm it is too "risky"

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2022

I'm not in favor to upgrade a lib that is used extensively in the repo just because one example needs a new feature.

However, if you want to make a separate PR with upgrading lil-gui and verify that the existing examples work, that's fine with me.

@abernier
Copy link
Contributor Author

I understand, I will downgrade to 0.16 then -> added to my tasks ;)

@abernier
Copy link
Contributor Author

abernier commented Sep 17, 2022

@Mugen87 About formatting, is there a tool to run against my code? Thank you

Ok, found the eslint tooling

@abernier
Copy link
Contributor Author

✅ ok, all my tasks are now complete if you want to review @Mugen87 ;)

@abernier abernier marked this pull request as draft September 18, 2022 10:22
@abernier abernier marked this pull request as ready for review September 18, 2022 14:20
@abernier
Copy link
Contributor Author

abernier commented Sep 18, 2022

About the screenshot, I've initially and naively generated it manually...
webgl_animation_skinning_ik

Since I've checked on this failed CI task and now I understand it is supposed to be auto-generated to diff them...

So I've tried generated a new one locally npm run make-screenshot examples/webgl_animation_skinning_ik.html and the screenshot generated is always my preloader "loading..."

webgl_animation_skinning_ik

Is there a way to delay the moment the screenshot is taken? (in my case 2.5s may be enough)

@LeviPesin
Copy link
Contributor

Is there a way to delay the moment the screenshot is taken? (in my case 2.5s may be enough)

Currently no (will be fixed in #24109). Please just add the example to the test's exception list for now.

@abernier
Copy link
Contributor Author

Is there a way to delay the moment the screenshot is taken? (in my case 2.5s may be enough)

Currently no (will be fixed in #24109). Please just add the example to the test's exception list for now.

✅ done

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 20, 2022

I've downloaded the branch today and studied the example code in detail. Sorry for saying this but the code is still way too complicated.

Yes, examples should be visually appealing but more importantly easy to understand and read. webgl_animation_skinning_ik is fully packed with stuff which is not required to demonstrate the unique feature the example is actually about: inverse kinematics.

I suggest to post the code in its current (or original) state in the showcase category at the forum. An example in the repository has to be more simple. An example about inverse kinematics does not need any camera animations, multiple controls selection as well as post processing.

Besides, the example does still not use the formatting and coding style of the other official examples.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 21, 2022

Looks much better now! Added a small clean up commit.

I see the following warnings in the browser console though:

THREE.CCDIKSolver: bone hand_l is not the child of bone Upperarm_l
THREE.CCDIKSolver: bone Upperarm_l is not the child of bone lowerarm_l
THREE.CCDIKSolver: bone lowerarm_l is not the child of bone hand_l

How can we get rid of these?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 21, 2022

Do you mind regenerating the screenshot? I've edited webgl_animation_skinning_ik.html over the GitHub UI.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 21, 2022

It's quite easy to "break" the example by translating the sphere along the x-axis. The result is an endless ping-pong effect:

2022-09-21 11 58 16

How do you think about setting transformControls.showX = false; and allowing translation only in a YZ plane.

Is that something that needs to be fixed in CCDIKSolver at some point (not with this PR)?

@abernier
Copy link
Contributor Author

abernier commented Sep 21, 2022

It's quite easy to "break" the example by translating the sphere along the x-axis. The result is an endless ping-pong effect

Indeed, I think this is due to rotationMin/rotationMax constraints on links... but those are needed in order her elbow stays below her hand.

I think this could ideally be solved by adding "pole targets" feature to CCDIKSolver, so we can tell some bones to always point somewhere in particular...

How do you think about setting transformControls.showX = false; and allowing translation only in a YZ plane.

Sure, or we could get rid of rotation* constraints, but should render strange... I don't know which is best, it's a trade-off, as you prefer

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 21, 2022

Let's use transformControls.showX = false; for now. That should mitigate the most obvious glitches.

I think this could ideally be solved by adding "pole targets" feature to CCDIKSolver, so we can tell some bones to always point somewhere in particular...

Sounds good!

@abernier
Copy link
Contributor Author

abernier commented Sep 21, 2022

Let's use transformControls.showX = false; for now. That should mitigate the most obvious glitches.

I will add it tomorrow + some tweaks on the glb and will mark the PR as ready once it is done ;)

@abernier abernier marked this pull request as ready for review September 22, 2022 10:04
@abernier
Copy link
Contributor Author

abernier commented Sep 22, 2022

ok

  • showX=false
  • fixed broken link
  • updated screenshot

https://abernier.name/three.js/examples/#webgl_animation_skinning_ik

lgtm, but don't hesitate if anything else I can do

@Mugen87 Mugen87 added this to the r145 milestone Sep 22, 2022
@Mugen87 Mugen87 merged commit a53977c into mrdoob:dev Sep 23, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 23, 2022

I've realized right now that the model is from the Unity store and not available under a CC license. Since it is now part of a bigger asset, it is necessary to define a license for kira.glb (otherwise I'm afraid we can't add it).

@abernier
Copy link
Contributor Author

abernier commented Sep 23, 2022

it is necessary to define a license for kira.glb

How should I proceed for this?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 23, 2022

Since you have exported the model from Blender, you are the author of the final asset. So you can define the license, in most cases one of CC, see https://creativecommons.org/licenses/. We usually add the license in the description like in https://threejs.org/examples/webgl_animation_skinning_morph (CC0 in this case).

The important question is: Since you have purchased a model from the Unity store, you have to ensure if it's okay to reuse it for a different asset which is then licensed under CC.

@abernier
Copy link
Contributor Author

done in #24688

@joshuaellis joshuaellis mentioned this pull request Sep 27, 2022
17 tasks
@mrdoob
Copy link
Owner

mrdoob commented Sep 29, 2022

@abernier Amazing example! 🔥

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.

None yet

4 participants