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

matrix code issues #17

Closed
gvissers opened this issue Feb 24, 2016 · 3 comments
Closed

matrix code issues #17

gvissers opened this issue Feb 24, 2016 · 3 comments

Comments

@gvissers
Copy link

There are various issues with the matrix code in the utility directory. The most serious issues are:
*) The memset() calls in the constructors clear too many bytes, since the sizeof(double) is also squared.
*) the minor_matrix() function does not work, because the colCount variable is never reset. It actually accesses outside the _cell_data array. Consequently, the determinant() and invert() functions also fail.
*) The invert function should divide by the determinant of the matrix, not multiply by it.

Further issues:
*) The _cell pointer is superfluous
*) the copy constructor unnecessarily clears the matrix (it immediately overwrites the cleared data afterwards)
*) In matrix multiplication, row is unnecessarily copied for every element
*) matrices as arguments should really be passed as references
*) there are no const accessor functions (cell(), operator())

@microbuilder
Copy link
Contributor

Thanks for the heads up. We haven't used the matrix code ourselves (the main interest in adding these was working purely in quats) and the helpers come from a third party, but we'd be happy to integrate any pull requests to improve the code. Alternatively, if you're not familiar with generating pull requests if you have any specific suggestions you can paste the class update here and I'll integrate it myself.

If you haven't made any local changes you can share, though, let me know and I'll try to find a moment to look into this myself, although we don't have any particular use cases around the matrix class ourselves.

@gvissers
Copy link
Author

okay, I'll create a pull request later today.

@microbuilder
Copy link
Contributor

Thanks, I'm sure other customers will find that useful. I'd like to rework all of these helpers classes in general, there are just a couple other big projects in the pipeline we need to clear through first, but any pull requests around these helpers are welcome!

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

No branches or pull requests

2 participants