Skip to content

Optimize Skeleton3D#113441

Open
TokageItLab wants to merge 1 commit into
godotengine:masterfrom
TokageItLab:opt-skel
Open

Optimize Skeleton3D#113441
TokageItLab wants to merge 1 commit into
godotengine:masterfrom
TokageItLab:opt-skel

Conversation

@TokageItLab
Copy link
Copy Markdown
Member

co-authored-by: Ryan-000 <73148864+Ryan-000@users.noreply.github.com>
Copy link
Copy Markdown
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Core-wise (adding the SKELETON_3D ancestral class, and how it's used) this is fine.

I can't comment on whether this makes a difference in Skeleton3D code performance, but we have ~10 bits left, so we don't need to be stingy with it.
So I'll leave it to the animation team to test the impact and review the functionality of this change.

@Repiteo Repiteo changed the title Optimization Skeleton3D Optimize Skeleton3D Dec 2, 2025
Comment on lines +1882 to +1884
if (t->loc_used && t->rot_used && t->scale_used) {
t_skeleton->set_bone_pose_components(t->bone_idx, t->loc, t->rot, t->scale);
} else {
Copy link
Copy Markdown
Member Author

@TokageItLab TokageItLab Dec 3, 2025

Choose a reason for hiding this comment

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

What concerns me is that cases where all loc/rot/scale components are enable are quite rare.

In cases where both position and rotation are active, setting them simultaneously might potentially lead to improvements. So if I were to implement this, I would try passing loc/rot/scale as a bitmask as an argument and performing conditional branching within the set_bone_pose_components() method to determine what to set and always use that method, like:

void Skeleton3D::set_bone_pose_components(int p_bone, int p_component_mask, const Vector3 &p_position, const Quaternion &p_rotation, const Vector3 &p_scale) {
	const int bone_size = bones.size();
	ERR_FAIL_INDEX(p_bone, bone_size);

	if (p_component_mask & COMPONENT_POSITION) {
		bones[p_bone].pose_position = p_position;
	}
	if (p_component_mask & COMPONENT_ROTATION) {
		bones[p_bone].pose_rotation = p_rotation;
	}
	if (p_component_mask & COMPONENT_SCALE) {
		bones[p_bone].pose_scale = p_scale;
	}

But I'm not sure which cost is lower until I test it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree and think this is a good idea since most skeletal animations only use position and rotation.

If performance is a concern, marking the set_bone_pose_components method as inline should allow the compiler to eliminate the branches when it is in Skeleton3D::set_bone_pose.

Or we can have just both methods, one with flags and one without.

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.

3 participants