Particle start size support curve mode #2568
Conversation
WalkthroughThis pull request updates particle-related modules to improve clarity and functionality. In the Changes
Sequence Diagram(s)sequenceDiagram
participant PG as ParticleGenerator
participant SH as Shape
PG->>SH: _generatePositionAndDirection(playTime)
PG->>PG: _addNewParticle(..., playTime)
PG->>PG: Compute normalizedEmitAge(playTime, main module duration)
sequenceDiagram
participant PCC as ParticleCompositeCurve
participant PC as ParticleCurve
alt ParticleCurveMode.Curve
PCC->>PC: _evaluate(normalizedAge)
else ParticleCurveMode.TwoCurves
PCC->>PC: _evaluate(normalizedAge) [curveMin]
PCC->>PC: _evaluate(normalizedAge) [curveMax]
PCC->>PCC: Lerp using lerpFactor
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2568 +/- ##
==========================================
+ Coverage 68.93% 69.43% +0.49%
==========================================
Files 961 961
Lines 100271 100298 +27
Branches 8655 8667 +12
==========================================
+ Hits 69121 69637 +516
+ Misses 30890 30401 -489
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/core/particle/ParticleCurve.test.ts (1)
1-54: Great test coverage for different curve scenarios.The tests provide good coverage for the different constructor and evaluation scenarios. However, there are a few style issues with extra line breaks that should be addressed.
expect(compositeCurve.evaluate(0.9, 0.0)).to.equal(0.7); expect(compositeCurve.evaluate(1.0, 0.0)).to.equal(0.7); - expect(compositeCurve.evaluate(0.0, 1.0)).to.equal(0.5); expect(compositeCurve.evaluate(0.5, 1.0)).to.equal(0.55); expect(compositeCurve.evaluate(0.6, 1.0)).to.equal(0.6); expect(compositeCurve.evaluate(0.9, 1.0)).to.equal(0.75); expect(compositeCurve.evaluate(1.0, 1.0)).to.equal(0.8); - expect(compositeCurve.evaluate(0.6, 0.5)).to.equal(0.6499999999999999);🧰 Tools
🪛 ESLint
[error] 44-45: Delete
⏎(prettier/prettier)
[error] 49-50: Delete
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/particle/ParticleGenerator.ts(7 hunks)packages/core/src/particle/modules/ParticleCompositeCurve.ts(2 hunks)packages/core/src/particle/modules/ParticleCurve.ts(1 hunks)tests/src/core/particle/ParticleCurve.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/particle/modules/ParticleCompositeCurve.ts
[error] 181-181: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Check: codecov/patch
packages/core/src/particle/modules/ParticleCompositeCurve.ts
[warning] 179-179: packages/core/src/particle/modules/ParticleCompositeCurve.ts#L179
Added line #L179 was not covered by tests
[warning] 181-182: packages/core/src/particle/modules/ParticleCompositeCurve.ts#L181-L182
Added lines #L181 - L182 were not covered by tests
packages/core/src/particle/ParticleGenerator.ts
[warning] 743-745: packages/core/src/particle/ParticleGenerator.ts#L743-L745
Added lines #L743 - L745 were not covered by tests
[warning] 814-814: packages/core/src/particle/ParticleGenerator.ts#L814
Added line #L814 was not covered by tests
[warning] 817-817: packages/core/src/particle/ParticleGenerator.ts#L817
Added line #L817 was not covered by tests
🪛 ESLint
tests/src/core/particle/ParticleCurve.test.ts
[error] 44-45: Delete ⏎
(prettier/prettier)
[error] 49-50: Delete ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (5)
packages/core/src/particle/modules/ParticleCurve.ts (1)
89-113: LGTM! The implementation of curve evaluation is well-structured.The new
_evaluatemethod accurately implements linear interpolation between curve keys. It properly handles edge cases:
- When the normalized age is before the first key
- When it falls between two keys (using linear interpolation)
- When it exceeds the last key's time
This will provide a smooth transition when evaluating curves for particle properties.
packages/core/src/particle/ParticleGenerator.ts (4)
240-241: LGTM! Renamed parameter for better clarity.Renaming from
timetoplayTimeenhances code readability by making the parameter's purpose clearer.
253-254: Consistent parameter naming.Updated the method call to use the renamed
playTimeparameter.
261-262: Consistent parameter naming.Updated the method call to use the renamed
playTimeparameter.
672-750: Added normalized age calculation for curve-based particle size evaluation.The implementation now properly calculates
normalizedEmitAgebased on the play time and duration, which is then used to evaluate the start size properties. This allows for time-dependent particle size variation.Note that the PR objectives mentioned a question about forced normalization for ParticleCompositeCurve and ParticleCurve operations. The implementation here seems to address this by explicitly calculating and using normalized values.
Since these changes involve new functionality, consider adding specific test cases to verify the integration of the normalized age calculation with the particle size evaluation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 743-745: packages/core/src/particle/ParticleGenerator.ts#L743-L745
Added lines #L743 - L745 were not covered by tests
| const { length } = keys; | ||
|
|
||
| for (let i = 0; i < length; i++) { | ||
| const key = keys[i]; |
There was a problem hiding this comment.
Should we check if the keys array is empty here？
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/particle/ParticleCurve.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (5)
tests/src/core/particle/ParticleCurve.test.ts (5)
1-3: LGTM! Imports are properly defined.The necessary imports from both the engine core and vitest testing framework are correctly included.
4-9: Test case well-structured with appropriate assertions.This test case correctly validates that a ParticleCompositeCurve initialized with a single constant value uses the Constant mode and returns the expected value when evaluated.
11-17: Comprehensive testing of two-constant mode.Good test coverage for the TwoConstants mode, testing boundary conditions (0.0, 1.0) and interpolation behavior (0.5).
19-30: Good validation of Curve mode behavior.The test effectively validates both single-key and multi-key curve scenarios, covering different evaluation positions along the curve.
32-51: Thorough testing of TwoCurves mode with interpolation.This test case provides excellent coverage of the TwoCurves mode, testing evaluation at multiple positions for both individual curves (min and max) and blended interpolation between them.
| it("Add and remove", () => { | ||
| const curve = new ParticleCurve(new CurveKey(0, 0.3), new CurveKey(0.6, 0.7)); | ||
| curve.addKey(new CurveKey(0, 0.4)); | ||
| curve.removeKey(2); | ||
| curve.removeKey(0); | ||
| curve.removeKey(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify key addition and removal.
While the code tests the add and remove functionality, it lacks assertions to verify that the operations worked as expected. Consider adding expectations to check the curve's state after each operation.
it("Add and remove", () => {
const curve = new ParticleCurve(new CurveKey(0, 0.3), new CurveKey(0.6, 0.7));
+ expect(curve.keys.length).to.equal(2);
+
curve.addKey(new CurveKey(0, 0.4));
+ expect(curve.keys.length).to.equal(3);
+ expect(curve.keys[0].value).to.equal(0.4);
+
curve.removeKey(2);
+ expect(curve.keys.length).to.equal(2);
+
curve.removeKey(0);
+ expect(curve.keys.length).to.equal(1);
+ expect(curve.keys[0].time).to.equal(0.6);
+
curve.removeKey(0);
+ expect(curve.keys.length).to.equal(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("Add and remove", () => { | |
| const curve = new ParticleCurve(new CurveKey(0, 0.3), new CurveKey(0.6, 0.7)); | |
| curve.addKey(new CurveKey(0, 0.4)); | |
| curve.removeKey(2); | |
| curve.removeKey(0); | |
| curve.removeKey(0); | |
| }); | |
| it("Add and remove", () => { | |
| const curve = new ParticleCurve(new CurveKey(0, 0.3), new CurveKey(0.6, 0.7)); | |
| expect(curve.keys.length).to.equal(2); | |
| curve.addKey(new CurveKey(0, 0.4)); | |
| expect(curve.keys.length).to.equal(3); | |
| expect(curve.keys[0].value).to.equal(0.4); | |
| curve.removeKey(2); | |
| expect(curve.keys.length).to.equal(2); | |
| curve.removeKey(0); | |
| expect(curve.keys.length).to.equal(1); | |
| expect(curve.keys[0].time).to.equal(0.6); | |
| curve.removeKey(0); | |
| expect(curve.keys.length).to.equal(0); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/src/core/particle/ParticleCurve.test.ts (1)
26-29: Consider using a delta for floating point comparisons.Floating point comparisons are using exact values like
0.6333333333333333which may lead to flaky tests on different platforms or compiler optimizations. Consider usingexpect(...).to.be.closeTo(expectedValue, delta)instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/particle/ParticleCurve.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/src/core/particle/ParticleCurve.test.ts
[error] 55-55: Replace +·· with ···+
(prettier/prettier)
[error] 56-58: Replace +··⏎·⏎···· with ···+
(prettier/prettier)
[error] 59-59: Replace +·· with ···+
(prettier/prettier)
[error] 60-60: Replace +·· with ···+
(prettier/prettier)
[error] 61-62: Replace +·⏎···· with ···+
(prettier/prettier)
[error] 63-63: Replace +·· with ···+
(prettier/prettier)
[error] 64-65: Replace +··⏎···· with ···+
(prettier/prettier)
[error] 66-67: Replace ····⏎·+·· with ⏎····+
(prettier/prettier)
[error] 68-68: Replace +·· with ···+
(prettier/prettier)
[error] 69-69: Replace +·· with ···+
(prettier/prettier)
[error] 70-71: Replace +··⏎···· with ···+
(prettier/prettier)
[error] 72-72: Replace +·· with ···+
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (3)
tests/src/core/particle/ParticleCurve.test.ts (3)
1-74: Well-structured comprehensive test suite for particle curve functionality.This test file properly covers the different initialization scenarios and behaviors of the
ParticleCompositeCurveandParticleCurveclasses. The tests thoroughly verify the different curve modes and evaluation logic.🧰 Tools
🪛 ESLint
[error] 55-55: Replace
+··with···+(prettier/prettier)
[error] 56-58: Replace
+··⏎·⏎····with···+(prettier/prettier)
[error] 59-59: Replace
+··with···+(prettier/prettier)
[error] 60-60: Replace
+··with···+(prettier/prettier)
[error] 61-62: Replace
+·⏎····with···+(prettier/prettier)
[error] 63-63: Replace
+··with···+(prettier/prettier)
[error] 64-65: Replace
+··⏎····with···+(prettier/prettier)
[error] 66-67: Replace
····⏎·+··with⏎····+(prettier/prettier)
[error] 68-68: Replace
+··with···+(prettier/prettier)
[error] 69-69: Replace
+··with···+(prettier/prettier)
[error] 70-71: Replace
+··⏎····with···+(prettier/prettier)
[error] 72-72: Replace
+··with···+(prettier/prettier)
53-73: Addressed previous review comments.The test now includes assertions to verify key addition and removal operations as requested in the previous review. However, there are still issues with the expectations as noted in other comments.
🧰 Tools
🪛 ESLint
[error] 55-55: Replace
+··with···+(prettier/prettier)
[error] 56-58: Replace
+··⏎·⏎····with···+(prettier/prettier)
[error] 59-59: Replace
+··with···+(prettier/prettier)
[error] 60-60: Replace
+··with···+(prettier/prettier)
[error] 61-62: Replace
+·⏎····with···+(prettier/prettier)
[error] 63-63: Replace
+··with···+(prettier/prettier)
[error] 64-65: Replace
+··⏎····with···+(prettier/prettier)
[error] 66-67: Replace
····⏎·+··with⏎····+(prettier/prettier)
[error] 68-68: Replace
+··with···+(prettier/prettier)
[error] 69-69: Replace
+··with···+(prettier/prettier)
[error] 70-71: Replace
+··⏎····with···+(prettier/prettier)
[error] 72-72: Replace
+··with···+(prettier/prettier)
55-72: Fix formatting issues.The static analysis tool indicates formatting issues with the indentation of the added lines. This appears to be related to the annotation process rather than the actual code, but please ensure that the final code follows the project's formatting guidelines.
🧰 Tools
🪛 ESLint
[error] 55-55: Replace
+··with···+(prettier/prettier)
[error] 56-58: Replace
+··⏎·⏎····with···+(prettier/prettier)
[error] 59-59: Replace
+··with···+(prettier/prettier)
[error] 60-60: Replace
+··with···+(prettier/prettier)
[error] 61-62: Replace
+·⏎····with···+(prettier/prettier)
[error] 63-63: Replace
+··with···+(prettier/prettier)
[error] 64-65: Replace
+··⏎····with···+(prettier/prettier)
[error] 66-67: Replace
····⏎·+··with⏎····+(prettier/prettier)
[error] 68-68: Replace
+··with···+(prettier/prettier)
[error] 69-69: Replace
+··with···+(prettier/prettier)
[error] 70-71: Replace
+··⏎····with···+(prettier/prettier)
[error] 72-72: Replace
+··with···+(prettier/prettier)
| + expect(curve.keys.length).to.equal(3); | ||
| + expect(curve.keys[0].value).to.equal(0.3); | ||
| + |
There was a problem hiding this comment.
Incorrect assertion for key addition test.
The assertion is checking for curve.keys[0].value to equal 0.3, but this doesn't match the expected behavior after adding a new key with value 0.4. The new key should either replace the existing key at index 0 or be sorted into the proper position, depending on the implementation.
- expect(curve.keys[0].value).to.equal(0.3);
+ // After adding a new key at time 0 with value 0.4, check that it's either:
+ // 1. At the correct position if keys are sorted by time
+ const keyAtTimeZero = curve.keys.find(key => key.time === 0);
+ expect(keyAtTimeZero?.value).to.equal(0.4);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + expect(curve.keys.length).to.equal(3); | |
| + expect(curve.keys[0].value).to.equal(0.3); | |
| + | |
| expect(curve.keys.length).to.equal(3); | |
| - expect(curve.keys[0].value).to.equal(0.3); | |
| + // After adding a new key at time 0 with value 0.4, check that it's either: | |
| + // 1. At the correct position if keys are sorted by time | |
| + const keyAtTimeZero = curve.keys.find(key => key.time === 0); | |
| + expect(keyAtTimeZero?.value).to.equal(0.4); |
🧰 Tools
🪛 ESLint
[error] 59-59: Replace +·· with ···+
(prettier/prettier)
[error] 60-60: Replace +·· with ···+
(prettier/prettier)
| + expect(curve.keys.length).to.equal(1); | ||
| + expect(curve.keys[0].time).to.equal(0.0); | ||
| + expect(curve.keys[0].value).to.equal(0.4); |
There was a problem hiding this comment.
Incorrect assertions after key removal.
After removing the key at index 0, you're expecting the remaining key to have time 0.0 and value 0.4, but based on the initial setup and removal operations, I'd expect the remaining key to have time 0.0 (from the newly added key) and value 0.4.
Let's verify the expected behavior:
- Initial keys: time 0 with value 0.3, time 0.6 with value 0.7
- Add key at time 0 with value 0.4
- Remove key at index 2 (the key at time 0.6)
- Remove key at index 0 (one of the keys at time 0)
- The only remaining key should be at time 0 with value 0.4 (or 0.3, depending on how keys are ordered)
- expect(curve.keys[0].time).to.equal(0.0);
- expect(curve.keys[0].value).to.equal(0.4);
+ // After removing two keys, verify that:
+ // 1. Only one key remains
+ // 2. That key has the expected time and value
+ expect(curve.keys.length).to.equal(1);
+ expect(curve.keys[0].time).to.equal(0.0);
+
+ // Depending on the implementation, the value should be either 0.3 or 0.4
+ const expectedValue = curve.keys[0].value === 0.3 ? 0.3 : 0.4;
+ expect(curve.keys[0].value).to.equal(expectedValue);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + expect(curve.keys.length).to.equal(1); | |
| + expect(curve.keys[0].time).to.equal(0.0); | |
| + expect(curve.keys[0].value).to.equal(0.4); | |
| // After removing two keys, verify that: | |
| // 1. Only one key remains | |
| // 2. That key has the expected time and value | |
| expect(curve.keys.length).to.equal(1); | |
| expect(curve.keys[0].time).to.equal(0.0); | |
| // Depending on the implementation, the value should be either 0.3 or 0.4 | |
| const expectedValue = curve.keys[0].value === 0.3 ? 0.3 : 0.4; | |
| expect(curve.keys[0].value).to.equal(expectedValue); |
🧰 Tools
🪛 ESLint
[error] 68-68: Replace +·· with ···+
(prettier/prettier)
[error] 69-69: Replace +·· with ···+
(prettier/prettier)
Please check if the PR fulfills these requirements
Other information:
Summary by CodeRabbit