Conversation
Summary of ChangesHello, 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 significantly enhances the WebGAL engine by adding comprehensive support for dynamic Spine animation skin changes. It provides the necessary API, integrates skin selection into game scripts, and updates the state management to allow for seamless application and persistence of different character skins during gameplay. This feature allows for greater visual customization and variety for Spine models. Highlights
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. Footnotes
|
Deploying webgal-dev with
|
| Latest commit: |
2dfafcb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d52550f6.webgal-dev.pages.dev |
| Branch Preview URL: | https://fix-spine-skins.webgal-dev.pages.dev |
![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 skin support for Spine figures in the WebGAL project. It adds a new function changeSpineSkinByKey to PixiController.ts to change the skin of a Spine figure based on a key and skin name. It also modifies spine.ts to include a function applySpineSkin that applies the skin to the Spine object. Additionally, it updates changeFigure.ts, useSetFigure.ts, stageInterface.ts, and stageReducer.ts to integrate the new skin functionality into the game's scripting and state management systems. The changes look good overall, but there are some areas where improvements can be made to enhance code clarity and prevent potential issues.
| figureSpine.interactive = false; | ||
|
|
||
| // 检查状态中是否有指定的动画 | ||
| const motionFromState = webgalStore.getState().stage.live2dMotion.find((e) => e.target === key); |
There was a problem hiding this comment.
This line reintroduces the original issue of checking the state for animations. It would be better to ensure that this logic is consistent with the new skin implementation.
| function applySpineSkin(spineObject: any, skinName: string) { | ||
| // @ts-ignore | ||
| const skeleton = spineObject.skeleton; | ||
| // @ts-ignore | ||
| const skeletonData = skeleton?.data ?? spineObject.spineData; | ||
| const skin = | ||
| // @ts-ignore | ||
| skeletonData?.findSkin?.(skinName) ?? | ||
| // @ts-ignore | ||
| skeletonData?.skins?.find((item: any) => item.name === skinName); | ||
| if (!skeleton || !skin) { | ||
| return false; | ||
| } | ||
| try { | ||
| // @ts-ignore | ||
| if (typeof skeleton.setSkinByName === 'function') { | ||
| // @ts-ignore | ||
| skeleton.setSkinByName(skinName); | ||
| } else { | ||
| // @ts-ignore | ||
| skeleton.setSkin(skin); | ||
| } | ||
| } catch (error) { | ||
| // @ts-ignore | ||
| skeleton.setSkin?.(skin); | ||
| } | ||
| // @ts-ignore | ||
| if (typeof skeleton.setSlotsToSetupPose === 'function') { | ||
| // @ts-ignore | ||
| skeleton.setSlotsToSetupPose(); | ||
| } else { | ||
| // @ts-ignore | ||
| skeleton.setupPoseSlots?.(); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This function duplicates the skin application logic found in PixiController.ts. Consolidate this logic into a single, reusable function to avoid redundancy and improve maintainability.
/**
* Applies a skin to a Spine object.
* @param spineObject The Spine object to apply the skin to.
* @param skinName The name of the skin to apply.
* @returns True if the skin was successfully applied, false otherwise.
*/
function applySpineSkin(spineObject: any, skinName: string): boolean {
if (!spineObject?.skeleton) {
logger.warn("Spine object has no skeleton.");
return false;
}
const skeleton = spineObject.skeleton;
const skeletonData = skeleton.data || spineObject.spineData;
const skin = skeletonData?.findSkin?.(skinName) || skeletonData?.skins?.find((item: any) => item.name === skinName);
if (!skin) {
logger.warn(`Spine skin not found: ${skinName}`);
return false;
}
try {
if (typeof skeleton.setSkinByName === 'function') {
skeleton.setSkinByName(skinName);
} else {
skeleton.setSkin(skin);
}
} catch (error) {
logger.error(`Failed to set skin ${skinName}:`, error);
skeleton.setSkin?.(skin);
}
if (typeof skeleton.setSlotsToSetupPose === 'function') {
skeleton.setSlotsToSetupPose();
} else {
skeleton.setupPoseSlots?.();
}
return true;
}| public changeSpineSkinByKey(key: string, skin: string) { | ||
| if (!skin) return; | ||
|
|
||
| const target = this.figureObjects.find((e) => e.key === key && !e.isExiting); | ||
| if (target?.sourceType !== 'spine') return; | ||
|
|
||
| const container = target.pixiContainer; | ||
| if (!container) return; | ||
| const sprite = container.children[0] as PIXI.Container; | ||
| if (sprite?.children?.[0]) { | ||
| const spineObject = sprite.children[0]; | ||
| // @ts-ignore | ||
| const skeleton = spineObject.skeleton; | ||
| // @ts-ignore | ||
| const skeletonData = skeleton?.data ?? spineObject.spineData; | ||
| const skinObject = | ||
| // @ts-ignore | ||
| skeletonData?.findSkin?.(skin) ?? | ||
| // @ts-ignore | ||
| skeletonData?.skins?.find((item: any) => item.name === skin); | ||
|
|
||
| if (!skeleton || !skinObject) { | ||
| logger.warn(`Spine skin not found: ${skin} on ${key}`); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // @ts-ignore | ||
| if (typeof skeleton.setSkinByName === 'function') { | ||
| // @ts-ignore | ||
| skeleton.setSkinByName(skin); | ||
| } else { | ||
| // @ts-ignore | ||
| skeleton.setSkin(skinObject); | ||
| } | ||
| } catch (error) { | ||
| // @ts-ignore | ||
| skeleton.setSkin?.(skinObject); | ||
| } | ||
|
|
||
| // @ts-ignore | ||
| if (typeof skeleton.setSlotsToSetupPose === 'function') { | ||
| // @ts-ignore | ||
| skeleton.setSlotsToSetupPose(); | ||
| } else { | ||
| // @ts-ignore | ||
| skeleton.setupPoseSlots?.(); | ||
| } | ||
|
|
||
| // @ts-ignore | ||
| spineObject.state?.apply?.(skeleton); | ||
| // @ts-ignore | ||
| skeleton.updateWorldTransform?.(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function contains a lot of duplicated code and multiple // @ts-ignore comments. It would be better to extract the skin application logic into a separate, reusable function to improve readability and maintainability. Also, consider using optional chaining instead of multiple if statements to check for nested properties.
| } catch (error) { | ||
| // @ts-ignore | ||
| skeleton.setSkin?.(skinObject); |
There was a problem hiding this comment.
The catch block only contains a // @ts-ignore comment and then attempts to set the skin. It would be better to log the error and potentially provide a more graceful fallback, or rethrow the error if it's unrecoverable.
1 participant
No description provided.