Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement Default for vector and matrices #4

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

rlane
Copy link
Contributor

@rlane rlane commented Sep 9, 2023

We're using maths-rs for vector types in http://oort.rs. It would be nice if these types could be used with derive(Default).

@polymonster
Copy link
Owner

polymonster commented Sep 11, 2023

Hi,

Thanks for this, hope you are finding the library useful and fun to use!

Happy to merge these changes, I was just wondering before doing so... should the default of a matrix be identity instead of just zeros?

Same goes for quaternion, if Vec and Matrix have a default then Quat should as well and it also has the same question regarding identity also applies. My feeling is that probably the default should be identity because it is a useful default where as all zeros in either quaternion or matrix are not very useful... Although I understand that the default is required to satisfy derive default and in those cases you would be probably initialising the matrix with useful values.

Once we have defaults for all the types then I can publish a new build to cargo, I can add the quaternion default myself once we decide but feel free to contribute if you want.

Thanks!

@rlane
Copy link
Contributor Author

rlane commented Sep 12, 2023

Yeah, identity is a better default for matrices. I pushed a commit to change that.

@polymonster
Copy link
Owner

Thanks, I will add the default quaternion and then publish to crates.

@polymonster polymonster merged commit 0dded2c into polymonster:master Sep 12, 2023
1 check passed
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