Add function support copy vector/color/quaternion to like object #2428
Conversation
singlecoder
requested review from
eyworldwide,
GuoLei1990 and
cptbtptpbcptdtptp
WalkthroughThe changes introduced in this pull request involve the addition of a new method named Changes
Poem
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
==========================================
+ Coverage 69.11% 69.42% +0.30%
==========================================
Files 524 524
Lines 27411 27439 +28
Branches 4108 4109 +1
==========================================
+ Hits 18946 19049 +103
+ Misses 6968 6884 -84
- Partials 1497 1506 +9
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: 0
🧹 Outside diff range and nitpick comments (7)
tests/src/math/Vector2.test.ts (1)
151-156: LGTM! Consider adding more test cases for comprehensive coverage.The basic test case looks good and follows the established patterns. However, consider adding the following test cases for better coverage:
it("copyTo - should handle existing values in target", () => { const a = new Vector2(3, 4); const out = new Vector2(1, 2); a.copyTo(out); expect(toString(out)).to.eq("vec2(3, 4)"); expect(toString(a)).to.eq("vec2(3, 4)"); // verify source unchanged }); it("copyTo - should handle edge cases", () => { const a = new Vector2(Number.MAX_VALUE, Number.MIN_VALUE); const out = new Vector2(); a.copyTo(out); expect(toString(out)).to.eq(toString(a)); });tests/src/math/Vector4.test.ts (1)
161-166: Enhance test coverage for the copyTo method.While the basic test case is good, consider adding more test cases to ensure robust functionality:
- Test copying to a Vector4Like object (not just Vector4)
- Verify the source vector remains unchanged after copy
- Add edge cases (e.g., copying to self)
- Use direct value comparisons instead of toString
Here's a suggested expansion of the test cases:
it("copyTo", () => { const a = new Vector4(3, 4, 5, 0); const out = new Vector4(); a.copyTo(out); expect(toString(a)).to.eq(toString(out)); + + // Test copying to Vector4Like object + const vector4Like = { x: 0, y: 0, z: 0, w: 0 }; + a.copyTo(vector4Like); + expect(vector4Like.x).to.eq(3); + expect(vector4Like.y).to.eq(4); + expect(vector4Like.z).to.eq(5); + expect(vector4Like.w).to.eq(0); + + // Verify source remains unchanged + expect(a.x).to.eq(3); + expect(a.y).to.eq(4); + expect(a.z).to.eq(5); + expect(a.w).to.eq(0); + + // Test copying to self + a.copyTo(a); + expect(a.x).to.eq(3); + expect(a.y).to.eq(4); + expect(a.z).to.eq(5); + expect(a.w).to.eq(0); });tests/src/math/Vector3.test.ts (1)
179-184: Enhance test coverage for the copyTo method.While the basic test case looks good, consider adding the following test scenarios for better coverage:
- Verify source vector remains unchanged after copy
- Test edge cases (e.g., null/undefined target)
- Verify that modifying the target doesn't affect the source
Example enhancement:
it("copyTo", () => { const a = new Vector3(3, 4, 5); const out = new Vector3(); + const originalToString = toString(a); a.copyTo(out); expect(toString(a)).to.eq(toString(out)); + // Verify source remains unchanged + expect(toString(a)).to.eq(originalToString); + + // Verify independent copies + out.x = 10; + expect(a.x).to.eq(3); + + // Test edge cases + expect(() => a.copyTo(null)).to.throw(); + expect(() => a.copyTo(undefined)).to.throw(); });packages/math/src/Vector2.ts (1)
360-364: Consider adding parameter validation.The method could benefit from defensive programming by validating the target parameter.
copyTo(target: Vector2Like): Vector2Like { + if (!target) { + throw new Error("The target vector cannot be null or undefined"); + } target.x = this._x; target.y = this._y; return target; }packages/math/src/Vector4.ts (1)
480-491: LGTM! The implementation is correct and well-integrated.The new
copyTomethod:
- Correctly implements value copying to target Vector4Like object
- Maintains symmetry with existing
copyFrommethod- Properly documented following the codebase style
- Correctly returns the modified target object
Minor suggestion for documentation completeness
Consider adding an example in the documentation to show usage:
/** * Copy to vector4 like object. * @param target - Vector4 like object * @returns This Vector4 like object + * @example + * ```typescript + * const source = new Vector4(1, 2, 3, 4); + * const target = { x: 0, y: 0, z: 0, w: 0 }; + * source.copyTo(target); // target is now { x: 1, y: 2, z: 3, w: 4 } + * ``` */packages/math/src/Vector3.ts (2)
569-574: Consider adding parameter validation.The implementation looks good, but consider adding a null check for the target parameter to prevent runtime errors.
copyTo(target: Vector3Like): Vector3Like { + if (!target) { + throw new Error("Target cannot be null or undefined"); + } target.x = this._x; target.y = this._y; target.z = this._z; return target; }
564-568: Enhance method documentation.The JSDoc could be more descriptive about the method's purpose and behavior.
/** * Copy to vector3 like object. + * Copies the values of this vector to a target object that implements the Vector3Like interface. * @param target - Vector3 like object + * @throws {Error} If target is null or undefined * @returns This Vector3 like object */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/math/src/Vector2.ts(1 hunks)packages/math/src/Vector3.ts(1 hunks)packages/math/src/Vector4.ts(1 hunks)tests/src/math/Vector2.test.ts(1 hunks)tests/src/math/Vector3.test.ts(1 hunks)tests/src/math/Vector4.test.ts(1 hunks)
🔇 Additional comments (5)
tests/src/math/Vector2.test.ts (1)
151-156: Verify consistent test coverage across vector classes.
Since this is part of adding copyTo to all vector classes, let's ensure consistent test coverage exists in Vector3 and Vector4 test files.
✅ Verification successful
Test coverage for copyTo is consistent across vector classes
The verification shows that all vector classes (Vector2, Vector3, Vector4) have identical test coverage for the copyTo method, following the same pattern:
- Creating a source vector with test values
- Creating an empty destination vector
- Calling
copyTo - Verifying equality using
toString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar test coverage exists for Vector3 and Vector4
echo "Checking for copyTo tests in Vector3 and Vector4 test files..."
rg -A 5 'it\("copyTo"' tests/src/math/Vector3.test.ts tests/src/math/Vector4.test.ts
echo "Checking test patterns used across vector test files..."
rg -A 2 'toString\(' tests/src/math/Vector{2,3,4}.test.ts
Length of output: 14718
tests/src/math/Vector4.test.ts (1)
161-166: Verify consistent test coverage across vector classes.
Since similar copyTo methods were added to Vector2 and Vector3 classes, let's verify that they have equivalent test coverage.
✅ Verification successful
Test coverage for copyTo is consistent across vector classes
The verification shows that:
- All vector classes (Vector2, Vector3, Vector4) have identical test coverage for the
copyTomethod - The implementation of
copyTois present in all vector classes with appropriate type signatures - The test structure and assertions are consistent across all test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Vector2 and Vector3 test files have similar copyTo test coverage
echo "Checking Vector2 and Vector3 test files for copyTo tests..."
# Search for copyTo test cases in Vector2 and Vector3 test files
rg -A 5 'it\("copyTo"' tests/src/math/Vector[23].test.ts
# Also check the actual implementation files to ensure copyTo is implemented
echo -e "\nVerifying copyTo implementation in Vector classes..."
rg -A 3 'copyTo\(' 'packages/math/src/Vector[234].ts'
Length of output: 1159
Script:
#!/bin/bash
# Let's try with the correct path pattern for implementation files
echo "Verifying copyTo implementation in Vector classes..."
rg -A 3 'copyTo\(' packages/core/src/math/Vector[234].ts
# Double check the test coverage with correct paths
echo -e "\nChecking test coverage..."
rg -A 5 'it\("copyTo"' tests/src/math/Vector[234].test.ts
Length of output: 1485
Script:
#!/bin/bash
# Let's try finding the correct path for Vector implementation files
echo "Finding Vector implementation files..."
fd Vector[234].ts
# Then verify copyTo implementation in the found files
echo -e "\nVerifying copyTo implementation..."
fd Vector[234].ts --exec rg -A 3 'copyTo\('
Length of output: 674
packages/math/src/Vector2.ts (2)
355-364: LGTM! The implementation is clean and consistent.
The new copyTo method follows the established patterns in the codebase and provides a symmetrical operation to the existing copyFrom method.
355-364: Verify consistency with Vector3 and Vector4 implementations.
Let's ensure the implementation is consistent across all vector classes.
packages/math/src/Vector4.ts (1)
Line range hint 9-9: Verify ICopy interface implementation
The new copyTo method completes the ICopy interface implementation, working in tandem with the existing copyFrom method. This provides a symmetric API for copying vector values between objects.
✅ Verification successful
Let me verify the implementation of ICopy interface across these Vector classes.
Let me try a different approach using ripgrep to check the implementations.
ICopy interface implementation is consistent and complete
The implementation of ICopy interface is verified across Vector2, Vector3, and Vector4 classes. All classes:
- Properly implement both
copyFromandcopyTomethods - Handle their respective dimensional data (2D, 3D, 4D)
- Maintain consistent return types and parameter types
- Follow the same pattern of implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify ICopy interface implementation completeness
# Search for other Vector classes to ensure consistent implementation
# Look for Vector2 and Vector3 implementations
rg "implements.*ICopy.*Vector\dLike" --type ts
Length of output: 372
Script:
#!/bin/bash
# Check implementation of copyFrom and copyTo methods in all Vector classes
ast-grep --pattern 'copyFrom($_: $_) {
$$$
}'
ast-grep --pattern 'copyTo($_: $_) {
$$$
}'
Length of output: 97
Script:
#!/bin/bash
# Check copyFrom and copyTo methods in Vector classes with context
rg "copyFrom|copyTo" packages/math/src/Vector*.ts -A 5 -B 1
Length of output: 4880
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/math/src/ICopy.ts (1)
11-15: Enhance documentation for optional copyTo method.The JSDoc could be more descriptive about the optional nature of the method and its relationship with
copyFrom./** * Copy to target object. + * This optional method provides symmetrical functionality to copyFrom, + * allowing objects to copy their values to a target object. * @param target - The target object to copy values to * @returns This target object */tests/src/math/Color.test.ts (1)
61-67: LGTM! Consider adding more test cases for comprehensive coverage.The test case correctly verifies the basic functionality of the
copyTomethod. However, to ensure robust implementation, consider adding tests for:
- Edge cases with extreme values (0,0,0,0) and (1,1,1,1)
- Invalid inputs (null/undefined target)
- Chaining behavior (if supported)
Here's a suggested expansion of the test cases:
it("copyTo", () => { // Basic functionality const a = new Color(1, 0, 0, 1); const out = new Color(); a.copyTo(out); expect(Color.equals(a, out)).to.eq(true); // Edge cases const black = new Color(0, 0, 0, 0); const white = new Color(1, 1, 1, 1); const target = new Color(); black.copyTo(target); expect(Color.equals(black, target)).to.eq(true); white.copyTo(target); expect(Color.equals(white, target)).to.eq(true); // Invalid input expect(() => a.copyTo(null)).to.throw(); expect(() => a.copyTo(undefined)).to.throw(); });tests/src/math/Quaternion.test.ts (1)
264-269: Consider expanding test coverage for copyTo method.While the current test provides a good baseline verification, consider adding the following test cases to ensure comprehensive coverage:
- Test with a QuaternionLike object as target
- Verify source remains unchanged after copy
- Test edge cases (e.g., identity quaternion)
- Verify individual components directly instead of string comparison
Example expansion:
it("copyTo", () => { const a = new Quaternion(3, 4, 5, 0); // Test 1: Basic quaternion copy const out = new Quaternion(); a.copyTo(out); expect(toString(a)).to.eq(toString(out)); // Test 2: Copy to QuaternionLike object const outLike = { x: 0, y: 0, z: 0, w: 0 }; a.copyTo(outLike); expect(outLike.x).to.eq(3); expect(outLike.y).to.eq(4); expect(outLike.z).to.eq(5); expect(outLike.w).to.eq(0); // Test 3: Verify source unchanged expect(a.x).to.eq(3); expect(a.y).to.eq(4); expect(a.z).to.eq(5); expect(a.w).to.eq(0); // Test 4: Identity quaternion const identity = new Quaternion(0, 0, 0, 1); const outIdentity = new Quaternion(); identity.copyTo(outIdentity); expect(outIdentity.x).to.eq(0); expect(outIdentity.y).to.eq(0); expect(outIdentity.z).to.eq(0); expect(outIdentity.w).to.eq(1); });packages/math/src/Color.ts (1)
261-272: Consider adding parameter validation.The implementation looks good and follows the established patterns. Consider adding parameter validation to handle null/undefined targets.
copyTo(target: ColorLike): ColorLike { + if (!target) { + throw new Error("The target cannot be null or undefined."); + } target.r = this._r; target.g = this._g; target.b = this._b; target.a = this._a; return target; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/math/src/Color.ts(1 hunks)packages/math/src/ICopy.ts(1 hunks)packages/math/src/Quaternion.ts(1 hunks)tests/src/math/Color.test.ts(1 hunks)tests/src/math/Quaternion.test.ts(1 hunks)
🔇 Additional comments (3)
packages/math/src/ICopy.ts (1)
10-15: LGTM! Well-designed interface extension.
The optional copyTo method is a good addition that:
- Maintains backward compatibility by being optional
- Provides symmetrical functionality to
copyFrom - Uses proper TypeScript generic types
packages/math/src/Color.ts (1)
261-272: Verify consistency across vector-like classes.
Let's verify that this implementation pattern is consistent across other classes mentioned in the PR summary (Vector2, Vector3, Vector4, Quaternion).
✅ Verification successful
Implementation pattern is consistent across all vector-like classes
The copyTo implementation in Color follows the same pattern as all other vector-like classes (Vector2, Vector3, Vector4, Quaternion):
- All methods accept a
*Likeinterface as parameter and return type - All implementations directly copy internal fields to target object properties
- All return the target object
- None of them include any additional processing or callbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation consistency of copyTo method across vector-like classes
# Expected: All implementations should follow similar pattern without _onValueChanged calls
# Search for copyTo implementations in vector-like classes
echo "Searching for copyTo implementations:"
rg -A 6 'copyTo.*:.*Like.*{' packages/math/src/
Length of output: 234
Script:
#!/bin/bash
# Let's fix the regex and search for copyTo implementations
echo "Searching for copyTo implementations in Vector2:"
rg -A 6 "copyTo.*\(.*\).*\{" packages/math/src/Vector2.ts
echo -e "\nSearching for copyTo implementations in Vector3:"
rg -A 6 "copyTo.*\(.*\).*\{" packages/math/src/Vector3.ts
echo -e "\nSearching for copyTo implementations in Vector4:"
rg -A 6 "copyTo.*\(.*\).*\{" packages/math/src/Vector4.ts
echo -e "\nSearching for copyTo implementations in Quaternion:"
rg -A 6 "copyTo.*\(.*\).*\{" packages/math/src/Quaternion.ts
Length of output: 2151
packages/math/src/Quaternion.ts (1)
733-744: Verify usage of the new copyTo method.
Let's verify if there are any existing usages of this new method to ensure backward compatibility.
✅ Verification successful
The copyTo method follows a consistent pattern across math types and has test coverage
The copyTo method implementation in Quaternion follows the same pattern as other math types (Vector2, Vector3, Vector4, Color) in the codebase. Each implementation copies component values to a target object of the corresponding type. The method is also properly tested in tests/src/math/Quaternion.test.ts, ensuring it works as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of the copyTo method
rg -A 2 "copyTo\(" .
Length of output: 1837
| /** | ||
| * Copy this quaternion to the specified quaternion. | ||
| * @param target - The specified quaternion | ||
| * @returns This specified quaternion | ||
| */ | ||
| copyTo(target: QuaternionLike): QuaternionLike { | ||
| target.x = this._x; | ||
| target.y = this._y; | ||
| target.z = this._z; | ||
| target.w = this._w; | ||
| return target; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and maintain consistency with other methods.
The implementation could be improved in the following areas:
- Add parameter validation to handle null/undefined target.
- Consider calling
_onValueChangedon the target if it has the callback, maintaining consistency with other methods. - Improve documentation by mentioning that the target must implement the QuaternionLike interface.
Consider applying these improvements:
/**
* Copy this quaternion to the specified quaternion.
* @param target - The specified quaternion
+ * @throws {Error} If target is null or undefined
* @returns This specified quaternion
*/
copyTo(target: QuaternionLike): QuaternionLike {
+ if (!target) {
+ throw new Error("Target cannot be null or undefined");
+ }
target.x = this._x;
target.y = this._y;
target.z = this._z;
target.w = this._w;
+ if (target instanceof Quaternion && target._onValueChanged) {
+ target._onValueChanged();
+ }
return target;
}📝 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.
| /** | |
| * Copy this quaternion to the specified quaternion. | |
| * @param target - The specified quaternion | |
| * @returns This specified quaternion | |
| */ | |
| copyTo(target: QuaternionLike): QuaternionLike { | |
| target.x = this._x; | |
| target.y = this._y; | |
| target.z = this._z; | |
| target.w = this._w; | |
| return target; | |
| } | |
| /** | |
| * Copy this quaternion to the specified quaternion. | |
| * @param target - The specified quaternion | |
| * @throws {Error} If target is null or undefined | |
| * @returns This specified quaternion | |
| */ | |
| copyTo(target: QuaternionLike): QuaternionLike { | |
| if (!target) { | |
| throw new Error("Target cannot be null or undefined"); | |
| } | |
| target.x = this._x; | |
| target.y = this._y; | |
| target.z = this._z; | |
| target.w = this._w; | |
| if (target instanceof Quaternion && target._onValueChanged) { | |
| target._onValueChanged(); | |
| } | |
| return target; | |
| } |
GuoLei1990
changed the title
GuoLei1990
changed the title
GuoLei1990
changed the title
Summary by CodeRabbit
New Features
copyTomethod in theVector2,Vector3,Vector4,Color, andQuaternionclasses, allowing users to copy values to target objects.Tests
copyTomethod inVector2,Vector3,Vector4,Color, andQuaternionto ensure functionality and accuracy.