Skip to content

Conversation

@stackotter
Copy link
Contributor

Description

This PR implements 2x2 matrices (because I need them in Delta Client) and adds the * multiplication operator to both 3x3 and 2x2 matrices (only 4x4 matrices had the operator).

Detailed Design

I've essentially just copied the 3x3 implementations and modified them to be 2x2.

Documentation

The 2x2 matrix implementation is no less commented than the 3x3 matrix implementation :)

Testing

The 2x2 matrix implementation is no less tested than the 3x3 matrix implementation :) Although I am planning on PR'ing some tests for both 3x3 and 2x2 matrices in the future (and for other parts of the library too).

Performance

Uses the same implementation as the 3x3 matrices so it's probably slightly faster just cause they're smaller :)

Source Impact

I'm pretty sure I have avoided introducing any breaking changes.

Checklist

  • I've read the Contribution Guidelines
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (to the extent possible).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexpected benchmark regressions.
  • I've updated the documentation (if appropriate).

@stackotter stackotter requested a review from ctreffs as a code owner November 2, 2022 12:28
@stackotter stackotter changed the title Add 2x2 matrices Implement 2x2 matrices Nov 2, 2022
@ctreffs
Copy link
Member

ctreffs commented Dec 8, 2022

Thanks for your contribution - sorry for the long delay of my review.

LGTM 🚀

@ctreffs ctreffs merged commit d42d22c into fireblade-engine:master Dec 8, 2022
@stackotter
Copy link
Contributor Author

Thanks for merging 🙏 I’m away from my laptop for a month so I’m glad I didn’t have to make any changes hahah

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