Convert 3.x ESCN skeletons, animations, and shaders upon import #87106
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -257,6 +257,101 @@ TypedArray<StringName> AnimationMixer::_get_animation_library_list() const { | |
| return ret; | ||
| } | ||
|
|
||
| #if !defined(_3D_DISABLED) && !defined(DISABLE_DEPRECATED) | ||
| bool AnimationMixer::_recalc_animation(const Ref<Animation> &p_anim) { | ||
| HashMap<int, Vector<real_t>> new_track_values_map; | ||
| Node *parent = get_node_or_null(root_node); | ||
| if (!parent) { | ||
| return false; | ||
| } | ||
|
|
||
| for (int i = 0; i < p_anim->get_track_count(); i++) { | ||
| int track_type = p_anim->track_get_type(i); | ||
| if (!p_anim->track_is_relative_to_rest(i)) { | ||
| continue; | ||
| } | ||
| if (track_type == Animation::TYPE_POSITION_3D || track_type == Animation::TYPE_ROTATION_3D || track_type == Animation::TYPE_SCALE_3D) { | ||
| NodePath path = p_anim->track_get_path(i); | ||
| Node *node = parent->get_node(path); | ||
| ERR_FAIL_NULL_V(node, false); | ||
| Skeleton3D *skel = Object::cast_to<Skeleton3D>(node); | ||
| if (!skel) { // transforming non-skeleton node, not relative to rest | ||
| continue; | ||
| } | ||
| StringName bone = path.get_subname(0); | ||
| int bone_idx = skel->find_bone(bone); | ||
| if (bone_idx == -1) { | ||
| continue; | ||
| } | ||
| Transform3D rest = skel->get_bone_rest(bone_idx); | ||
| new_track_values_map[i] = Vector<real_t>(); | ||
| const int32_t POSITION_TRACK_SIZE = 5; | ||
| const int32_t ROTATION_TRACK_SIZE = 6; | ||
| const int32_t SCALE_TRACK_SIZE = 5; | ||
| int32_t track_size = POSITION_TRACK_SIZE; | ||
| if (track_type == Animation::TYPE_ROTATION_3D) { | ||
| track_size = ROTATION_TRACK_SIZE; | ||
| } | ||
| new_track_values_map[i].resize(track_size * p_anim->track_get_key_count(i)); | ||
| real_t *r = new_track_values_map[i].ptrw(); | ||
| for (int j = 0; j < p_anim->track_get_key_count(i); j++) { | ||
| real_t time = p_anim->track_get_key_time(i, j); | ||
| real_t transition = p_anim->track_get_key_transition(i, j); | ||
| if (track_type == Animation::TYPE_POSITION_3D) { | ||
| Vector3 a_pos = p_anim->track_get_key_value(i, j); | ||
| Transform3D t; | ||
| t.set_origin(a_pos); | ||
| Vector3 new_a_pos = (rest * t).origin; | ||
|
|
||
| real_t *ofs = &r[j * POSITION_TRACK_SIZE]; | ||
| ofs[0] = time; | ||
| ofs[1] = transition; | ||
| ofs[2] = new_a_pos.x; | ||
| ofs[3] = new_a_pos.y; | ||
| ofs[4] = new_a_pos.z; | ||
| } else if (track_type == Animation::TYPE_ROTATION_3D) { | ||
| Quaternion q = p_anim->track_get_key_value(i, j); | ||
| Transform3D t; | ||
| t.basis.rotate(q); | ||
| Quaternion new_q = (rest * t).basis.get_rotation_quaternion(); | ||
| real_t *ofs = &r[j * ROTATION_TRACK_SIZE]; | ||
| ofs[0] = time; | ||
| ofs[1] = transition; | ||
| ofs[2] = new_q.x; | ||
| ofs[3] = new_q.y; | ||
| ofs[4] = new_q.z; | ||
| ofs[5] = new_q.w; | ||
| } else if (track_type == Animation::TYPE_SCALE_3D) { | ||
| Vector3 v = p_anim->track_get_key_value(i, j); | ||
| Transform3D t; | ||
| t.scale(v); | ||
| Vector3 new_v = (rest * t).basis.get_scale(); | ||
|
|
||
| real_t *ofs = &r[j * SCALE_TRACK_SIZE]; | ||
| ofs[0] = time; | ||
| ofs[1] = transition; | ||
| ofs[2] = new_v.x; | ||
| ofs[3] = new_v.y; | ||
| ofs[4] = new_v.z; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if (new_track_values_map.is_empty()) { | ||
| return false; | ||
| } | ||
| for (int i = 0; i < p_anim->get_track_count(); i++) { | ||
| if (!new_track_values_map.has(i)) { | ||
| continue; | ||
| } | ||
| p_anim->set("tracks/" + itos(i) + "/keys", new_track_values_map[i]); | ||
| p_anim->set("tracks/" + itos(i) + "/relative_to_rest", false); | ||
| } | ||
| p_anim->emit_changed(); | ||
| return true; | ||
| } | ||
| #endif // !defined(_3D_DISABLED) || !defined(DISABLE_DEPRECATED) | ||
|
|
||
| void AnimationMixer::get_animation_library_list(LocalVector<StringName> *p_libraries) const { | ||
| for (const AnimationLibraryData &lib : animation_libraries) { | ||
| p_libraries->push_back(lib.name); | ||
|
|
@@ -691,6 +786,11 @@ bool AnimationMixer::_update_caches() { | |
| } | ||
| for (const StringName &E : sname_list) { | ||
| const Ref<Animation> &anim = get_animation(E); | ||
| #if !defined(_3D_DISABLED) && !defined(DISABLE_DEPRECATED) | ||
| if (anim->has_tracks_relative_to_rest()) { | ||
| _recalc_animation(anim); | ||
| } | ||
|
Comment on lines
+790
to
+792
Contributor
There was a problem hiding this comment. This is adding code in runtime which I am not happy with. It's also doing conversions in such a way that we now have an additional Clearly @TokageItLab would need to give the final call on this, but given the complexity the animation system already has, I'm uncertain if this is a good idea. I really don't like that this introduces a strange interaction between the track cache and the Animation keyframes but only if a legacy flag is in use, and one that can never be removed. When Godot 5.0 comes out, no doubt there will still be Animation resources floating around with this flag that will have the same problem. It also means that the animation track editor might be broken for these animations, since the actual tracks in the resource are rest-relative. How would we even know which Animation's are running this track conversion during every playback, and which aren't? I understand the problem though... this conversion cannot be done during import because the skeleton rest is not known until runtime, and in fact the Animation resource could be loaded independently of the tscn with the Skeleton3D and only attached later on, so Godot would have no way to adapt the Animation resource for the associated Skeleton3D. That said, if we have a way of knowing which Skeleton3D corresponds to the given Animation, then I think I would prefer if the escn importer should do the work of converting the legacy animation tracks to not be rest-relative.
Contributor
Author
There was a problem hiding this comment.
This is originally how I had this setup, but I think I misconstrued your original comment a while back. While this method has the added benefit of supporting older TSCNs as well, I understand the concerns about runtime complexity. So, would it be enough to simply remove the
Member
There was a problem hiding this comment. I believe adding this as compatibility code is not acceptable. Relative animation was removed in 4.0. In the past, after discussing with reduz, the alternative provided at that time was Supporting this would be akin to shipping that custom module in core, but considering the circumstances under which it was provided as a custom module, this would likely be rejected. Please note that what can be provided as compatibility code is resource conversion, such as converting animation keys from relative to absolute animation. Runtime-based solutions are almost never permitted. |
||
| #endif | ||
| for (int i = 0; i < anim->get_track_count(); i++) { | ||
| if (!anim->track_is_enabled(i)) { | ||
| continue; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we can do the track conversion here using an explicit function call instead of
player->advance(0)and remove all the code about caches, then I'd feel a lot less bad about the runtime cost issue.But it doesn't change that this is bleeding importer complexity into the animation system.