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

support armature #15

Merged
merged 2 commits into from
May 2, 2018
Merged

support armature #15

merged 2 commits into from
May 2, 2018

Conversation

Jason0214
Copy link
Collaborator

@Jason0214 Jason0214 commented Apr 27, 2018

  1. I tried very hard to prevent armature pose to deform the mesh, but still failed to do it, however I did some workaround to solve this issue
    • the main problem comes from the lazy update of Blender scene( in mesh.py L96 where I add a FIXME ). I can clear the pose transform either set armature.pose_position or clear pose bone one by one, but the mesh data is not updated even I call bpy.context.scene.update(). Same script can take affect in console, however, not work as a whole script, that's weird.
    • I checked the collada-exporter, same issue exists, when using setting 'apply modifier', the exported mesh deformed by pose
    • my workaround is to set 'use_mesh_modifier' to False when exporting a mesh with pose armature. I already changed the code to deal with vertex's bone weights, so whether the armature modifier is applied or not, the bone weights are exported correctly. So it may be approachable to set 'use_mesh_modifier' default to False (it is not done yet) until this issue being solved.
  2. the pose matrix appearing in tscn file will cause the Godot throw a Condition ' !is_inside_tree() ' is true I don't know why it happens, so I comment out the pose matrix in armature.py L59.
  3. if this branch got merged, it may better to open two issues for the above two problem, hope other contributor may have a solution for them
  4. the constructor of Bone has six arguments, and I got a 'too many arguments' warning from pylint, so can we change pylint rule a little bit to solve it :P
  5. I add some test cases, reference-exports could be uploaded after this PR got reviewed

@karroffel
Copy link

I checked the collada-exporter, same issue exists, when using setting 'apply modifier', the exported mesh deformed by pose

No, not anymore, this was fixed a few days ago by godotengine/collada-exporter#65

@sdfgeoff
Copy link
Collaborator

sdfgeoff commented Apr 27, 2018

  1. Have a look and see if karroffel's solution will fix our one as well. I imagine it will.

  2. Because this is a godot issue, uncomment it here and open an issue. If I recall correctly the error shouldn't do any harm until it's fixed. (I've had other objects throwing this one lots)

  3. I'd prefer the first solution was fixed, and the second was uncommented (but reported).

  4. The problem with allowing more arguments is that what happens when godot adds another dozen features to the bone? Do we add everything into that constructor and keep enlarging the pylint limits. I'd rather reduce the constructor to the minimum, keeping probably the boneID, name and parentID. Everything else can be written into it after construction (use sane default values in the init function).

  5. Thanks for adding test cases. Unfortunately I'm pretty sure the reference-exports will have to be updated nearly every merge.


if has_bone_array:
return bone_array, weights_array
return [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, put the no-bones case before iterating through vertices. On high poly models the time to iterate through all the vertices/bones may be significant.

@Jason0214 Jason0214 force-pushed the armature branch 2 times, most recently from 41e288c to 65b7f2a Compare April 28, 2018 00:51
@Jason0214
Copy link
Collaborator Author

Jason0214 commented Apr 29, 2018

@karroffel I tried collada-exporter for Blender on my macOS platform, if I set 'apply modifier' to true, the exported mesh is still deformed by pose. The fix in godotengine/collada-exporter#65 use similar implementation as mine in this PR, by setting the Armature.pose_position. The issue that mesh and mesh modifier s' does not get updated after armature changed still exists.

However, I am not sure if it's a problem only with my platform. Anyway, wait for @sdfgeoff to take a look at it

@sdfgeoff
Copy link
Collaborator

sdfgeoff commented May 1, 2018

I've just tested out commit 65b7f2a and noticed that in armature_with_mesh and armature_with_pose the monkey head has no bone or weight associations - nor does it display.
However, armature_with_physics seems to work correctly.

@Jason0214
Copy link
Collaborator Author

Jason0214 commented May 2, 2018

  • find the issue and fixed it. The problem comes from the type of armature modifier, the type can be string literal 'Armature' or 'ARMATURE' depending on platforms. (the armature_with_physics is the one you send to me, so it has 'ARMATURE', the monkey heads are built by my self and have 'Armature') . Seems Blender has platform difference more than we thought, we do need to check thoroughly.

the main problem comes from the lazy update of Blender scene( in mesh.py L96 where I add a FIXME ). I can clear the pose transform either set armature.pose_position or clear pose bone one by one, but the mesh data is not updated even I call bpy.context.scene.update(). Same script can take affect in console, however, not work as a whole script, that's weird.

  • suddenly it seems this problem is fixed
    @sdfgeoff

.gitignore Outdated
@@ -6,3 +6,9 @@ tests/exports/*.escn

.import
*.import

# for macOS
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are probably better in your global gitignore. See https://gist.github.com/subfuzion/db7f57fff2fb6998a16c

@Jason0214
Copy link
Collaborator Author

never know git has a global ignore..
yeah, these are better to go into global config

return "bones/{}/{}".format(self.id, attr_name).lower()


def export_bone_tree(pose_bone, parent_bone_id, bone_list):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you chose to do this recursively so you could pass in the parent ID. An alternative would be to iterate through node.pose.bones.values() which is a python list and so supports the index operation. Thus it could be written non-recursively like:

bone_list = list()
raw_bone_list = node.pose.bones.values()
for pose_bone_id, pose_bone in raw_bone_list:
    if pose_bone.parent is None:
        parent_id = -1
    else:
        parent_id = raw_bone_list.index(pose_bone.parent)

    bone_list.append(export_bone(pose_bone, pose_bone_id, parent_id))

Doing it iteratively has two advantages:

  • You can export skeletons with hierarchies of bones > 1000 deep (fortunately this is unlikely to be a limit anyone ever encounters).
  • You can sort the raw_bone_list by (for example) the bone names. This ensures that the bone id's are deterministic, this may be needed when exporting animations (bones have multiple poses)

It may be that I'm missing some other advantage of the recursion, so if you can justify why you did it recursively, then it can stay this way.

Copy link
Collaborator Author

@Jason0214 Jason0214 May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think about the animation exporting, in my plan the pose of bone would be access through Blender data structure not escn tree, so it will not be a problem.


class NodePath:
"""Nodes in scene refers to other nodes, for example MeshInstane
refers Skeleton. Provides parameters to choose use relative or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't actually seem to provide a parameter for relative/absolute. If you just remove this from the documentation, it should be fine.

@sdfgeoff sdfgeoff merged commit 2cd6b18 into godotengine:master May 2, 2018
@Jason0214 Jason0214 deleted the armature branch May 10, 2018 17:04
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