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

Removed deprecated animation entities #11252

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Removed deprecated animation entities #11252

merged 1 commit into from
Apr 27, 2017

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 27, 2017

In the last month @jostschmithals and i removed dependencies to deprecated animation entities and re-created respective examples (#10939, #11111 and #11235). I think it's time for clean up now: This PR nukes the following entities and all remaining references (e.g docs, comments etc.):

  • examples/js/loaders/collada/Animation.js
  • examples/js/loaders/collada/AnimationHandler.js
  • examples/js/loaders/collada/KeyFrameAnimation.js

@mrdoob
Copy link
Owner

mrdoob commented Apr 27, 2017

Sweet! Great job guys!

@mrdoob mrdoob merged commit 7c5ab91 into mrdoob:dev Apr 27, 2017
@mrdoob
Copy link
Owner

mrdoob commented Apr 27, 2017

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 27, 2017

Yay! 🎉 🙌

@jostschmithals
Copy link
Contributor

Maybe this removal was a little early?

Indeed it would be really nice to get rid of all this stuff!

But the reason for designing my JSON keyframe version as an addition to the examples and not as a replacement for the Collada keyframe example was that I wanted to avoid the situation which we have now, after this removement (if I see it right):

Now, if a user loads a Collada model with ColladaLoader, the skeletal animations like in https://threejs.org/examples/?q=collada#webgl_loader_collada_skinning will work as usual, but playing the kind of keyframe animations which affect the objects directly (like in the pump example) will no longer be possible, since they are depending on the removed AnimationHandler.js and KeyFrameAnimation.js.

For example: The core animation system expects this nice format for the animation of the first object of the pump example:

json_animations

But unfortunately ColladaLoader outputs a structure which is quite different (This screenshot shows only the beginning of the analogue animation for the first object, otherwise you would have to scroll ca. 3 meters 😉 ):

collada1
collada2

Thus I guess this couldn’t work?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 28, 2017

@jostschmithals The the mentioned example (pump) is also part of the removal. Your re-creation from #11235 is now the new leading example for keyframe animations (json).

@jostschmithals
Copy link
Contributor

jostschmithals commented Apr 28, 2017

That's absolutely clear. But I used the former example only as an example :-)
(because I have no other Collada model at hand).

The problem I wanted to point out is independent from this special example:

If a user loads an arbitrary own Collada model with keyframes (moving/rotating the objects, not bones), he has to use ColladaLoader, and ColladaLoader produces an animation structure which can't be handled by the core animation system.

So I guess: as long as ColladaLoader doesn't output a structure which can be parsed by the core animation system, the old helper classes are needed for Collada models (and the old example how to handle them) - unfortunately.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 28, 2017

Ah, okay. So currently it's not possible to use a collada model with keyframe animations (except skeleton animation) anymore. The question is if we still want to support this feature. I actually tend to deprecate it and note this in the upcoming migration notes.

@jostschmithals
Copy link
Contributor

In my estimation the xml based Collada format in general seems to be no longer very important (I’m not a 3D modeler, but I spoke with professional 3D artists which didn’t even know this format)?

So perhaps it would not be as bad, if this kind of animations could no longer be supported by ColladaLoader, and the advantage of getting rid of all this completely outdated animation stuff would outweigh the absence of this feature.

But I think to avoid confusion the related parts of the ColladaLoader code should be replaced by a no-longer-supported-console-warning then?

BTW: There is a ColladaLoader2.js, which seems to be referenced only in the editor(?). What about this?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 28, 2017

There is a ColladaLoader2.js, which seems to be referenced only in the editor(?). What about this?

ColladaLoader2 is a much nicer implementation, but i has no animation support so far (see #10592).

But I think to avoid confusion the related parts of the ColladaLoader code should be replaced by a no-longer-supported-console-warning then?

That might be a good idea! Do you think you can have a look at this?

@jostschmithals
Copy link
Contributor

jostschmithals commented Apr 28, 2017

Do you think you can have a look at this?

Sorry - I don't have the possibility to do anything for three.js from now on for the next four or five days.
So it would perhaps be better, if you would accomplish this (if @mrdoob decides to go this way)?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 28, 2017

No problem. But i also think there's no hurry. We are at the beginning of a new dev cycle...

@mrdoob
Copy link
Owner

mrdoob commented Apr 28, 2017

Yeah... Aiming for the 15th.

Ideally we would add animation support to ColladaLoader2. I should have some proper focus-time in two weeks.

@Mugen87 Mugen87 deleted the animation branch April 28, 2017 19:05
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.

3 participants