mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.73k stars 35.38k forks source link

Skeletal Animation Problem With Multiple Spawns of a Mesh #5516

Closed titansoftime closed 10 years ago

titansoftime commented 10 years ago

Per suggestion in a lengthy Stackoverflow conversation, I am posting this potential issue here.

Basically I am having to rewrite the line "if ( data.initialized === true ) return;" to "if ( data.initialized === true ) return data;" in THREE.AnimationHandler.init. Without doing that, only one of each type of skinned mesh is animated. If I don't check for existence of animation.data before playing I get the error ""Uncaught TypeError: Cannot read property 'name' of undefined " on line 29665". Something could very well be wrong with my code, but maybe there could be a bug here.

This SO question has all the details: http://stackoverflow.com/questions/26570689

jonnenauha commented 10 years ago

I also encountered this while porting skeleton/animation related code to r68. Was not sure if this was intentional or not. I ended up cloning the data for each animation per unique animated character. Not entirely sure if you can share the animation hierarchy data with multiple animations without them sharing some JIT state there? Does it work if you change to return the initialized data?

Edit: Now that I think of it, if its just the JIT cache that they share, each animation should be able to interpolate on that data individually. hmm...

titansoftime commented 10 years ago

If I return data on initialized animations in the AnimationHandler init function everything works just fine. My only concern is if by doing this I am bypassing some kind of caching. I can have about 50 animated models on screen (about 5k polys each). If i go up over about 50 things start to slow down but that may be a natural bottleneck.

jonnenauha commented 10 years ago

I think this actually is a bug. I don't see any abvious reason why one could not reuse/share the generated keyframe cache with multiple skeletons as long as you new TREE.Animation for each individual character with unique bones to that character.

It seems to be that the THREE.Animation.data is just keyframes that get "checked" once: array rotation converted to Quats, cleaninig up the data etc. Then THREE.Animation.update() will iterate and update bones and doing some caching at the same time. So actually the cache seems to reside in the bone not in animation?

Oh well, lets see what the authors here think of it... :)

mrdoob commented 10 years ago

Could you guys set up a test showing the issue for us?

titansoftime commented 10 years ago

Sure, I'll set one up when I get home from work.

titansoftime commented 10 years ago

http://www.titansoftime.com/animtest.php?mod=0&num=1 This link will load an unmodified r69 build of three.js with one model. Change the value of num to load additional models.

"mod=1" will load the modified build.

The ONLY modification made in this modified build is changing "if ( data.initialized === true ) return;" to "if ( data.initialized === true ) return data;" on line 29407

Just to reiterate: loading one model in the unmodified source works fine. The problem is loading 2 or more SkinnedMeshes with the same geometry (AnimationHandler.init returns nothing if that particular animation has already been initialized).

kdrnic commented 10 years ago

@mrdoob he is simply proposing that AnimationHandler.init should return the data even if already initialized for convenience.

The convenience is that this way one can keep track of an animation's name through Animation.data.name. Otherwise Animation.data would be undefined and name not accessible from the Animation object.

This is not, IMHO, an actual bug with having multiple animated skin meshes, as one can keep track of Animation objects through other ways, for example, one can create an object that maps string keys to Animation objects, equivalent to "std::map<std::string, THREE.Animation>" in C++. One could even create something equivalent to "std::map<std::pair<std::string, THREE.SkinnedMesh>, THREE.Animation>" with obvious convenience.

titansoftime commented 10 years ago

I disagree. This is more than referencing an animation via name. The data contains animation hierarchy which is necessary.

If it was just about the data.name then "if ( data.initialized === true ) return {name:data.name};" would work, but it does not.

Try it yourself and see.

kdrnic commented 10 years ago

In my own code, I tested multiple skinned meshes with different animations playing and they work fine.

titansoftime commented 10 years ago

Do your meshes share geometry? Do you have a link to this case?

In this thread I have a link to a test case that shows it clearly not working.

kdrnic commented 10 years ago

That example of yours uses your very specific mesh treatment code.

titansoftime commented 10 years ago

No it doesn't. Your thinking of the first case in SO. This one is handled based on the original three.js skinning demo.

If you have a case with meshes with shared geometry that are animated successfully simultaneously, I'd sure love to see it.

kdrnic commented 10 years ago

Actually, messing around with my code here, I saw the exact same issue as @titansoftime. Sorry for being skeptic about your problem. I will soon post a link to a demo showcasing the problem, it showcases it quite well and with a nice interface.

kdrnic commented 10 years ago

Here is it: https://12823f0b8aa8cbc71a4940ccf4e7c6a2e58690bd.googledrive.com/host/0B9scOMN0JFaXeVExeTdFTUF5VEE/test.html You can test creating a SkinnedMesh with or without setting data.initialized = false; on all datas before creating THREE.Animation objects. You can create multiple SkinnedMeshes and selectors will appear enabling you to select animations for which one of them. So @mrdoob you should immediately commit "if(data.initialized === true) return data;" to AnimationHandler.js.

titansoftime commented 10 years ago

Pull Request #5542 has been created.

kdrnic commented 10 years ago

You could rewrite the wording of the pull request, it isn't specific to shared geometries. While the JSON loader keeps the animation datas in geometry, it is separated from geometry, and in my code I load it separately altogether (I wrote a custom loader) since it is possible for you to want to load an animation separately from the rest of the geometry (e.g. for different geometries for different characters that have the same skeleton and animations).

titansoftime commented 10 years ago

Right on.

mrdoob commented 10 years ago

Thanks guys! Merged :)

jonnenauha commented 10 years ago

@lucasdealmeidasm From my point of view the issue is the following. Maybe this will clarify it for you. I do agree that the issue title/name is not totally accurate :)

Before this pull request it was not possible to share the keyframe data. Which was silly. You were required to clone the whole keyframe hierarchy (potentially a lot of data) for each Animation. Lets say the "Walk" animation.

After this pull request all my N "Walk" Animations can reference to the same hierarchy (keyframe data). This keyframe data is not modified by the Animation as far as I see it, as they have their own internal cache. So it makes sense multiple Animations can reference the same data.

This PR should resolve the issue. Thanks @titansoftime and @mrdoob.

titansoftime commented 10 years ago

Glad to be of assistance =]

I've been mooching off the hard work of the three.js contributors since r47. It was about time to give back a little.

ericaokamura commented 9 years ago

I am having the same issue with three.js (r71), even with the bug fixed. It says "TypeError: undefined is not an object (evaluating 'data.initialized')". Can someone help me please?