[WIP] Fix spine animation events#245
Conversation
JiepengTan
changed the title
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 marks a significant enhancement by integrating a full-fledged Spine runtime module into the Godot engine. It provides native support for Spine 2D animations, offering a comprehensive API to control and interact with animated skeletons. A key aspect of this integration is the proper handling and exposure of animation events, which allows for dynamic responses to animation cues within Godot projects. The module also includes essential editor tooling to streamline the import and configuration of Spine assets. 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. Ignored Files
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
|
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| Ref<SpineSkin> SpineSkeletonDataResource::get_default_skin() const { | ||
| SPINE_CHECK(skeleton_data, nullptr) | ||
| auto skin = skeleton_data->getDefaultSkin(); | ||
| if (skin) |
There was a problem hiding this comment.
CRITICAL: Inverted null check logic
This condition is backwards. When skin is valid (non-null), you return nullptr. When skin is null, you proceed to wrap it.
Should be:
| if (skin) | |
| if (!skin) |
This bug will cause get_default_skin() to always return null when a default skin exists.
|
|
||
| void SpineSkeletonDataResource::set_animation_mixes(Array _animation_mixes) { | ||
| for (int i = 0; i < _animation_mixes.size(); i++) { | ||
| auto objectId = Object::cast_to<EncodedObjectAsID>(_animation_mixes[0]); |
There was a problem hiding this comment.
Array indexing bug: always checks index 0
The loop variable i is not used. This always checks _animation_mixes[0] instead of _animation_mixes[i], so only the first element is validated.
| auto objectId = Object::cast_to<EncodedObjectAsID>(_animation_mixes[0]); | |
| auto objectId = Object::cast_to<EncodedObjectAsID>(_animation_mixes[i]); |
| if (error != OK) return error; | ||
|
|
||
| const Ref<SpineAtlasResource> &spineAtlas = static_cast<const Ref<SpineAtlasResource> &>(p_resource); | ||
| this->clear(); |
There was a problem hiding this comment.
Potential double-free vulnerability
This copy_from implementation transfers ownership of raw pointers without self-assignment checks. If p_resource refers to this, the sequence would be:
this->clear()deletesatlasandtexture_loaderthis->atlas = spineAtlas->atlascopies from already-deleted memory
Add a self-assignment check:
if (this == p_resource.ptr()) return OK;Also consider using smart pointers or proper move semantics instead of manual pointer transfer.
| event_ref = Ref<SpineEvent>(memnew(SpineEvent)); | ||
| event_ref->set_spine_object(this, event); | ||
| } | ||
|
|
There was a problem hiding this comment.
Performance: Excessive allocations in hot path
This callback creates new Godot wrapper objects (SpineTrackEntry, SpineEvent) on every animation event, which can fire multiple times per frame. This creates significant GC pressure.
Consider implementing an object pool to reuse wrapper objects, or cache frequently-accessed wrappers.
| auto &bones = skeleton->getBones(); | ||
| result.resize((int) bones.size()); | ||
| for (int i = 0; i < result.size(); ++i) { | ||
| auto bone = bones[i]; |
There was a problem hiding this comment.
Performance: Repeated allocations in getter
This method allocates and populates a new Array with wrapped objects every time it's called. If called frequently (e.g., in update loops), this creates O(n) allocations per call.
Consider caching the result or returning a lightweight iterator/view instead of creating new wrapper objects each time.
| }; | ||
|
|
||
| static unsigned char readByte(BinaryInput *input) { | ||
| return *input->cursor++; |
There was a problem hiding this comment.
SECURITY CRITICAL: Missing bounds check - buffer overflow
This function increments the cursor pointer without verifying it stays within input->end. An attacker can craft a malicious .spskel file to read past the buffer boundary.
Add bounds checking:
static unsigned char readByte(BinaryInput *input) {
if (input->cursor >= input->end) {
// Handle error appropriately
return 0;
}
return *input->cursor++;
}This same issue affects readVarint(), readInt(), readFloat(), and readString(). All read functions need bounds validation.
| if (length == 0) { | ||
| return NULL; | ||
| } | ||
| string = spine::SpineExtension::alloc<char>(length, __FILE__, __LINE__); |
There was a problem hiding this comment.
SECURITY: Unchecked memcpy with attacker-controlled length
The length value comes from readVarint() which reads from untrusted file input. No validation ensures:
lengthis within bounds of the remaining bufferlengthwon't cause integer overflow when used in calculations- The allocation won't exhaust memory
Add validation:
static const int MAX_STRING_LENGTH = 65536; // reasonable limit
int length = readVarint(input, true);
if (length <= 0 || length > MAX_STRING_LENGTH) return NULL;
if (input->cursor + length - 1 > input->end) return NULL;
Code Review SummaryComprehensive review completed using specialized agents. Found critical issues requiring immediate attention. Critical Issues (3):
Security Concerns: Performance: See inline comments for details and suggested fixes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the spine-godot module, enabling Spine animation support in the Godot Engine. It includes the necessary C++ classes, resource loaders/savers, and build configurations. The code adds support for rendering Spine animations, handling animation states, and manipulating bones and attachments. The review focuses on identifying potential issues related to resource management, error handling, and adherence to coding conventions.
| this->source_path = spineAtlas->source_path; | ||
| this->atlas_data = spineAtlas->atlas_data; | ||
| this->normal_map_prefix = spineAtlas->normal_map_prefix; | ||
| this->specular_map_prefix = spineAtlas->specular_map_prefix; | ||
| this->textures = spineAtlas->textures; | ||
| this->normal_maps = spineAtlas->normal_maps; | ||
| this->specular_maps = spineAtlas->specular_maps; | ||
| emit_signal(SNAME("skeleton_file_changed")); | ||
|
|
||
| return OK; | ||
| } | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The atlas and texture_loader are assigned directly from the spineAtlas object. This means that the spineAtlas object will be deallocated, and the atlas and texture_loader will be pointing to invalid memory. Consider using a copy constructor to copy the atlas and texture_loader objects to prevent potential dangling pointers.
| char *GodotSpineExtension::_readFile(const spine::String &path, int *length) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
The _readFile function currently returns NULL. This means that the Spine runtime will not be able to load external files, such as shader programs, directly. This could limit the functionality of the module. Consider implementing file reading functionality to support loading external resources.
| for (int i = 0; i < (int) spineEvents.size(); ++i) { | ||
| auto event_ref = memnew(SpineEvent); | ||
| event_ref->set_spine_object(skeleton->get_spine_owner(), spineEvents[i]); | ||
| events.append(event_ref); | ||
| } |
There was a problem hiding this comment.
The event_ref is allocated using memnew but is then appended to the events array. The ownership of this event_ref is transferred to the events array. The event_ref should not be manually deallocated, as the Godot engine will handle the memory management of the array elements. Consider removing the manual deallocation to prevent potential double frees.
| SpineAnimationState::~SpineAnimationState() { | ||
| delete animation_state; |
There was a problem hiding this comment.
The animation_state is allocated using new but is then deallocated in the destructor. It is important to ensure that the animation_state is not deallocated elsewhere, as this would lead to a double free. Consider using a smart pointer to manage the memory of animation_state to prevent potential double frees.
| delete atlas; | ||
| delete texture_loader; |
There was a problem hiding this comment.
The atlas and texture_loader are allocated using new but are then deallocated in the destructor. It is important to ensure that the atlas and texture_loader are not deallocated elsewhere, as this would lead to a double free. Consider using a smart pointer to manage the memory of atlas and texture_loader to prevent potential double frees.
| SpineAttachment::~SpineAttachment() { | ||
| if (get_spine_object()) get_spine_object()->dereference(); | ||
| } |
There was a problem hiding this comment.
The spine_object is dereferenced in the destructor. It's important to ensure that the spine_object is not deallocated elsewhere, as this would lead to a double free. Consider using a smart pointer to manage the memory of spine_object to prevent potential double frees.
| if (!EMPTY(animation_name)) { | ||
| auto entry = animation_state->setAnimation(track_index, SPINE_STRING(animation_name), loop); | ||
| if (should_set_mix) entry->setMixDuration(mix_duration); |
There was a problem hiding this comment.
The entry variable is assigned the result of animation_state->setAnimation, but the return value is not checked for errors. If setAnimation fails, entry will be null, and dereferencing it will cause a crash. Consider adding a check to ensure that entry is not null before dereferencing it.
| auto entry = animation_state->setEmptyAnimation(track_index, should_set_mix ? mix_duration : 0); | ||
| entry->setTrackEnd(FLT_MAX); | ||
| if (debug) print_line(String("Setting empty animation with mix_duration {0} on track {1} on {2}").format(varray(mix_duration, track_index, sprite->get_name())).utf8().ptr()); |
There was a problem hiding this comment.
The entry variable is assigned the result of animation_state->setEmptyAnimation, but the return value is not checked for errors. If setEmptyAnimation fails, entry will be null, and dereferencing it will cause a crash. Consider adding a check to ensure that entry is not null before dereferencing it.
| opts.Add(BoolVariable("builtin_zstd", "Use the built-in Zstd library", True)) | ||
|
|
||
| opts.Add(BoolVariable("spx", "Enable the spx library", True)) | ||
| opts.Add(BoolVariable("spine_godot", "Enable the spine-godot library", True)) |
There was a problem hiding this comment.
This line adds a boolean variable to enable the spine-godot library. It's good to see this feature is configurable during the build process.
| void SpineAnimationTrack::_notification(int what) { | ||
| switch (what) { | ||
| case NOTIFICATION_PARENTED: { | ||
| sprite = Object::cast_to<SpineSprite>(get_parent()); | ||
| if (sprite) | ||
| #if VERSION_MAJOR > 3 | ||
| sprite->connect(SNAME("before_animation_state_update"), callable_mp(this, &SpineAnimationTrack::update_animation_state)); | ||
| #else | ||
| sprite->connect(SNAME("before_animation_state_update"), this, SNAME("update_animation_state")); | ||
| #endif | ||
| NOTIFY_PROPERTY_LIST_CHANGED(); | ||
| break; | ||
| } | ||
| case NOTIFICATION_READY: { | ||
| setup_animation_player(); | ||
| break; | ||
| } | ||
| case NOTIFICATION_UNPARENTED: { | ||
| if (sprite) { | ||
| #if VERSION_MAJOR > 3 | ||
| sprite->disconnect(SNAME("before_animation_state_update"), callable_mp(this, &SpineAnimationTrack::update_animation_state)); | ||
| #else | ||
| sprite->disconnect(SNAME("before_animation_state_update"), this, SNAME("update_animation_state")); | ||
| #endif | ||
| sprite = nullptr; | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
The _notification function connects and disconnects the update_animation_state signal. It's important to ensure that the signal is properly disconnected when the node is unparented to avoid potential dangling connections. Consider adding a check to ensure that the sprite is valid before attempting to disconnect the signal.
1 participant
No description provided.