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剧本文件 |
1 participant
#807
介绍
对时间线动画中的变换参数进行解耦，不再捆绑更新所有参数。并添加了setTransform、setTempAnimation、setAnimation指令的平行变换动画支持，允许对同一个目标同时进行多条针对不同变换参数的变换动画指令
更改
注意事项