-
-
Notifications
You must be signed in to change notification settings - Fork 20.9k
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
GLTF: Implement KHR_animation_pointer for animating custom properties #94165
base: master
Are you sure you want to change the base?
Conversation
Wow, I didn’t even know that GLTF could store such data, I was surprised by the waterfall, how does it work? UVs are recorded and animated? |
Could you add this link when talking about JSON pointers in your intro? I had to search about the concept. |
7641446
to
27bdcad
Compare
@JekSun97 The UV offset values can be stored in glTF in a KHR_texture_transform. This is then mapped to Godot's BaseMaterial3D uv1_offset. This PR adds support for importing and exporting animations that animate this property, but it's already possible to store a frozen non-animated version of this value before this PR. |
998e973
to
c87f4db
Compare
c87f4db
to
c2e9314
Compare
c2e9314
to
e3113da
Compare
8a74c2b
to
d0d4675
Compare
https://github.com/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_animation_pointer is a ratified official extension so as soon as the Godot Engine portion is done, we can merge. Some of the other extensions are still in draft stage. |
d0d4675
to
8155c65
Compare
8155c65
to
127d7e0
Compare
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.
There are currently several cases involving TYPE_VALUE tracks for :scale, :rotation, :quaternion, :position and :transform that I think do not work for cubic interpolation.
I think these cases were also broken before, so I do not want fixing this to block this PR. Just making a note of where the issues are.
double d = *p_src; | ||
buffer.write[dst_i] = d; |
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.
We should add a FIXME that the double precision p_src
loses precision when encoding 64-bit integers. I do not think we should fix this until there is an issue or a tangible usecase for full precision long types.
bool last = false; | ||
while (true) { | ||
Quaternion rotation; | ||
Error err = p_godot_animation->try_rotation_track_interpolate(p_godot_anim_track_index, time, &rotation); |
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.
Both the old and the new code have broken cubic interpolation TYPE_VALUE trs tracks.
This is incorrect if track is TRACK_VALUE:
See scene/resources/animation.cpp:
ERR_FAIL_COND_V(t->type != TYPE_ROTATION_3D, ERR_INVALID_PARAMETER);
bool last = false; | ||
while (true) { | ||
Vector3 position; | ||
Error err = p_godot_animation->try_position_track_interpolate(p_godot_anim_track_index, time, &position); |
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.
Both the old and the new code have broken cubic interpolation TYPE_VALUE trs tracks.
ERR_FAIL_COND_V(t->type != TYPE_POSITION_3D, ERR_INVALID_PARAMETER);
bool last = false; | ||
while (true) { | ||
Vector3 scale; | ||
Error err = p_godot_animation->try_scale_track_interpolate(p_godot_anim_track_index, time, &scale); |
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.
Both the old and the new code have broken cubic interpolation TYPE_VALUE trs tracks.
ERR_FAIL_COND_V(t->type != TYPE_SCALE_3D, ERR_INVALID_PARAMETER);
err = p_godot_animation->try_rotation_track_interpolate(p_godot_anim_track_index, time, &rotation); | ||
ERR_CONTINUE(err != OK); | ||
err = p_godot_animation->try_scale_track_interpolate(p_godot_anim_track_index, time, &scale); | ||
ERR_CONTINUE(err != OK); |
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.
Both the old and the new code have broken cubic interpolation TYPE_VALUE trs tracks.
This is incorrect if track is TRACK_VALUE:
See scene/resources/animation.cpp:
ERR_FAIL_COND_V(t->type != TYPE_ROTATION_3D, ERR_INVALID_PARAMETER);
modules/gltf/gltf_document.cpp
Outdated
} | ||
p_gltf_node_track.position_track.values.write[key_i] = bezier_track; | ||
} | ||
} else if (node_prop == "rotation") { |
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.
rotation -> quaternion
if (p_handle_skeletons && skeleton != -1) { | ||
// Special case for skeleton nodes, skip all bones so that the path is to the Skeleton3D node. | ||
// A path that would otherwise be `A/B/C/Bone1/Bone2/Bone3` becomes `A/B/C/Skeleton3D:Bone3`. | ||
subpath.append(get_name()); |
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.
Test animating blend shapes or materials on a MeshInstance3D that is a child of a BoneAttachment3D (bone) which does not have skin
Add get_scene_node_path and has_additional_data to GLTFNode, remove center of mass ignore warning in physics (it's supported now), rename `d` to `mesh_dict` in mesh import code.
127d7e0
to
f56a23d
Compare
// At this point, we have found a node with the shape index we were looking for. | ||
if (_will_gltf_shape_become_subnode(p_state, gltf_node, node_index)) { | ||
Vector<StringName> sname_path = node_path.get_names(); | ||
sname_path.append(gltf_node->get_name() + "Shape"); |
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 this causes a name collision, the physics animations will not work as expected, since "Shape" will already be in use.
We should consider reserving node name + "Shape" when importing nodes.
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.
Reviewed together in a call with @fire and @aaronfranke (took 3 hours!)
This seems great! No major issues. It would be good to get it merged so we can start testing it for imports and exports
const int track_index = animation->get_track_count(); | ||
animation->add_track(Animation::TYPE_VALUE); |
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.
this will cause individually animated .../weights/N tracks to be TYPE_VALUE, but we should convert them to TYPE_BLEND_SHAPE with the name of the blend shape (will have to find the mesh and get the shape name)
This PR implements the glTF Object Model and the KHR_animation_pointer extension. The object model allows accessing properties of the glTF JSON using JSON pointers, mapped to Godot properties. This can be used for things like engine-agnostic scripting systems, where simple behavior graphs can be defined inside of glTF files to manipulate properties.
KHR_animation_pointer is the first extension prominently making use of the object model, to allow for animations to control properties. With vanilla glTF animations, you can animate position, rotation, scale, and weights. However, animation pointer allows animating much more. For example, you could animate the color of a light, the FOV of a camera, the albedo color of a material, the UV offset of a material, and more.
khr_anim_ptr.mp4
Test project: gltf_khr_animation_pointer.zip
These test files were created by the Khronos group, not me. The source is here: KhronosGroup/glTF-Sample-Assets#106
Both the object model and the KHR_animation_pointer extension are already final and ratified by the Khronos group. This must be core to Godot Engine because it requires a lot of low-level hooks to do it right, and other extensions need to be able to build on these features. See also godotengine/godot-proposals#10164
For ease of reviewability, I have split this PR into 8 commits:
_node_and_or_bone_to_gltf_node_index
, overhaul and simplify_convert_animation
.d
tomesh_dict
in mesh import code.If desired, I can move some of these into a separate PR.