feat(particle): add transform to shape module#2965
feat(particle): add transform to shape module#2965cptbtptpbcptdtptp merged 20 commits intogalacean:dev/2.0from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded transform (position, rotation, scale) support to particle emission shapes. Changes
Sequence DiagramsequenceDiagram
participant Emitter as ParticleRenderer<br/>(Emitter)
participant Shape as BaseShape<br/>(Shape)
participant SubShape as Concrete Shape<br/>(e.g., BoxShape)
participant Transform as Transform<br/>Logic
participant Output as Particle<br/>(Output)
Emitter->>Shape: _generatePositionAndDirection()
Shape->>SubShape: _generateLocalPositionAndDirection()
SubShape->>SubShape: Generate local position & direction
SubShape-->>Shape: Return local values
alt Has Transform
Shape->>Transform: _getMatrix() (scale, rotation, position)
Transform-->>Shape: Affine transform matrix
Shape->>Transform: Apply matrix to position & direction
Transform-->>Shape: Transformed position & direction
else No Transform
Shape->>Shape: Use local values as-is
end
Shape-->>Emitter: Final position & direction
Emitter->>Output: Emit particle with transformed values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2965 +/- ##
===========================================
- Coverage 77.54% 77.10% -0.45%
===========================================
Files 900 901 +1
Lines 98724 98869 +145
Branches 9802 9848 +46
===========================================
- Hits 76560 76228 -332
- Misses 21998 22471 +473
- Partials 166 170 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/particle/ParticleGenerator.ts (1)
281-283: Please add automated coverage for the new transformed shape path.These lines are now the sole integration points for translated/rotated shapes in both emission and bounds, but the PR notes those scenarios are still manual-only. A small regression test for
shape.positionand another forshape.rotation/rotated bounds would make this change much safer to evolve.Also applies to: 1231-1232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleGenerator.ts` around lines 281 - 283, Add automated unit tests that exercise the transformed-shape code path triggered by shape._generateTransformedPositionAndDirection: create a Shape with a non-zero shape.position and assert emitted particle positions are translated by that offset, and create a Shape with a non-zero shape.rotation and/or rotated bounds and assert emitted particle positions/directions reflect the rotation (use ParticleGenerator or the emission code that calls shape._generateTransformedPositionAndDirection, passing an emission._shapeRand and a fixed playTime, position, direction, and positionScale). In each test seed the RNG (emission._shapeRand) or use deterministic inputs, call the same method path used in production (so the multiply(positionScale) and normalize()/multiply(positionScale) lines execute), and assert the resulting position and direction vectors match expected transformed values (translation and rotation) within a tight tolerance.packages/core/src/particle/modules/shape/BaseShape.ts (1)
23-30: Harden the cached rotation state against clone rehydration paths.
_rotationQuaternionis derived from_rotation, but_rotationDirtystartsfalseand both are marked@ignoreClone. During cloning,_rotationis deep cloned as a new Vector3 instance, but the callback binding established in the constructor (this._rotation._onValueChanged = this._onRotationChanged.bind(this)) is not replayed on the cloned Vector3. This means a cloned shape's rotation cache can become stale: if the cloned shape's rotation is modified, the cache won't be invalidated because the callback is missing.Initializing
_rotationDirty = trueor implementing a_cloneTohook to rebind the vector callback would make this robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/shape/BaseShape.ts` around lines 23 - 30, The cloned shape can have a stale rotation cache because _rotation is deep-cloned but its callback binding (this._rotation._onValueChanged = this._onRotationChanged.bind(this)) is not reapplied; fix by either (A) initialize private _rotationDirty = true so any clone recomputes the quaternion on first use (update the field on BaseShape), or (B) implement a _cloneTo(clone: BaseShape) hook that rebinds the cloned vector's callback (call clone._rotation._onValueChanged = clone._onRotationChanged.bind(clone)) and ensure clone._rotationDirty is set appropriately; reference the fields _rotation, _rotationQuaternion, _rotationDirty and the method _onRotationChanged when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/particle/modules/shape/BaseShape.ts`:
- Around line 23-30: The cloned shape can have a stale rotation cache because
_rotation is deep-cloned but its callback binding
(this._rotation._onValueChanged = this._onRotationChanged.bind(this)) is not
reapplied; fix by either (A) initialize private _rotationDirty = true so any
clone recomputes the quaternion on first use (update the field on BaseShape), or
(B) implement a _cloneTo(clone: BaseShape) hook that rebinds the cloned vector's
callback (call clone._rotation._onValueChanged =
clone._onRotationChanged.bind(clone)) and ensure clone._rotationDirty is set
appropriately; reference the fields _rotation, _rotationQuaternion,
_rotationDirty and the method _onRotationChanged when applying the change.
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 281-283: Add automated unit tests that exercise the
transformed-shape code path triggered by
shape._generateTransformedPositionAndDirection: create a Shape with a non-zero
shape.position and assert emitted particle positions are translated by that
offset, and create a Shape with a non-zero shape.rotation and/or rotated bounds
and assert emitted particle positions/directions reflect the rotation (use
ParticleGenerator or the emission code that calls
shape._generateTransformedPositionAndDirection, passing an emission._shapeRand
and a fixed playTime, position, direction, and positionScale). In each test seed
the RNG (emission._shapeRand) or use deterministic inputs, call the same method
path used in production (so the multiply(positionScale) and
normalize()/multiply(positionScale) lines execute), and assert the resulting
position and direction vectors match expected transformed values (translation
and rotation) within a tight tolerance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f28acea-0f9a-45a5-84ab-e0d12fd47470
📒 Files selected for processing (2)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/modules/shape/BaseShape.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/particle/modules/shape/BaseShape.ts`:
- Around line 23-30: The clone lifecycle is broken because deep-cloned Vector3
fields (_position and _rotation) lose their _onValueChanged hooks and rotation
cache fields (_rotationDirty and _rotationQuaternion) are excluded from cloning;
implement a _cloneTo(target: BaseShape) method on BaseShape that rebinds the
callbacks on the cloned target (set target._position._onValueChanged to
target._updateManager.dispatch.bind(target._updateManager) and
target._rotation._onValueChanged to target._onRotationChanged.bind(target)) and
set target._rotationDirty = true so the cloned instance recomputes
_rotationQuaternion; reference the existing symbols _position, _rotation,
_rotationDirty, _rotationQuaternion, _onValueChanged, _updateManager.dispatch
and _onRotationChanged when adding the hook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e861800c-0b64-43fb-aee1-f4d6b1c9b2d9
📒 Files selected for processing (1)
packages/core/src/particle/modules/shape/BaseShape.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/particle/modules/shape/BaseShape.ts`:
- Around line 136-165: Current code uses _hasShapeTransform() as a single gate
which causes unnecessary quaternion transforms and bounding-box rotations when
only translation (_position) is set; change each helper to split rotation vs
translation checks: in _generateTransformedPositionAndDirection call
_generatePositionAndDirection first, then if a rotation is present (e.g. replace
or augment _hasShapeTransform() with a rotation-specific check or test whether
_getRotationQuaternion() returns a non-identity), apply the quaternion via
_getRotationQuaternion() and Vector3.transformByQuat to position and direction,
and after that if a translation (_position) is non-zero add this._position to
position; in _getTransformedPositionRange call _getPositionRange, then only call
_rotateBoundingBox(outMin,outMax) when rotation is present and afterward add
this._position to outMin/outMax only when translation is present; in
_getTransformedDirectionRange call _getDirectionRange and only call
_rotateBoundingBox(outMin,outMax) when rotation is present (do not add
translation to directions). Use the existing symbols
(_generatePositionAndDirection, _getRotationQuaternion, _rotateBoundingBox,
_position, _getPositionRange, _getDirectionRange) to locate and implement these
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36601127-7868-49c2-bbae-21c4d2bb2904
📒 Files selected for processing (2)
packages/core/src/particle/modules/shape/BaseShape.tspackages/core/src/particle/modules/shape/MeshShape.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add Vector3 scale (default 1,1,1) to shape transform, aligning with Unity ParticleSystem.ShapeModule's full position/rotation/scale TRS. Scale is applied before rotation in emission and bounds calculation.
Use arrow properties for callbacks and @ignoreClone + manual copyFrom in _cloneTo, consistent with Transform's clone pattern.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Shape transform logic is now inside the original _generatePositionAndDirection / _getPositionRange / _getDirectionRange methods. Subclass implementations renamed to _generateLocalPositionAndDirection etc. ParticleGenerator callers unchanged.
Replace separate scale multiply + quaternion rotate with a cached Matrix3x3 RS matrix. Both emission and Arvo AABB transform share the same matrix, rebuilt only when position/rotation/scale changes.
Replace Matrix3x3 with Matrix, use affineTransformation to build TRS matrix, transformToVec3 for position and transformNormal for direction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/core/particle/ParticleShapeTransform.test.ts (1)
6-9: Reset the mutable test fixtures per case.
Randis stateful, so sharing it across the whole suite makes generation assertions order-dependent. RecreaterandalongsidepositionanddirectioninbeforeEach()to keep the tests isolated.♻️ Small cleanup
describe("ParticleShapeTransform", function () { - const position = new Vector3(); - const direction = new Vector3(); - const rand = new Rand(0, 1234); + let position: Vector3; + let direction: Vector3; + let rand: Rand; const epsilon = 1e-5; + + beforeEach(() => { + position = new Vector3(); + direction = new Vector3(); + rand = new Rand(0, 1234); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/particle/ParticleShapeTransform.test.ts` around lines 6 - 9, The test suite currently shares a single Rand instance across cases which makes tests order-dependent; move creation of the mutable fixtures into a beforeEach block so each test gets a fresh Rand and fresh Vector3s. Specifically, in ParticleShapeTransform.test.ts, initialize position, direction and rand inside beforeEach (recreating Rand with new Rand(0, 1234)), rather than as top-level constants, so tests using position, direction and rand are isolated and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/particle/modules/shape/BaseShape.ts`:
- Around line 147-174: The direction AABB logic is inconsistent with generation:
_generatePositionAndDirection applies the full transform (including scale) to
direction then normalizes, but _getDirectionRange/_transformBoundingBox only
uses the scaled AABB, causing under-estimated ranges when scale != 1. Fix by
making direction transforms scale-agnostic: either (a) apply rotation-only part
of the matrix to directions (extract rotation from _getMatrix() or zero out
scale in the matrix) in both _generatePositionAndDirection and in the direction-
range path so _getLocalDirectionRange/_getDirectionRange use the same
rotation-only transform, or (b) compute the direction range by transforming the
local direction AABB corners with the full matrix and normalizing each
transformed vector before expanding the outMin/outMax; update
_getDirectionRange, _generatePositionAndDirection, and any helper like
_transformBoundingBox or add a new _transformDirectionBoundingBox to ensure both
generation and reported ranges match.
---
Nitpick comments:
In `@tests/src/core/particle/ParticleShapeTransform.test.ts`:
- Around line 6-9: The test suite currently shares a single Rand instance across
cases which makes tests order-dependent; move creation of the mutable fixtures
into a beforeEach block so each test gets a fresh Rand and fresh Vector3s.
Specifically, in ParticleShapeTransform.test.ts, initialize position, direction
and rand inside beforeEach (recreating Rand with new Rand(0, 1234)), rather than
as top-level constants, so tests using position, direction and rand are isolated
and deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb4ab1c8-fb7f-4ea3-9b82-e79bb4d67c55
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-shape-transform.jpgis excluded by!**/*.jpg
📒 Files selected for processing (11)
e2e/case/particleRenderer-shape-transform.tse2e/config.tspackages/core/src/particle/modules/shape/BaseShape.tspackages/core/src/particle/modules/shape/BoxShape.tspackages/core/src/particle/modules/shape/CircleShape.tspackages/core/src/particle/modules/shape/ConeShape.tspackages/core/src/particle/modules/shape/HemisphereShape.tspackages/core/src/particle/modules/shape/MeshShape.tspackages/core/src/particle/modules/shape/SphereShape.tstests/src/core/particle/ParticleBoundingBox.test.tstests/src/core/particle/ParticleShapeTransform.test.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/config.ts
- tests/src/core/particle/ParticleBoundingBox.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/modules/shape/MeshShape.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
hhhhkrx
force-pushed
the
feat/particle-shape-transform
branch
from
8128fc9 to
d53a708
Compare
Replace center+extent+abs approach with direct min/max Arvo method. Eliminates getCenter/getExtent/subtract/add intermediate steps since BoundingBox already stores min/max. Mathematically equivalent with slightly better floating-point precision.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Change _getPositionRange to accept BoundingBox directly, eliminating intermediate copies. Direction range keeps RS-only Arvo method. Also optimize BoundingBox.transform with Arvo min/max method.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- _getPositionRange accepts BoundingBox directly, reuses BoundingBox.transform - BoundingBox.transform adds zero-guard to avoid 0 * Infinity = NaN - Update tests for new _getPositionRange signature and edge cases
hhhhkrx
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // @ts-ignore | ||
| scale._onValueChanged = target._onTransformChanged; | ||
| target._transformDirty = true; | ||
| } |
There was a problem hiding this comment.
如果将 this._position，this._rotation 和 this._scale 设置为 deepclone ，这十七行代码可以都干掉。
| @ignoreClone | ||
| private _scale = new Vector3(1, 1, 1); | ||
| @ignoreClone | ||
| private _rotationQuaternion = new Quaternion(); |
There was a problem hiding this comment.
这个你只做临时变量的话，可以使用静态变量 static _rotationQuaternion 代替
… simplify _cloneTo
| return this._matrix; | ||
| } | ||
|
|
||
| private _hasShapeTransform(): boolean { |
There was a problem hiding this comment.
_hasShapeTransform 改成一个 boolean 值，在 _onTransformChanged 中初始化。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| static _tempVector30 = new Vector3(); | ||
| /** @internal */ | ||
| static _tempVector31 = new Vector3(); | ||
| private static _tempQuaternion = new Quaternion(); |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| # | 问题 | 状态 |
|---|---|---|
| 1 | clone 后 _onValueChanged 回调丢失、_rotationDirty 默认 false |
✅ 已修复（@deepClone + constructor 绑定回调，clone 时 copyFrom 触发回调链自动设置 dirty 和 _hasShapeTransform） |
| 2 | setter dispatch 建议 | ✅ 已关闭（Vector3.copyFrom 内部触发 _onValueChanged） |
| 3 | _rotateBoundingBox 可直接从四元数提取旋转矩阵列 |
✅ 已关闭（最终方案用 Matrix.affineTransformation + 矩阵 elements 直接取） |
总结
为粒子 BaseShape 添加 position、rotation 和 scale 局部变换（TRS），完整对齐 Unity ParticleSystem.ShapeModule。
自上次 review 以来新增 3 个 commit，均来自 cptbtptpbcptdtptp 的 inline review 反馈：
- 22dbc68 — 将
_hasShapeTransform()方法改为_hasShapeTransformboolean 缓存字段，在_onTransformChanged回调中更新。移除_cloneTo——因为@deepClone的copyFrom会触发_onValueChanged→_onTransformChanged，自动设置_transformDirty = true和正确的_hasShapeTransform值，_cloneTo不再需要。经 CloneManager 源码验证：target 已有 Vector3（constructor 中创建并绑定回调），@deepClone走copyFrom路径，回调不丢失。 - 4328fe5 + 59946be — 将静态声明移到类顶部，符合 cptbtptpbcptdtptp 要求的代码风格。
最终设计简洁完整：
- 零子类改动：基类 wrapper 包装
_generateLocalPositionAndDirection等抽象方法 - 零开销快路径：
_hasShapeTransformboolean 缓存，默认false直接短路 - 矩阵缓存：
_transformDirty标记，仅 TRS 变化时重建矩阵 - clone 安全：
@deepClone+ constructor 绑定回调，copyFrom自动触发_onTransformChanged设置 dirty 和_hasShapeTransform，无需_cloneTo - position range 复用
BoundingBox.transform：统一 Arvo AABB 变换 - BoundingBox.transform 零值守卫：避免
0 * Infinity = NaN
问题
无新问题。
LGTM，代码质量高，已合入。
3 participants
Summary
position(Vector3) androtation(Vector3, euler degrees) properties toBaseShape, enabling per-shape local transform offset for particle emission_generateTransformedPositionAndDirection,_getTransformedPositionRange,_getTransformedDirectionRange)Test plan
shape.position.set(2, 0, 0)and verify emission offsetshape.rotation.set(0, 0, 90)and verify rotated emission areaSummary by CodeRabbit
New Features
Tests