-
-
Notifications
You must be signed in to change notification settings - Fork 315
feat: Decouple properties in timeline animations & support parallel animation #814
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.
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
base: dev
Are you sure you want to change the base?
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/webgal/src/Core/controller/stage/pixi/PixiController.ts
Outdated
Show resolved
Hide resolved
packages/webgal/src/Core/controller/stage/pixi/animations/generateTransformAnimationObj.ts
Outdated
Show resolved
Hide resolved
|
添加了setAnimation的-parallel支持,并添加了一个演示平行变换动画使用的demo剧本文件 |
|
你好,由于涉及到多项可能影响演出控制逻辑的修改,本 PR 可能需要更长的时间审阅和验证。感谢你的付出! |
|
Warning 有相当多 lint 警告,请安装 Eslint 以匹配 WebGAL 项目规范 Caution 构建无法通过检查,请运行 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
虽然感觉为了规避 unmountPerform(performName: string) 而新加入 isParallel 有点把演出系统搞复杂了,但从功能上看确实成功的实现了平行动画。虽然把 ITransform 的所有属性变可选看着有点冒险,但因为出场时肯定已经融合了一遍完整的 baseTransform,所以存档也没什么问题,就是可能以后要注意一下 baseTransform 得写全。
其中最需要修改的应该是 lint 和构建时 tsc 检查不过需要更改,其他可改可不改吧。
…tial runtime error caused by undefined parameters reported by review bot
650fcfd to
d661a76
Compare
HardyNLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
简介
下文
- 简称
setAnimationsetTransformsetTempAnimation为 Timeline 动画演出
改造演出系统
此 PR 为 Timeline 动画演出命令添加了 -parallel 参数, 实现了对同目标同时使用两个 timeline 动画, 同时对演出系统做了些改造;
-parallel 与其说是平行动画, 原理上不如说是"不打断上一个相同目标的动画", 几乎所有的改造都是围绕这点进行;
这是因为在此之前, 如果有作用于同目标的多个动画, 前面的动画必须被强制完成, 然后开始最靠后的动画, 此 PR 从几个方面着手:
- 有
-parallel参数时, 不使用unmountPerform打断上一个同目标的动画 - IPerform 倒计时结束, 不使用
unmountPerform(performName: string)结束演出
(1). 因为这有可能错误地把另一个同目标的动画演出结束, 所有的动画类演出performName都是animation-${target}
(2). 新造了一个函数softUnmountPerform(perform: IPerform), 倒计时结束直接结束自身 IPerform新增了一个参数isParallel参数
这是由于arrangeNewPerform会检查是否有同名演出, 有就干掉所有同名演出, 新增该参数是为了避开这个问题
理解这几点, 就容易理解绝大部分更改的出发点, 这么做似乎也没什么问题
改造 ITransform
剩下的更改是把 ITransform 所有参数都改成可选的了;
因为此 PR 之前在生成 timeline 对象前, 会先融合一遍现有的 transform, 出于 一些原因 , 现有的 transform 永远是完整的, 所以这会导致无法使用平行动画, 因为一个 timeline 动画会 set 所有的参数, 平行动画需要的是按需 set 参数
这是否会导致一些问题
从存档的角度出发应该没什么问题, 因为对象在出场时必先融合一遍完整的 transform, 所有存档应该永远是完整的;
唯一需要注意的是, 以后再往 ITransform 加属性, 一定要把 baseTransform 写完整, 现在参数可选了之后是不会有语法提示的;
实际测试了一段时间, 在编辑器那边也没有问题, 我觉得此 PR 可被合并
#807
介绍
对时间线动画中的变换参数进行解耦,不再捆绑更新所有参数。并添加了setTransform、setTempAnimation、setAnimation指令的平行变换动画支持,允许对同一个目标同时进行多条针对不同变换参数的变换动画指令
更改
注意事项