Add UnProject function to Matrix class

google / mathfu

C++ math library developed primarily for games focused on simplicity and efficiency.

http://google.github.io/mathfu

Apache License 2.0

1.39k stars 189 forks source link

Add UnProject function to Matrix class #16

Closed billhsu closed 8 years ago

billhsu commented 8 years ago

Below is the added function:

+  static inline Vector<T, 3> UnProject(const Vector<T, 3>  windowCoord, 
+                                       const Matrix<T, 4, 4> modelView,
+                                       const Matrix<T, 4, 4> projection,
+                                       const float windowWidth,
+                                       const float windowHeight)

It's has the same functionality as the gluUnProject from the OpenGL Utility Library.

billhsu commented 8 years ago

Anyone here to review this?

billhsu commented 8 years ago

rebased with upstream develop.

stewartmiles commented 8 years ago

Thanks for the pull request, I'll take a look in the morning.

billhsu commented 8 years ago

@stewartmiles Thanks. I'll address the review comments soon.

billhsu commented 8 years ago

@stewartmiles updated for review comments 👍

stewartmiles commented 8 years ago

Looking good, a couple of higher level points:

Could you squash your series of commits into a single commit? This results in 1 pull request == 1 commit which reduces the amount of stuff we have to merge / cherry pick etc.
Why split out the method into a *Helper() function rather than just implementing as the method UnProject() ?

stewartmiles commented 8 years ago

LGTM thanks

billhsu commented 8 years ago

@stewartmiles Thanks for the review 👍 As for why splitting out the method into a *Helper() function, I think it's a convention in that class(like OrthoHelper and LookAtHelper).

stewartmiles commented 8 years ago

Ok, we'll take another look when I bring this into our internal branch. It should bubble back out to master soonish.