Skip to content

Fix spine callback issues #18672

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

Merged
dumganhar merged 6 commits into from
Apr 28, 2025
Merged

Fix spine callback issues #18672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69d0474
Fix spine issues:
bofeng-song Apr 27, 2025
3d612cb
refine
bofeng-song Apr 27, 2025
a79098b
refine
bofeng-song Apr 27, 2025
8d9f1a5
update external version
bofeng-song Apr 28, 2025
d440994
refine
bofeng-song Apr 28, 2025
0e68569
update external version
bofeng-song Apr 28, 2025
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
10 changes: 9 additions & 1 deletion native/cocos/editor-support/spine-creator-support/SkeletonAnimation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,15 @@ struct TrackEntryListeners {
};

void animationCallback(AnimationState *state, EventType type, TrackEntry *entry, Event *event) {
(static_cast<SkeletonAnimation *>(state->getRendererObject()))->cacheAnimationEvent(entry, type, event);
auto *skeletonAnimation = static_cast<SkeletonAnimation *>(state->getRendererObject());
skeletonAnimation->cacheAnimationEvent(entry, type, event);
if (type == EventType::EventType_Dispose) {
/**
* In the official implementation of Spine's AnimationState class, the animationCallback is invoked after the trackEntryCallback.
* After the AnimationState completes the EventType_Dispose callback, the TrackEntry will be reclaimed, so this event must be dispatched immediately.
*/
skeletonAnimation->dispatchEvents();
Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

Could this cause other issues if something is done in the dispose callback?

Copy link
Copy Markdown
Contributor

@gaojie111119 gaojie111119 Apr 27, 2025

Choose a reason for hiding this comment

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

8f12cbe6-f0c2-45d9-9c17-1f68f1267569
I Think that EventType_End disposeTrackEntry as same as EventType_Dispose.

Copy link
Copy Markdown
Contributor Author

@bofeng-song bofeng-song Apr 27, 2025

Choose a reason for hiding this comment

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

EventType_end event is cached by cacheAnimationEvent, while EventType_Dispose is dispatched it will dispatch EventType_end firstly.

Copy link
Copy Markdown
Contributor Author

@bofeng-song bofeng-song Apr 27, 2025

Choose a reason for hiding this comment

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

Could this cause other issues if something is done in the dispose callback?

trackEntry will be enter _trackEntryPool only after EventType_dispose is dispatched. So, it should not cause other issues.

}
}

void trackEntryCallback(AnimationState *state, EventType type, TrackEntry *entry, Event *event) {
Expand Down
53 changes: 33 additions & 20 deletions native/cocos/editor-support/spine-wasm/spine-skeleton-instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ static void animationCallback(AnimationState *state, EventType type, TrackEntry
info.eventType = type;
info.event = event;
instance->animationEvents.add(info);

if (type == spine::EventType::EventType_Dispose) {
/**
* In the official implementation of Spine's AnimationState class, the animationCallback is invoked after the trackEntryCallback.
* After the AnimationState completes the EventType_Dispose callback, the TrackEntry will be reclaimed, so this event must be dispatched immediately.
*/
instance->dispatchEvents();
}
}
}

Expand All @@ -37,12 +45,6 @@ static void trackEntryCallback(AnimationState *state, EventType type, TrackEntry
info.eventType = type;
info.event = event;
instance->trackEvents.add(info);

if (type == EventType_Dispose) {
if (entry->getRendererObject()) {
entry->setRendererObject(nullptr);
}
}
}
}

Expand Down Expand Up @@ -135,20 +137,7 @@ void SpineSkeletonInstance::updateAnimation(float dltTime) {
_skeleton->update(dltTime);
_animState->update(dltTime);
_animState->apply(*_skeleton);
//Cache animation events then call back to JS.
auto vecAnimationEvents = animationEvents;
animationEvents.clear();
//Cache track events then call back to JS.
auto vecTrackEvents = trackEvents;
trackEvents.clear();
for (int i = 0; i < vecAnimationEvents.size(); i++) {
auto& info = vecAnimationEvents[i];
onAnimationStateEvent(info.entry, info.eventType, info.event);
}
for (int i = 0; i < vecTrackEvents.size(); i++) {
auto& info = vecTrackEvents[i];
onTrackEntryEvent(info.entry, info.eventType, info.event);
}
dispatchEvents();
}

SpineModel *SpineSkeletonInstance::updateRenderData() {
Expand Down Expand Up @@ -522,6 +511,7 @@ void SpineSkeletonInstance::onTrackEntryEvent(TrackEntry *entry, EventType type,
SpineWasmUtil::s_currentEvent = event;
spineTrackListenerCallback();
if (type == EventType_Dispose) {
entry->setRendererObject(nullptr);
_trackListenerSet.remove(entry);
}
}
Expand Down Expand Up @@ -678,4 +668,27 @@ void SpineSkeletonInstance::setSlotTexture(const spine::String &slotName, const
attachmentVertices->_textureUUID = textureUuid;
}
}
}

void SpineSkeletonInstance::dispatchEvents() {
spine::Vector<SpineEventInfo> vecAnimationEvents;
spine::Vector<SpineEventInfo> vecTrackEvents;
if (animationEvents.size() > 0) {
//Cache animation events then call back to JS.
vecAnimationEvents.addAll(animationEvents);
animationEvents.clear();
}
if (trackEvents.size() > 0) {
//Cache track events then call back to JS.
vecTrackEvents.addAll(trackEvents);
trackEvents.clear();
}
for (int i = 0; i < vecAnimationEvents.size(); i++) {
auto& info = vecAnimationEvents[i];
onAnimationStateEvent(info.entry, info.eventType, info.event);
}
for (int i = 0; i < vecTrackEvents.size(); i++) {
auto& info = vecTrackEvents[i];
onTrackEntryEvent(info.entry, info.eventType, info.event);
}
Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

Optimize the code by checking whether the original vector is emtpy to avoid creating an empty vector and copying the empty vector.

    if (animationEvents.size() > 0) {
        // Cache animation events then call back to JS.
        auto vecAnimationEvents = animationEvents;
        animationEvents.clear();
        for (int i = 0; i < vecAnimationEvents.size(); i++) {
            auto& info = vecAnimationEvents[i];
            onAnimationStateEvent(info.entry, info.eventType, info.event);
        }
    }

    if (trackEvents.size() > 0) {
        // Cache track events then call back to JS.
        auto vecTrackEvents = trackEvents;
        trackEvents.clear();
        for (int i = 0; i < vecTrackEvents.size(); i++) {
            auto& info = vecTrackEvents[i];
            onTrackEntryEvent(info.entry, info.eventType, info.event);
        }
    }

Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

Please notice that spine::Vector doesn't provide a operator= implementation, see
https://github.com/cocos/cocos-engine/blob/v3.8.7/native/cocos/editor-support/spine/3.8/spine/Vector.h#L231

So the copy assignment will use a default one which only assigns the buffer pointer and size. If so, we need to fix that for spine::Vector.

Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

Please notice that spine::Vector doesn't provide a operator= implementation

It seems to be fine that the assignment auto vecAnimationEvents = animationEvents will trigger the spine::Vector copy constructor.

https://github.com/cocos/cocos-engine/blob/v3.8.7/native/cocos/editor-support/spine/3.8/spine/Vector.h#L49

Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

I wrote a test case that could reproduce the issue I said, but yes, it's not relevant to this PR.

    {
        spine::Vector<int> arr;
        arr.add(1);
        arr.add(2);
        arr.add(3);
        
        spine::Vector<int> arr2;
        arr2 = arr; // --> call the default assignment operator
        
        arr2[0] = 100;
        
        printf("arr --------\n");
        for (size_t i = 0; i < arr.size(); ++i) {
            printf("%d, ", arr[i]);
        }
        printf("\n");
        
        printf("arr2 --------\n");
        for (size_t i = 0; i < arr2.size(); ++i) {
            printf("%d, ", arr2[i]);
        }
        printf("\n");
        printf("\n");
    }

The output is wrong:

arr --------
100, 2, 3, 
arr2 --------
100, 2, 3,

Copy link
Copy Markdown
Contributor

@dumganhar dumganhar Apr 27, 2025

Choose a reason for hiding this comment

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

I sent a PR to spine runtime official repo ( EsotericSoftware/spine-runtimes#2828 ) to fix the copy assignment issue.

Copy link
Copy Markdown
Contributor Author

@bofeng-song bofeng-song Apr 27, 2025

Choose a reason for hiding this comment

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

Currently, the assignment type of 'operator =' is not used by spine's module

}
2 changes: 2 additions & 0 deletions native/cocos/editor-support/spine-wasm/spine-skeleton-instance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class SpineSkeletonInstance {
// Used internal for cache event
spine::Vector<SpineEventInfo> animationEvents;
spine::Vector<SpineEventInfo> trackEvents;
// Used internal for dispatch event
void dispatchEvents();
private:
void collectMeshData();

Expand Down
13 changes: 9 additions & 4 deletions native/cocos/editor-support/spine/3.8/spine/Vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,14 @@ class SP_API Vector : public SpineObject {
}
}

void addAll(Vector<T> &inValue) {
void addAll(const Vector<T> &inValue) {
ensureCapacity(this->size() + inValue.size());
for (size_t i = 0; i < inValue.size(); i++) {
add(inValue[i]);
}
}

void clearAndAddAll(Vector<T> &inValue) {
void clearAndAddAll(const Vector<T> &inValue) {
this->clear();
this->addAll(inValue);
}
Expand Down Expand Up @@ -199,6 +199,13 @@ class SP_API Vector : public SpineObject {
return _buffer;
}

Vector &operator=(const Vector &inVector) {
if (this != &inVector) {
clearAndAddAll(inVector);
}
return *this;
}

private:
size_t _size;
size_t _capacity;
Expand Down Expand Up @@ -227,8 +234,6 @@ class SP_API Vector : public SpineObject {
inline void destroy(T *buffer) {
buffer->~T();
}

// Vector &operator=(const Vector &inVector) {};
};
} // namespace spine

Expand Down
13 changes: 9 additions & 4 deletions native/cocos/editor-support/spine/4.2/spine/Vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,14 @@ namespace spine {
}
}

inline void addAll(Vector<T> &inValue) {
void addAll(const Vector<T> &inValue) {
ensureCapacity(this->size() + inValue.size());
for (size_t i = 0; i < inValue.size(); i++) {
add(inValue[i]);
}
}

inline void clearAndAddAll(Vector<T> &inValue) {
void clearAndAddAll(const Vector<T> &inValue) {
this->clear();
this->addAll(inValue);
}
Expand Down Expand Up @@ -194,6 +194,13 @@ namespace spine {
return !(lhs == rhs);
}

Vector &operator=(const Vector &inVector) {
if (this != &inVector) {
clearAndAddAll(inVector);
}
return *this;
}

inline T *buffer() {
return _buffer;
}
Expand Down Expand Up @@ -226,8 +233,6 @@ namespace spine {
inline void destroy(T *buffer) {
buffer->~T();
}

// Vector &operator=(const Vector &inVector) {};
};
}

Expand Down
2 changes: 1 addition & 1 deletion native/external-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
"type": "github",
"owner": "cocos",
"name": "cocos-engine-external",
"checkout": "v3.8.7-7"
"checkout": "v3.8.7-9"
}
}
Loading