Skip to content

Review windows-numerics: fix π literal, add Neg trait for vectors, cross product, and expand test coverage#4015

Merged
kennykerr merged 6 commits intomasterfrom
copilot/review-windows-numerics-crate
Mar 17, 2026
Merged

Review windows-numerics: fix π literal, add Neg trait for vectors, cross product, and expand test coverage#4015
kennykerr merged 6 commits intomasterfrom
copilot/review-windows-numerics-crate

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 16, 2026

What's this all about?

Bug fixes

  • matrix4x4.rs: Replace hardcoded 3.141592654 / 180.0 with degree.to_radians() — idiomatic, self-documenting, and avoids literal drift
  • lib.rs: Remove clippy::all from #![expect] — it was fulfilled by the old approximate PI literal triggering clippy::approx_constant; after switching to to_radians() no clippy::all lints fire, so the expectation was unfulfilled and broke the clippy CI job
  • Cargo.toml (test crate): Add features = ["std"] to the windows-numerics dependency — length, normalize, and distance are #[cfg(feature = "std")] gated, but the workspace default is default-features = false, causing test compilation failures

New functionality

  • Neg (-x) unary operator added to the three vector types (Vector2, Vector3, Vector4), both owned and borrowed forms, matching the existing operator impl pattern
  • Vector3::cross(&self, rhs: &Self) -> Self — the standard 3D cross-product was absent
let v1 = Vector3 { X: 1.0, Y: 0.0, Z: 0.0 };
let v2 = Vector3 { X: 0.0, Y: 1.0, Z: 0.0 };
assert_eq!(v1.cross(&v2), Vector3 { X: 0.0, Y: 0.0, Z: 1.0 });
assert_eq!(-v1, Vector3 { X: -1.0, Y: 0.0, Z: 0.0 });

Test coverage
Previously only the four arithmetic operators were tested. Added coverage for:

  • dot, length, length_squared, normalize, distance, distance_squared — all vector types
  • cross including anti-commutativity (v2×v1 == -(v1×v2))
  • Neg — all three vector types (Vector2, Vector3, Vector4)
  • Matrix3x2 constructors: identity, translation, scale, scale_around
  • Matrix3x2 * identity == matrix identity property
  • Matrix4x4 constructors: translation, perspective_projection (including zero/negative depth edge cases)

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Copilot AI and others added 2 commits March 16, 2026 15:05
…and test coverage

Co-authored-by: kennykerr <9845234+kennykerr@users.noreply.github.com>
@kennykerr kennykerr marked this pull request as ready for review March 16, 2026 15:25
@kennykerr kennykerr requested a review from robmikh March 16, 2026 15:25
@kennykerr
Copy link
Copy Markdown
Collaborator

@robmikh would you mind reviewing? I asked Copilot to review this crate and it came up with some improvements. I'm just not 100% sure they're all applicable.

@kennykerr
Copy link
Copy Markdown
Collaborator

@copilot fix build breaks

…n test crate

Co-authored-by: kennykerr <9845234+kennykerr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 16, 2026

@copilot fix build breaks

Fixed in c6d7ad4. Two issues:

  1. Clippy: #![expect(clippy::all)] became unfulfilled after replacing 3.141592654 / 180.0 with degree.to_radians() — the old literal was triggering clippy::approx_constant. Removed clippy::all from the #![expect] attribute since nothing in the crate needs it anymore.

  2. Tests: length, normalize, and distance are #[cfg(feature = "std")] gated, but the test crate depended on windows-numerics with default-features = false (the workspace default). Added features = ["std"] to the test crate's dependency.

Copilot AI requested a review from kennykerr March 16, 2026 16:02
@robmikh
Copy link
Copy Markdown
Member

robmikh commented Mar 17, 2026

I think neg makes sense for vectors, and having the cross product is handy for Vec3. But neg for the matrices doesn't make sense to me, I'm not sure when you'd ever use it.

@kennykerr
Copy link
Copy Markdown
Collaborator

I think neg makes sense for vectors, and having the cross product is handy for Vec3. But neg for the matrices doesn't make sense to me, I'm not sure when you'd ever use it.

@copilot please work on this.

Co-authored-by: kennykerr <9845234+kennykerr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 17, 2026

I think neg makes sense for vectors, and having the cross product is handy for Vec3. But neg for the matrices doesn't make sense to me, I'm not sure when you'd ever use it.

@copilot please work o...

Done in df72c75 — removed Neg (and the associated impl_neg helper) from both Matrix3x2 and Matrix4x4, along with the corresponding tests. Neg is kept for Vector2, Vector3, and Vector4.

Copilot AI changed the title Review windows-numerics: fix π literal, add Neg trait, cross product, and expand test coverage Review windows-numerics: fix π literal, add Neg trait for vectors, cross product, and expand test coverage Mar 17, 2026
@kennykerr
Copy link
Copy Markdown
Collaborator

Thanks Rob!

@kennykerr kennykerr merged commit 0a7453c into master Mar 17, 2026
38 of 39 checks passed
@kennykerr kennykerr deleted the copilot/review-windows-numerics-crate branch March 17, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants