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

Extension/bugfixes on my fork #3

Closed
panmari opened this issue Jul 3, 2014 · 4 comments
Closed

Extension/bugfixes on my fork #3

panmari opened this issue Jul 3, 2014 · 4 comments

Comments

@panmari
Copy link
Contributor

panmari commented Jul 3, 2014

Hey there!

I'm using a fork of your package for a ray tracer I'm implementing right now. In the process of developing it, I found some bugs, wrote some tests and implemented additional stuff (determinant of a mat4, inverse of a mat4...)
Would you be interested in merging some of the changes? I must admit though that my tests/commits are not too clean...

see https://github.com/panmari/go3d

@ungerik
Copy link
Owner

ungerik commented Jul 3, 2014

Sure, I would be grateful for all bug fixes.

-Erik

On Thu, Jul 3, 2014 at 4:00 PM, panmari notifications@github.com wrote:

Hey there!

I'm using a fork of your package for a ray tracer I'm implementing right
now. In the process of developing it, I found some bugs, wrote some tests
and implemented additional stuff (determinant of a mat4, inverse of a
mat4...)
Would you be interested in merging some of the changes? I must admit
though that my tests/commits are not too clean...


Reply to this email directly or view it on GitHub
#3.

+43 650 58 33055
erik@erikunger.com
Skype: ungerik
http://erikunger.com
http://github.com/ungerik
http://twitter.com/ungerik
http://facebook.com/ungerik
http://www.linkedin.com/in/ungerik
http://www.xing.com/profile/Erik_Unger2

@panmari
Copy link
Contributor Author

panmari commented Jul 3, 2014

One thing before I make a pull request:

Your scale method for matrices does seem to scale only the diagonal
entries, which is pretty unusual (eg. java vecmath multiplies all entries).
May i adapt the behaviour of this method to scale all values?
On Jul 3, 2014 5:48 PM, "Erik Unger" notifications@github.com wrote:

Sure, I would be grateful for all bug fixes.

-Erik

On Thu, Jul 3, 2014 at 4:00 PM, panmari notifications@github.com wrote:

Hey there!

I'm using a fork of your package for a ray tracer I'm implementing right
now. In the process of developing it, I found some bugs, wrote some
tests
and implemented additional stuff (determinant of a mat4, inverse of a
mat4...)
Would you be interested in merging some of the changes? I must admit
though that my tests/commits are not too clean...


Reply to this email directly or view it on GitHub
#3.

+43 650 58 33055
erik@erikunger.com
Skype: ungerik
http://erikunger.com
http://github.com/ungerik
http://twitter.com/ungerik
http://facebook.com/ungerik
http://www.linkedin.com/in/ungerik
http://www.xing.com/profile/Erik_Unger2


Reply to this email directly or view it on GitHub
#3 (comment).

@ungerik
Copy link
Owner

ungerik commented Jul 3, 2014

On Thu, Jul 3, 2014 at 6:08 PM, panmari notifications@github.com wrote:

Your scale method for matrices does seem to scale only the diagonal
entries, which is pretty unusual

I don't think so. See
http://mathforum.org/mathimages/index.php/Transformation_Matrix#Scaling

It has also always worked for me.

-Erik

+43 650 58 33055
erik@erikunger.com
Skype: ungerik
http://erikunger.com
http://github.com/ungerik
http://twitter.com/ungerik
http://facebook.com/ungerik
http://www.linkedin.com/in/ungerik
http://www.xing.com/profile/Erik_Unger2

@panmari
Copy link
Contributor Author

panmari commented Jul 3, 2014

Fair enough. I created a new method (Mul) for multiplying a matrix with a scalar.
What is your stance about tests? I did some unit testing/benchmarking for my stuff, but haven't added it in the pull request

@ungerik ungerik closed this as completed Jul 20, 2014
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