Feat: Add skeleton animation plugin spine#233
Conversation
86d8ecc to
1b62af9
Compare
Summary of ChangesHello @JiepengTan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive Spine animation plugin for the Godot engine. It integrates the Spine C++ runtime, provides Godot-specific wrappers for Spine's core components (skeletons, animations, skins, etc.), and extends the Godot editor with custom resource types and inspector functionalities. This allows Godot developers to import, manage, and display Spine-created 2D skeletal animations directly within their projects, offering robust control over animation playback, mixing, and interaction with other Godot nodes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is a massive pull request that adds a full-featured Spine animation module. The overall architecture with custom resources, wrappers for the Spine API, and editor integration is well-designed. The code is generally clean and follows Godot's conventions.
I've found a few potential bugs, mostly related to inverted logic in getters and inconsistencies in handling Y-axis scaling. There are also some minor maintainability issues, like confusing variable names and non-English comments in a build script.
Given this is a work-in-progress, these issues are understandable. Great work so far on this complex integration!
| float rotation = spine::MathUtil::Rad_Deg * transform.get_rotation(); | ||
| Vector2 scale = transform.get_scale(); | ||
| Vector2 local_position = position; | ||
| float local_rotation = bone->worldToLocalRotation(rotation) - 180; |
There was a problem hiding this comment.
The calculation for local_rotation subtracts 180 degrees after converting from world to local rotation. This is suspicious and might be a bug related to coordinate system conversions. worldToLocalRotation should ideally return the correct local rotation. This subtraction might be causing incorrect bone orientations when setting global transforms.
| Ref<SpineAttachment> SpineSkin::get_attachment(int slot_index, const String &name) { | ||
| SPINE_CHECK(get_spine_object(), nullptr) | ||
| auto attachment = get_spine_object()->getAttachment(slot_index, SPINE_STRING(name)); | ||
| if (attachment) return nullptr; |
| float SpineSkeleton::get_scale_y() { | ||
| SPINE_CHECK(skeleton, 1) | ||
| return -skeleton->getScaleY(); | ||
| } | ||
|
|
||
| void SpineSkeleton::set_scale_y(float v) { | ||
| SPINE_CHECK(skeleton, ) | ||
| skeleton->setScaleY(v); | ||
| } |
There was a problem hiding this comment.
get_scale_y() returns the negated scale, but set_scale_y() sets the value directly. This inconsistency can lead to bugs. For example, set_scale_y(get_scale_y()) would flip the skeleton's scale every time it's called.
To ensure consistency, the setter should also negate the value, like skeleton->setScaleY(-v);.
void SpineSkeleton::set_scale_y(float v) {
SPINE_CHECK(skeleton, )
skeleton->setScaleY(-v);
}| if (skin) | ||
| return nullptr; |
| void *GodotSpineExtension::_calloc(size_t size, const char *file, int line) { | ||
| auto p = memalloc(size); | ||
| memset(p, 0, size); | ||
| return p; | ||
| } |
There was a problem hiding this comment.
| # Spine C++ 运行时的头文件路径 | ||
| env_spine_godot.Append(CPPPATH=["spine-cpp/include"]) | ||
|
|
||
| # 如果是生成 VS 项目,也需要添加到主 env | ||
| if env["vsproj"]: | ||
| env.Append(CPPPATH=["#modules/spine/spine-cpp/include"]) | ||
|
|
||
| # 编译 spine-cpp 源文件 | ||
| env_spine_godot.add_source_files(env.modules_sources, "spine-cpp/src/spine/*.cpp") | ||
|
|
||
| # 编译 spine_godot 绑定文件 | ||
| env_spine_godot.add_source_files(env.modules_sources, "*.cpp") | ||
|
|
||
| # Clang 下禁用 override 警告 |
There was a problem hiding this comment.
| Ref<Animation> animation_ref = create_animation(animation, false); | ||
| Ref<Animation> animation_looped_ref = create_animation(animation, true); |
There was a problem hiding this comment.
The variable names animation_ref and animation_looped_ref are swapped with respect to the animations they hold. This is because the logic in create_animation is inverted: create_animation(..., false) creates a looping animation, and create_animation(..., true) creates a non-looping one, which is very confusing.
I suggest making the logic in create_animation more intuitive and renaming the variables at this call site to match what they hold.
For example, in create_animation (lines 264 and 268), you could change the logic to:
animation_ref->set_name(name + (loop ? "_looped" : ""));
#if VERSION_MAJOR > 3
// animation_ref->set_loop_mode(loop ? Animation::LOOP_LINEAR : Animation::LOOP_NONE);
#else
animation_ref->set_loop(loop);
#endifWith that change, the variable assignments here would be correct as they are.
Ref<Animation> animation_looped_ref = create_animation(animation, false);
Ref<Animation> animation_ref = create_animation(animation, true);| this->textures = spineAtlas->textures; | ||
| this->normal_maps = spineAtlas->normal_maps; | ||
| this->specular_maps = spineAtlas->specular_maps; | ||
| emit_signal(SNAME("skeleton_file_changed")); |
03868fd to
2b6fe25
Compare
2b6fe25 to
e26be29
Compare
72054f6 to
598a7ca
Compare
| SPINE_CHECK(skeleton_data, nullptr) | ||
| auto skin = skeleton_data->getDefaultSkin(); | ||
| if (skin) | ||
| return nullptr; |
There was a problem hiding this comment.
Critical Bug: Inverted Logic
The condition is inverted - this returns nullptr when a skin exists and attempts to wrap a null pointer when no skin exists. Should be:
| return nullptr; | |
| if (!skin) |
| Ref<SpineAttachment> SpineSkin::get_attachment(int slot_index, const String &name) { | ||
| SPINE_CHECK(get_spine_object(), nullptr) | ||
| auto attachment = get_spine_object()->getAttachment(slot_index, SPINE_STRING(name)); | ||
| if (attachment) return nullptr; |
There was a problem hiding this comment.
Critical Bug: Inverted Logic
Same issue - returns null when attachment exists, wraps null pointer when it doesn't. Should be:
| if (attachment) return nullptr; | |
| if (!attachment) return nullptr; |
|
|
||
| void SpineBone::set_global_transform(Transform2D transform) { | ||
| SPINE_CHECK(get_spine_object(), ) | ||
| if (!get_spine_owner()) set_transform(transform); |
There was a problem hiding this comment.
Critical Bug: Null Pointer Dereference
Missing return statement - if get_spine_owner() is null, line 477 will dereference it. Should be:
| if (!get_spine_owner()) set_transform(transform); | |
| if (!get_spine_owner()) { | |
| set_transform(transform); | |
| return; | |
| } |
| } | ||
|
|
||
| auto indices_changed = false; | ||
| if (mesh_instance->indices.size() == indices->size()) { |
There was a problem hiding this comment.
Performance Issue: Inefficient Per-Frame Comparison
This linear O(n) comparison runs every frame for every mesh. Consider using memcmp for better performance:
indices_changed = (memcmp(old_indices, new_indices, indices->size() * sizeof(unsigned short)) != 0);| if (indices->size() > 0) { | ||
| mesh_instance->set_light_mask(get_light_mask()); | ||
| size_t num_vertices = vertices->size() / 2; | ||
| mesh_instance->vertices.resize((int) num_vertices); |
There was a problem hiding this comment.
Performance Issue: Excessive Per-Frame Allocations
Every frame, for every slot, these arrays are resized causing memory allocations. Consider pre-allocating buffers to max expected size and tracking actual usage separately to avoid GC pressure in complex animations.
|
Great Spine integration implementation! The code is well-structured and comprehensive. However, found 3 critical bugs and several performance concerns: Critical Bugs:
Security: Binary parser lacks bounds checking - validates file size before parsing to prevent crashes from malformed files Performance: Per-frame allocations in See inline comments for details. |
test demo: 09_Spine.zip
test script:
SpineAnimation4.mp4