Skip to content

Conversation

@vmarcella
Copy link
Member

Summary

Refactors the math library to return Result types with descriptive errors instead of panicking. This allows users to handle math errors gracefully and makes the API more idiomatic Rust. The change aligns with the repository guidelines that state: "avoid using panic unless absolutely necessary and allow for the user handle errors whenever possible."

Related Issues

Changes

  • Add new MathError enum in crates/lambda-rs/src/math/error.rs with descriptive error variants for all fallible operations
  • Update Vector::cross() to return Result<Self, MathError> instead of panicking on dimension mismatches
  • Update Vector::normalize() to return Result<Self, MathError> instead of panicking on zero-length vectors
  • Update Matrix::determinant() to return Result<f32, MathError> instead of panicking on empty or non-square matrices
  • Update rotate_matrix() to return Result<MatrixLike, MathError> instead of panicking on invalid inputs
  • Simplify rotate_matrix() axis parameter from generic InputVector to concrete [f32; 3] array
  • Update examples (reflective_room.rs, textured_cube.rs) and internal usage (scene_math.rs) to handle the new Result types
  • Update tutorials (reflective-room.md, textured-cube.md) to reflect the API changes

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (updates to docs, specs, tutorials, or comments)
  • Refactor (code change that neither fixes a bug nor adds a feature)
  • Performance (change that improves performance)
  • Test (adding or updating tests)
  • Build/CI (changes to build process or CI configuration)

Affected Crates

  • lambda-rs
  • lambda-rs-platform
  • lambda-rs-args
  • lambda-rs-logging
  • Other:

Checklist

  • Code follows the repository style guidelines (cargo +nightly fmt --all)
  • Code passes clippy (cargo clippy --workspace --all-targets -- -D warnings)
  • Tests pass (cargo test --workspace)
  • New code includes appropriate documentation
  • Public API changes are documented
  • Breaking changes are noted in this PR description

Testing

Commands run:

cargo build --workspace
cargo test --workspace
cargo run --example reflective_room
cargo run --example textured_cube

Manual verification steps (if applicable):

  1. Run reflective_room example and verify rotation behavior is unchanged
  2. Run textured_cube example and verify cube renders correctly with rotation
  3. Verify error messages are descriptive when invalid inputs are provided

Screenshots/Recordings

N/A - No visual changes; API behavior remains the same for valid inputs.

Platform Testing

  • macOS
  • Windows
  • Linux

Additional Notes

Breaking Changes

The following public APIs now return Result types:

Function/Method Old Signature New Signature
Vector::cross() fn cross(&self, other: &Self) -> Self fn cross(&self, other: &Self) -> Result<Self, MathError>
Vector::normalize() fn normalize(&self) -> Self fn normalize(&self) -> Result<Self, MathError>
Matrix::determinant() fn determinant(&self) -> f32 fn determinant(&self) -> Result<f32, MathError>
rotate_matrix() fn rotate_matrix<...>(...) -> OutputMatrix fn rotate_matrix<...>(...) -> Result<MatrixLike, MathError>

Migration Guide

Users calling these functions will need to handle the Result:

// Before
let normal = vec_a.cross(&vec_b);
let unit = normal.normalize();

// After
let normal = vec_a.cross(&vec_b)?;  // or .expect("msg") / .unwrap()
let unit = normal.normalize()?;

New Error Types

The MathError enum provides detailed context for each failure case:

  • CrossProductDimension { actual } - Cross product requires 3D vectors
  • MismatchedVectorDimensions { left, right } - Vectors must have matching dimensions
  • InvalidRotationAxis { axis } - Rotation axis must be a unit axis vector
  • InvalidRotationMatrixSize { rows, cols } - Rotation requires a 4x4 matrix
  • NonSquareMatrix { rows, cols } - Operation requires square matrix
  • EmptyMatrix - Operation requires a non-empty matrix
  • ZeroLengthVector - Cannot normalize a zero-length vector

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the math library to return Result types instead of panicking, aligning with Rust best practices and the repository's guidelines to "avoid using panic unless absolutely necessary." The change converts four key math operations (Vector::cross, Vector::normalize, Matrix::determinant, and rotate_matrix) to return descriptive errors when given invalid inputs.

Changes:

  • Introduced comprehensive MathError enum with variants for all fallible math operations
  • Converted panic-based error handling to Result-based error handling for cross product, normalize, determinant, and matrix rotation operations
  • Updated all examples, tests, and documentation to handle the new Result types appropriately

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/lambda-rs/src/math/error.rs New error module with MathError enum and Display/Error trait implementations
crates/lambda-rs/src/math/mod.rs Exports the new error module and MathError type
crates/lambda-rs/src/math/vector.rs Updated cross() and normalize() to return Result types; updated tests
crates/lambda-rs/src/math/matrix.rs Updated determinant() and rotate_matrix() to return Result types; updated tests
crates/lambda-rs/src/render/scene_math.rs Updated compute_model_matrix to handle rotate_matrix Result
crates/lambda-rs/examples/textured_cube.rs Updated to handle rotate_matrix Result with expect()
crates/lambda-rs/examples/reflective_room.rs Updated to handle rotate_matrix Result with expect()
docs/tutorials/textured-cube.md Updated tutorial code samples and metadata to reflect API changes
docs/tutorials/reflective-room.md Updated tutorial code samples and metadata to reflect API changes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vmarcella vmarcella merged commit 0ffa1bf into main Jan 20, 2026
5 checks passed
@vmarcella vmarcella deleted the vmarcella/fix-math-panics branch January 20, 2026 21:47
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.

2 participants