feat: Decouple properties in timeline animations & support parallel animation #814
Conversation
Summary of ChangesHello @TremblingMoeNew, 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 animation and transformation system within WebGAL by introducing a more modular and flexible approach. The core improvement lies in decoupling how animation effects are applied, allowing for precise, partial updates to an object's transform properties rather than wholesale replacements. This change not only prevents unintended overwrites but also paves the way for more sophisticated animation control. Additionally, the PR adds crucial support for parallel animations, enabling multiple visual effects to run simultaneously, and refines animation naming for better system stability. 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
|
TremblingMoeNew
changed the title
![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 a feature to "ecouple" animations, allowing for more flexible and parallel animations. A significant change is making all properties of the ITransform interface optional, which provides more flexibility but also introduces risks if not handled carefully. My review focuses on several critical issues where undefined values are not handled correctly, which could lead to runtime errors. I've provided suggestions to make the code more robust against these undefined values, particularly when using lodash functions like pickBy and omitBy, and when dealing with non-null assertions. Other changes, such as using uuid for animation names and refactoring for efficiency, are good improvements.
| const targetScale = pickBy(targetSetEffect.transform.scale, (source, key)=> unionScaleKeys.has(key)) | ||
| const targetPosition = pickBy(targetSetEffect.transform.position, (source, key)=> unionPositionKeys.has(key)) |
There was a problem hiding this comment.
The properties scale and position on targetSetEffect.transform are optional and can be undefined. Calling pickBy with undefined as the first argument will cause a runtime error.
You should provide a fallback empty object to pickBy to prevent this.
| const targetScale = pickBy(targetSetEffect.transform.scale, (source, key)=> unionScaleKeys.has(key)) | |
| const targetPosition = pickBy(targetSetEffect.transform.position, (source, key)=> unionPositionKeys.has(key)) | |
| const targetScale = pickBy(targetSetEffect.transform.scale || {}, (source, key)=> unionScaleKeys.has(key)) | |
| const targetPosition = pickBy(targetSetEffect.transform.position || {}, (source, key)=> unionPositionKeys.has(key)) |
| if (target.scale) Object.assign(targetScale!, omitBy(source.scale,isUndefined)); | ||
| if (target.position) Object.assign(targetPosition!, omitBy(source.position,isUndefined)); |
There was a problem hiding this comment.
The properties scale and position on source are optional and can be undefined. Calling omitBy with undefined as the first argument will cause a runtime error.
You should provide a fallback empty object to omitBy to handle cases where these properties are not present on the source object.
| if (target.scale) Object.assign(targetScale!, omitBy(source.scale,isUndefined)); | |
| if (target.position) Object.assign(targetPosition!, omitBy(source.position,isUndefined)); | |
| if (target.scale) Object.assign(targetScale!, omitBy(source.scale || {},isUndefined)); | |
| if (target.position) Object.assign(targetPosition!, omitBy(source.position || {},isUndefined)); |
| const targetScale = pickBy(targetEffect!.transform!.scale, (source, key)=> has(applyFrame.scale, key)) | ||
| const targetPosition = pickBy(targetEffect!.transform!.position, (source, key)=> has(applyFrame.position, key)) | ||
| const effectWithDuration = { ...pickBy(targetEffect!.transform!, (source, key)=> has(applyFrame, key) ), duration: 0, ease }; | ||
| effectWithDuration.scale = targetScale | ||
| effectWithDuration.position = targetPosition |
There was a problem hiding this comment.
There are multiple potential runtime errors in this block due to unsafe handling of optional properties. The ITransform interface properties are all optional, so targetEffect.transform, targetEffect.transform.scale, targetEffect.transform.position, applyFrame.scale, and applyFrame.position can all be undefined.
- Using non-null assertions (
!) ontargetEffect.transformis unsafe, asIEffectdefinestransformas optional. - Calling
pickByorhaswithundefinedwill cause a crash.
You should add defensive checks and fallbacks for all these optional properties.
| const targetScale = pickBy(targetEffect!.transform!.scale, (source, key)=> has(applyFrame.scale, key)) | |
| const targetPosition = pickBy(targetEffect!.transform!.position, (source, key)=> has(applyFrame.position, key)) | |
| const effectWithDuration = { ...pickBy(targetEffect!.transform!, (source, key)=> has(applyFrame, key) ), duration: 0, ease }; | |
| effectWithDuration.scale = targetScale | |
| effectWithDuration.position = targetPosition | |
| const currentTransform = targetEffect?.transform || {}; | |
| const targetScale = pickBy(currentTransform.scale || {}, (source, key)=> has(applyFrame.scale || {}, key)); | |
| const targetPosition = pickBy(currentTransform.position || {}, (source, key)=> has(applyFrame.position || {}, key)); | |
| const effectWithDuration = { ...pickBy(currentTransform, (source, key)=> has(applyFrame, key) ), duration: 0, ease }; | |
| effectWithDuration.scale = targetScale; | |
| effectWithDuration.position = targetPosition; |
| if (transform!.scale) Object.assign(targetScale!, omitBy(transform!.scale,isUndefined)); | ||
| if (transform!.position) Object.assign(targetPosition!, omitBy(transform!.position,isUndefined)); | ||
| Object.assign(state.effects[effectIndex]!.transform!, omitBy(transform,isUndefined)) |
There was a problem hiding this comment.
The transform property on the updateEffect payload is optional and can be undefined. Using non-null assertions (!) on it will lead to a runtime error if transform is not provided in the action payload. Additionally, Object.assign on line 131 is not guarded and will fail if transform is undefined.
You should use optional chaining (?.) and guard the Object.assign call to safely handle cases where transform is undefined.
| if (transform!.scale) Object.assign(targetScale!, omitBy(transform!.scale,isUndefined)); | |
| if (transform!.position) Object.assign(targetPosition!, omitBy(transform!.position,isUndefined)); | |
| Object.assign(state.effects[effectIndex]!.transform!, omitBy(transform,isUndefined)) | |
| if (transform?.scale) Object.assign(targetScale!, omitBy(transform.scale,isUndefined)); | |
| if (transform?.position) Object.assign(targetPosition!, omitBy(transform.position,isUndefined)); | |
| if (transform) Object.assign(state.effects[effectIndex]!.transform!, omitBy(transform,isUndefined)); |
…tial runtime error caused by undefined parameters reported by review bot
|
添加了setAnimation的-parallel支持，并添加了一个演示平行变换动画使用的demo剧本文件 |
|
你好，由于涉及到多项可能影响演出控制逻辑的修改，本 PR 可能需要更长的时间审阅和验证。感谢你的付出！ |
|
Warning 有相当多 lint 警告，请安装 Eslint 以匹配 WebGAL 项目规范 Caution 构建无法通过检查，请运行 |
| export function getAnimateDurationFromObj(animation: IUserAnimation) { | ||
| let duration = 0; | ||
| animation.effects.forEach((e) => { | ||
| duration += e.duration; | ||
| }); | ||
| return duration; | ||
| } | ||
|
|
There was a problem hiding this comment.
似乎不太需要这个函数，原来的 getAnimateDuration 可以很好地完成职责。或者你确实喜欢这么写的话，也可以和 getAnimateDuration 融合一下。
| } | ||
| else if (transform){ | ||
| const targetScale = state.effects[effectIndex].transform.scale || {}; | ||
| const targetPosition = state.effects[effectIndex].transform.position || {}; | ||
| if (transform.scale) Object.assign(targetScale, omitBy(transform.scale,isUndefined)); | ||
| if (transform.position) Object.assign(targetPosition, omitBy(transform.position,isUndefined)); | ||
| Object.assign(state.effects[effectIndex].transform, omitBy(transform,isUndefined)) | ||
| state.effects[effectIndex].transform.scale = targetScale; | ||
| state.effects[effectIndex].transform.position = targetPosition; |
There was a problem hiding this comment.
应付一下 tsc 检查
| } | |
| else if (transform){ | |
| const targetScale = state.effects[effectIndex].transform.scale || {}; | |
| const targetPosition = state.effects[effectIndex].transform.position || {}; | |
| if (transform.scale) Object.assign(targetScale, omitBy(transform.scale,isUndefined)); | |
| if (transform.position) Object.assign(targetPosition, omitBy(transform.position,isUndefined)); | |
| Object.assign(state.effects[effectIndex].transform, omitBy(transform,isUndefined)) | |
| state.effects[effectIndex].transform.scale = targetScale; | |
| state.effects[effectIndex].transform.position = targetPosition; | |
| } else if (transform) { | |
| const targetScale = state.effects[effectIndex].transform!.scale || {}; | |
| const targetPosition = state.effects[effectIndex].transform!.position || {}; | |
| if (transform.scale) Object.assign(targetScale, omitBy(transform.scale, isUndefined)); | |
| if (transform.position) Object.assign(targetPosition, omitBy(transform.position, isUndefined)); | |
| Object.assign(state.effects[effectIndex].transform!, omitBy(transform, isUndefined)); | |
| state.effects[effectIndex].transform!.scale = targetScale; | |
| state.effects[effectIndex].transform!.position = targetPosition; |
| WebGAL.animationManager.addAnimation(newAnimation); | ||
| const animationDuration = getAnimateDuration(animationName); | ||
|
|
||
| const animationDuration = getAnimateDurationFromObj(newAnimation); |
There was a problem hiding this comment.
实际上原来的函数就可以正常使用，可以看一下上一句的
WebGAL.animationManager.addAnimation(newAnimation);和 getAnimateDuration 的实现
setTempAnimation 那边的改动也是同理
| PixiStage.assignTransform(target?.pixiContainer, assignValue); | ||
| if (target?.pixiContainer) { | ||
| if (!isUndefined(scale.x)) { | ||
| if (scale && target?.pixiContainer) { |
There was a problem hiding this comment.
其实下面的
!isUndefined(scale?.x)已经包含了对 scale 的判 undefined，不过这样也行。
3 participants
#807
介绍
对时间线动画中的变换参数进行解耦，不再捆绑更新所有参数。并添加了setTransform、setTempAnimation、setAnimation指令的平行变换动画支持，允许对同一个目标同时进行多条针对不同变换参数的变换动画指令
更改
注意事项