pycollada / pycollada

A python COLLADA library. Can be used to create, edit and load COLLADA documents.
http://pycollada.readthedocs.org/
Other
227 stars 71 forks source link

support for collada 1.5 specification #25

Open rdiankov opened 11 years ago

rdiankov commented 11 years ago

i would really like support for adding kinematics and physics elements to pycollada. For starters, just getting the following elements working would be awesome:

library_physics_scenes
physics_scene
physics_model
instance_rigid_body
articulated_system
instance_articulated_system
articulated_system/motion
articulated_system/kinematics
kinematics_scene
library_articulated_systems
library_kinematics_models
library_kinematics_scenes

I would be happy to do the coding for this if i can get some pointers as to what to change and where.

skrat commented 11 years ago

afaik, the library closely mirrors COLLADA data types, so adding these should be quite straightforward, have a look how geometry is wired (for instance) and follow that with kinematics types.

rdiankov commented 11 years ago

ok... that knocks down articulated_system, physics_model, and rigid_body and the associated libraries.

what about kinematics_scene, physics_scene and those libraries? how should i connect them with the current scene?

skrat commented 11 years ago

well, it's common to start with some use cases, how You would like to work with that? what API would suite the clients of this library and so on. I never did anything with that kind of data, therefore I can't really judge it.

rdiankov commented 11 years ago

the goal right now is to be consistent with the rest of the pycollada programming style, which is just a way of converting collada xml to python objects.

jterrace commented 11 years ago

Like skrat, I haven't actually used the physics data either, so you're probably a better judge for how the objects should work than we are. That being said, if you look at the Collada class, you will see a method for loading each type of object. I would say create a new file for loading physics-related things and add them as an instance attribute on the Collada object, like e.g. Collada.effects, etc. For the scene information, you might want to add them to scene.py, or it might make more sense to have a physics_scene.py instead. I don't have time now to look at the 1.5 spec for how physics scenes work, but if they can be part of a normal scene, then we probably want it in scene.py, if not then its own file.

rdiankov commented 11 years ago

i started a branch called collada15spec for creating the 1.5 spec classes.

https://github.com/rdiankov/pycollada/tree/collada15spec

please keep in mind i'm only going to implement what is necessary for my application to work, and am not necessarily aiming for completeness. the articulated_system.py file is already working.

rdiankov commented 11 years ago

And yes Jeff, i'll make sure it follows closely to how you've structured things.

jterrace commented 11 years ago

@rdiankov I gave you access to the repo and pushed a couple changes to the collada15spec branch:

jterrace commented 11 years ago

I also updated readthedocs to build this new branch on commit: http://pycollada.readthedocs.org/en/collada15spec/

We have to update the docs to generate files for the new classes. That's all I have time for today. Thanks, this is a great start

rdiankov commented 11 years ago

looks great! i just committed a function to reconfigure the number dtype being used when creating numpy.array, it's called set_number_dtype(). All modules now do something like:

numpy.array(values, dtype=get_number_dtype())

we still have to implemented the object definition

jterrace commented 11 years ago

I updated your float32/float64 code a bit. Added some tests for it and verything should work properly now.

There are still a few more things outstanding before this can be merged:

  1. documentation for set_collada_version and set_precision need to be added
  2. have to run sphinx autogen for the new classes and make sure all the docstrings look right
  3. I'm not sure I like the names ikscene and ipscene. Can you explain the names? I'd prefer something more explicit - kinematics_scene and physics_scene maybe?
  4. Is extras added to all classes now? We should write some tests for it
rdiankov commented 11 years ago

looks great!

  1. ikscene is InstanceKinematicsScene, which internally holds a kscene variable for KinematicsScene object. Same with ipscene. Unfortunately in order to support external references, we'll need the instance_* versions of the objects. I think we'll need to change your NodeNode class to support this, which i'll be testing in the next couple of days.
  2. yes extras should be added to everything. i'm not sure how to test things though..
jterrace commented 11 years ago

For tests, you can take a look at some of the other test files. Basically, for each class, you want to instantiate it, check to make sure default values work, check to make sure you can modify values, save it, reload it, and assert that the values are correct.

I think ikscene and ipscene should not be properties of Collada. Each <scene> can have 0 or 1 <instance_kinematics_scene> and 0 or more <instance_physics_scene>, so they should be properties of the Scene object. If mesh is an instance of Collada, I would say let's put it at mesh.scene.instance_kinematics_scene.kinematics_scene. You could also get to it from mesh.scenes[0].instance_kinematics_scene.kinematics_scene. And since there can be more than one physics, how about mesh.scene.instance_physics_scenes[0].physics_scene? I see you use short names now, ikscene.kscene, but I'd rather make them more verbose so it's a little clearer.

Ideally, pycollada should have had InstanceVisualScene and VisualScene objects underneath the Scene object, but right now it treats Scene as just a single pointer to a visual_scene. NodeNode really should have been called InstanceNode, so I'm fine with InstanceKinematicsScene and KinematicsScene.

rdiankov commented 11 years ago

feel free to change the names to longer versions, it really doesn't matter to me.

And yes things were confusing in regards to where instance_kinematics_scene should be put. You were mixing Scene and InstanceVisualScene together, so I though Collada.scene variable was for visual_scene, and that the Collada class itself points to ..

so which one do we go with? Scene== or Collada==? If you go with the former, you'll have to add a InstanceVisualScene and VisualScene classes.

jterrace commented 11 years ago

Yeah, I think we should go with the former. We can change Scene to have properties instance_visual_scene, instance_kinematics_scene, and instance_physics_scenes and add the VisualScene and InstanceVisualScene classes, but we can keep the Scene.scene property as a shortcut pointing to Scene.instance_visual_scene.visual_scene to maintain backwards compatibility. We could also add Scene.visual_scene and Scene.kinematics_scene and Scene.kscene as shortcuts for convenience.

rdiankov commented 11 years ago

interesting, i thought the latter would be better. ;0)

from the collada spec, each collada document can have one and only one element. There would be a nice one-to-one relationship between the Collada class, which there is only one instance per document anyway.

rdiankov commented 11 years ago

as for the tests, it is easy to write one test for one specific object. my question was how do we write it to make sure all objects that have extra tags work correctly?

a lot of these tests and checks are trivial, except they take time to write, which goes back to my previous comment of auto-generating the py files directly from the xml schema. although neither you nor me has time for such an exercises, it'll make pycollada really awesome and make automating testing even better.

jterrace commented 11 years ago

Ah, I see what you mean. I hadn't thought if it that way, but maybe that makes more sense. Leave Collada.scene as a shortcut to Collada.instance_visual_scene.visual_scene, then have Collada.instance_kinematics_scene.kinematics_scene and Collada.kinematics_scene and Collada.kscene as shortcuts. I think that sounds okay.

As for testing, yeah auto-generating would be nice, but once the classes are fleshed out, it doesn't really have to change. To add extra tests, we can just add in a couple lines in each of the existing test classes to check that the .extras property is empty, add a sample Extra object, then check to make sure it still exists after loading. I think it would be a few lines per object. I just committed tests for extras under PerspectiveCamera and it already exposed 2 bugs that I fixed :) You can see the diff in ff95e06a5265f34b050847bcbd0d48da9a7a27a6

rdiankov commented 11 years ago

ok sounds good, i don't mind verbose code, so we can completely remove the Collada.kscene shortcuts

mbc commented 11 years ago

hello!

We would very much like to use this branch. What do we need to do to get it merged into master?

thanks!