fix: assign transform to a destroyed container problem #775
Conversation
Summary of ChangesHello @MakinoharaShoko, 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 stability and robustness of the rendering and animation system by addressing issues where operations were attempted on PIXI containers that had been destroyed or were otherwise null. The changes introduce explicit nullability for container references, implement comprehensive null checks throughout the codebase, and ensure that animations are properly unmounted when their target figures are replaced. This prevents critical runtime errors and improves the overall reliability of graphical object manipulation. 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. 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
|
![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 addresses a critical issue by adding null checks to prevent operations on destroyed PIXI containers, which is a great improvement for stability. The changes are generally on the right track. I've identified one instance where the fix is incomplete and could still lead to a runtime error, and a couple of places where the code can be refactored for better clarity and to reduce redundancy. My review includes suggestions to address these points.
| if (container) | ||
| if (container.alpha < 1) { | ||
| container.alpha += increasement; | ||
| } |
There was a problem hiding this comment.
This null check for container is a good addition. However, the fix is incomplete. The access to container.blur on line 48 is still unguarded and will cause a runtime error if container is null.
To fix this, you should wrap all code that uses container within a single null check. For example:
if (target) {
const container = target.pixiContainer;
if (container) {
const baseDuration = WebGAL.gameplay.pixiStage!.frameDuration;
const currentAddOplityDelta = (duration / baseDuration) * delta;
const increasement = 1 / currentAddOplityDelta;
const decreasement = 5 / currentAddOplityDelta;
if (container.alpha < 1) {
container.alpha += increasement;
}
// @ts-ignore
if (container.blur < 5) {
// @ts-ignore
container.blur += decreasement;
}
}
}| if (bgSprite.pixiContainer) | ||
| for (const element of bgSprite.pixiContainer.children) { | ||
| element.destroy(); | ||
| } | ||
| if (bgSprite.pixiContainer) { | ||
| bgSprite.pixiContainer.destroy(); | ||
| this.figureContainer.removeChild(bgSprite.pixiContainer); | ||
| } |
There was a problem hiding this comment.
The two if (bgSprite.pixiContainer) checks are redundant. You can combine them into a single block for better readability and to avoid checking the same condition twice.
if (bgSprite.pixiContainer) {
for (const element of bgSprite.pixiContainer.children) {
element.destroy();
}
bgSprite.pixiContainer.destroy();
this.figureContainer.removeChild(bgSprite.pixiContainer);
}| if (bgSprite.pixiContainer) | ||
| for (const element of bgSprite.pixiContainer.children) { | ||
| element.destroy(); | ||
| } | ||
| if (bgSprite.pixiContainer) { | ||
| bgSprite.pixiContainer.destroy(); | ||
| this.backgroundContainer.removeChild(bgSprite.pixiContainer); | ||
| } |
There was a problem hiding this comment.
Similar to the figureObjects cleanup above, the two if (bgSprite.pixiContainer) checks here are redundant. They can be combined into a single block to improve code clarity.
if (bgSprite.pixiContainer) {
for (const element of bgSprite.pixiContainer.children) {
element.destroy();
}
bgSprite.pixiContainer.destroy();
this.backgroundContainer.removeChild(bgSprite.pixiContainer);
}
|
鼠标手动点击时, effect 被错误应用至新立绘上 changeFigure: 1/closed_mouth.png -id=aaa -next;
测试对话:aaa 入场;
setTransform: {"colorRed":0} -target=aaa -duration=5000 -keep -next;
测试对话:动画;
changeFigure: 1/closed_mouth.png -id=aaa -zIndex=5 -next;
测试对话:aaa zIndex 5|模拟同 url 改参数;
changeFigure: 2/closed_mouth.png -id=aaa -zIndex=5 -transform={"position":{"x":500}, "colorGreen":0} -next;
测试对话:换人; |
Deploying webgal-dev with
|
| Latest commit: |
674a669
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6c76d2de.webgal-dev.pages.dev |
| Branch Preview URL: | https://fix-assign-to-destroyed-cont.webgal-dev.pages.dev |
解决了 timeline 动画，在 update 或设置终止状态 setEndState 时，如果 container 正在或已经被销毁，无法正确读取属性的问题。