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.

Sign up for GitHub

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

Jump to bottom

Fix desynced duplicate GLTFNode transform properties #83231

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Fix desynced duplicate GLTFNode transform properties #83231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 37 additions & 47 deletions modules/gltf/gltf_document.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,20 +430,22 @@ Error GLTFDocument::_serialize_nodes(Ref<GLTFState> p_state) {
}
if (gltf_node->skeleton != -1 && gltf_node->skin < 0) {
}
if (gltf_node->xform != Transform3D()) {
node["matrix"] = _xform_to_array(gltf_node->xform);
}

if (!gltf_node->rotation.is_equal_approx(Quaternion())) {
node["rotation"] = _quaternion_to_array(gltf_node->rotation);
}

if (!gltf_node->scale.is_equal_approx(Vector3(1.0f, 1.0f, 1.0f))) {
node["scale"] = _vec3_to_arr(gltf_node->scale);
}

if (!gltf_node->position.is_zero_approx()) {
node["translation"] = _vec3_to_arr(gltf_node->position);
if (gltf_node->transform.basis.is_orthogonal()) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is necessarily a good idea, there are scenes (and common in skeletons) where you can have a non uniform scale and this is needed for proper importing, even if orthogonal.

Copy link
Member

@reduz reduz Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These may not import correctly if you convert to TRS (or they may, I am not sure). Either case, I would test this well.

Copy link
Member

Choose a reason for hiding this comment

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

Oh if this is just for exporting it may not matter so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

The is_orthgonal check is to ensure there is no skew/shear. Non-uniform scale is still allowed.

Copy link
Member

Choose a reason for hiding this comment

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

A non uniform scale is de composable into TRS as mentioned above, polar decomposition and such for nested non uniform scale is probably not needed for sheer and skew, but I am not sure what happens when we export it in this failure case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will export the data Godot has in the best possible format. This code will prefer TRS if TRS can store all the data, otherwise it will use matrix.

// An orthogonal transform is decomposable into TRS, so prefer that.
const Vector3 position = gltf_node->get_position();
if (!position.is_zero_approx()) {
node["translation"] = _vec3_to_arr(position);
}
const Quaternion rotation = gltf_node->get_rotation();
if (!rotation.is_equal_approx(Quaternion())) {
node["rotation"] = _quaternion_to_array(rotation);
}
const Vector3 scale = gltf_node->get_scale();
if (!scale.is_equal_approx(Vector3(1.0f, 1.0f, 1.0f))) {
node["scale"] = _vec3_to_arr(scale);
}
} else {
node["matrix"] = _xform_to_array(gltf_node->transform);
}
if (gltf_node->children.size()) {
Array children;
Expand Down Expand Up @@ -609,20 +611,17 @@ Error GLTFDocument::_parse_nodes(Ref<GLTFState> p_state) {
node->skin = n["skin"];
}
if (n.has("matrix")) {
node->xform = _arr_to_xform(n["matrix"]);
node->transform = _arr_to_xform(n["matrix"]);
} else {
if (n.has("translation")) {
node->position = _arr_to_vec3(n["translation"]);
node->set_position(_arr_to_vec3(n["translation"]));
}
if (n.has("rotation")) {
node->rotation = _arr_to_quaternion(n["rotation"]);
node->set_rotation(_arr_to_quaternion(n["rotation"]));
}
if (n.has("scale")) {
node->scale = _arr_to_vec3(n["scale"]);
node->set_scale(_arr_to_vec3(n["scale"]));
}

node->xform.basis.set_quaternion_scale(node->rotation, node->scale);
node->xform.origin = node->position;
}

if (n.has("extensions")) {
Expand Down Expand Up @@ -4802,10 +4801,10 @@ Error GLTFDocument::_create_skeletons(Ref<GLTFState> p_state) {
node->set_name(_gen_unique_bone_name(p_state, skel_i, node->get_name()));

skeleton->add_bone(node->get_name());
skeleton->set_bone_rest(bone_index, node->xform);
skeleton->set_bone_pose_position(bone_index, node->position);
skeleton->set_bone_pose_rotation(bone_index, node->rotation.normalized());
skeleton->set_bone_pose_scale(bone_index, node->scale);
skeleton->set_bone_rest(bone_index, node->transform);
skeleton->set_bone_pose_position(bone_index, node->get_position());
skeleton->set_bone_pose_rotation(bone_index, node->get_rotation());
skeleton->set_bone_pose_scale(bone_index, node->get_scale());

if (node->parent >= 0 && p_state->nodes[node->parent]->skeleton == skel_i) {
const int bone_parent = skeleton->find_bone(p_state->nodes[node->parent]->get_name());
Expand Down Expand Up @@ -5511,10 +5510,7 @@ GLTFLightIndex GLTFDocument::_convert_light(Ref<GLTFState> p_state, Light3D *p_l
}

void GLTFDocument::_convert_spatial(Ref<GLTFState> p_state, Node3D *p_spatial, Ref<GLTFNode> p_node) {
Transform3D xform = p_spatial->get_transform();
p_node->scale = xform.basis.get_scale();
p_node->rotation = xform.basis.get_rotation_quaternion();
p_node->position = xform.origin;
p_node->transform = p_spatial->get_transform();
}

Node3D *GLTFDocument::_generate_spatial(Ref<GLTFState> p_state, const GLTFNodeIndex p_node_index) {
Expand Down Expand Up @@ -5628,7 +5624,7 @@ void GLTFDocument::_convert_csg_shape_to_gltf(CSGShape3D *p_current, GLTFNodeInd
GLTFMeshIndex mesh_i = p_state->meshes.size();
p_state->meshes.push_back(gltf_mesh);
p_gltf_node->mesh = mesh_i;
p_gltf_node->xform = csg->get_meshes()[0];
p_gltf_node->transform = csg->get_meshes()[0];
p_gltf_node->set_name(_gen_unique_name(p_state, csg->get_name()));
}
#endif // MODULE_CSG_ENABLED
Expand Down Expand Up @@ -5704,7 +5700,7 @@ void GLTFDocument::_convert_grid_map_to_gltf(GridMap *p_grid_map, GLTFNodeIndex
gltf_mesh->set_mesh(_mesh_to_importer_mesh(p_grid_map->get_mesh_library()->get_item_mesh(cell)));
new_gltf_node->mesh = p_state->meshes.size();
p_state->meshes.push_back(gltf_mesh);
new_gltf_node->xform = cell_xform * p_grid_map->get_transform();
new_gltf_node->transform = cell_xform * p_grid_map->get_transform();
new_gltf_node->set_name(_gen_unique_name(p_state, p_grid_map->get_mesh_library()->get_item_name(cell)));
}
}
Expand Down Expand Up @@ -5772,7 +5768,7 @@ void GLTFDocument::_convert_multi_mesh_instance_to_gltf(
Ref<GLTFNode> new_gltf_node;
new_gltf_node.instantiate();
new_gltf_node->mesh = mesh_index;
new_gltf_node->xform = transform;
new_gltf_node->transform = transform;
new_gltf_node->set_name(_gen_unique_name(p_state, p_multi_mesh_instance->get_name()));
p_gltf_node->children.push_back(p_state->nodes.size());
p_state->nodes.push_back(new_gltf_node);
Expand All @@ -5797,10 +5793,7 @@ void GLTFDocument::_convert_skeleton_to_gltf(Skeleton3D *p_skeleton3d, Ref<GLTFS
// Note that we cannot use _gen_unique_bone_name here, because glTF spec requires all node
// names to be unique regardless of whether or not they are used as joints.
joint_node->set_name(_gen_unique_name(p_state, skeleton->get_bone_name(bone_i)));
Transform3D xform = skeleton->get_bone_pose(bone_i);
joint_node->scale = xform.basis.get_scale();
joint_node->rotation = xform.basis.get_rotation_quaternion();
joint_node->position = xform.origin;
joint_node->transform = skeleton->get_bone_pose(bone_i);
joint_node->joint = true;
GLTFNodeIndex current_node_i = p_state->nodes.size();
p_state->scene_nodes.insert(current_node_i, skeleton);
Expand Down Expand Up @@ -5949,7 +5942,7 @@ void GLTFDocument::_generate_scene_node(Ref<GLTFState> p_state, const GLTFNodeIn
Array args;
args.append(p_scene_root);
current_node->propagate_call(StringName("set_owner"), args);
current_node->set_transform(gltf_node->xform);
current_node->set_transform(gltf_node->transform);
}

p_state->scene_nodes.insert(p_node_index, current_node);
Expand Down Expand Up @@ -6276,7 +6269,7 @@ void GLTFDocument::_import_animation(Ref<GLTFState> p_state, AnimationPlayer *p_
if (track.position_track.values.size()) {
bool is_default = true; //discard the track if all it contains is default values
if (p_remove_immutable_tracks) {
Vector3 base_pos = p_state->nodes[track_i.key]->position;
Vector3 base_pos = gltf_node->get_position();
for (int i = 0; i < track.position_track.times.size(); i++) {
int value_index = track.position_track.interpolation == GLTFAnimation::INTERP_CUBIC_SPLINE ? (1 + i * 3) : i;
ERR_FAIL_COND_MSG(value_index >= track.position_track.values.size(), "Animation sampler output accessor with 'CUBICSPLINE' interpolation doesn't have enough elements.");
Expand All @@ -6301,7 +6294,7 @@ void GLTFDocument::_import_animation(Ref<GLTFState> p_state, AnimationPlayer *p_
if (track.rotation_track.values.size()) {
bool is_default = true; //discard the track if all it contains is default values
if (p_remove_immutable_tracks) {
Quaternion base_rot = p_state->nodes[track_i.key]->rotation.normalized();
Quaternion base_rot = gltf_node->get_rotation();
for (int i = 0; i < track.rotation_track.times.size(); i++) {
int value_index = track.rotation_track.interpolation == GLTFAnimation::INTERP_CUBIC_SPLINE ? (1 + i * 3) : i;
ERR_FAIL_COND_MSG(value_index >= track.rotation_track.values.size(), "Animation sampler output accessor with 'CUBICSPLINE' interpolation doesn't have enough elements.");
Expand All @@ -6326,7 +6319,7 @@ void GLTFDocument::_import_animation(Ref<GLTFState> p_state, AnimationPlayer *p_
if (track.scale_track.values.size()) {
bool is_default = true; //discard the track if all it contains is default values
if (p_remove_immutable_tracks) {
Vector3 base_scale = p_state->nodes[track_i.key]->scale;
Vector3 base_scale = gltf_node->get_scale();
for (int i = 0; i < track.scale_track.times.size(); i++) {
int value_index = track.scale_track.interpolation == GLTFAnimation::INTERP_CUBIC_SPLINE ? (1 + i * 3) : i;
ERR_FAIL_COND_MSG(value_index >= track.scale_track.values.size(), "Animation sampler output accessor with 'CUBICSPLINE' interpolation doesn't have enough elements.");
Expand Down Expand Up @@ -6357,15 +6350,15 @@ void GLTFDocument::_import_animation(Ref<GLTFState> p_state, AnimationPlayer *p_
Vector3 base_scale = Vector3(1, 1, 1);

if (rotation_idx == -1) {
base_rot = p_state->nodes[track_i.key]->rotation.normalized();
base_rot = gltf_node->get_rotation();
}

if (position_idx == -1) {
base_pos = p_state->nodes[track_i.key]->position;
base_pos = gltf_node->get_position();
}

if (scale_idx == -1) {
base_scale = p_state->nodes[track_i.key]->scale;
base_scale = gltf_node->get_scale();
}

bool last = false;
Expand Down Expand Up @@ -6472,10 +6465,7 @@ void GLTFDocument::_convert_mesh_instances(Ref<GLTFState> p_state) {
if (!mi) {
continue;
}
Transform3D mi_xform = mi->get_transform();
node->scale = mi_xform.basis.get_scale();
node->rotation = mi_xform.basis.get_rotation_quaternion();
node->position = mi_xform.origin;
node->transform = mi->get_transform();

Node *skel_node = mi->get_node_or_null(mi->get_skeleton_path());
Skeleton3D *godot_skeleton = Object::cast_to<Skeleton3D>(skel_node);
Expand Down
16 changes: 8 additions & 8 deletions modules/gltf/structures/gltf_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@ void GLTFNode::set_height(int p_height) {
}

Transform3D GLTFNode::get_xform() {
return xform;
return transform;
}

void GLTFNode::set_xform(Transform3D p_xform) {
xform = p_xform;
transform = p_xform;
}

GLTFMeshIndex GLTFNode::get_mesh() {
Expand Down Expand Up @@ -129,27 +129,27 @@ void GLTFNode::set_skeleton(GLTFSkeletonIndex p_skeleton) {
}

Vector3 GLTFNode::get_position() {
return position;
return transform.origin;
}

void GLTFNode::set_position(Vector3 p_position) {
position = p_position;
transform.origin = p_position;
}

Quaternion GLTFNode::get_rotation() {
return rotation;
return transform.basis.get_rotation_quaternion();
}

void GLTFNode::set_rotation(Quaternion p_rotation) {
rotation = p_rotation;
transform.basis.set_quaternion_scale(p_rotation, transform.basis.get_scale());
}

Vector3 GLTFNode::get_scale() {
return scale;
return transform.basis.get_scale();
}

void GLTFNode::set_scale(Vector3 p_scale) {
scale = p_scale;
transform.basis = transform.basis.orthonormalized() * Basis::from_scale(p_scale);
}

Vector<int> GLTFNode::get_children() {
Expand Down
6 changes: 1 addition & 5 deletions modules/gltf/structures/gltf_node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,14 @@ class GLTFNode : public Resource {
friend class GLTFDocument;

private:
// matrices need to be transformed to this
GLTFNodeIndex parent = -1;
int height = -1;
Transform3D xform;
Transform3D transform;
GLTFMeshIndex mesh = -1;
GLTFCameraIndex camera = -1;
GLTFSkinIndex skin = -1;
GLTFSkeletonIndex skeleton = -1;
bool joint = false;
Vector3 position;
Quaternion rotation;
Vector3 scale = Vector3(1, 1, 1);
Vector<int> children;
GLTFLightIndex light = -1;
Dictionary additional_data;
Expand Down
Loading