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

Remove REST transform influence in skeleton bones #53765

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Oct 13, 2021

Implements: godotengine/godot-proposals#3377

  • Animations and Skeletons are now pose-only.
  • Rest transform is kept as reference (when it exists) and for IK
  • Removed custom bone poses (redundant with local and global pose overrides).
  • Improves 3D model compatibility (non uniform transforms will properly work, as well as all animations coming from Autodesk products).

This is a compatibility breaking change.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 13, 2021

@reduz It make to crash when selecting Skeleton in the SceneTreeDock after importing gltf assets and makeing inherited scene.

skelman.gltf.zip

ERROR: Condition "p_skin.is_null()" is true. Returning: Ref<SkinReference>()
   at: register_skin (scene/3d/skeleton_3d.cpp:972)

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.dev.custom_build (687fd77c6b2840b07fadc991f1e65ee4c0710ce9)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] 1   libsystem_platform.dylib            0x00007fff20395d7d _sigtramp + 29
[2] 2   ???                                 0x00007fc601cd0c10 0x0 + 140488410467344
[3] Ref<Skin>::Ref(Ref<Skin> const&) (in godot.osx.tools.x86_64) (ref_counted.h:178)
[4] Ref<Skin>::Ref(Ref<Skin> const&) (in godot.osx.tools.x86_64) (ref_counted.h:178)
[5] SkinReference::get_skin() const (in godot.osx.tools.x86_64) (skeleton_3d.cpp:59)
[6] MeshInstance3D::_resolve_skeleton_path() (in godot.osx.tools.x86_64) (mesh_instance_3d.cpp:152)
[7] MeshInstance3D::_notification(int) (in godot.osx.tools.x86_64) (mesh_instance_3d.cpp:312)
[8] MeshInstance3D::_notificationv(int, bool) (in godot.osx.tools.x86_64) (mesh_instance_3d.h:40)
[9] Object::notification(int, bool) (in godot.osx.tools.x86_64) (object.cpp:841)
[10] Node::_propagate_enter_tree() (in godot.osx.tools.x86_64) (node.cpp:214)
[11] Node::_set_tree(SceneTree*) (in godot.osx.tools.x86_64) (node.cpp:2451)
[12] Node::_add_child_nocheck(Node*, StringName const&) (in godot.osx.tools.x86_64) (node.cpp:0)
[13] Node::add_child(Node*, bool, Node::InternalMode) (in godot.osx.tools.x86_64) (node.cpp:1125)
[14] Skeleton3DEditor::create_editors() (in godot.osx.tools.x86_64) (skeleton_3d_editor_plugin.cpp:784)
[15] Skeleton3DEditor::_notification(int) (in godot.osx.tools.x86_64) (skeleton_3d_editor_plugin.cpp:839)
[16] Skeleton3DEditor::_notificationv(int, bool) (in godot.osx.tools.x86_64) (skeleton_3d_editor_plugin.h:126)
[17] Object::notification(int, bool) (in godot.osx.tools.x86_64) (object.cpp:841)
[18] Node::_propagate_enter_tree() (in godot.osx.tools.x86_64) (node.cpp:214)
[19] Node::_set_tree(SceneTree*) (in godot.osx.tools.x86_64) (node.cpp:2451)
[20] Node::_add_child_nocheck(Node*, StringName const&) (in godot.osx.tools.x86_64) (node.cpp:0)
[21] Node::add_child(Node*, bool, Node::InternalMode) (in godot.osx.tools.x86_64) (node.cpp:1125)
[22] EditorInspector::_parse_added_editors(VBoxContainer*, Ref<EditorInspectorPlugin>) (in godot.osx.tools.x86_64) (editor_inspector.cpp:2290)
[23] EditorInspector::update_tree() (in godot.osx.tools.x86_64) (editor_inspector.cpp:2426)
[24] EditorInspector::edit(Object*) (in godot.osx.tools.x86_64) (editor_inspector.cpp:2957)
[25] EditorNode::_edit_current() (in godot.osx.tools.x86_64) (editor_node.cpp:2173)
[26] EditorNode::push_item(Object*, String const&, bool) (in godot.osx.tools.x86_64) (editor_node.cpp:2079)
[27] SceneTreeDock::_node_selected() (in godot.osx.tools.x86_64) (scene_tree_dock.cpp:1375)
[28] void call_with_variant_args_helper<SceneTreeDock>(SceneTreeDock*, void (SceneTreeDock::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (in godot.osx.tools.x86_64) (binder_common.h:227)
[29] void call_with_variant_args<SceneTreeDock>(SceneTreeDock*, void (SceneTreeDock::*)(), Variant const**, int, Callable::CallError&) (in godot.osx.tools.x86_64) (binder_common.h:337)
[30] CallableCustomMethodPointer<SceneTreeDock>::call(Variant const**, int, Variant&, Callable::CallError&) const (in godot.osx.tools.x86_64) (callable_method_pointer.h:97)
[31] Callable::call(Variant const**, int, Variant&, Callable::CallError&) const (in godot.osx.tools.x86_64) (callable.cpp:51)
[32] MessageQueue::_call_function(Callable const&, Variant const*, int, bool) (in godot.osx.tools.x86_64) (message_queue.cpp:258)
[33] MessageQueue::flush() (in godot.osx.tools.x86_64) (message_queue.cpp:306)
[34] SceneTree::process(double) (in godot.osx.tools.x86_64) (scene_tree.cpp:448)
[35] Main::iteration() (in godot.osx.tools.x86_64) (main.cpp:2570)
[36] OS_OSX::run() (in godot.osx.tools.x86_64) (os_osx.mm:510)
[37] main (in godot.osx.tools.x86_64) (godot_main_osx.mm:79)
[38] 38  libdyld.dylib                       0x00007fff2036bf3d start + 1
-- END OF BACKTRACE --
================================================================

@TokageItLab
Copy link
Member

TokageItLab commented Oct 13, 2021

Previously, skin was dynamically generated when p_skin was null. This is a case that can occur when calling from within MeshInstance3D. If you don't want to generate it in the Skeleton3D::register_skin(), you need to generate the skin in MeshInstance3D.

This is a patch to keep the previous behavior.

diff --git a/scene/3d/mesh_instance_3d.cpp b/scene/3d/mesh_instance_3d.cpp
index 67f4a88228..cfd90e5da0 100644
--- a/scene/3d/mesh_instance_3d.cpp
+++ b/scene/3d/mesh_instance_3d.cpp
@@ -146,11 +146,13 @@ void MeshInstance3D::_resolve_skeleton_path() {
 	if (!skeleton_path.is_empty()) {
 		Skeleton3D *skeleton = Object::cast_to<Skeleton3D>(get_node(skeleton_path));
 		if (skeleton) {
-			new_skin_reference = skeleton->register_skin(skin_internal);
 			if (skin_internal.is_null()) {
+				new_skin_reference = skeleton->register_skin(skeleton->create_skin_from_rest_transforms());
 				//a skin was created for us
 				skin_internal = new_skin_reference->get_skin();
 				notify_property_list_changed();
+			} else {
+				new_skin_reference = skeleton->register_skin(skin_internal);
 			}
 		}
 	}

@TokageItLab
Copy link
Member

Animation key insertion in SkeletonEditor and Inspector is broken, but this is not in the scope of this PR as it is an effect of getting rid of the previous Transform3DTrack. I will send you a PR with a fix after @reduz 's some overhauls to the animation features are done.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 13, 2021

Also, it seems that the InitPose and ApplyPoseToRest functions of SkeletonEditor are broken. This is the effect of this PR, but at least considering the fact that the calculation of rest in skinning is correct, it can be fixed later...

@reduz reduz force-pushed the skeleton-remove-rest-influence branch from 687fd77 to f98e5ab Compare October 13, 2021 16:27
@reduz
Copy link
Member Author

reduz commented Oct 13, 2021

@TokageItLab Regarding key insertion, I think we need to add a TRS key insertion menu in the 3D editor similar to the 2D one, also asking for using either the new TRS tracks or beziers:

image

@reduz
Copy link
Member Author

reduz commented Oct 13, 2021

@TokageItLab alright applied your suggested patch

@TokageItLab
Copy link
Member

TokageItLab commented Oct 13, 2021

@TokageItLab Regarding key insertion, I think we need to add a TRS key insertion menu in the 3D editor similar to the 2D one, also asking for using either the new TRS tracks or beziers:

image

Roger, for now, I'll fix the broken track assignments and work on the implementation of the transform record in a separate PR.

* Animations and Skeletons are now pose-only.
* Rest transform is kept as reference (when it exists) and for IK
* Improves 3D model compatibility (non uniform transforms will properly work, as well as all animations coming from Autodesk products).
@reduz reduz force-pushed the skeleton-remove-rest-influence branch from f98e5ab to 2dc8232 Compare October 13, 2021 17:51
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Reviewed together with @fire

@reduz
Copy link
Member Author

reduz commented Oct 13, 2021

Thanks! Will merge when CI completes and move on to the next task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants